From ef5fee4778d05cab50960b723c50d3000a5e539f Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Thu, 28 Sep 2023 16:45:53 -0700 Subject: [PATCH] user_groups: Improve bulk membership change logic. Adds support for bulk-adjusting a single user's membership in multiple user groups in a single transaction in the low-level actions functions, for future use by work on #9957. --- zerver/actions/user_groups.py | 43 +++++++++++++++++++++++++--------- zerver/tests/test_audit_log.py | 8 +++---- zerver/tests/test_events.py | 8 +++---- zerver/views/user_groups.py | 8 +++---- 4 files changed, 44 insertions(+), 23 deletions(-) diff --git a/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index 5b5ce51b04..4892e15cc7 100644 --- a/zerver/actions/user_groups.py +++ b/zerver/actions/user_groups.py @@ -151,13 +151,13 @@ def update_users_in_full_members_system_group( new_full_member_ids = [user["id"] for user in new_full_members] if len(old_full_members) > 0: - remove_members_from_user_group( - full_members_system_group, old_full_member_ids, acting_user=acting_user + bulk_remove_members_from_user_groups( + [full_members_system_group], old_full_member_ids, acting_user=acting_user ) if len(new_full_members) > 0: - bulk_add_members_to_user_group( - full_members_system_group, new_full_member_ids, acting_user=acting_user + bulk_add_members_to_user_groups( + [full_members_system_group], new_full_member_ids, acting_user=acting_user ) @@ -269,12 +269,21 @@ def do_send_user_group_members_update_event( @transaction.atomic(savepoint=False) -def bulk_add_members_to_user_group( - user_group: UserGroup, user_profile_ids: List[int], *, acting_user: Optional[UserProfile] +def bulk_add_members_to_user_groups( + user_groups: List[UserGroup], + user_profile_ids: List[int], + *, + acting_user: Optional[UserProfile], ) -> None: + # All intended callers of this function involve a single user + # being added to one or more groups, or many users being added to + # a single group; but it's easy enough for the implementation to + # support both. + memberships = [ UserGroupMembership(user_group_id=user_group.id, user_profile_id=user_id) for user_id in user_profile_ids + for user_group in user_groups ] UserGroupMembership.objects.bulk_create(memberships) now = timezone_now() @@ -288,17 +297,27 @@ def bulk_add_members_to_user_group( acting_user=acting_user, ) for user_id in user_profile_ids + for user_group in user_groups ) - do_send_user_group_members_update_event("add_members", user_group, user_profile_ids) + for user_group in user_groups: + do_send_user_group_members_update_event("add_members", user_group, user_profile_ids) @transaction.atomic(savepoint=False) -def remove_members_from_user_group( - user_group: UserGroup, user_profile_ids: List[int], *, acting_user: Optional[UserProfile] +def bulk_remove_members_from_user_groups( + user_groups: List[UserGroup], + user_profile_ids: List[int], + *, + acting_user: Optional[UserProfile], ) -> None: + # All intended callers of this function involve a single user + # being added to one or more groups, or many users being added to + # a single group; but it's easy enough for the implementation to + # support both. + UserGroupMembership.objects.filter( - user_group_id=user_group.id, user_profile_id__in=user_profile_ids + user_group__in=user_groups, user_profile_id__in=user_profile_ids ).delete() now = timezone_now() RealmAuditLog.objects.bulk_create( @@ -311,9 +330,11 @@ def remove_members_from_user_group( acting_user=acting_user, ) for user_id in user_profile_ids + for user_group in user_groups ) - do_send_user_group_members_update_event("remove_members", user_group, user_profile_ids) + for user_group in user_groups: + do_send_user_group_members_update_event("remove_members", user_group, user_profile_ids) def do_send_subgroups_update_event( diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 63d6d4cbb0..c130e68760 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -47,12 +47,12 @@ from zerver.actions.streams import ( ) from zerver.actions.user_groups import ( add_subgroups_to_user_group, - bulk_add_members_to_user_group, + bulk_add_members_to_user_groups, + bulk_remove_members_from_user_groups, check_add_user_group, do_change_user_group_permission_setting, do_update_user_group_description, do_update_user_group_name, - remove_members_from_user_group, remove_subgroups_from_user_group, ) from zerver.actions.user_settings import ( @@ -1202,7 +1202,7 @@ class TestRealmAuditLog(ZulipTestCase): now = timezone_now() user_group = check_add_user_group(hamlet.realm, "foo", [], acting_user=None) - bulk_add_members_to_user_group(user_group, [hamlet.id, cordelia.id], acting_user=hamlet) + bulk_add_members_to_user_groups([user_group], [hamlet.id, cordelia.id], acting_user=hamlet) audit_log_entries = RealmAuditLog.objects.filter( acting_user=hamlet, realm=hamlet.realm, @@ -1214,7 +1214,7 @@ class TestRealmAuditLog(ZulipTestCase): self.assertEqual(audit_log_entries[0].modified_user, hamlet) self.assertEqual(audit_log_entries[1].modified_user, cordelia) - remove_members_from_user_group(user_group, [hamlet.id], acting_user=hamlet) + bulk_remove_members_from_user_groups([user_group], [hamlet.id], acting_user=hamlet) audit_log_entries = RealmAuditLog.objects.filter( acting_user=hamlet, realm=hamlet.realm, diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 81a4cbb8e8..b2ecafd696 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -102,13 +102,13 @@ from zerver.actions.submessage import do_add_submessage from zerver.actions.typing import check_send_typing_notification, do_send_stream_typing_notification from zerver.actions.user_groups import ( add_subgroups_to_user_group, - bulk_add_members_to_user_group, + bulk_add_members_to_user_groups, + bulk_remove_members_from_user_groups, check_add_user_group, check_delete_user_group, do_change_user_group_permission_setting, do_update_user_group_description, do_update_user_group_name, - remove_members_from_user_group, remove_subgroups_from_user_group, ) from zerver.actions.user_settings import ( @@ -1461,14 +1461,14 @@ class NormalActionsTest(BaseAction): # Test add members hamlet = self.example_user("hamlet") events = self.verify_action( - lambda: bulk_add_members_to_user_group(backend, [hamlet.id], acting_user=None) + lambda: bulk_add_members_to_user_groups([backend], [hamlet.id], acting_user=None) ) check_user_group_add_members("events[0]", events[0]) # Test remove members hamlet = self.example_user("hamlet") events = self.verify_action( - lambda: remove_members_from_user_group(backend, [hamlet.id], acting_user=None) + lambda: bulk_remove_members_from_user_groups([backend], [hamlet.id], acting_user=None) ) check_user_group_remove_members("events[0]", events[0]) diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 44c8dab69d..601637b7b0 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -9,13 +9,13 @@ from django.utils.translation import override as override_language from zerver.actions.message_send import do_send_messages, internal_prep_private_message from zerver.actions.user_groups import ( add_subgroups_to_user_group, - bulk_add_members_to_user_group, + bulk_add_members_to_user_groups, + bulk_remove_members_from_user_groups, check_add_user_group, check_delete_user_group, do_change_user_group_permission_setting, do_update_user_group_description, do_update_user_group_name, - remove_members_from_user_group, remove_subgroups_from_user_group, ) from zerver.decorator import require_member_or_admin, require_user_group_edit_permission @@ -258,7 +258,7 @@ def add_members_to_group_backend( ) member_user_ids = [member_user.id for member_user in member_users] - bulk_add_members_to_user_group(user_group, member_user_ids, acting_user=user_profile) + bulk_add_members_to_user_groups([user_group], member_user_ids, acting_user=user_profile) notify_for_user_group_subscription_changes( acting_user=user_profile, recipient_users=member_users, @@ -285,7 +285,7 @@ def remove_members_from_group_backend( ) user_profile_ids = [user.id for user in user_profiles] - remove_members_from_user_group(user_group, user_profile_ids, acting_user=user_profile) + bulk_remove_members_from_user_groups([user_group], user_profile_ids, acting_user=user_profile) notify_for_user_group_subscription_changes( acting_user=user_profile, recipient_users=user_profiles,