message_edit: Send only changed settings in event data and api response.

Previously, we included all three message edit related settings ("allow_message_editing", "message_content_edit_limit_seconds" and "edit_topic_policy") in the event data and api response irrespective of which of these settings were changed. Now, we only include changed settings and separate events are sent for each setting if more than one of them is changed. Note that the previous typed in event_schema.py for `message_content_edit_limit_seconds` incorrectly did not allow `None` as a value, which is used to encode no limit.
2022-09-22 14:23:37 +05:30 · 2022-09-22 14:23:37 +05:30 · 04693b6ac1
parent 522c159441
commit 04693b6ac1
11 changed files with 131 additions and 190 deletions
--- a/docs/tutorials/new-feature-tutorial.md
+++ b/docs/tutorials/new-feature-tutorial.md
@ -481,7 +481,7 @@ with the new value. E.g., for `authentication_methods`, we created

 # import do_set_realm_authentication_methods from actions.py
 from zerver.actions.realm_settings import (
-    do_set_realm_message_editing,
+    do_reactivate_realm,
    do_set_realm_authentication_methods,
    # ...
 )
--- a/templates/zerver/api/changelog.md
+++ b/templates/zerver/api/changelog.md
@ -20,6 +20,17 @@ format used by the Zulip server that they are interacting with.

 ## Changes in Zulip 6.0

+**Feature level 150**
+
+* [`GET /events`](/api/get-events): Separate events are now sent on changing
+  `allow_message_editing`, `message_content_edit_limit_seconds` and
+  `edit_topic_policy` settings, whereas previously one event was sent including
+  all of these setting values irrespective of which of them were actually changed.
+* [`PATCH /realm`]: Only changed settings are included in the response data now
+  when changing `allow_message_editing`, `edit_topic_policy` and
+  `message_content_edit_limit_seconds` settings, instead of including all the
+  fields even if one of these settings was changed.
+
 **Feature level 149**

 * [`POST /register`](/api/register-queue): The `client_gravatar` and
--- a/version.py
+++ b/version.py
@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3"
 # Changes should be accompanied by documentation explaining what the
 # new level means in templates/zerver/api/changelog.md, as well as
 # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`.
-API_FEATURE_LEVEL = 149
+API_FEATURE_LEVEL = 150

 # Bump the minor PROVISION_VERSION to indicate that folks should provision
 # only when going from an old version of the code to a newer version. Bump
--- a/zerver/actions/realm_settings.py
+++ b/zerver/actions/realm_settings.py
@ -61,6 +61,21 @@ def do_set_realm_property(
        property=name,
        value=value,
    )
+
+    # These settings have a different event format due to their history.
+    message_edit_settings = [
+        "allow_message_editing",
+        "edit_topic_policy",
+        "message_content_edit_limit_seconds",
+    ]
+    if name in message_edit_settings:
+        event = dict(
+            type="realm",
+            op="update_dict",
+            property="default",
+            data={name: value},
+        )
+
    transaction.on_commit(lambda: send_event(realm, event, active_user_ids(realm.id)))

    event_time = timezone_now()
