diff --git a/docs/tutorials/new-feature-tutorial.md b/docs/tutorials/new-feature-tutorial.md index 6a169a34bb..23af3550ed 100644 --- 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, # ... ) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 4af7b9f842..fec5904a2c 100644 --- 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 diff --git a/version.py b/version.py index 213c7b53ec..5c4fc65c7d 100644 --- 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 diff --git a/zerver/actions/realm_settings.py b/zerver/actions/realm_settings.py index e5be6c82b4..efa8052c54 100644 --- 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"], diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index e8fa731776..5b34087f36 100644 --- 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: diff --git a/zerver/models.py b/zerver/models.py index 94d107978f..d751dfa031 100644 --- 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, diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 32480cd5e9..a9523b8be4 100644 --- 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 diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index e3d3eac9ac..cf6121cf8a 100644 --- 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: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 8f52b23f8f..4d7bd990d3 100644 --- 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, diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 9393d01a71..d871282d16 100644 --- 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) diff --git a/zerver/views/realm.py b/zerver/views/realm.py index 8c9660b819..62bb5287c5 100644 --- 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: