From 7518d550f20b3236f0a1a1fb81ec1698e6e3c4b6 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 18 Apr 2024 22:29:50 +0530 Subject: [PATCH] user_groups: Remove unneeded fields from UserGroup model. This commit removes name, description, is_system_group and can_mention_group fields from UserGroup model and rename them in NamedUserGroup model. Fixes #29554. --- zerver/actions/user_groups.py | 13 +---- zerver/lib/import_realm.py | 15 ++--- zerver/lib/user_groups.py | 20 +------ ...medusergroup_can_mention_group_and_more.py | 56 +++++++++++++++++++ zerver/models/groups.py | 18 ++---- zerver/openapi/curl_param_value_generators.py | 2 - zerver/tests/test_import_export.py | 15 ++--- zerver/tests/test_user_groups.py | 2 +- 8 files changed, 80 insertions(+), 61 deletions(-) create mode 100644 zerver/migrations/0515_rename_named_group_can_mention_group_namedusergroup_can_mention_group_and_more.py diff --git a/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index b9ef2822ae..43e51dfd60 100644 --- a/zerver/actions/user_groups.py +++ b/zerver/actions/user_groups.py @@ -47,16 +47,11 @@ def create_user_group_in_database( realm=realm, description=description, is_system_group=is_system_group, - named_group_name=name, - named_group_description=description, - named_group_is_system_group=is_system_group, realm_for_sharding=realm, ) for setting_name, setting_value in group_settings_map.items(): setattr(user_group, setting_name, setting_value) - named_group_setting_name = "named_group_" + setting_name - setattr(user_group, named_group_setting_name, setting_value) system_groups_name_dict = get_role_based_system_groups_dict(realm) user_group = set_defaults_for_group_settings( @@ -224,8 +219,7 @@ def do_update_user_group_name( try: old_value = user_group.name user_group.name = name - user_group.named_group_name = name - user_group.save(update_fields=["name", "named_group_name"]) + user_group.save(update_fields=["name"]) RealmAuditLog.objects.create( realm=user_group.realm, modified_user_group=user_group, @@ -248,8 +242,7 @@ def do_update_user_group_description( ) -> None: old_value = user_group.description user_group.description = description - user_group.named_group_description = description - user_group.save(update_fields=["description", "named_group_description"]) + user_group.save(update_fields=["description"]) RealmAuditLog.objects.create( realm=user_group.realm, modified_user_group=user_group, @@ -447,8 +440,6 @@ def do_change_user_group_permission_setting( ) -> None: old_value = getattr(user_group, setting_name) setattr(user_group, setting_name, setting_value_group) - named_group_setting_name = "named_group_" + setting_name - setattr(user_group, named_group_setting_name, setting_value_group) user_group.save() RealmAuditLog.objects.create( realm=user_group.realm, diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index 6479b28d53..5d9a1151ad 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -694,10 +694,10 @@ def bulk_import_named_user_groups(data: TableData) -> None: ( group["usergroup_ptr_id"], group["realm_for_sharding_id"], - group["named_group_name"], - group["named_group_description"], - group["named_group_is_system_group"], - group["named_group_can_mention_group_id"], + group["name"], + group["description"], + group["is_system_group"], + group["can_mention_group_id"], ) for group in data["zerver_namedusergroup"] ] @@ -1056,10 +1056,6 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int = 1) -> Rea if "zerver_usergroup" in data: re_map_foreign_keys(data, "zerver_usergroup", "realm", related_table="realm") - for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: - re_map_foreign_keys( - data, "zerver_usergroup", setting_name, related_table="usergroup" - ) bulk_import_model(data, UserGroup) if "zerver_namedusergroup" in data: @@ -1070,11 +1066,10 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int = 1) -> Rea data, "zerver_namedusergroup", "realm_for_sharding", related_table="realm" ) for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: - named_group_setting_name = "named_group_" + setting_name re_map_foreign_keys( data, "zerver_namedusergroup", - named_group_setting_name, + setting_name, related_table="usergroup", ) bulk_import_named_user_groups(data) diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index d77a943464..24180ec74e 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -432,9 +432,6 @@ def set_defaults_for_group_settings( default_group = system_groups_name_dict[default_group_name].usergroup_ptr setattr(user_group, setting_name, default_group) - setting_name_for_named_object = "named_group_" + setting_name - setattr(user_group, setting_name_for_named_object, default_group) - return user_group @@ -443,20 +440,11 @@ def bulk_create_system_user_groups(groups: List[Dict[str, str]], realm: Realm) - # settings since we can only set them to the correct values after the groups # are created. initial_group_setting_value = -1 - rows = [ - SQL("({},{},{},{},{})").format( - Literal(realm.id), - Literal(group["name"]), - Literal(group["description"]), - Literal(True), - Literal(initial_group_setting_value), - ) - for group in groups - ] + rows = [SQL("({})").format(Literal(realm.id))] * len(groups) query = SQL( """ - INSERT INTO zerver_usergroup (realm_id, name, description, is_system_group, can_mention_group_id) + INSERT INTO zerver_usergroup (realm_id) VALUES {rows} RETURNING id """ @@ -559,9 +547,7 @@ def create_system_user_groups_for_realm(realm: Realm) -> Dict[int, NamedUserGrou for group in system_user_groups_list: user_group = set_defaults_for_group_settings(group, {}, system_groups_name_dict) groups_with_updated_settings.append(user_group) - NamedUserGroup.objects.bulk_update( - groups_with_updated_settings, ["can_mention_group", "named_group_can_mention_group"] - ) + NamedUserGroup.objects.bulk_update(groups_with_updated_settings, ["can_mention_group"]) subgroup_objects: List[GroupGroupMembership] = [] # "Nobody" system group is not a subgroup of any user group, since it is already empty. diff --git a/zerver/migrations/0515_rename_named_group_can_mention_group_namedusergroup_can_mention_group_and_more.py b/zerver/migrations/0515_rename_named_group_can_mention_group_namedusergroup_can_mention_group_and_more.py new file mode 100644 index 0000000000..5af989a16a --- /dev/null +++ b/zerver/migrations/0515_rename_named_group_can_mention_group_namedusergroup_can_mention_group_and_more.py @@ -0,0 +1,56 @@ +# Generated by Django 4.2.12 on 2024-04-19 04:05 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0514_update_usergroup_foreign_keys_to_namedusergroup"), + ] + + operations = [ + migrations.AlterUniqueTogether( + name="usergroup", + unique_together=set(), + ), + migrations.RemoveField( + model_name="usergroup", + name="can_mention_group", + ), + migrations.RemoveField( + model_name="usergroup", + name="description", + ), + migrations.RemoveField( + model_name="usergroup", + name="is_system_group", + ), + migrations.RemoveField( + model_name="usergroup", + name="name", + ), + migrations.RenameField( + model_name="namedusergroup", + old_name="named_group_can_mention_group", + new_name="can_mention_group", + ), + migrations.RenameField( + model_name="namedusergroup", + old_name="named_group_description", + new_name="description", + ), + migrations.RenameField( + model_name="namedusergroup", + old_name="named_group_is_system_group", + new_name="is_system_group", + ), + migrations.RenameField( + model_name="namedusergroup", + old_name="named_group_name", + new_name="name", + ), + migrations.AlterUniqueTogether( + name="namedusergroup", + unique_together={("realm_for_sharding", "name")}, + ), + ] diff --git a/zerver/models/groups.py b/zerver/models/groups.py index c58897cc25..f300ea2ad0 100644 --- a/zerver/models/groups.py +++ b/zerver/models/groups.py @@ -19,7 +19,6 @@ class SystemGroups: class UserGroup(models.Model): # type: ignore[django-manager-missing] # django-stubs cannot resolve the custom CTEManager yet https://github.com/typeddjango/django-stubs/issues/1023 objects: CTEManager = CTEManager() - name = models.CharField(max_length=100) direct_members = models.ManyToManyField( UserProfile, through="zerver.UserGroupMembership", related_name="direct_groups" ) @@ -31,13 +30,6 @@ class UserGroup(models.Model): # type: ignore[django-manager-missing] # django- related_name="direct_supergroups", ) realm = models.ForeignKey("zerver.Realm", on_delete=CASCADE) - description = models.TextField(default="") - is_system_group = models.BooleanField(default=False) - - can_mention_group = models.ForeignKey("self", on_delete=models.RESTRICT) - - class Meta: - unique_together = (("realm", "name"),) class NamedUserGroup(UserGroup): # type: ignore[django-manager-missing] # django-stubs cannot resolve the custom CTEManager yet https://github.com/typeddjango/django-stubs/issues/1023 @@ -58,11 +50,11 @@ class NamedUserGroup(UserGroup): # type: ignore[django-manager-missing] # djang # setting also points to a UserGroup object. related_name="named_user_group", ) - named_group_name = models.CharField(max_length=MAX_NAME_LENGTH, db_column="name") - named_group_description = models.TextField(default="", db_column="description") - named_group_is_system_group = models.BooleanField(default=False, db_column="is_system_group") + name = models.CharField(max_length=MAX_NAME_LENGTH, db_column="name") + description = models.TextField(default="", db_column="description") + is_system_group = models.BooleanField(default=False, db_column="is_system_group") - named_group_can_mention_group = models.ForeignKey( + can_mention_group = models.ForeignKey( UserGroup, on_delete=models.RESTRICT, db_column="can_mention_group_id" ) @@ -108,7 +100,7 @@ class NamedUserGroup(UserGroup): # type: ignore[django-manager-missing] # djang } class Meta: - unique_together = (("realm_for_sharding", "named_group_name"),) + unique_together = (("realm_for_sharding", "name"),) class UserGroupMembership(models.Model): diff --git a/zerver/openapi/curl_param_value_generators.py b/zerver/openapi/curl_param_value_generators.py index 35c43eb3b1..fb243593c6 100644 --- a/zerver/openapi/curl_param_value_generators.py +++ b/zerver/openapi/curl_param_value_generators.py @@ -266,8 +266,6 @@ def get_temp_user_group_id() -> Dict[str, object]: name="temp", realm=get_realm("zulip"), can_mention_group_id=11, - named_group_name="temp", - named_group_can_mention_group_id=11, realm_for_sharding=get_realm("zulip"), ) return { diff --git a/zerver/tests/test_import_export.py b/zerver/tests/test_import_export.py index 20e37746d1..5120495ed1 100644 --- a/zerver/tests/test_import_export.py +++ b/zerver/tests/test_import_export.py @@ -454,13 +454,12 @@ class RealmImportExportTest(ExportFile): exported_usergroups = data["zerver_usergroup"] self.assert_length(exported_usergroups, 9) - self.assertEqual(exported_usergroups[2]["name"], "role:administrators") self.assertFalse("direct_members" in exported_usergroups[2]) self.assertFalse("direct_subgroups" in exported_usergroups[2]) exported_namedusergroups = data["zerver_namedusergroup"] self.assert_length(exported_namedusergroups, 9) - self.assertEqual(exported_namedusergroups[2]["named_group_name"], "role:administrators") + self.assertEqual(exported_namedusergroups[2]["name"], "role:administrators") self.assertTrue("usergroup_ptr" in exported_namedusergroups[2]) self.assertTrue("realm_for_sharding" in exported_namedusergroups[2]) self.assertFalse("realm" in exported_namedusergroups[2]) @@ -1072,7 +1071,7 @@ class RealmImportExportTest(ExportFile): @getter def get_group_names_for_group_settings(r: Realm) -> Set[str]: return { - getattr(r, permission_name).name + getattr(r, permission_name).named_user_group.name for permission_name in Realm.REALM_PERMISSION_GROUP_SETTINGS } @@ -1231,7 +1230,7 @@ class RealmImportExportTest(ExportFile): @getter def get_user_group_names(r: Realm) -> Set[str]: - return {group.name for group in UserGroup.objects.filter(realm=r)} + return {group.named_user_group.name for group in UserGroup.objects.filter(realm=r)} @getter def get_named_user_group_names(r: Realm) -> Set[str]: @@ -1248,7 +1247,9 @@ class RealmImportExportTest(ExportFile): def get_group_group_membership(r: Realm) -> Set[str]: usergroup = NamedUserGroup.objects.get(realm=r, name="role:members") group_group_membership = GroupGroupMembership.objects.filter(supergroup=usergroup) - subgroups = {membership.subgroup.name for membership in group_group_membership} + subgroups = { + membership.subgroup.named_user_group.name for membership in group_group_membership + } return subgroups @getter @@ -1268,13 +1269,13 @@ class RealmImportExportTest(ExportFile): # correctly since we do not include this in export data. usergroup = NamedUserGroup.objects.get(realm=r, name="role:members") direct_subgroups = usergroup.direct_subgroups.all() - direct_subgroup_names = {group.name for group in direct_subgroups} + direct_subgroup_names = {group.named_user_group.name for group in direct_subgroups} return direct_subgroup_names @getter def get_user_group_can_mention_group_setting(r: Realm) -> str: user_group = NamedUserGroup.objects.get(realm=r, name="hamletcharacters") - return user_group.can_mention_group.name + return user_group.can_mention_group.named_user_group.name # test botstoragedata and botconfigdata @getter diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 714955c5fc..ab8bc47405 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -104,7 +104,7 @@ class UserGroupTestCase(ZulipTestCase): 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. - user_group_names = [group.name for group in user_groups] + user_group_names = [group.named_user_group.name for group in user_groups] self.assertEqual( set(user_group_names), {"support", SystemGroups.MEMBERS, SystemGroups.FULL_MEMBERS},