diff --git a/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index bff5678927..3b03d5f0b6 100644 --- a/zerver/actions/user_groups.py +++ b/zerver/actions/user_groups.py @@ -448,13 +448,13 @@ def do_change_user_group_permission_setting( setting_name: str, setting_value_group: UserGroup, *, + old_setting_api_value: Union[int, AnonymousSettingGroupDict], acting_user: Optional[UserProfile], ) -> None: old_value = getattr(user_group, setting_name) setattr(user_group, setting_name, setting_value_group) user_group.save() - old_setting_api_value = get_group_setting_value_for_api(old_value) new_setting_api_value = get_group_setting_value_for_api(setting_value_group) if not hasattr(old_value, "named_user_group") and hasattr( diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 0b9162a294..8559cd2239 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -742,12 +742,10 @@ def are_both_group_setting_values_equal( def validate_group_setting_value_change( - current_value: UserGroup, + current_setting_api_value: Union[int, AnonymousSettingGroupDict], new_setting_value: Union[int, AnonymousSettingGroupDict], expected_current_setting_value: Optional[Union[int, AnonymousSettingGroupDict]], ) -> bool: - current_setting_api_value = get_group_setting_value_for_api(current_value) - if expected_current_setting_value is not None and not are_both_group_setting_values_equal( expected_current_setting_value, current_setting_api_value, diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 3d87835ed9..1e842d6f45 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -72,6 +72,7 @@ from zerver.lib.streams import create_stream_if_needed from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import get_test_image_file from zerver.lib.types import LinkifierDict, RealmPlaygroundDict +from zerver.lib.user_groups import get_group_setting_value_for_api from zerver.lib.utils import assert_is_not_none from zerver.models import ( Message, @@ -1318,10 +1319,14 @@ class TestRealmAuditLog(ZulipTestCase): old_group = user_group.can_mention_group new_group = NamedUserGroup.objects.get( name=SystemGroups.EVERYONE_ON_INTERNET, realm=user_group.realm - ) + ).usergroup_ptr self.assertNotEqual(old_group.id, new_group.id) do_change_user_group_permission_setting( - user_group, "can_mention_group", new_group, acting_user=None + user_group, + "can_mention_group", + new_group, + old_setting_api_value=old_group.id, + acting_user=None, ) audit_log_entries = RealmAuditLog.objects.filter( event_type=RealmAuditLog.USER_GROUP_GROUP_BASED_SETTING_CHANGED, @@ -1348,7 +1353,11 @@ class TestRealmAuditLog(ZulipTestCase): now = timezone_now() do_change_user_group_permission_setting( - user_group, "can_mention_group", new_group, acting_user=None + user_group, + "can_mention_group", + new_group, + old_setting_api_value=old_group.id, + acting_user=None, ) audit_log_entries = RealmAuditLog.objects.filter( event_type=RealmAuditLog.USER_GROUP_GROUP_BASED_SETTING_CHANGED, @@ -1369,13 +1378,21 @@ class TestRealmAuditLog(ZulipTestCase): ) othello = self.example_user("othello") - new_group = UserGroup.objects.create(realm=user_group.realm) + old_setting_api_value = get_group_setting_value_for_api(user_group.can_mention_group) + # Since the old setting value was a anonymous group, we just update the + # members and subgroups of the already existing UserGroup instead of creating + # a new UserGroup object to keep this consistent with the actual code. + new_group = user_group.can_mention_group new_group.direct_members.set([othello.id]) new_group.direct_subgroups.set([moderators_group.id]) now = timezone_now() do_change_user_group_permission_setting( - user_group, "can_mention_group", new_group, acting_user=None + user_group, + "can_mention_group", + new_group, + old_setting_api_value=old_setting_api_value, + acting_user=None, ) audit_log_entries = RealmAuditLog.objects.filter( event_type=RealmAuditLog.USER_GROUP_GROUP_BASED_SETTING_CHANGED, @@ -1398,12 +1415,17 @@ class TestRealmAuditLog(ZulipTestCase): }, ) + old_setting_api_value = get_group_setting_value_for_api(user_group.can_mention_group) new_group = NamedUserGroup.objects.get( name=SystemGroups.EVERYONE, realm=user_group.realm, is_system_group=True ) now = timezone_now() do_change_user_group_permission_setting( - user_group, "can_mention_group", new_group, acting_user=None + user_group, + "can_mention_group", + new_group, + old_setting_api_value=old_setting_api_value, + acting_user=None, ) audit_log_entries = RealmAuditLog.objects.filter( event_type=RealmAuditLog.USER_GROUP_GROUP_BASED_SETTING_CHANGED, diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 44cb29becc..984c7a5eda 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1812,7 +1812,11 @@ class NormalActionsTest(BaseAction): # Test can_mention_group setting update with self.verify_action() as events: do_change_user_group_permission_setting( - backend, "can_mention_group", moderators_group, acting_user=None + backend, + "can_mention_group", + moderators_group, + old_setting_api_value=everyone_group.id, + acting_user=None, ) check_user_group_update("events[0]", events[0], "can_mention_group") self.assertEqual(events[0]["data"]["can_mention_group"], moderators_group.id) @@ -1822,7 +1826,11 @@ class NormalActionsTest(BaseAction): setting_group.direct_subgroups.set([moderators_group.id]) with self.verify_action() as events: do_change_user_group_permission_setting( - backend, "can_mention_group", setting_group, acting_user=None + backend, + "can_mention_group", + setting_group, + old_setting_api_value=moderators_group.id, + acting_user=None, ) check_user_group_update("events[0]", events[0], "can_mention_group") self.assertEqual( diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 5b953c73f5..52d4a8d1cf 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -32,6 +32,7 @@ from zerver.lib.user_groups import ( access_user_group_for_setting, check_user_group_name, get_direct_memberships_of_users, + get_group_setting_value_for_api, get_subgroup_ids, get_user_group_direct_member_ids, get_user_group_member_ids, @@ -136,8 +137,9 @@ def edit_user_group( expected_current_setting_value = parse_group_setting_value(setting_value.old) current_value = getattr(user_group, setting_name) + current_setting_api_value = get_group_setting_value_for_api(current_value) if validate_group_setting_value_change( - current_value, new_setting_value, expected_current_setting_value + current_setting_api_value, new_setting_value, expected_current_setting_value ): setting_value_group = access_user_group_for_setting( new_setting_value, @@ -147,7 +149,11 @@ def edit_user_group( current_setting_value=current_value, ) do_change_user_group_permission_setting( - user_group, setting_name, setting_value_group, acting_user=user_profile + user_group, + setting_name, + setting_value_group, + old_setting_api_value=current_setting_api_value, + acting_user=user_profile, ) return json_success(request)