@ -135,60 +150,6 @@ def do_set_realm_authentication_methods(
    send_event(realm, event, active_user_ids(realm.id))


-def do_set_realm_message_editing(
-    realm: Realm,
-    allow_message_editing: bool,
-    message_content_edit_limit_seconds: Optional[int],
-    edit_topic_policy: int,
-    *,
-    acting_user: Optional[UserProfile],
-) -> None:
-    old_values = dict(
-        allow_message_editing=realm.allow_message_editing,
-        message_content_edit_limit_seconds=realm.message_content_edit_limit_seconds,
-        edit_topic_policy=realm.edit_topic_policy,
-    )
-
-    realm.allow_message_editing = allow_message_editing
-    realm.message_content_edit_limit_seconds = message_content_edit_limit_seconds
-    realm.edit_topic_policy = edit_topic_policy
-
-    event_time = timezone_now()
-    updated_properties = dict(
-        allow_message_editing=allow_message_editing,
-        message_content_edit_limit_seconds=message_content_edit_limit_seconds,
-        edit_topic_policy=edit_topic_policy,
-    )
-
-    with transaction.atomic():
-        for updated_property, updated_value in updated_properties.items():
-            if updated_value == old_values[updated_property]:
-                continue
-            RealmAuditLog.objects.create(
-                realm=realm,
-                event_type=RealmAuditLog.REALM_PROPERTY_CHANGED,
-                event_time=event_time,
-                acting_user=acting_user,
-                extra_data=orjson.dumps(
-                    {
-                        RealmAuditLog.OLD_VALUE: old_values[updated_property],
-                        RealmAuditLog.NEW_VALUE: updated_value,
-                        "property": updated_property,
-                    }
-                ).decode(),
-            )
-
-        realm.save(update_fields=list(updated_properties.keys()))
-
-    event = dict(
-        type="realm",
-        op="update_dict",
-        property="default",
-        data=updated_properties,
-    )
-    send_event(realm, event, active_user_ids(realm.id))
-
-
 def do_set_realm_stream(
    realm: Realm,
    field: Literal["notifications_stream", "signup_notifications_stream"],
--- a/zerver/lib/event_schema.py
+++ b/zerver/lib/event_schema.py
@ -977,10 +977,20 @@ logo_data = DictType(
    ]
 )

-message_edit_data = DictType(
+allow_message_editing_data = DictType(
    required_keys=[
        ("allow_message_editing", bool),
-        ("message_content_edit_limit_seconds", int),
+    ]
+)
+
+message_content_edit_limit_seconds_data = DictType(
+    required_keys=[
+        ("message_content_edit_limit_seconds", OptionalType(int)),
+    ]
+)
+
+edit_topic_policy_data = DictType(
+    required_keys=[
        ("edit_topic_policy", int),
    ]
 )
@ -996,10 +1006,12 @@ night_logo_data = DictType(
 update_dict_data = UnionType(
    [
        # force vertical
+        allow_message_editing_data,
        authentication_data,
+        edit_topic_policy_data,
        icon_data,
        logo_data,
-        message_edit_data,
+        message_content_edit_limit_seconds_data,
        night_logo_data,
    ]
 )
@ -1026,7 +1038,11 @@ def check_realm_update_dict(
        assert isinstance(event["data"], dict)

        if "allow_message_editing" in event["data"]:
-            sub_type = message_edit_data
+            sub_type = allow_message_editing_data
+        elif "message_content_edit_limit_seconds" in event["data"]:
+            sub_type = message_content_edit_limit_seconds_data
+        elif "edit_topic_policy" in event["data"]:
+            sub_type = edit_topic_policy_data
        elif "authentication_methods" in event["data"]:
            sub_type = authentication_data
        else:
--- a/zerver/models.py
+++ b/zerver/models.py
@ -700,6 +700,7 @@ class Realm(models.Model):
    property_types: Dict[str, Union[type, Tuple[type, ...]]] = dict(
        add_custom_emoji_policy=int,
        allow_edit_history=bool,
+        allow_message_editing=bool,
        avatar_changes_disabled=bool,
        bot_creation_policy=int,
        create_private_stream_policy=int,
@ -712,6 +713,7 @@ class Realm(models.Model):
        digest_emails_enabled=bool,
        digest_weekday=int,
        disallow_disposable_email_addresses=bool,
+        edit_topic_policy=int,
        email_address_visibility=int,
        email_changes_disabled=bool,
        emails_restricted_to_domains=bool,
@ -725,6 +727,7 @@ class Realm(models.Model):
        invite_to_stream_policy=int,
        mandatory_topics=bool,
        message_content_allowed_in_email_notifications=bool,
+        message_content_edit_limit_seconds=(int, type(None)),
        message_content_delete_limit_seconds=(int, type(None)),
        message_retention_days=(int, type(None)),
        move_messages_between_streams_policy=int,
--- a/zerver/openapi/zulip.yaml
+++ b/zerver/openapi/zulip.yaml
@ -3782,6 +3782,12 @@ paths:
                                  type: object
                                  description: |
                                    An object containing the properties that have changed.
+
+                                    **Changes**: Before Zulip 6.0 (feature level 150), on changing any of
+                                    `allow_message_editing`, `message_content_edit_limit_seconds`, or
+                                    `edit_topic_policy` settings, this object included all the three settings
+                                    irrespective of which of these settings were changed. Now, a separate event
+                                    is sent for each changed setting.
                                  properties:
                                    add_custom_emoji_policy:
                                      type: integer
@ -4222,11 +4228,7 @@ paths:
                                  "op": "update_dict",
                                  "property": "default",
                                  "data":
-                                    {
-                                      "allow_message_editing": false,
-                                      "message_content_edit_limit_seconds": 600,
-                                      "edit_topic_policy": 2,
-                                    },
+                                    {"message_content_edit_limit_seconds": 600},
                                  "id": 0,
                                }
                            - type: object
--- a/zerver/tests/test_audit_log.py
+++ b/zerver/tests/test_audit_log.py
@ -34,8 +34,8 @@ from zerver.actions.realm_settings import (
    do_deactivate_realm,
    do_reactivate_realm,
    do_set_realm_authentication_methods,
-    do_set_realm_message_editing,
    do_set_realm_notifications_stream,
+    do_set_realm_property,
    do_set_realm_signup_notifications_stream,
 )
 from zerver.actions.streams import (
@ -440,30 +440,42 @@ class TestRealmAuditLog(ZulipTestCase):
        now = timezone_now()
        realm = get_realm("zulip")
        user = self.example_user("hamlet")
-        values_expected = [
-            {
-                "property": "message_content_edit_limit_seconds",
-                RealmAuditLog.OLD_VALUE: realm.message_content_edit_limit_seconds,
-                RealmAuditLog.NEW_VALUE: 1000,
-            },
-            {
-                "property": "edit_topic_policy",
-                RealmAuditLog.OLD_VALUE: Realm.POLICY_EVERYONE,
-                RealmAuditLog.NEW_VALUE: Realm.POLICY_ADMINS_ONLY,
-            },
-        ]
+        value_expected = {
+            RealmAuditLog.OLD_VALUE: realm.message_content_edit_limit_seconds,
+            RealmAuditLog.NEW_VALUE: 1000,
+            "property": "message_content_edit_limit_seconds",
+        }

-        do_set_realm_message_editing(realm, True, 1000, Realm.POLICY_ADMINS_ONLY, acting_user=user)
-        realm_audit_logs = RealmAuditLog.objects.filter(
-            realm=realm,
-            event_type=RealmAuditLog.REALM_PROPERTY_CHANGED,
-            event_time__gte=now,
-            acting_user=user,
-        ).order_by("id")
-        self.assertEqual(realm_audit_logs.count(), 2)
+        do_set_realm_property(realm, "message_content_edit_limit_seconds", 1000, acting_user=user)
        self.assertEqual(
-            [orjson.loads(assert_is_not_none(entry.extra_data)) for entry in realm_audit_logs],
-            values_expected,
+            RealmAuditLog.objects.filter(
+                realm=realm,
+                event_type=RealmAuditLog.REALM_PROPERTY_CHANGED,
+                event_time__gte=now,
+                acting_user=user,
+                extra_data=orjson.dumps(value_expected).decode(),
+            ).count(),
+            1,
+        )
+
+        value_expected = {
+            RealmAuditLog.OLD_VALUE: Realm.POLICY_EVERYONE,
+            RealmAuditLog.NEW_VALUE: Realm.POLICY_ADMINS_ONLY,
+            "property": "edit_topic_policy",
+        }
+
+        do_set_realm_property(
+            realm, "edit_topic_policy", Realm.POLICY_ADMINS_ONLY, acting_user=user
+        )
+        self.assertEqual(
+            RealmAuditLog.objects.filter(
+                realm=realm,
+                event_type=RealmAuditLog.REALM_PROPERTY_CHANGED,
+                event_time__gte=now,
+                acting_user=user,
+                extra_data=orjson.dumps(value_expected).decode(),
+            ).count(),
+            1,
        )

    def test_set_realm_notifications_stream(self) -> None:
--- a/zerver/tests/test_events.py
+++ b/zerver/tests/test_events.py
@ -70,7 +70,6 @@ from zerver.actions.realm_settings import (
    do_change_realm_plan_type,
    do_deactivate_realm,
    do_set_realm_authentication_methods,
-    do_set_realm_message_editing,
    do_set_realm_notifications_stream,
    do_set_realm_property,
    do_set_realm_signup_notifications_stream,
@ -1604,27 +1603,6 @@ class NormalActionsTest(BaseAction):
                    value=value,
                )

-    def test_change_realm_message_edit_settings(self) -> None:
-        # Test every transition among the four possibilities {T,F} x {0, non-0}
-        for (allow_message_editing, message_content_edit_limit_seconds) in (
-            (True, 0),
-            (False, 0),
-            (False, 1234),
-            (True, 600),
-            (False, 0),
-            (True, 1234),
-        ):
-            events = self.verify_action(
-                lambda: do_set_realm_message_editing(
-                    self.user_profile.realm,
-                    allow_message_editing,
-                    message_content_edit_limit_seconds,
-                    Realm.POLICY_ADMINS_ONLY,
-                    acting_user=None,
-                )
-            )
-            check_realm_update_dict("events[0]", events[0])
-
    def test_change_realm_notifications_stream(self) -> None:

        stream = get_stream("Rome", self.user_profile.realm)
@ -2551,6 +2529,8 @@ class RealmPropertyActionTest(BaseAction):
            move_messages_between_streams_policy=Realm.COMMON_POLICY_TYPES,
            add_custom_emoji_policy=Realm.COMMON_POLICY_TYPES,
            delete_own_message_policy=Realm.COMMON_MESSAGE_POLICY_TYPES,
+            edit_topic_policy=Realm.COMMON_MESSAGE_POLICY_TYPES,
+            message_content_edit_limit_seconds=[1000, 1100, 1200, None],
        )

        vals = test_values.get(name)
@ -2607,7 +2587,15 @@ class RealmPropertyActionTest(BaseAction):
                ).count(),
                1,
            )
-            check_realm_update("events[0]", events[0], name)
+
+            if name in [
+                "allow_message_editing",
+                "edit_topic_policy",
+                "message_content_edit_limit_seconds",
+            ]:
+                check_realm_update_dict("events[0]", events[0])
+            else:
+                check_realm_update("events[0]", events[0], name)

            if name == "email_address_visibility" and Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE in [
                old_value,
--- a/zerver/tests/test_realm.py
+++ b/zerver/tests/test_realm.py
@ -605,6 +605,8 @@ class RealmTest(ZulipTestCase):
            move_messages_between_streams_policy=10,
            add_custom_emoji_policy=10,
            delete_own_message_policy=10,
+            edit_topic_policy=10,
+            message_content_edit_limit_seconds=0,
        )

        # We need an admin user.
@ -1141,6 +1143,8 @@ class RealmAPITest(ZulipTestCase):
            move_messages_between_streams_policy=Realm.COMMON_POLICY_TYPES,
            add_custom_emoji_policy=Realm.COMMON_POLICY_TYPES,
            delete_own_message_policy=Realm.COMMON_MESSAGE_POLICY_TYPES,
+            edit_topic_policy=Realm.COMMON_MESSAGE_POLICY_TYPES,
+            message_content_edit_limit_seconds=[1000, 1100, 1200],
        )

        vals = test_values.get(name)
@ -1281,61 +1285,6 @@ class RealmAPITest(ZulipTestCase):
        self.assertIn("ignored_parameters_unsupported", result)
        self.assertEqual(result["ignored_parameters_unsupported"], ["emoji_set"])

-    def test_update_realm_allow_message_editing(self) -> None:
-        """Tests updating the realm property 'allow_message_editing'."""
-        self.set_up_db("allow_message_editing", False)
-        self.set_up_db("message_content_edit_limit_seconds", None)
-        self.set_up_db("edit_topic_policy", Realm.POLICY_ADMINS_ONLY)
-        realm = self.update_with_api("allow_message_editing", True)
-        realm = self.update_with_api("message_content_edit_limit_seconds", 100)
-        realm = self.update_with_api("edit_topic_policy", Realm.POLICY_EVERYONE)
-        self.assertEqual(realm.allow_message_editing, True)
-        self.assertEqual(realm.message_content_edit_limit_seconds, 100)
-        self.assertEqual(realm.edit_topic_policy, Realm.POLICY_EVERYONE)
-        realm = self.update_with_api("allow_message_editing", False)
-        self.assertEqual(realm.allow_message_editing, False)
-        self.assertEqual(realm.message_content_edit_limit_seconds, 100)
-        self.assertEqual(realm.edit_topic_policy, Realm.POLICY_EVERYONE)
-        realm = self.update_with_api("message_content_edit_limit_seconds", 200)
-        self.assertEqual(realm.allow_message_editing, False)
-        self.assertEqual(realm.message_content_edit_limit_seconds, 200)
-        self.assertEqual(realm.edit_topic_policy, Realm.POLICY_EVERYONE)
-        realm = self.update_with_api("edit_topic_policy", Realm.POLICY_ADMINS_ONLY)
-        self.assertEqual(realm.allow_message_editing, False)
-        self.assertEqual(realm.message_content_edit_limit_seconds, 200)
-        self.assertEqual(realm.edit_topic_policy, Realm.POLICY_ADMINS_ONLY)
-
-        realm = self.update_with_api("edit_topic_policy", Realm.POLICY_MODERATORS_ONLY)
-        self.assertEqual(realm.allow_message_editing, False)
-        self.assertEqual(realm.message_content_edit_limit_seconds, 200)
-        self.assertEqual(realm.edit_topic_policy, Realm.POLICY_MODERATORS_ONLY)
-
-        realm = self.update_with_api("edit_topic_policy", Realm.POLICY_FULL_MEMBERS_ONLY)
-        self.assertEqual(realm.allow_message_editing, False)
-        self.assertEqual(realm.message_content_edit_limit_seconds, 200)
-        self.assertEqual(realm.edit_topic_policy, Realm.POLICY_FULL_MEMBERS_ONLY)
-
-        realm = self.update_with_api("edit_topic_policy", Realm.POLICY_MEMBERS_ONLY)
-        self.assertEqual(realm.allow_message_editing, False)
-        self.assertEqual(realm.message_content_edit_limit_seconds, 200)
-        self.assertEqual(realm.edit_topic_policy, Realm.POLICY_MEMBERS_ONLY)
-
-        # Test an invalid value for edit_topic_policy
-        invalid_edit_topic_policy_value = 10
-        req = {"edit_topic_policy": orjson.dumps(invalid_edit_topic_policy_value).decode()}
-        result = self.client_patch("/json/realm", req)
-        self.assert_json_error(result, "Invalid edit_topic_policy")
-
-        # Test 0 is invalid value of message_content_edit_limit_seconds
-        invalid_message_content_edit_limit_seconds_value = 0
-        req = {
-            "message_content_edit_limit_seconds": orjson.dumps(
-                invalid_message_content_edit_limit_seconds_value
-            ).decode()
-        }
-        result = self.client_patch("/json/realm", req)
-        self.assert_json_error(result, "Bad value for 'message_content_edit_limit_seconds': 0")
-
    def test_update_realm_delete_own_message_policy(self) -> None:
        """Tests updating the realm property 'delete_own_message_policy'."""
        self.set_up_db("delete_own_message_policy", Realm.POLICY_EVERYONE)
--- a/zerver/views/realm.py
+++ b/zerver/views/realm.py
@ -13,7 +13,6 @@ from zerver.actions.realm_settings import (
    do_deactivate_realm,
    do_reactivate_realm,
    do_set_realm_authentication_methods,
-    do_set_realm_message_editing,
    do_set_realm_notifications_stream,
    do_set_realm_property,
    do_set_realm_signup_notifications_stream,
@ -217,6 +216,29 @@ def update_realm(
            )
            data["message_content_delete_limit_seconds"] = message_content_delete_limit_seconds

+    message_content_edit_limit_seconds: Optional[int] = None
+    if message_content_edit_limit_seconds_raw is not None:
+        message_content_edit_limit_seconds = parse_message_content_edit_or_delete_limit(
+            message_content_edit_limit_seconds_raw,
+            Realm.MESSAGE_CONTENT_EDIT_OR_DELETE_LIMIT_SPECIAL_VALUES_MAP,
+            setting_name="message_content_edit_limit_seconds",
+        )
+
+        if (
+            message_content_edit_limit_seconds is None
+            and realm.message_content_edit_limit_seconds is not None
+        ):
+            # We handle 'None' here separately, since in the loop below,
+            # do_set_realm_property is called only if setting value is
+            # not None.
+            do_set_realm_property(
+                realm,
+                "message_content_edit_limit_seconds",
+                message_content_edit_limit_seconds,
+                acting_user=user_profile,
+            )
+            data["message_content_edit_limit_seconds"] = message_content_edit_limit_seconds
+
    # The user of `locals()` here is a bit of a code smell, but it's
    # restricted to the elements present in realm.property_types.
    #
@ -252,29 +274,6 @@ def update_realm(
            setting_name="message_content_edit_limit_seconds",
        )

-    if (
-        (allow_message_editing is not None and realm.allow_message_editing != allow_message_editing)
-        or (
-            message_content_edit_limit_seconds_raw is not None
-            and message_content_edit_limit_seconds != realm.message_content_edit_limit_seconds
-        )
-        or (edit_topic_policy is not None and realm.edit_topic_policy != edit_topic_policy)
-    ):
-        if allow_message_editing is None:
-            allow_message_editing = realm.allow_message_editing
-        if edit_topic_policy is None:
-            edit_topic_policy = realm.edit_topic_policy
-        do_set_realm_message_editing(
-            realm,
-            allow_message_editing,
-            message_content_edit_limit_seconds,
-            edit_topic_policy,
-            acting_user=user_profile,
-        )
-        data["allow_message_editing"] = allow_message_editing
-        data["message_content_edit_limit_seconds"] = message_content_edit_limit_seconds
-        data["edit_topic_policy"] = edit_topic_policy
-
    # Realm.notifications_stream and Realm.signup_notifications_stream are not boolean,
    # str or integer field, and thus doesn't fit into the do_set_realm_property framework.
    if notifications_stream_id is not None: