user_groups: Do not include deactivated users in members list.

This commit updates code to not include deactivated users in
members list in the user groups object sent in "/register"
and "GET /user_groups" response and also in the response
returned by endpoint like "GET /user_groups/{group_id}/members".

The events code is also update to handle this -
- We expect clients to update the members list on receiving
"realm_user/update" event on deactivation. But for guests
who cannot access the user, "user_group/remove_members"
event is sent to update the group members list on deactivation.
- "user_group/add_members" event is sent to all the users on
reactivating the user.

api_docs/changelog.md

@@ -24,6 +24,13 @@ format used by the Zulip server that they are interacting with.
 
 * [`GET /events`](/api/get-events): User reactivation event is not sent
   to users who cannot access the reactivated user anymore.
+* [`GET /events`](/api/get-events): `add_members` and `remove_members`
+  events with `user_group` type can now also be sent when reactivating
+  and deactivating a user respectively.
+* [`POST /register`](/api/register-queue), [`GET /user_groups`](/api/get-user-groups),
+  [`GET /user_groups/{user_group_id}/members/{user_id}`](/api/get-is-user-group-member),
+  [`GET /user_groups/{user_group_id}/members`](/api/get-user-group-members):
+  Deactivated users are not considered as members of a user group anymore.
 
 **Feature level 302**
 

zerver/actions/create_user.py

@@ -771,3 +771,11 @@ def do_reactivate_user(user_profile: UserProfile, *, acting_user: UserProfile |
         stream_dict=stream_dict,
         subscriber_peer_info=subscriber_peer_info,
     )
+
+    member_user_groups = user_profile.direct_groups.exclude(named_user_group=None).select_related(
+        "named_user_group"
+    )
+    for user_group in member_user_groups:
+        do_send_user_group_members_update_event(
+            "add_members", user_group.named_user_group, [user_profile.id]
+        )

zerver/actions/users.py

@@ -311,6 +311,30 @@ def send_events_for_user_deactivation(user_profile: UserProfile) -> None:
             realm, event_deactivate_user, list(users_with_access_to_deactivated_user)
         )
 
+    all_active_user_ids = active_user_ids(realm.id)
+    users_without_access_to_deactivated_user = (
+        set(all_active_user_ids) - users_with_access_to_deactivated_user
+    )
+    if users_without_access_to_deactivated_user:
+        # Guests who have access to the deactivated user receive
+        # 'realm_user/update' event and can update the user groups
+        # data, but guests who cannot access the deactivated user
+        # need an explicit 'user_group/remove_members' event to
+        # update the user groups data.
+        deactivated_user_groups = user_profile.direct_groups.exclude(
+            named_user_group=None
+        ).select_related("named_user_group")
+        for user_group in deactivated_user_groups:
+            event = dict(
+                type="user_group",
+                op="remove_members",
+                group_id=user_group.id,
+                user_ids=[user_profile.id],
+            )
+            send_event_on_commit(
+                user_group.realm, event, list(users_without_access_to_deactivated_user)
+            )
+
     users_losing_access_to_deactivated_user = (
         peer_stream_subscribers - users_with_access_to_deactivated_user
     )

zerver/lib/events.py

@@ -1092,18 +1092,26 @@ def apply_event(
                 if "new_email" in person:
                     p["email"] = person["new_email"]
 
-                if "is_active" in person and not person["is_active"] and include_subscribers:
+                if "is_active" in person and not person["is_active"]:
-                    for sub_dict in [
+                    if include_subscribers:
-                        state["subscriptions"],
+                        for sub_dict in [
-                        state["unsubscribed"],
+                            state["subscriptions"],
-                        state["never_subscribed"],
+                            state["unsubscribed"],
-                    ]:
+                            state["never_subscribed"],
-                        for sub in sub_dict:
+                        ]:
-                            sub["subscribers"] = [
+                            for sub in sub_dict:
-                                user_id
+                                sub["subscribers"] = [
-                                for user_id in sub["subscribers"]
+                                    user_id
-                                if user_id != person_user_id
+                                    for user_id in sub["subscribers"]
-                            ]
+                                    if user_id != person_user_id
+                                ]
+
+                    for user_group in state["realm_user_groups"]:
+                        user_group["members"] = [
+                            user_id
+                            for user_id in user_group["members"]
+                            if user_id != person_user_id
+                        ]
         elif event["op"] == "remove":
             if person_user_id in state["raw_users"]:
                 if user_list_incomplete:

zerver/lib/user_groups.py

@@ -563,8 +563,10 @@ def user_groups_in_realm_serialized(
     if not include_deactivated_groups:
         realm_groups = realm_groups.filter(deactivated=False)
 
-    membership = UserGroupMembership.objects.filter(user_group__realm=realm).values_list(
+    membership = (
-        "user_group_id", "user_profile_id"
+        UserGroupMembership.objects.filter(user_group__realm=realm)
+        .exclude(user_profile__is_active=False, user_group__named_user_group__isnull=False)
+        .values_list("user_group_id", "user_profile_id")
     )
 
     group_membership = GroupGroupMembership.objects.filter(subgroup__realm=realm).values_list(
@@ -632,16 +634,19 @@ def get_direct_user_groups(user_profile: UserProfile) -> list[UserGroup]:
 def get_user_group_direct_member_ids(
     user_group: UserGroup,
 ) -> QuerySet[UserGroupMembership, int]:
-    return UserGroupMembership.objects.filter(user_group=user_group).values_list(
+    return UserGroupMembership.objects.filter(
-        "user_profile_id", flat=True
+        user_group=user_group, user_profile__is_active=True
-    )
+    ).values_list("user_profile_id", flat=True)
 
 
 def get_user_group_direct_members(user_group: UserGroup) -> QuerySet[UserProfile]:
-    return user_group.direct_members.all()
+    return user_group.direct_members.filter(is_active=True)
 
 
 def get_direct_memberships_of_users(user_group: UserGroup, members: list[UserProfile]) -> list[int]:
+    # Returns the subset of the provided members list who are direct subscribers of the group.
+    # If a deactivated user is passed, it will be returned if the user was a member of the
+    # group when deactivated.
     return list(
         UserGroupMembership.objects.filter(
             user_group=user_group, user_profile__in=members
@@ -684,7 +689,9 @@ def get_recursive_strict_subgroups(user_group: UserGroup) -> QuerySet[NamedUserG
 
 
 def get_recursive_group_members(user_group: UserGroup) -> QuerySet[UserProfile]:
-    return UserProfile.objects.filter(direct_groups__in=get_recursive_subgroups(user_group))
+    return UserProfile.objects.filter(
+        is_active=True, direct_groups__in=get_recursive_subgroups(user_group)
+    )
 
 
 def get_recursive_membership_groups(user_profile: UserProfile) -> QuerySet[UserGroup]:

zerver/openapi/zulip.yaml

@@ -3288,6 +3288,12 @@ paths:
                               additionalProperties: false
                               description: |
                                 Event sent to all users when users have been added to a user group.
+
+                                This event is also sent when reactivating a user for all the user
+                                groups the reactivated user was a member of before being deactivated.
+
+                                **Changes**: Starting with Zulip 10.0 (feature level 303), this
+                                event can also be sent when reactivating a user.
                               properties:
                                 id:
                                   $ref: "#/components/schemas/EventIdSchema"
@@ -3324,6 +3330,13 @@ paths:
                               description: |
                                 Event sent to all users when users have been removed from
                                 a user group.
+
+                                This event is also sent when deactivating a user, for all
+                                the user groups the deactivated user is a member of, but only
+                                to the users who cannot access the deactivated user.
+
+                                **Changes**: Starting with Zulip 10.0 (feature level 303),
+                                this event can also be sent when deactivating a user.
                               properties:
                                 id:
                                   $ref: "#/components/schemas/EventIdSchema"
@@ -20596,6 +20609,10 @@ paths:
                               type: array
                               description: |
                                 The integer user IDs of the user group members.
+
+                                **Changes**: Prior to Zulip 10.0 (feature level 303), this
+                                list also included deactivated users who were members of
+                                the user group before being deactivated.
                               items:
                                 type: integer
                             direct_subgroup_ids:
@@ -20818,7 +20835,11 @@ paths:
       description: |
         Check whether a user is member of user group.
 
-        **Changes**: New in Zulip 6.0 (feature level 127).
+        **Changes**: Prior to Zulip 10.0 (feature level 303),
+        this returned true when the user was deactivated but
+        was member of the user group before being deactivated.
+
+        New in Zulip 6.0 (feature level 127).
       parameters:
         - $ref: "#/components/parameters/UserGroupId"
         - $ref: "#/components/parameters/UserId"
@@ -21940,6 +21961,10 @@ components:
           description: |
             Array containing the ID of the users who are
             members of this user group.
+
+            **Changes**: Prior to Zulip 10.0 (feature level 303), this
+            list also included deactivated users who were members of
+            the user group before being deactivated.
         direct_subgroup_ids:
           type: array
           items:

zerver/tests/test_events.py

@@ -3057,12 +3057,32 @@ class NormalActionsTest(BaseAction):
         do_reactivate_user(user_profile, acting_user=None)
         self.set_up_db_for_testing_user_access()
 
-        # Test that guest users receive event only
+        # Test that users who can access the deactivated user
-        # if they can access the deactivated user.
+        # do not receive the 'user_group/remove_members' event.
+        user_profile = self.example_user("cordelia")
+        with self.verify_action(num_events=1) as events:
+            do_deactivate_user(user_profile, acting_user=None)
+        check_realm_user_update("events[0]", events[0], "is_active")
+
+        do_reactivate_user(user_profile, acting_user=None)
+
+        # Test that guest users receive 'user_group/remove_members'
+        # event if they cannot access the deactivated user.
         user_profile = self.example_user("cordelia")
         self.user_profile = self.example_user("polonius")
-        with self.verify_action(num_events=0, state_change_expected=False) as events:
+        with self.verify_action(num_events=3) as events:
             do_deactivate_user(user_profile, acting_user=None)
+        check_user_group_remove_members("events[0]", events[0])
+        check_user_group_remove_members("events[1]", events[1])
+        check_user_group_remove_members("events[2]", events[2])
+
+        user_profile = self.example_user("cordelia")
+        do_reactivate_user(user_profile, acting_user=None)
+        with self.verify_action(num_events=3, user_list_incomplete=True) as events:
+            do_deactivate_user(user_profile, acting_user=None)
+        check_user_group_remove_members("events[0]", events[0])
+        check_user_group_remove_members("events[1]", events[1])
+        check_user_group_remove_members("events[2]", events[2])
 
         user_profile = self.example_user("shiva")
         with self.verify_action(num_events=1) as events:
@@ -3072,9 +3092,12 @@ class NormalActionsTest(BaseAction):
         # Guest loses access to deactivated user if the user
         # was not involved in DMs.
         user_profile = self.example_user("hamlet")
-        with self.verify_action(num_events=1) as events:
+        with self.verify_action(num_events=4) as events:
             do_deactivate_user(user_profile, acting_user=None)
-        check_realm_user_remove("events[0]", events[0])
+        check_user_group_remove_members("events[0]", events[0])
+        check_user_group_remove_members("events[1]", events[1])
+        check_user_group_remove_members("events[2]", events[2])
+        check_realm_user_remove("events[3]]", events[3])
 
         user_profile = self.example_user("aaron")
         # One update event is for a deactivating a bot owned by aaron.
@@ -3089,15 +3112,17 @@ class NormalActionsTest(BaseAction):
         self.make_stream("Test private stream", invite_only=True)
         self.subscribe(bot, "Test private stream")
         do_deactivate_user(bot, acting_user=None)
-        with self.verify_action(num_events=3) as events:
+        with self.verify_action(num_events=5) as events:
             do_reactivate_user(bot, acting_user=None)
         check_realm_bot_update("events[1]", events[1], "is_active")
         check_subscription_peer_add("events[2]", events[2])
+        check_user_group_add_members("events[3]", events[3])
+        check_user_group_add_members("events[4]", events[4])
 
         # Test 'peer_add' event for private stream is received only if user is subscribed to it.
         do_deactivate_user(bot, acting_user=None)
         self.subscribe(self.example_user("hamlet"), "Test private stream")
-        with self.verify_action(num_events=4) as events:
+        with self.verify_action(num_events=6) as events:
             do_reactivate_user(bot, acting_user=None)
         check_realm_bot_update("events[1]", events[1], "is_active")
         check_subscription_peer_add("events[2]", events[2])
@@ -3110,7 +3135,7 @@ class NormalActionsTest(BaseAction):
         bot.refresh_from_db()
 
         self.user_profile = self.example_user("iago")
-        with self.verify_action(num_events=7) as events:
+        with self.verify_action(num_events=9) as events:
             do_reactivate_user(bot, acting_user=self.example_user("iago"))
         check_realm_bot_update("events[1]", events[1], "is_active")
         check_realm_bot_update("events[2]", events[2], "owner_id")
@@ -3119,20 +3144,26 @@ class NormalActionsTest(BaseAction):
         check_stream_delete("events[5]", events[5])
 
         self.set_up_db_for_testing_user_access()
-        # Test that guest users receive event only
+        # Test that guest users receive realm_user/update event
-        # if they can access the reactivated user.
+        # only if they can access the reactivated user.
         user_profile = self.example_user("cordelia")
         do_deactivate_user(user_profile, acting_user=None)
 
         self.user_profile = self.example_user("polonius")
-        with self.verify_action(num_events=0, state_change_expected=False) as events:
+        # Guest users receives group members update event for three groups -
+        # members group, full members group and hamletcharacters group.
+        with self.verify_action(num_events=3) as events:
             do_reactivate_user(user_profile, acting_user=None)
+        check_user_group_add_members("events[0]", events[0])
+        check_user_group_add_members("events[1]", events[1])
+        check_user_group_add_members("events[2]", events[2])
 
         user_profile = self.example_user("shiva")
         do_deactivate_user(user_profile, acting_user=None)
-        with self.verify_action(num_events=1) as events:
+        with self.verify_action(num_events=2) as events:
             do_reactivate_user(user_profile, acting_user=None)
         check_realm_user_update("events[0]", events[0], "is_active")
+        check_user_group_add_members("events[1]", events[1])
 
     def test_do_deactivate_realm(self) -> None:
         realm = self.user_profile.realm

zerver/tests/test_user_groups.py

@@ -91,6 +91,7 @@ class UserGroupTestCase(ZulipTestCase):
         user_group = NamedUserGroup.objects.filter(realm=realm).first()
         assert user_group is not None
         empty_user_group = check_add_user_group(realm, "newgroup", [], acting_user=user)
+        do_deactivate_user(self.example_user("hamlet"), acting_user=None)
 
         user_groups = user_groups_in_realm_serialized(realm, include_deactivated_groups=False)
         self.assert_length(user_groups, 10)
@@ -133,6 +134,10 @@ class UserGroupTestCase(ZulipTestCase):
         # Check that owners system group is present in "direct_subgroup_ids"
         self.assertEqual(user_groups[2]["direct_subgroup_ids"], [owners_system_group.id])
 
+        self.assertEqual(user_groups[8]["name"], "hamletcharacters")
+        # Test deactivated user is not included in the members list.
+        self.assertEqual(user_groups[8]["members"], [self.example_user("cordelia").id])
+
         everyone_group = NamedUserGroup.objects.get(
             name=SystemGroups.EVERYONE, realm=realm, is_system_group=True
         )
@@ -286,6 +291,10 @@ class UserGroupTestCase(ZulipTestCase):
 
         self.assertIn(everyone_group.usergroup_ptr, get_recursive_membership_groups(shiva))
 
+        do_deactivate_user(iago, acting_user=None)
+        self.assertCountEqual(list(get_recursive_group_members(staff_group)), [desdemona])
+        self.assertCountEqual(list(get_recursive_group_members(everyone_group)), [desdemona, shiva])
+
     def test_subgroups_of_role_based_system_groups(self) -> None:
         realm = get_realm("zulip")
         owners_group = NamedUserGroup.objects.get(
@@ -371,6 +380,10 @@ class UserGroupTestCase(ZulipTestCase):
         self.assertFalse(is_user_in_group(moderators_group, hamlet))
         self.assertFalse(is_user_in_group(moderators_group, hamlet, direct_member_only=True))
 
+        do_deactivate_user(iago, acting_user=None)
+        self.assertFalse(is_user_in_group(moderators_group, iago))
+        self.assertFalse(is_user_in_group(administrators_group, iago, direct_member_only=True))
+
     def test_is_any_user_in_group(self) -> None:
         realm = get_realm("zulip")
         shiva = self.example_user("shiva").id
@@ -1685,7 +1698,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
         initial_last_message = self.get_last_message()
         result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params)
         self.assert_json_success(result)
-        self.assert_user_membership(user_group, [hamlet, othello, desdemona, iago, webhook_bot])
+        self.assert_user_membership(user_group, [hamlet, othello, desdemona, webhook_bot])
 
         # No notification message is sent for adding to user group.
         self.assertEqual(self.get_last_message(), initial_last_message)
@@ -1698,7 +1711,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
         initial_last_message = self.get_last_message()
         result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params)
         self.assert_json_success(result)
-        self.assert_user_membership(user_group, [hamlet, desdemona, iago, webhook_bot])
+        self.assert_user_membership(user_group, [hamlet, desdemona, webhook_bot])
 
         # A notification message is sent for removing from user group.
         self.assertNotEqual(self.get_last_message(), initial_last_message)
@@ -1711,13 +1724,13 @@ class UserGroupAPITestCase(UserGroupTestCase):
         params = {"delete": orjson.dumps([othello.id]).decode()}
         result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params)
         self.assert_json_error(result, f"There is no member '{othello.id}' in this user group")
-        self.assert_user_membership(user_group, [hamlet, desdemona, iago, webhook_bot])
+        self.assert_user_membership(user_group, [hamlet, desdemona, webhook_bot])
 
         # Test user remove itself,bot and deactivated user from user group.
         desdemona = self.example_user("desdemona")
         self.login_user(desdemona)
 
-        params = {"delete": orjson.dumps([desdemona.id, iago.id, webhook_bot.id]).decode()}
+        params = {"delete": orjson.dumps([desdemona.id, webhook_bot.id]).decode()}
         initial_last_message = self.get_last_message()
         result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params)
         self.assert_json_success(result)
@@ -2765,6 +2778,11 @@ class UserGroupAPITestCase(UserGroupTestCase):
         )
         self.assertFalse(result_dict["is_user_group_member"])
 
+        # Check membership of deactivated user.
+        do_deactivate_user(iago, acting_user=None)
+        result = self.client_get(f"/json/user_groups/{admins_group.id}/members/{iago.id}")
+        self.assert_json_error(result, "User is deactivated")
+
     def test_get_user_group_members(self) -> None:
         realm = get_realm("zulip")
         iago = self.example_user("iago")
@@ -2811,6 +2829,19 @@ class UserGroupAPITestCase(UserGroupTestCase):
         )
         self.assertCountEqual(result_dict["members"], [shiva.id])
 
+        # Check deactivated users are not returned in members list.
+        do_deactivate_user(shiva, acting_user=None)
+        result_dict = orjson.loads(
+            self.client_get(f"/json/user_groups/{moderators_group.id}/members", info=params).content
+        )
+        self.assertCountEqual(result_dict["members"], [])
+
+        params = {"direct_member_only": orjson.dumps(False).decode()}
+        result_dict = orjson.loads(
+            self.client_get(f"/json/user_groups/{moderators_group.id}/members", info=params).content
+        )
+        self.assertCountEqual(result_dict["members"], [desdemona.id, iago.id])
+
     def test_get_subgroups_of_user_group(self) -> None:
         realm = get_realm("zulip")
         owners_group = NamedUserGroup.objects.get(