diff --git a/web/src/settings_components.ts b/web/src/settings_components.ts index 95229a1a82..35381ac075 100644 --- a/web/src/settings_components.ts +++ b/web/src/settings_components.ts @@ -23,7 +23,7 @@ import * as scroll_util from "./scroll_util"; import * as settings_config from "./settings_config"; import * as settings_data from "./settings_data"; import type {CustomProfileField, group_setting_type_schema} from "./state_data"; -import {realm} from "./state_data"; +import {current_user, realm} from "./state_data"; import * as stream_data from "./stream_data"; import type {StreamSubscription} from "./sub_store"; import type {GroupSettingPillContainer} from "./typeahead_helper"; @@ -1489,7 +1489,14 @@ export function create_group_setting_widget({ setting_name, "group", )!.default_group_name; - const default_group_id = user_groups.get_user_group_from_name(default_group_name)!.id; - set_group_setting_widget_value("new_group_" + setting_name, default_group_id); + if (default_group_name === "group_creator") { + set_group_setting_widget_value("new_group_" + setting_name, { + direct_members: [current_user.user_id], + direct_subgroups: [], + }); + } else { + const default_group_id = user_groups.get_user_group_from_name(default_group_name)!.id; + set_group_setting_widget_value("new_group_" + setting_name, default_group_id); + } } } diff --git a/web/src/settings_data.ts b/web/src/settings_data.ts index ba4805a484..35a21aef56 100644 --- a/web/src/settings_data.ts +++ b/web/src/settings_data.ts @@ -208,14 +208,6 @@ export function can_manage_user_group(group_id: number): boolean { const group = user_groups.get_user_group_from_id(group_id); - // This is a temporary exception and this should be removed as soon - // as `group_creator` is set as a default for `can_manage_group` - // property of user groups. See this topic for more details: - // https://chat.zulip.org/#narrow/stream/3-backend/topic/Group.20creation.20-.20who.20can.20change.20the.20setting.2E/near/1943861 - if (group.creator_id && group.creator_id === current_user.user_id) { - return true; - } - if ( !current_user.is_admin && !current_user.is_moderator && diff --git a/web/tests/settings_data.test.js b/web/tests/settings_data.test.js index a71d006f79..844d8bdc2b 100644 --- a/web/tests/settings_data.test.js +++ b/web/tests/settings_data.test.js @@ -387,7 +387,10 @@ run_test("can_manage_user_group", () => { members: new Set([1, 2]), is_system_group: false, direct_subgroup_ids: new Set([4, 5]), - can_manage_group: 4, + can_manage_group: { + direct_members: [4], + direct_subgroups: [], + }, can_mention_group: 3, creator_id: 4, }; diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index 9d18801a6a..d6c25c85b3 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -760,7 +760,6 @@ def bulk_import_named_user_groups(data: TableData) -> None: group["can_manage_group_id"], group["can_mention_group_id"], group["deactivated"], - group["creator_id"], group["date_created"], ) for group in data["zerver_namedusergroup"] @@ -768,7 +767,7 @@ def bulk_import_named_user_groups(data: TableData) -> None: query = SQL( """ - INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_manage_group_id, can_mention_group_id, deactivated, creator_id, date_created) + INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_manage_group_id, can_mention_group_id, deactivated, date_created) VALUES %s """ ) @@ -1200,6 +1199,7 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int = 1) -> Rea # We need to enforce the invariant that every realm must have a PresenceSequence. PresenceSequence.objects.create(realm=realm, last_update_id=0) + named_user_group_id_to_creator_id = {} if "zerver_usergroup" in data: re_map_foreign_keys(data, "zerver_usergroup", "realm", related_table="realm") bulk_import_model(data, UserGroup) @@ -1215,6 +1215,9 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int = 1) -> Rea re_map_foreign_keys( data, "zerver_namedusergroup", "realm_for_sharding", related_table="realm" ) + for group in data["zerver_namedusergroup"]: + creator_id = group.pop("creator_id", None) + named_user_group_id_to_creator_id[group["id"]] = creator_id for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: re_map_foreign_keys( data, @@ -1307,6 +1310,16 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int = 1) -> Rea stream.creator_id = stream_id_to_creator_id[stream.id] Stream.objects.bulk_update(streams, ["creator_id"]) + if "zerver_namedusergroup" in data: + # UserProfiles have been loaded, so now we're ready to set .creator_id + # for groups based on the mapping we saved earlier. + named_user_groups = NamedUserGroup.objects.filter( + id__in=named_user_group_id_to_creator_id.keys() + ) + for group in named_user_groups: + group.creator_id = named_user_group_id_to_creator_id[group.id] + NamedUserGroup.objects.bulk_update(named_user_groups, ["creator_id"]) + re_map_foreign_keys(data, "zerver_defaultstream", "stream", related_table="stream") re_map_foreign_keys(data, "zerver_realmemoji", "author", related_table="user_profile") diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 2e778ad6b8..f50a909533 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -139,13 +139,6 @@ def check_permission_for_managing_all_groups( permission, which is a permission that requires either certain roles or membership in the group itself to be used. """ - # This is a temporary exception and this should be removed as soon - # as `group_creator` is set as a default for `can_manage_group` - # property of user groups. See this topic for more details: - # https://chat.zulip.org/#narrow/stream/3-backend/topic/Group.20creation.20-.20who.20can.20change.20the.20setting.2E/near/1943861 - if user_group.creator and user_group.creator.id == user_profile.id: - return True - can_manage_all_groups = user_profile.can_manage_all_groups() if can_manage_all_groups: if user_profile.is_realm_admin or user_profile.is_moderator: @@ -789,7 +782,19 @@ def set_defaults_for_group_settings( else: default_group_name = permission_config.default_group_name - default_group = system_groups_name_dict[default_group_name].usergroup_ptr + if default_group_name == "group_creator": + if user_group.creator: + default_group = UserGroup( + realm=user_group.realm, + ) + default_group.save() + UserGroupMembership.objects.create( + user_profile=user_group.creator, user_group=default_group + ) + else: + raise AssertionError("Group creator should not be None.") + else: + default_group = system_groups_name_dict[default_group_name].usergroup_ptr setattr(user_group, setting_name, default_group) return user_group diff --git a/zerver/models/groups.py b/zerver/models/groups.py index c785c9412e..0703501d90 100644 --- a/zerver/models/groups.py +++ b/zerver/models/groups.py @@ -100,7 +100,7 @@ class NamedUserGroup(UserGroup): # type: ignore[django-manager-missing] # djang allow_owners_group=True, allow_nobody_group=True, allow_everyone_group=False, - default_group_name=SystemGroups.NOBODY, + default_group_name="group_creator", default_for_system_groups=SystemGroups.NOBODY, id_field_name="can_manage_group_id", ), diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index c34f6227f0..24c96f9948 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -23940,7 +23940,7 @@ components: The ID of the target user group. schema: type: integer - example: 33 + example: 34 required: true QueueId: name: queue_id diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 8a3521712d..9b2e1c40c8 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -1201,7 +1201,7 @@ class TestRealmAuditLog(ZulipTestCase): hamlet = self.example_user("hamlet") cordelia = self.example_user("cordelia") now = timezone_now() - user_group = check_add_user_group(hamlet.realm, "foo", [], acting_user=None) + user_group = check_add_user_group(hamlet.realm, "foo", [], acting_user=hamlet) bulk_add_members_to_user_groups([user_group], [hamlet.id, cordelia.id], acting_user=hamlet) audit_log_entries = RealmAuditLog.objects.filter( @@ -1228,7 +1228,7 @@ class TestRealmAuditLog(ZulipTestCase): def test_change_user_group_subgroups_memberships(self) -> None: hamlet = self.example_user("hamlet") - user_group = check_add_user_group(hamlet.realm, "main", [], acting_user=None) + user_group = check_add_user_group(hamlet.realm, "main", [], acting_user=hamlet) subgroups = [ check_add_user_group(hamlet.realm, f"subgroup{num}", [], acting_user=hamlet) for num in range(3) diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 815a9410c0..8a4117ccde 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -7804,7 +7804,7 @@ class LDAPGroupSyncTest(ZulipTestCase): ) create_user_group_in_database( - "cool_test_group", [], realm, acting_user=None, description="Created by LDAP sync" + "cool_test_group", [], realm, acting_user=hamlet, description="Created by LDAP sync" ) self.assertTrue( diff --git a/zerver/tests/test_event_queue.py b/zerver/tests/test_event_queue.py index dbde312b03..99930afc4a 100644 --- a/zerver/tests/test_event_queue.py +++ b/zerver/tests/test_event_queue.py @@ -528,7 +528,7 @@ class MissedMessageHookTest(ZulipTestCase): self.cordelia.realm, "hamlet_and_cordelia", [self.user_profile, self.cordelia], - acting_user=None, + acting_user=self.user_profile, ) msg_id = self.send_stream_message( self.iago, "Denmark", content="@*hamlet_and_cordelia* what's up?" diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index f376c771d3..1ce4c6e804 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1832,13 +1832,13 @@ class NormalActionsTest(BaseAction): othello = self.example_user("othello") with self.verify_action() as events: check_add_user_group( - self.user_profile.realm, "backend", [othello], "Backend team", acting_user=None + self.user_profile.realm, "backend", [othello], "Backend team", acting_user=othello ) check_user_group_add("events[0]", events[0]) - nobody_group = NamedUserGroup.objects.get( - name=SystemGroups.NOBODY, realm=self.user_profile.realm, is_system_group=True + self.assertEqual( + events[0]["group"]["can_manage_group"], + AnonymousSettingGroupDict(direct_members=[12], direct_subgroups=[]), ) - self.assertEqual(events[0]["group"]["can_manage_group"], nobody_group.id) everyone_group = NamedUserGroup.objects.get( name=SystemGroups.EVERYONE, realm=self.user_profile.realm, is_system_group=True ) @@ -1857,7 +1857,7 @@ class NormalActionsTest(BaseAction): [othello], "", {"can_manage_group": user_group, "can_mention_group": user_group}, - acting_user=None, + acting_user=othello, ) check_user_group_add("events[0]", events[0]) self.assertEqual( @@ -1930,7 +1930,7 @@ class NormalActionsTest(BaseAction): check_user_group_remove_members("events[0]", events[0]) api_design = check_add_user_group( - hamlet.realm, "api-design", [hamlet], description="API design team", acting_user=None + hamlet.realm, "api-design", [hamlet], description="API design team", acting_user=othello ) # Test add subgroups diff --git a/zerver/tests/test_import_export.py b/zerver/tests/test_import_export.py index 123ed2815e..9f841d4249 100644 --- a/zerver/tests/test_import_export.py +++ b/zerver/tests/test_import_export.py @@ -503,7 +503,7 @@ class RealmImportExportTest(ExportFile): self.assertEqual(exported_realm_user_default[0]["default_language"], "de") exported_usergroups = data["zerver_usergroup"] - self.assert_length(exported_usergroups, 9) + self.assert_length(exported_usergroups, 10) self.assertFalse("direct_members" in exported_usergroups[2]) self.assertFalse("direct_subgroups" in exported_usergroups[2]) @@ -1406,7 +1406,12 @@ class RealmImportExportTest(ExportFile): @getter def get_user_group_names(r: Realm) -> set[str]: - return {group.named_user_group.name for group in UserGroup.objects.filter(realm=r)} + result = set() + for group in UserGroup.objects.filter(realm=r): + if hasattr(group, "named_user_group"): + result.add(group.named_user_group.name) + + return result @getter def get_named_user_group_names(r: Realm) -> set[str]: diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py index 83911b547f..00490fdd68 100644 --- a/zerver/tests/test_markdown.py +++ b/zerver/tests/test_markdown.py @@ -2694,7 +2694,7 @@ class MarkdownMentionTest(ZulipTestCase): def create_user_group_for_test(self, user_group_name: str) -> NamedUserGroup: othello = self.example_user("othello") return check_add_user_group( - get_realm("zulip"), user_group_name, [othello], acting_user=None + get_realm("zulip"), user_group_name, [othello], acting_user=othello ) def test_user_group_mention_single(self) -> None: diff --git a/zerver/tests/test_message_delete.py b/zerver/tests/test_message_delete.py index 29c4a1f482..a860b5eaf1 100644 --- a/zerver/tests/test_message_delete.py +++ b/zerver/tests/test_message_delete.py @@ -411,7 +411,9 @@ class DeleteMessageTest(ZulipTestCase): # of the group which is allowed to delete any message. polonius = self.example_user("polonius") hamlet = self.example_user("hamlet") - user_group = check_add_user_group(realm, "test-group", [hamlet, polonius], acting_user=None) + user_group = check_add_user_group( + realm, "test-group", [hamlet, polonius], acting_user=hamlet + ) do_change_realm_permission_group_setting( realm, "can_delete_any_message_group", diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 293dfd796f..c6f92c3959 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -1460,8 +1460,10 @@ class EditMessageTest(ZulipTestCase): self.subscribe(othello, "test_stream") self.subscribe(cordelia, "test_stream") - leadership = check_add_user_group(othello.realm, "leadership", [othello], acting_user=None) - support = check_add_user_group(othello.realm, "support", [othello], acting_user=None) + leadership = check_add_user_group( + othello.realm, "leadership", [othello], acting_user=othello + ) + support = check_add_user_group(othello.realm, "support", [othello], acting_user=othello) moderators_system_group = NamedUserGroup.objects.get( realm=iago.realm, name=SystemGroups.MODERATORS, is_system_group=True @@ -1526,7 +1528,7 @@ class EditMessageTest(ZulipTestCase): ) self.assert_json_success(result) - test = check_add_user_group(shiva.realm, "test", [shiva], acting_user=None) + test = check_add_user_group(shiva.realm, "test", [shiva], acting_user=shiva) add_subgroups_to_user_group(leadership, [test], acting_user=None) support.can_mention_group = leadership support.save() diff --git a/zerver/tests/test_message_notification_emails.py b/zerver/tests/test_message_notification_emails.py index 2a52590340..2d25a4efcc 100644 --- a/zerver/tests/test_message_notification_emails.py +++ b/zerver/tests/test_message_notification_emails.py @@ -679,10 +679,10 @@ class TestMessageNotificationEmails(ZulipTestCase): cordelia = self.example_user("cordelia") hamlet_only = check_add_user_group( - get_realm("zulip"), "hamlet_only", [hamlet], acting_user=None + get_realm("zulip"), "hamlet_only", [hamlet], acting_user=hamlet ) hamlet_and_cordelia = check_add_user_group( - get_realm("zulip"), "hamlet_and_cordelia", [hamlet, cordelia], acting_user=None + get_realm("zulip"), "hamlet_and_cordelia", [hamlet, cordelia], acting_user=hamlet ) hamlet_only_message_id = self.send_stream_message(othello, "Denmark", "@*hamlet_only*") @@ -717,7 +717,7 @@ class TestMessageNotificationEmails(ZulipTestCase): othello = self.example_user("othello") hamlet_and_cordelia = check_add_user_group( - get_realm("zulip"), "hamlet_and_cordelia", [hamlet, cordelia], acting_user=None + get_realm("zulip"), "hamlet_and_cordelia", [hamlet, cordelia], acting_user=hamlet ) user_group_mentioned_message_id = self.send_stream_message( @@ -754,7 +754,7 @@ class TestMessageNotificationEmails(ZulipTestCase): othello = self.example_user("othello") hamlet_and_cordelia = check_add_user_group( - get_realm("zulip"), "hamlet_and_cordelia", [hamlet, cordelia], acting_user=None + get_realm("zulip"), "hamlet_and_cordelia", [hamlet, cordelia], acting_user=hamlet ) topic_wildcard_mentioned_in_followed_topic_message_id = self.send_stream_message( @@ -1628,14 +1628,14 @@ class TestMessageNotificationEmails(ZulipTestCase): # user groups having upto 'MAX_GROUP_SIZE_FOR_MENTION_REACTIVATION' # members are small user groups. small_user_group = check_add_user_group( - zulip_realm, "small_user_group", [hamlet, othello], acting_user=None + zulip_realm, "small_user_group", [hamlet, othello], acting_user=hamlet ) large_user_group = check_add_user_group( - zulip_realm, "large_user_group", [hamlet], acting_user=None + zulip_realm, "large_user_group", [hamlet], acting_user=hamlet ) subgroup = check_add_user_group( - zulip_realm, "subgroup", [othello, cordelia], acting_user=None + zulip_realm, "subgroup", [othello, cordelia], acting_user=hamlet ) add_subgroups_to_user_group(large_user_group, [subgroup], acting_user=None) diff --git a/zerver/tests/test_message_send.py b/zerver/tests/test_message_send.py index f90e82b040..0fd0913177 100644 --- a/zerver/tests/test_message_send.py +++ b/zerver/tests/test_message_send.py @@ -2134,8 +2134,10 @@ class StreamMessagesTest(ZulipTestCase): self.subscribe(othello, "test_stream") self.subscribe(cordelia, "test_stream") - leadership = check_add_user_group(othello.realm, "leadership", [othello], acting_user=None) - support = check_add_user_group(othello.realm, "support", [othello], acting_user=None) + leadership = check_add_user_group( + othello.realm, "leadership", [othello], acting_user=othello + ) + support = check_add_user_group(othello.realm, "support", [othello], acting_user=othello) moderators_system_group = NamedUserGroup.objects.get( realm=iago.realm, name=SystemGroups.MODERATORS, is_system_group=True @@ -2169,7 +2171,7 @@ class StreamMessagesTest(ZulipTestCase): result = self.api_get(iago, "/api/v1/messages/" + str(msg_id)) self.assert_json_success(result) - test = check_add_user_group(shiva.realm, "test", [shiva], acting_user=None) + test = check_add_user_group(shiva.realm, "test", [shiva], acting_user=shiva) add_subgroups_to_user_group(leadership, [test], acting_user=None) support.can_mention_group = leadership support.save() diff --git a/zerver/tests/test_notification_data.py b/zerver/tests/test_notification_data.py index 3367b0383a..0c49e2deb6 100644 --- a/zerver/tests/test_notification_data.py +++ b/zerver/tests/test_notification_data.py @@ -373,9 +373,9 @@ class TestNotificationData(ZulipTestCase): cordelia = self.example_user("cordelia") realm = hamlet.realm - hamlet_only = check_add_user_group(realm, "hamlet_only", [hamlet], acting_user=None) + hamlet_only = check_add_user_group(realm, "hamlet_only", [hamlet], acting_user=hamlet) hamlet_and_cordelia = check_add_user_group( - realm, "hamlet_and_cordelia", [hamlet, cordelia], acting_user=None + realm, "hamlet_and_cordelia", [hamlet, cordelia], acting_user=hamlet ) mention_backend = MentionBackend(realm.id) diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 7166f5adfe..fb1fe0eb7d 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -3753,14 +3753,14 @@ class HandlePushNotificationTest(PushNotificationTest): zulip_realm, "small_user_group", [self.user_profile, othello], - acting_user=None, + acting_user=othello, ) large_user_group = check_add_user_group( - zulip_realm, "large_user_group", [self.user_profile], acting_user=None + zulip_realm, "large_user_group", [self.user_profile], acting_user=othello ) subgroup = check_add_user_group( - zulip_realm, "subgroup", [othello, cordelia], acting_user=None + zulip_realm, "subgroup", [othello, cordelia], acting_user=othello ) add_subgroups_to_user_group(large_user_group, [subgroup], acting_user=None) @@ -4266,7 +4266,7 @@ class TestGetAPNsPayload(PushNotificationTest): def test_get_message_payload_apns_user_group_mention(self) -> None: user_profile = self.example_user("othello") user_group = check_add_user_group( - get_realm("zulip"), "test_user_group", [user_profile], acting_user=None + get_realm("zulip"), "test_user_group", [user_profile], acting_user=user_profile ) stream = Stream.objects.filter(name="Verona").get() message = self.get_message(Recipient.STREAM, stream.id, stream.realm_id) diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 496daac06d..4fc4c3245e 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -2087,9 +2087,8 @@ class RealmAPITest(ZulipTestCase): with self.subTest(property=prop): self.do_test_realm_update_api(prop) - check_add_user_group( - get_realm("zulip"), "leadership", [self.example_user("hamlet")], acting_user=None - ) + hamlet = self.example_user("hamlet") + check_add_user_group(get_realm("zulip"), "leadership", [hamlet], acting_user=hamlet) for prop in Realm.REALM_PERMISSION_GROUP_SETTINGS: with self.subTest(property=prop): self.do_test_realm_permission_group_setting_update_api(prop) diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 72f017e3ac..36465cb655 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -2832,15 +2832,15 @@ class StreamAdminTest(ZulipTestCase): def test_can_remove_subscribers_group(self) -> None: realm = get_realm("zulip") + iago = self.example_user("iago") leadership_group = check_add_user_group( realm, "leadership", - [self.example_user("iago"), self.example_user("shiva")], - acting_user=None, - ) - managers_group = check_add_user_group( - realm, "managers", [self.example_user("hamlet")], acting_user=None + [iago, self.example_user("shiva")], + acting_user=iago, ) + hamlet = self.example_user("hamlet") + managers_group = check_add_user_group(realm, "managers", [hamlet], acting_user=hamlet) add_subgroups_to_user_group(managers_group, [leadership_group], acting_user=None) cordelia = self.example_user("cordelia") @@ -4469,7 +4469,9 @@ class SubscriptionAPITest(ZulipTestCase): do_set_realm_property(realm, "waiting_period_threshold", 0, acting_user=None) self.common_subscribe_to_streams(cordelia, ["new_stream2"], invite_only=invite_only) - leadership_group = check_add_user_group(realm, "Leadership", [desdemona], acting_user=None) + leadership_group = check_add_user_group( + realm, "Leadership", [desdemona], acting_user=desdemona + ) do_change_realm_permission_group_setting( realm, stream_policy, leadership_group, acting_user=None ) @@ -4484,7 +4486,7 @@ class SubscriptionAPITest(ZulipTestCase): self.common_subscribe_to_streams(desdemona, ["new_stream3"], invite_only=invite_only) - staff_group = check_add_user_group(realm, "Staff", [iago], acting_user=None) + staff_group = check_add_user_group(realm, "Staff", [iago], acting_user=iago) setting_group = self.create_or_update_anonymous_group_for_setting([cordelia], [staff_group]) do_change_realm_permission_group_setting( realm, stream_policy, setting_group, acting_user=None diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 341bfc88b3..2a97ccb8cb 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -75,7 +75,7 @@ class UserGroupTestCase(ZulipTestCase): self.assertSetEqual(set(subgroup_ids), {member.id for member in members}) def create_user_group_for_test( - self, group_name: str, acting_user: UserProfile | None = None + self, group_name: str, acting_user: UserProfile ) -> NamedUserGroup: members = [self.example_user("othello")] return check_add_user_group( @@ -145,7 +145,10 @@ class UserGroupTestCase(ZulipTestCase): self.assertEqual(user_groups[9]["name"], "newgroup") self.assertEqual(user_groups[9]["description"], "") self.assertEqual(user_groups[9]["members"], []) - self.assertEqual(user_groups[9]["can_manage_group"], user_group.id) + self.assertEqual( + user_groups[9]["can_manage_group"], + AnonymousSettingGroupDict(direct_members=[11], direct_subgroups=[]), + ) self.assertEqual(user_groups[9]["can_mention_group"], everyone_group.id) self.assertFalse(user_groups[0]["deactivated"]) @@ -162,7 +165,7 @@ class UserGroupTestCase(ZulipTestCase): "can_manage_group": setting_group, "can_mention_group": setting_group, }, - acting_user=None, + acting_user=self.example_user("desdemona"), ) user_groups = user_groups_in_realm_serialized(realm, include_deactivated_groups=False) self.assertEqual(user_groups[10]["id"], new_user_group.id) @@ -190,9 +193,8 @@ class UserGroupTestCase(ZulipTestCase): ) self.assertFalse(user_groups[0]["deactivated"]) - another_new_group = check_add_user_group( - realm, "newgroup3", [self.example_user("hamlet")], acting_user=None - ) + hamlet = self.example_user("hamlet") + another_new_group = check_add_user_group(realm, "newgroup3", [hamlet], acting_user=hamlet) add_subgroups_to_user_group( new_user_group, [another_new_group, owners_system_group], acting_user=None ) @@ -220,7 +222,7 @@ class UserGroupTestCase(ZulipTestCase): def test_get_direct_user_groups(self) -> None: othello = self.example_user("othello") - self.create_user_group_for_test("support") + self.create_user_group_for_test("support", acting_user=self.example_user("desdemona")) user_groups = get_direct_user_groups(othello) self.assert_length(user_groups, 3) # othello is a direct member of two role-based system groups also. @@ -236,12 +238,14 @@ class UserGroupTestCase(ZulipTestCase): desdemona = self.example_user("desdemona") shiva = self.example_user("shiva") - leadership_group = check_add_user_group(realm, "Leadership", [desdemona], acting_user=None) + leadership_group = check_add_user_group( + realm, "Leadership", [desdemona], acting_user=desdemona + ) - staff_group = check_add_user_group(realm, "Staff", [iago], acting_user=None) + staff_group = check_add_user_group(realm, "Staff", [iago], acting_user=iago) GroupGroupMembership.objects.create(supergroup=staff_group, subgroup=leadership_group) - everyone_group = check_add_user_group(realm, "Everyone", [shiva], acting_user=None) + everyone_group = check_add_user_group(realm, "Everyone", [shiva], acting_user=shiva) GroupGroupMembership.objects.create(supergroup=everyone_group, subgroup=staff_group) self.assertCountEqual( @@ -402,13 +406,13 @@ class UserGroupTestCase(ZulipTestCase): def test_has_user_group_access_for_subgroup(self) -> None: iago = self.example_user("iago") zulip_realm = get_realm("zulip") - zulip_group = check_add_user_group(zulip_realm, "zulip", [], acting_user=None) + zulip_group = check_add_user_group(zulip_realm, "zulip", [], acting_user=iago) moderators_group = NamedUserGroup.objects.get( name=SystemGroups.MODERATORS, realm=zulip_realm, is_system_group=True ) lear_realm = get_realm("lear") - lear_group = check_add_user_group(lear_realm, "test", [], acting_user=None) + lear_group = check_add_user_group(lear_realm, "test", [], acting_user=iago) self.assertFalse(has_user_group_access_for_subgroup(lear_group, iago)) self.assertTrue(has_user_group_access_for_subgroup(zulip_group, iago)) @@ -434,11 +438,8 @@ class UserGroupAPITestCase(UserGroupTestCase): everyone_system_group = NamedUserGroup.objects.get( name="role:everyone", realm=hamlet.realm, is_system_group=True ) - nobody_system_group = NamedUserGroup.objects.get( - name=SystemGroups.NOBODY, realm=hamlet.realm, is_system_group=True - ) support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) - self.assertEqual(support_group.can_manage_group, nobody_system_group.usergroup_ptr) + self.assertCountEqual(support_group.can_manage_group.direct_members.all(), [hamlet]) self.assertEqual(support_group.can_mention_group, everyone_system_group.usergroup_ptr) # Test invalid member error @@ -521,7 +522,7 @@ class UserGroupAPITestCase(UserGroupTestCase): permission_configuration = NamedUserGroup.GROUP_PERMISSION_SETTINGS[setting_name] leadership_group = check_add_user_group( - hamlet.realm, "leadership", [hamlet], acting_user=None + hamlet.realm, "leadership", [hamlet], acting_user=hamlet ) moderators_group = NamedUserGroup.objects.get( name="role:moderators", realm=hamlet.realm, is_system_group=True @@ -802,8 +803,9 @@ class UserGroupAPITestCase(UserGroupTestCase): self.assert_json_error(result, "Invalid user group") lear_realm = get_realm("lear") + lear_cordelia = self.lear_user("cordelia") lear_test_group = check_add_user_group( - lear_realm, "test", [self.lear_user("cordelia")], acting_user=None + lear_realm, "test", [lear_cordelia], acting_user=lear_cordelia ) result = self.client_patch(f"/json/user_groups/{lear_test_group.id}", info=params) self.assert_json_error(result, "Invalid user group") @@ -860,7 +862,7 @@ class UserGroupAPITestCase(UserGroupTestCase): name="role:moderators", realm=hamlet.realm, is_system_group=True ) - self.login("hamlet") + self.login("desdemona") params = {} params[setting_name] = orjson.dumps( { @@ -1064,7 +1066,7 @@ class UserGroupAPITestCase(UserGroupTestCase): def test_update_user_group_permission_settings(self) -> None: hamlet = self.example_user("hamlet") check_add_user_group(hamlet.realm, "support", [hamlet], acting_user=hamlet) - check_add_user_group(hamlet.realm, "marketing", [hamlet], acting_user=None) + check_add_user_group(hamlet.realm, "marketing", [hamlet], acting_user=hamlet) check_add_user_group(hamlet.realm, "leadership", [hamlet], acting_user=hamlet) for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: @@ -1089,7 +1091,7 @@ class UserGroupAPITestCase(UserGroupTestCase): hamlet = self.example_user("hamlet") support_group = check_add_user_group(hamlet.realm, "support", [hamlet], acting_user=hamlet) marketing_group = check_add_user_group( - hamlet.realm, "marketing", [hamlet], acting_user=None + hamlet.realm, "marketing", [hamlet], acting_user=hamlet ) everyone_group = NamedUserGroup.objects.get( name="role:everyone", realm=hamlet.realm, is_system_group=True @@ -1248,8 +1250,12 @@ class UserGroupAPITestCase(UserGroupTestCase): self.assert_json_error(result, "Invalid user group") def test_user_group_deactivation(self) -> None: - support_group = self.create_user_group_for_test("support") - leadership_group = self.create_user_group_for_test("leadership") + support_group = self.create_user_group_for_test( + "support", acting_user=self.example_user("desdemona") + ) + leadership_group = self.create_user_group_for_test( + "leadership", acting_user=self.example_user("othello") + ) add_subgroups_to_user_group(support_group, [leadership_group], acting_user=None) realm = get_realm("zulip") @@ -1402,10 +1408,11 @@ class UserGroupAPITestCase(UserGroupTestCase): def test_user_group_deactivation_with_group_used_for_settings(self) -> None: realm = get_realm("zulip") - othello = self.example_user("othello") hamlet = self.example_user("hamlet") - support_group = self.create_user_group_for_test("support", acting_user=othello) + support_group = self.create_user_group_for_test( + "support", acting_user=self.example_user("othello") + ) moderators_group = NamedUserGroup.objects.get( name=SystemGroups.MODERATORS, realm=realm, is_system_group=True ) @@ -1500,7 +1507,9 @@ class UserGroupAPITestCase(UserGroupTestCase): stream, setting_name, moderators_group, acting_user=None ) - leadership_group = self.create_user_group_for_test("leadership") + leadership_group = self.create_user_group_for_test( + "leadership", acting_user=self.example_user("othello") + ) for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: do_change_user_group_permission_setting( leadership_group, setting_name, support_group, acting_user=None @@ -1592,7 +1601,7 @@ class UserGroupAPITestCase(UserGroupTestCase): for i in range(50) ] - with self.assert_database_query_count(5): + with self.assert_database_query_count(7): user_group = create_user_group_in_database( name="support", members=[hamlet, cordelia, *original_users], @@ -1619,7 +1628,7 @@ class UserGroupAPITestCase(UserGroupTestCase): with ( mock.patch("zerver.views.user_groups.notify_for_user_group_subscription_changes"), - self.assert_database_query_count(11), + self.assert_database_query_count(14), ): result = self.client_post(f"/json/user_groups/{user_group.id}/members", info=params) self.assert_json_success(result) @@ -1760,7 +1769,7 @@ class UserGroupAPITestCase(UserGroupTestCase): self.subscribe(user, stream_name) check_add_user_group( - name=group_name, initial_members=list(support_team), realm=realm, acting_user=None + name=group_name, initial_members=list(support_team), realm=realm, acting_user=hamlet ) payload = dict( @@ -2070,7 +2079,9 @@ class UserGroupAPITestCase(UserGroupTestCase): check_update_user_group("help", "Troubleshooting team", "desdemona") def test_realm_level_setting_for_updating_members(self) -> None: - user_group = self.create_user_group_for_test("support") + user_group = self.create_user_group_for_test( + "support", acting_user=self.example_user("desdemona") + ) aaron = self.example_user("aaron") othello = self.example_user("othello") cordelia = self.example_user("cordelia") @@ -2194,11 +2205,11 @@ class UserGroupAPITestCase(UserGroupTestCase): def test_group_level_setting_for_updating_members(self) -> None: othello = self.example_user("othello") user_group = check_add_user_group( - get_realm("zulip"), "support", [othello], acting_user=None + get_realm("zulip"), "support", [othello], acting_user=self.example_user("desdemona") ) hamlet = self.example_user("hamlet") leadership_group = check_add_user_group( - get_realm("zulip"), "leadership", [hamlet], acting_user=None + get_realm("zulip"), "leadership", [hamlet], acting_user=hamlet ) aaron = self.example_user("aaron") @@ -2383,12 +2394,12 @@ class UserGroupAPITestCase(UserGroupTestCase): ) leadership_group = check_add_user_group( - realm, "leadership", [desdemona, iago, hamlet], acting_user=None + realm, "leadership", [desdemona, iago, hamlet], acting_user=desdemona ) support_group = check_add_user_group( realm, "support", [hamlet, othello], acting_user=hamlet ) - test_group = check_add_user_group(realm, "test", [hamlet], acting_user=None) + test_group = check_add_user_group(realm, "test", [hamlet], acting_user=hamlet) self.login("cordelia") # Non-admin and non-moderators who are not a member of group cannot add or remove subgroups. @@ -2470,8 +2481,9 @@ class UserGroupAPITestCase(UserGroupTestCase): self.assert_subgroup_membership(test_group, [support_group]) lear_realm = get_realm("lear") + lear_cordelia = self.lear_user("cordelia") lear_test_group = check_add_user_group( - lear_realm, "test", [self.lear_user("cordelia")], acting_user=None + lear_realm, "test", [lear_cordelia], acting_user=lear_cordelia ) result = self.client_post(f"/json/user_groups/{lear_test_group.id}/subgroups", info=params) self.assert_json_error(result, "Invalid user group") @@ -2534,7 +2546,7 @@ class UserGroupAPITestCase(UserGroupTestCase): lear_realm = get_realm("lear") lear_cordelia = self.lear_user("cordelia") lear_test_group = check_add_user_group( - lear_realm, "test", [lear_cordelia], acting_user=None + lear_realm, "test", [lear_cordelia], acting_user=lear_cordelia ) result = self.client_get( f"/json/user_groups/{lear_test_group.id}/members/{lear_cordelia.id}" @@ -2595,8 +2607,9 @@ class UserGroupAPITestCase(UserGroupTestCase): self.assert_json_error(result, "Invalid user group") lear_realm = get_realm("lear") + lear_cordelia = self.lear_user("cordelia") lear_test_group = check_add_user_group( - lear_realm, "test", [self.lear_user("cordelia")], acting_user=None + lear_realm, "test", [lear_cordelia], acting_user=lear_cordelia ) result = self.client_get(f"/json/user_groups/{lear_test_group.id}/members") self.assert_json_error(result, "Invalid user group") @@ -2643,8 +2656,9 @@ class UserGroupAPITestCase(UserGroupTestCase): self.assert_json_error(result, "Invalid user group") lear_realm = get_realm("lear") + lear_cordelia = self.lear_user("cordelia") lear_test_group = check_add_user_group( - lear_realm, "test", [self.lear_user("cordelia")], acting_user=None + lear_realm, "test", [lear_cordelia], acting_user=lear_cordelia ) result = self.client_get(f"/json/user_groups/{lear_test_group.id}/subgroups") self.assert_json_error(result, "Invalid user group") @@ -2678,9 +2692,9 @@ class UserGroupAPITestCase(UserGroupTestCase): self.assertCountEqual(result_dict["subgroups"], [admins_group.id]) def test_add_subgroup_from_wrong_realm(self) -> None: - other_realm = do_create_realm("other", "Other Realm") - other_user_group = check_add_user_group(other_realm, "user_group", [], acting_user=None) iago = self.example_user("iago") + other_realm = do_create_realm("other", "Other Realm") + other_user_group = check_add_user_group(other_realm, "user_group", [], acting_user=iago) realm = get_realm("zulip") zulip_group = check_add_user_group(realm, "zulip_test", [], acting_user=iago) diff --git a/zerver/transaction_tests/test_user_groups.py b/zerver/transaction_tests/test_user_groups.py index 3a4339e5de..b68eb7e08e 100644 --- a/zerver/transaction_tests/test_user_groups.py +++ b/zerver/transaction_tests/test_user_groups.py @@ -64,7 +64,7 @@ def dev_update_subgroups( class UserGroupRaceConditionTestCase(ZulipTransactionTestCase): - created_user_groups: list[UserGroup] = [] + created_user_groups: list[NamedUserGroup] = [] counter = 0 CHAIN_LENGTH = 3 @@ -73,7 +73,13 @@ class UserGroupRaceConditionTestCase(ZulipTransactionTestCase): # Clean up the user groups created to minimize leakage with transaction.atomic(): for group in self.created_user_groups: + # can_manage_group can be deleted as long as it's the + # default group_creator. If we start using non-default + # can_manage_group in this test, deleting that group + # should be reconsidered. + can_manage_group = group.can_manage_group group.delete() + can_manage_group.delete() transaction.on_commit(self.created_user_groups.clear) super().tearDown() diff --git a/zilencer/management/commands/populate_db.py b/zilencer/management/commands/populate_db.py index e7de46f6dd..3248f903c2 100644 --- a/zilencer/management/commands/populate_db.py +++ b/zilencer/management/commands/populate_db.py @@ -1414,10 +1414,11 @@ def choose_date_sent( def create_user_groups() -> None: zulip = get_realm("zulip") + cordelia = get_user_by_delivery_email("cordelia@zulip.com", zulip) members = [ get_user_by_delivery_email("cordelia@zulip.com", zulip), get_user_by_delivery_email("hamlet@zulip.com", zulip), ] create_user_group_in_database( - "hamletcharacters", members, zulip, description="Characters of Hamlet", acting_user=None + "hamletcharacters", members, zulip, description="Characters of Hamlet", acting_user=cordelia )