From d0c5c1cacbb2fda41d599df5a82c3283b266cec2 Mon Sep 17 00:00:00 2001 From: Saubhagya Patel Date: Tue, 25 Feb 2025 23:27:00 +0530 Subject: [PATCH] settings: Add backend to change allow_edit_history to integer field. This commit implements the backend of migrating the `allow_edit_history` setting to `message_edit_history_visibility_policy`. This allows organizations, to have an intermediate setting to view only the "Moves" history of the messages. We still pass `realm_allow_edit_history` in `/register` response though for older clients with its value being set depending on the value of `realm_message_edit_history_visibility_policy`. We set `realm_allow_edit_history` to `False` if the `realm_message_edit_history_visibility_policy` is "None", and `True` for "Moves only" or "All" message edit history. Fixes part of #21398. Co-authored-by: Shlok Patel Co-authored-by: Tim Abbott --- api_docs/changelog.md | 11 ++ version.py | 2 +- zerver/actions/message_edit.py | 2 +- zerver/actions/message_summary.py | 3 +- zerver/actions/realm_settings.py | 25 ++- zerver/lib/event_schema.py | 7 +- zerver/lib/events.py | 18 ++ zerver/lib/message.py | 43 ++++- ..._message_edit_history_visibility_policy.py | 17 ++ ..._message_edit_history_visibility_policy.py | 30 ++++ .../0678_remove_realm_allow_edit_history.py | 16 ++ zerver/models/realms.py | 16 +- zerver/openapi/zulip.yaml | 40 ++++- zerver/tests/test_events.py | 21 ++- zerver/tests/test_home.py | 1 + zerver/tests/test_message_dict.py | 16 +- zerver/tests/test_message_edit.py | 156 +++++++++++++++++- zerver/tests/test_realm.py | 20 +++ zerver/views/message_edit.py | 27 ++- zerver/views/message_fetch.py | 2 +- zerver/views/realm.py | 19 ++- 21 files changed, 446 insertions(+), 46 deletions(-) create mode 100644 zerver/migrations/0676_realm_message_edit_history_visibility_policy.py create mode 100644 zerver/migrations/0677_set_default_message_edit_history_visibility_policy.py create mode 100644 zerver/migrations/0678_remove_realm_allow_edit_history.py diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 1348e73ab5..8b5a0fb446 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,17 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 358** + +* `PATCH /realm`, [`GET /events`](/api/get-events): Changed `allow_edit_history` + boolean field to `message_edit_history_visibility_policy` integer field to + support an intermediate field for `Moves only` edit history of messages. +* [`POST /register`](/api/register-queue): `realm_allow_edit_history` field is + deprecated and has been replaced by `realm_message_edit_history_visibility_policy`. + The value of `realm_allow_edit_history` is set to `False` if + `realm_message_edit_history_visibility_policy` is configured as "None", + and `True` for "Moves only" or "All" message edit history. + **Feature level 357** * [`GET /users/me/subscriptions`](/api/get-subscriptions), diff --git a/version.py b/version.py index 4ca00cdc1a..24f722ed44 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 357 # Last bumped for adding 'can_subscribe_group' +API_FEATURE_LEVEL = 358 # 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/message_edit.py b/zerver/actions/message_edit.py index 6eb9f03401..bd54249218 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -458,7 +458,7 @@ def update_message_content( members = mention_data.get_group_members(group_id) rendering_result.mentions_user_ids.update(members) - # One could imagine checking realm.allow_edit_history here and + # One could imagine checking realm.message_edit_history_visibility_policy here and # modifying the events based on that setting, but doing so # doesn't really make sense. We need to send the edit event # to clients regardless, and a client already had access to diff --git a/zerver/actions/message_summary.py b/zerver/actions/message_summary.py index 91172da892..b1669dd744 100644 --- a/zerver/actions/message_summary.py +++ b/zerver/actions/message_summary.py @@ -15,6 +15,7 @@ from zerver.lib.narrow import ( fetch_messages, ) from zerver.models import UserProfile +from zerver.models.realms import MessageEditHistoryVisibilityPolicyEnum # Maximum number of messages that can be summarized in a single request. MAX_MESSAGES_SUMMARIZED = 100 @@ -120,7 +121,7 @@ def do_summarize_narrow( client_gravatar=True, allow_empty_topic_name=False, # Avoid fetching edit history, which won't be passed to the model. - allow_edit_history=False, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.none.value, user_profile=user_profile, realm=user_profile.realm, ) diff --git a/zerver/actions/realm_settings.py b/zerver/actions/realm_settings.py index ae5d276f67..797aa9c71f 100644 --- a/zerver/actions/realm_settings.py +++ b/zerver/actions/realm_settings.py @@ -2,6 +2,7 @@ import datetime import logging import zoneinfo from email.headerregistry import Address +from enum import Enum from typing import Any, Literal from django.conf import settings @@ -50,24 +51,33 @@ from zerver.models import ( ) from zerver.models.groups import SystemGroups from zerver.models.realm_audit_logs import AuditLogEventType -from zerver.models.realms import get_default_max_invites_for_realm_plan_type, get_realm +from zerver.models.realms import ( + MessageEditHistoryVisibilityPolicyEnum, + get_default_max_invites_for_realm_plan_type, + get_realm, +) from zerver.models.users import active_user_ids from zerver.tornado.django_api import send_event_on_commit @transaction.atomic(savepoint=False) def do_set_realm_property( - realm: Realm, name: str, value: Any, *, acting_user: UserProfile | None + realm: Realm, name: str, raw_value: Any, *, acting_user: UserProfile | None ) -> None: """Takes in a realm object, the name of an attribute to update, the value to update and the user who initiated the update. """ property_type = Realm.property_types[name] - assert isinstance(value, property_type), ( - f"Cannot update {name}: {value} is not an instance of {property_type}" + assert isinstance(raw_value, property_type), ( + f"Cannot update {name}: {raw_value} is not an instance of {property_type}" ) old_value = getattr(realm, name) + if isinstance(raw_value, Enum): + value = raw_value.value + else: + value = raw_value + if old_value == value: return @@ -93,6 +103,13 @@ def do_set_realm_property( property="default", data={name: value}, ) + if name == "message_edit_history_visibility_policy": + event = dict( + type="realm", + op="update", + property=name, + value=MessageEditHistoryVisibilityPolicyEnum(value).name, + ) send_event_on_commit(realm, event, active_user_ids(realm.id)) diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index bb71d4b4ad..3d87cc5505 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -5,7 +5,9 @@ # by a test in test_events.py with a schema checker here. # # See https://zulip.readthedocs.io/en/latest/subsystems/events-system.html +import inspect from collections.abc import Callable +from enum import Enum from pprint import PrettyPrinter from typing import cast @@ -426,7 +428,10 @@ def check_realm_update( return property_type = Realm.property_types[prop] - assert isinstance(value, property_type) + if inspect.isclass(property_type) and issubclass(property_type, Enum): + assert isinstance(value, str) + else: + assert isinstance(value, property_type) def check_realm_default_update( diff --git a/zerver/lib/events.py b/zerver/lib/events.py index a361019d95..4abec539de 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -97,6 +97,7 @@ from zerver.models.linkifiers import linkifiers_for_realm from zerver.models.realm_emoji import get_all_custom_emoji_for_realm from zerver.models.realm_playgrounds import get_realm_playgrounds from zerver.models.realms import ( + MessageEditHistoryVisibilityPolicyEnum, get_corresponding_policy_value_for_group_setting, get_realm_domains, get_realm_with_settings, @@ -474,6 +475,18 @@ def fetch_initial_state_data( ) state["realm_empty_topic_display_name"] = Message.EMPTY_TOPIC_FALLBACK_NAME + + state["realm_allow_edit_history"] = ( + realm.message_edit_history_visibility_policy + != MessageEditHistoryVisibilityPolicyEnum.none.value + ) + + state["realm_message_edit_history_visibility_policy"] = ( + MessageEditHistoryVisibilityPolicyEnum( + realm.message_edit_history_visibility_policy + ).name + ) + if want("realm_user_settings_defaults"): realm_user_default = RealmUserDefault.objects.get(realm=realm) state["realm_user_settings_defaults"] = {} @@ -1311,6 +1324,11 @@ def apply_event( else state["server_jitsi_server_url"] ) + if field == "realm_message_edit_history_visibility_policy": + state["realm_allow_edit_history"] = ( + event["value"] != MessageEditHistoryVisibilityPolicyEnum.none.name + ) + elif event["op"] == "update_dict": for key, value in event["data"].items(): if key == "max_file_upload_size_mib": diff --git a/zerver/lib/message.py b/zerver/lib/message.py index fd39ca20e7..4f832ef352 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -38,7 +38,7 @@ from zerver.lib.topic import ( maybe_rename_general_chat_to_empty_topic, messages_for_topic, ) -from zerver.lib.types import UserDisplayRecipient +from zerver.lib.types import FormattedEditHistoryEvent, UserDisplayRecipient from zerver.lib.user_groups import ( UserGroupMembershipDetails, get_recursive_membership_groups, @@ -60,6 +60,7 @@ from zerver.models import ( from zerver.models.constants import MAX_TOPIC_NAME_LENGTH from zerver.models.groups import SystemGroups from zerver.models.messages import get_usermessage_by_message_id +from zerver.models.realms import MessageEditHistoryVisibilityPolicyEnum from zerver.models.users import is_cross_realm_bot_email @@ -218,6 +219,29 @@ def truncate_topic(topic_name: str) -> str: return truncate_content(topic_name, MAX_TOPIC_NAME_LENGTH, "...") +def visible_edit_history_for_message( + message_edit_history_visibility_policy: int, + edit_history: list[FormattedEditHistoryEvent], +) -> list[FormattedEditHistoryEvent]: + # Makes sure that we send message edit history to clients + # in realms as per `message_edit_history_visibility_policy`. + if message_edit_history_visibility_policy == MessageEditHistoryVisibilityPolicyEnum.all.value: + return edit_history + + visible_edit_history: list[FormattedEditHistoryEvent] = [] + for edit_history_event in edit_history: + if "prev_content" in edit_history_event: + if "prev_topic" in edit_history_event: + del edit_history_event["prev_content"] + del edit_history_event["prev_rendered_content"] + del edit_history_event["content_html_diff"] + else: + continue + visible_edit_history.append(edit_history_event) + + return visible_edit_history + + def messages_for_ids( message_ids: list[int], user_message_flags: dict[int, list[str]], @@ -225,7 +249,7 @@ def messages_for_ids( apply_markdown: bool, client_gravatar: bool, allow_empty_topic_name: bool, - allow_edit_history: bool, + message_edit_history_visibility_policy: int, user_profile: UserProfile | None, realm: Realm, ) -> list[dict[str, Any]]: @@ -259,10 +283,17 @@ def messages_for_ids( msg_dict.update(flags=flags) if message_id in search_fields: msg_dict.update(search_fields[message_id]) - # Make sure that we never send message edit history to clients - # in realms with allow_edit_history disabled. - if "edit_history" in msg_dict and not allow_edit_history: - del msg_dict["edit_history"] + if "edit_history" in msg_dict: + if ( + message_edit_history_visibility_policy + == MessageEditHistoryVisibilityPolicyEnum.none.value + ): + del msg_dict["edit_history"] + else: + visible_edit_history = visible_edit_history_for_message( + message_edit_history_visibility_policy, msg_dict["edit_history"] + ) + msg_dict["edit_history"] = visible_edit_history msg_dict["can_access_sender"] = msg_dict["sender_id"] not in inaccessible_sender_ids message_list.append(msg_dict) diff --git a/zerver/migrations/0676_realm_message_edit_history_visibility_policy.py b/zerver/migrations/0676_realm_message_edit_history_visibility_policy.py new file mode 100644 index 0000000000..fa0f34c4c2 --- /dev/null +++ b/zerver/migrations/0676_realm_message_edit_history_visibility_policy.py @@ -0,0 +1,17 @@ +# Generated by Django 5.0.10 on 2025-02-16 10:41 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0675_alter_stream_can_subscribe_group"), + ] + + operations = [ + migrations.AddField( + model_name="realm", + name="message_edit_history_visibility_policy", + field=models.PositiveSmallIntegerField(default=1), + ), + ] diff --git a/zerver/migrations/0677_set_default_message_edit_history_visibility_policy.py b/zerver/migrations/0677_set_default_message_edit_history_visibility_policy.py new file mode 100644 index 0000000000..e7e6ddc751 --- /dev/null +++ b/zerver/migrations/0677_set_default_message_edit_history_visibility_policy.py @@ -0,0 +1,30 @@ +from django.db import migrations +from django.db.backends.base.schema import BaseDatabaseSchemaEditor +from django.db.migrations.state import StateApps + + +def set_value_for_message_edit_history_visibility_policy( + apps: StateApps, schema_editor: BaseDatabaseSchemaEditor +) -> None: + VISIBILITY_ALL = 1 + VISIBILITY_NONE = 3 + + Realm = apps.get_model("zerver", "Realm") + Realm.objects.filter(allow_edit_history=True).update( + message_edit_history_visibility_policy=VISIBILITY_ALL + ) + Realm.objects.filter(allow_edit_history=False).update( + message_edit_history_visibility_policy=VISIBILITY_NONE + ) + + +class Migration(migrations.Migration): + atomic = False + + dependencies = [ + ("zerver", "0676_realm_message_edit_history_visibility_policy"), + ] + + operations = [ + migrations.RunPython(set_value_for_message_edit_history_visibility_policy), + ] diff --git a/zerver/migrations/0678_remove_realm_allow_edit_history.py b/zerver/migrations/0678_remove_realm_allow_edit_history.py new file mode 100644 index 0000000000..202902bb10 --- /dev/null +++ b/zerver/migrations/0678_remove_realm_allow_edit_history.py @@ -0,0 +1,16 @@ +# Generated by Django 5.0.10 on 2025-02-16 10:47 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0677_set_default_message_edit_history_visibility_policy"), + ] + + operations = [ + migrations.RemoveField( + model_name="realm", + name="allow_edit_history", + ), + ] diff --git a/zerver/models/realms.py b/zerver/models/realms.py index 7f9814ddd5..b8f48ae55f 100644 --- a/zerver/models/realms.py +++ b/zerver/models/realms.py @@ -1,5 +1,5 @@ from email.headerregistry import Address -from enum import IntEnum +from enum import Enum, IntEnum from types import UnionType from typing import TYPE_CHECKING, Optional, TypedDict from uuid import uuid4 @@ -148,6 +148,13 @@ class DigestWeekdayEnum(IntEnum): SUNDAY = 6 +class MessageEditHistoryVisibilityPolicyEnum(Enum): + # The case is used by Pydantic in the API + all = 1 + moves = 2 + none = 3 + + class Realm(models.Model): # type: ignore[django-manager-missing] # django-stubs cannot resolve the custom CTEManager yet https://github.com/typeddjango/django-stubs/issues/1023 MAX_REALM_NAME_LENGTH = 40 MAX_REALM_DESCRIPTION_LENGTH = 1000 @@ -390,7 +397,10 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub ) # Whether users have access to message edit history - allow_edit_history = models.BooleanField(default=True) + message_edit_history_visibility_policy = models.PositiveSmallIntegerField( + default=MessageEditHistoryVisibilityPolicyEnum.all.value, + ) + MESSAGE_EDIT_HISTORY_VISIBILITY_POLICY_TYPES = list(MessageEditHistoryVisibilityPolicyEnum) # Defaults for new users default_language = models.CharField(default="en", max_length=MAX_LANGUAGE_ID_LENGTH) @@ -651,7 +661,6 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub # Define the types of the various automatically managed properties property_types: dict[str, type | UnionType] = dict( - allow_edit_history=bool, allow_message_editing=bool, avatar_changes_disabled=bool, default_code_block_language=str, @@ -675,6 +684,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub message_content_allowed_in_email_notifications=bool, message_content_edit_limit_seconds=int | None, message_content_delete_limit_seconds=int | None, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum, move_messages_between_streams_limit_seconds=int | None, move_messages_within_stream_limit_seconds=int | None, message_retention_days=int, diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 74d5f4ba60..618ad6557a 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -4578,11 +4578,6 @@ paths: irrespective of which of these settings were changed. Now, a separate event is sent for each changed setting. properties: - allow_edit_history: - type: boolean - description: | - Whether this organization is configured to allow users to access - [message edit history](/help/view-a-messages-edit-history). allow_message_editing: type: boolean description: | @@ -5062,6 +5057,19 @@ paths: **Changes**: Before Zulip 6.0 (feature level 138), no limit was represented using the special value `0`. + message_edit_history_visibility_policy: + type: string + description: | + Which type of message edit history is configured to allow users to + access [message edit history](/help/view-a-messages-edit-history). + + - "all" = All edit history is visible. + - "moves" = Only moves are visible. + - "none" = No edit history is visible. + + **Changes**: New in Zulip 10.0 (feature level 358), replacing the previous + `allow_edit_history` boolean setting; `true` corresponds to `all`, + and `false` to `none`. moderation_request_channel_id: type: integer description: | @@ -16895,13 +16903,35 @@ paths: type: string description: | The text describing the emoji set. + realm_message_edit_history_visibility_policy: + type: string + description: | + What typesof message edit history are accessible to users via + [message edit history](/help/view-a-messages-edit-history). + + - "all" = All edit history is visible. + - "moves" = Only moves are visible. + - "none" = No edit history is visible. + + **Changes**: New in Zulip 10.0 (feature level 358), replacing the previous + `allow_edit_history` boolean setting; `true` corresponds to `all`, + and `false` to `none`. realm_allow_edit_history: + deprecated: true type: boolean description: | Present if `realm` is present in `fetch_event_types`. Whether this organization is configured to allow users to access [message edit history](/help/view-a-messages-edit-history). + + The value of `realm_allow_edit_history` is set as `false` if the + `realm_message_edit_history_visibility_policy` is configured as "None" + and `true` if it is configured as "Moves only" or "All". + + **Changes**: Deprecated in Zulip 10.0 (feature level 358) and will be + removed in the future, as it is an inaccurate version + `realm_message_edit_history_visibility_policy`, which replaces this field. realm_can_add_custom_emoji_group: allOf: - description: | diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index b880ed75ea..f722b55ea0 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -9,6 +9,7 @@ import time from collections.abc import Iterator from contextlib import contextmanager from datetime import timedelta +from enum import Enum from io import StringIO from typing import Any from unittest import mock @@ -3846,6 +3847,7 @@ class RealmPropertyActionTest(BaseAction): default_language=["es", "de", "en"], description=["Realm description", "New description"], digest_weekday=[0, 1, 2], + message_edit_history_visibility_policy=Realm.MESSAGE_EDIT_HISTORY_VISIBILITY_POLICY_TYPES, message_retention_days=[10, 20], name=["Zulip", "New Name"], waiting_period_threshold=[1000, 2000], @@ -3875,7 +3877,9 @@ class RealmPropertyActionTest(BaseAction): do_set_realm_property(self.user_profile.realm, name, vals[0], acting_user=self.user_profile) - if vals[0] != original_val: + if vals[0] != original_val and not ( + isinstance(vals[0], Enum) and vals[0].value == original_val + ): self.assertEqual( RealmAuditLog.objects.filter( realm=self.user_profile.realm, @@ -3885,11 +3889,18 @@ class RealmPropertyActionTest(BaseAction): ).count(), 1, ) - for count, val in enumerate(vals[1:]): + for count, raw_value in enumerate(vals[1:]): now = timezone_now() state_change_expected = True - old_value = vals[count] num_events = 1 + raw_old_value = vals[count] + + if isinstance(raw_value, Enum): + value = raw_value.value + old_value = raw_old_value.value + else: + value = raw_value + old_value = raw_old_value with self.verify_action( state_change_expected=state_change_expected, num_events=num_events @@ -3897,7 +3908,7 @@ class RealmPropertyActionTest(BaseAction): do_set_realm_property( self.user_profile.realm, name, - val, + raw_value, acting_user=self.user_profile, ) @@ -3909,7 +3920,7 @@ class RealmPropertyActionTest(BaseAction): acting_user=self.user_profile, extra_data={ RealmAuditLog.OLD_VALUE: old_value, - RealmAuditLog.NEW_VALUE: val, + RealmAuditLog.NEW_VALUE: value, "property": name, }, ).count(), diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index a77d92abe1..5ad8721ab0 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -188,6 +188,7 @@ class HomeTest(ZulipTestCase): "realm_message_content_allowed_in_email_notifications", "realm_message_content_delete_limit_seconds", "realm_message_content_edit_limit_seconds", + "realm_message_edit_history_visibility_policy", "realm_message_retention_days", "realm_move_messages_between_streams_limit_seconds", "realm_move_messages_within_stream_limit_seconds", diff --git a/zerver/tests/test_message_dict.py b/zerver/tests/test_message_dict.py index e51ba5ca52..9482d4a706 100644 --- a/zerver/tests/test_message_dict.py +++ b/zerver/tests/test_message_dict.py @@ -14,7 +14,7 @@ from zerver.lib.test_helpers import make_client from zerver.lib.topic import TOPIC_LINKS from zerver.lib.types import DisplayRecipientT, UserDisplayRecipient from zerver.models import Message, Reaction, Realm, RealmFilter, Recipient, Stream, UserProfile -from zerver.models.realms import get_realm +from zerver.models.realms import MessageEditHistoryVisibilityPolicyEnum, get_realm from zerver.models.streams import get_stream @@ -421,7 +421,7 @@ class MessageHydrationTest(ZulipTestCase): apply_markdown=True, client_gravatar=True, allow_empty_topic_name=True, - allow_edit_history=False, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.none.value, user_profile=cordelia, realm=cordelia.realm, ) @@ -470,7 +470,7 @@ class MessageHydrationTest(ZulipTestCase): apply_markdown=True, client_gravatar=True, allow_empty_topic_name=True, - allow_edit_history=False, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.none.value, user_profile=self.example_user("polonius"), realm=realm, ) @@ -516,7 +516,7 @@ class MessageHydrationTest(ZulipTestCase): apply_markdown=True, client_gravatar=True, allow_empty_topic_name=True, - allow_edit_history=False, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.none.value, user_profile=cordelia, realm=cordelia.realm, ) @@ -562,7 +562,7 @@ class TestMessageForIdsDisplayRecipientFetching(ZulipTestCase): apply_markdown=True, client_gravatar=True, allow_empty_topic_name=True, - allow_edit_history=False, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.none.value, user_profile=cordelia, realm=cordelia.realm, ) @@ -586,7 +586,7 @@ class TestMessageForIdsDisplayRecipientFetching(ZulipTestCase): apply_markdown=True, client_gravatar=True, allow_empty_topic_name=True, - allow_edit_history=False, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.none.value, user_profile=cordelia, realm=cordelia.realm, ) @@ -611,7 +611,7 @@ class TestMessageForIdsDisplayRecipientFetching(ZulipTestCase): apply_markdown=True, client_gravatar=True, allow_empty_topic_name=True, - allow_edit_history=False, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.none.value, user_profile=cordelia, realm=cordelia.realm, ) @@ -648,7 +648,7 @@ class TestMessageForIdsDisplayRecipientFetching(ZulipTestCase): apply_markdown=True, client_gravatar=True, allow_empty_topic_name=True, - allow_edit_history=False, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.none.value, user_profile=cordelia, realm=cordelia.realm, ) diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 986d9cca60..a5d83a8964 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -23,7 +23,7 @@ from zerver.lib.utils import assert_is_not_none from zerver.models import Attachment, Message, NamedUserGroup, Realm, UserProfile, UserTopic from zerver.models.groups import SystemGroups from zerver.models.messages import UserMessage -from zerver.models.realms import get_realm +from zerver.models.realms import MessageEditHistoryVisibilityPolicyEnum, get_realm from zerver.models.streams import get_stream @@ -50,7 +50,7 @@ class EditMessageTest(ZulipTestCase): apply_markdown=False, client_gravatar=False, allow_empty_topic_name=True, - allow_edit_history=True, + message_edit_history_visibility_policy=MessageEditHistoryVisibilityPolicyEnum.all.value, user_profile=None, realm=msg.realm, ) @@ -513,7 +513,12 @@ class EditMessageTest(ZulipTestCase): def test_edit_message_history_disabled(self) -> None: user_profile = self.example_user("hamlet") - do_set_realm_property(user_profile.realm, "allow_edit_history", False, acting_user=None) + do_set_realm_property( + user_profile.realm, + "message_edit_history_visibility_policy", + MessageEditHistoryVisibilityPolicyEnum.none, + acting_user=None, + ) self.login("hamlet") # Single-line edit @@ -945,6 +950,151 @@ class EditMessageTest(ZulipTestCase): self.assertEqual(message_history[6]["content"], "content 1") self.assertEqual(message_history[6]["topic"], "topic 1") + def test_visible_edit_history_for_message(self) -> None: + user = self.example_user("hamlet") + self.login("hamlet") + stream_1 = self.make_stream("stream 1") + stream_2 = self.make_stream("stream 2") + stream_3 = self.make_stream("stream 3") + self.subscribe(user, stream_1.name) + self.subscribe(user, stream_2.name) + msg_id = self.send_stream_message( + self.example_user("hamlet"), + "stream 1", + topic_name="topic 1", + content="content 1", + ) + + result_1 = self.client_patch( + f"/json/messages/{msg_id}", + { + "content": "content 2", + }, + ) + self.assert_json_success(result_1) + + result_2 = self.client_patch( + f"/json/messages/{msg_id}", + { + "topic": "topic 2", + }, + ) + self.assert_json_success(result_2) + + result_3 = self.client_patch( + f"/json/messages/{msg_id}", + { + "stream_id": stream_2.id, + }, + ) + self.assert_json_success(result_3) + + result_4 = self.client_patch( + f"/json/messages/{msg_id}", + { + "topic": "topic 3", + "content": "content 3", + }, + ) + self.assert_json_success(result_4) + + result_5 = self.client_patch( + f"/json/messages/{msg_id}", + { + "topic": "topic 4", + "stream_id": stream_3.id, + }, + ) + self.assert_json_success(result_5) + + do_set_realm_property( + user.realm, + "message_edit_history_visibility_policy", + MessageEditHistoryVisibilityPolicyEnum.none, + acting_user=None, + ) + result_message_edit_history_none = self.client_get(f"/json/messages/{msg_id}/history") + self.assert_json_error( + result_message_edit_history_none, + "Message edit history is disabled in this organization", + ) + + do_set_realm_property( + user.realm, + "message_edit_history_visibility_policy", + MessageEditHistoryVisibilityPolicyEnum.moves, + acting_user=None, + ) + result_message_edit_history_moves_only = self.client_get(f"/json/messages/{msg_id}/history") + json_response = orjson.loads(result_message_edit_history_moves_only.content) + message_edit_history_moves_only = json_response["message_history"] + + for edit_history_entry in message_edit_history_moves_only: + self.assertNotIn("prev_content", edit_history_entry) + self.assertNotIn("prev_rendered_content", edit_history_entry) + self.assertNotIn("content_html_diff", edit_history_entry) + + self.assert_length(message_edit_history_moves_only, 5) + self.assertEqual(message_edit_history_moves_only[0]["content"], "content 1") + self.assertEqual(message_edit_history_moves_only[0]["topic"], "topic 1") + + self.assertEqual(message_edit_history_moves_only[1]["content"], "content 2") + self.assertEqual(message_edit_history_moves_only[1]["topic"], "topic 2") + self.assertEqual(message_edit_history_moves_only[1]["prev_topic"], "topic 1") + + self.assertEqual(message_edit_history_moves_only[2]["content"], "content 2") + self.assertEqual(message_edit_history_moves_only[2]["topic"], "topic 2") + self.assertEqual(message_edit_history_moves_only[2]["stream"], stream_2.id) + self.assertEqual(message_edit_history_moves_only[2]["prev_stream"], stream_1.id) + + self.assertEqual(message_edit_history_moves_only[3]["content"], "content 3") + self.assertEqual(message_edit_history_moves_only[3]["topic"], "topic 3") + self.assertEqual(message_edit_history_moves_only[3]["prev_topic"], "topic 2") + + self.assertEqual(message_edit_history_moves_only[4]["content"], "content 3") + self.assertEqual(message_edit_history_moves_only[4]["topic"], "topic 4") + self.assertEqual(message_edit_history_moves_only[4]["prev_topic"], "topic 3") + self.assertEqual(message_edit_history_moves_only[4]["stream"], stream_3.id) + self.assertEqual(message_edit_history_moves_only[4]["prev_stream"], stream_2.id) + + do_set_realm_property( + user.realm, + "message_edit_history_visibility_policy", + MessageEditHistoryVisibilityPolicyEnum.all, + acting_user=None, + ) + result_message_edit_history_all = self.client_get(f"/json/messages/{msg_id}/history") + json_response = orjson.loads(result_message_edit_history_all.content) + message_edit_history_all = json_response["message_history"] + + self.assert_length(message_edit_history_all, 6) + self.assertEqual(message_edit_history_all[0]["content"], "content 1") + self.assertEqual(message_edit_history_all[0]["topic"], "topic 1") + + self.assertEqual(message_edit_history_all[1]["content"], "content 2") + self.assertEqual(message_edit_history_all[1]["prev_content"], "content 1") + self.assertEqual(message_edit_history_all[1]["topic"], "topic 1") + + self.assertEqual(message_edit_history_all[2]["content"], "content 2") + self.assertEqual(message_edit_history_all[2]["topic"], "topic 2") + self.assertEqual(message_edit_history_all[2]["prev_topic"], "topic 1") + + self.assertEqual(message_edit_history_all[3]["content"], "content 2") + self.assertEqual(message_edit_history_all[3]["topic"], "topic 2") + self.assertEqual(message_edit_history_all[3]["stream"], stream_2.id) + self.assertEqual(message_edit_history_all[3]["prev_stream"], stream_1.id) + + self.assertEqual(message_edit_history_all[4]["content"], "content 3") + self.assertEqual(message_edit_history_all[4]["prev_content"], "content 2") + self.assertEqual(message_edit_history_all[4]["topic"], "topic 3") + self.assertEqual(message_edit_history_all[4]["prev_topic"], "topic 2") + + self.assertEqual(message_edit_history_all[5]["content"], "content 3") + self.assertEqual(message_edit_history_all[5]["topic"], "topic 4") + self.assertEqual(message_edit_history_all[5]["prev_topic"], "topic 3") + self.assertEqual(message_edit_history_all[5]["stream"], stream_3.id) + self.assertEqual(message_edit_history_all[5]["prev_stream"], stream_2.id) + def test_edit_message_content_limit(self) -> None: def set_message_editing_params( allow_message_editing: bool, diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 5e58a6ad15..55d38f948b 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -4,6 +4,7 @@ import random import re import string from datetime import datetime, timedelta +from enum import Enum from typing import Any from unittest import mock, skipUnless @@ -957,6 +958,7 @@ class RealmTest(ZulipTestCase): waiting_period_threshold=-10, digest_weekday=10, message_content_delete_limit_seconds=-10, + message_edit_history_visibility_policy=10, message_content_edit_limit_seconds=0, move_messages_within_stream_limit_seconds=0, move_messages_between_streams_limit_seconds=0, @@ -1926,6 +1928,7 @@ class RealmAPITest(ZulipTestCase): default_code_block_language=["javascript", ""], description=["Realm description", "New description"], digest_weekday=[0, 1, 2], + message_edit_history_visibility_policy=Realm.MESSAGE_EDIT_HISTORY_VISIBILITY_POLICY_TYPES, message_retention_days=[10, 20], name=["Zulip", "New Name"], waiting_period_threshold=[10, 20], @@ -1962,6 +1965,16 @@ class RealmAPITest(ZulipTestCase): do_set_realm_property(get_realm("zulip"), name, vals[0], acting_user=None) + if isinstance(vals[0], Enum): + for val in vals[1:]: + raw_value = val.name + value = val.value + realm = self.update_with_api(name, raw_value) + self.assertEqual(getattr(realm, name), value) + realm = self.update_with_api(name, vals[0].name) + self.assertEqual(getattr(realm, name), vals[0].value) + return + for val in vals[1:]: realm = self.update_with_api(name, val) self.assertEqual(getattr(realm, name), val) @@ -2317,6 +2330,13 @@ class RealmAPITest(ZulipTestCase): result = self.client_patch("/json/realm", {"org_type": invalid_org_type}) self.assert_json_error(result, "Invalid org_type") + def test_invalid_edit_history_visibility(self) -> None: + result = self.client_patch( + "/json/realm", + {"message_edit_history_visibility_policy": "invalid"}, + ) + self.assert_json_error(result, "Invalid message_edit_history_visibility_policy") + def update_with_realm_default_api(self, name: str, val: Any) -> None: if not isinstance(val, str): val = orjson.dumps(val).decode() diff --git a/zerver/views/message_edit.py b/zerver/views/message_edit.py index 46470d97ab..ca8270e49d 100644 --- a/zerver/views/message_edit.py +++ b/zerver/views/message_edit.py @@ -19,6 +19,7 @@ from zerver.lib.message import ( access_message_and_usermessage, access_web_public_message, messages_for_ids, + visible_edit_history_for_message, ) from zerver.lib.request import RequestNotes from zerver.lib.response import json_success @@ -27,6 +28,7 @@ from zerver.lib.topic import maybe_rename_empty_topic_to_general_chat from zerver.lib.typed_endpoint import OptionalTopic, PathOnly, typed_endpoint from zerver.lib.types import EditHistoryEvent, FormattedEditHistoryEvent from zerver.models import Message, UserProfile +from zerver.models.realms import MessageEditHistoryVisibilityPolicyEnum def fill_edit_history_entries( @@ -109,7 +111,13 @@ def get_message_edit_history( message_id: PathOnly[NonNegativeInt], allow_empty_topic_name: Json[bool] = False, ) -> HttpResponse: - if not user_profile.realm.allow_edit_history: + user_realm_message_edit_history_visibility_policy = ( + user_profile.realm.message_edit_history_visibility_policy + ) + if ( + user_realm_message_edit_history_visibility_policy + == MessageEditHistoryVisibilityPolicyEnum.none.value + ): raise JsonableError(_("Message edit history is disabled in this organization")) message = access_message(user_profile, message_id) @@ -123,7 +131,14 @@ def get_message_edit_history( message_edit_history = fill_edit_history_entries( raw_edit_history, message, allow_empty_topic_name ) - return json_success(request, data={"message_history": list(reversed(message_edit_history))}) + + visible_message_edit_history = visible_edit_history_for_message( + user_realm_message_edit_history_visibility_policy, message_edit_history + ) + + return json_success( + request, data={"message_history": list(reversed(visible_message_edit_history))} + ) @typed_endpoint @@ -218,13 +233,15 @@ def json_fetch_raw_message( flags = ["read"] if not maybe_user_profile.is_authenticated: - allow_edit_history = realm.allow_edit_history + message_edit_history_visibility_policy = realm.message_edit_history_visibility_policy else: if user_message: flags = user_message.flags_list() else: flags = ["read", "historical"] - allow_edit_history = maybe_user_profile.realm.allow_edit_history + message_edit_history_visibility_policy = ( + maybe_user_profile.realm.message_edit_history_visibility_policy + ) # Security note: It's important that we call this only with a # message already fetched via `access_message` type methods, @@ -235,7 +252,7 @@ def json_fetch_raw_message( search_fields={}, apply_markdown=apply_markdown, client_gravatar=True, - allow_edit_history=allow_edit_history, + message_edit_history_visibility_policy=message_edit_history_visibility_policy, allow_empty_topic_name=allow_empty_topic_name, user_profile=user_profile, realm=message.realm, diff --git a/zerver/views/message_fetch.py b/zerver/views/message_fetch.py index 2538b49de1..6c81011dab 100644 --- a/zerver/views/message_fetch.py +++ b/zerver/views/message_fetch.py @@ -305,7 +305,7 @@ def get_messages_backend( apply_markdown=apply_markdown, client_gravatar=client_gravatar, allow_empty_topic_name=allow_empty_topic_name, - allow_edit_history=realm.allow_edit_history, + message_edit_history_visibility_policy=realm.message_edit_history_visibility_policy, user_profile=user_profile, realm=realm, ) diff --git a/zerver/views/realm.py b/zerver/views/realm.py index 12798fda82..ba6ecf5a11 100644 --- a/zerver/views/realm.py +++ b/zerver/views/realm.py @@ -1,4 +1,5 @@ from collections.abc import Mapping +from enum import Enum from typing import Annotated, Any, Literal from django.conf import settings @@ -48,7 +49,11 @@ from zerver.lib.user_groups import ( from zerver.lib.validator import check_capped_url, check_string from zerver.models import Realm, RealmReactivationStatus, RealmUserDefault, UserProfile from zerver.models.groups import SystemGroups -from zerver.models.realms import DigestWeekdayEnum, OrgTypeEnum +from zerver.models.realms import ( + DigestWeekdayEnum, + MessageEditHistoryVisibilityPolicyEnum, + OrgTypeEnum, +) from zerver.views.user_settings import ( check_information_density_setting_values, check_settings_values, @@ -112,7 +117,11 @@ def update_realm( message_content_edit_limit_seconds_raw: Annotated[ Json[int | str] | None, ApiParamConfig("message_content_edit_limit_seconds") ] = None, - allow_edit_history: Json[bool] | None = None, + message_edit_history_visibility_policy_raw: Annotated[ + # TODO: Use MessageEditHistoryVisibilityPolicyEnum here with Pydantic + Literal["all", "moves", "none"] | None, + ApiParamConfig("message_edit_history_visibility_policy"), + ] = None, default_language: str | None = None, waiting_period_threshold: Json[NonNegativeInt] | None = None, authentication_methods: Json[dict[str, Any]] | None = None, @@ -329,6 +338,12 @@ def update_realm( data["jitsi_server_url"] = jitsi_server_url + message_edit_history_visibility_policy: Enum | None = None + if message_edit_history_visibility_policy_raw is not None: + message_edit_history_visibility_policy = MessageEditHistoryVisibilityPolicyEnum[ + message_edit_history_visibility_policy_raw + ] + # The user of `locals()` here is a bit of a code smell, but it's # restricted to the elements present in realm.property_types. #