user_groups: Fix audit log data when changing group settings.

This commit fixes the code store correct old value in audit
log data when changing can_mention_group setting from a
anonymous group to another anonymous group. The bug was
because the old value was being computed after updating
the UserGroup object with new members and subgroups and
is fixed by computing the old value for all the cases
and passing it to do_change_user_group_permission_setting.

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(

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,

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,

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(

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)