From b13bfa09c5cfc609587e587eb6ed19ebd3addd1d Mon Sep 17 00:00:00 2001 From: sahil839 Date: Mon, 14 Jun 2021 22:19:28 +0530 Subject: [PATCH] message: Make zero invalid value for message_content_delete_limit_seconds. We make zero invalid value for message_content_delete_limit_seconds and for handling the case of "Allow to delete message any time", the API-level value of message_content_delete_limit_seconds is "anytime" and "None" as the DB-level value. We also use these values for message retention setting, so it helps maintain consistency. --- frontend_tests/node_tests/message_edit.js | 2 +- static/js/message_edit.js | 2 +- static/js/settings_org.js | 14 +++-- templates/zerver/api/changelog.md | 9 +++ tools/linter_lib/pyflakes.py | 4 ++ version.py | 2 +- zerver/lib/message.py | 15 ++++- ...lm_message_content_delete_limit_seconds.py | 58 +++++++++++++++++++ zerver/models.py | 9 ++- zerver/openapi/zulip.yaml | 14 +++++ zerver/tests/test_message_edit.py | 14 +++-- zerver/tests/test_realm.py | 20 ++++++- zerver/views/message_edit.py | 6 +- zerver/views/realm.py | 22 ++++++- 14 files changed, 166 insertions(+), 25 deletions(-) create mode 100644 zerver/migrations/0354_alter_realm_message_content_delete_limit_seconds.py diff --git a/frontend_tests/node_tests/message_edit.js b/frontend_tests/node_tests/message_edit.js index fe8de5cd04..9d311f3154 100644 --- a/frontend_tests/node_tests/message_edit.js +++ b/frontend_tests/node_tests/message_edit.js @@ -125,7 +125,7 @@ run_test("get_editability", ({override}) => { run_test("get_deletability", () => { page_params.is_admin = true; page_params.realm_allow_message_deleting = false; - page_params.realm_message_content_delete_limit_seconds = 0; + page_params.realm_message_content_delete_limit_seconds = null; const message = { sent_by_me: false, locally_echoed: true, diff --git a/static/js/message_edit.js b/static/js/message_edit.js index 4176eb4eb4..82d38f75e0 100644 --- a/static/js/message_edit.js +++ b/static/js/message_edit.js @@ -171,7 +171,7 @@ export function get_deletability(message) { return false; } - if (page_params.realm_message_content_delete_limit_seconds === 0) { + if (page_params.realm_message_content_delete_limit_seconds === null) { // This means no time limit for message deletion. return true; } diff --git a/static/js/settings_org.js b/static/js/settings_org.js index b9d3eca9af..7bcbf3ea3a 100644 --- a/static/js/settings_org.js +++ b/static/js/settings_org.js @@ -162,6 +162,9 @@ function get_property_value(property_name) { if (!page_params.realm_allow_message_deleting) { return "never"; } + if (page_params.realm_message_content_delete_limit_seconds === null) { + return "any_time"; + } for (const [value, elem] of settings_config.msg_delete_limit_dropdown_values) { if (elem.seconds === page_params.realm_message_content_delete_limit_seconds) { return value; @@ -776,14 +779,17 @@ export function build_page() { break; } + case "any_time": { + data.allow_message_deleting = true; + data.message_content_delete_limit_seconds = JSON.stringify("unlimited"); + + break; + } case "custom_limit": { data.message_content_delete_limit_seconds = parse_time_limit( $("#id_realm_message_content_delete_limit_minutes"), ); - // Disable deleting if the parsed time limit is 0 seconds - data.allow_message_deleting = Boolean( - data.message_content_delete_limit_seconds, - ); + data.allow_message_deleting = true; break; } diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index ad8a2a8a56..108942b63a 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -11,6 +11,15 @@ below features are supported. ## Changes in Zulip 5.0 +**Feature level 100** + +* [`POST /register`](/api/register-queue), [`GET + /events`](/api/get-events): `message_content_delete_limit_seconds` + now represents no limit using `null`, instead of the integer 0. +* `PATCH /realm`: One now sets `message_content_delete_limit_seconds` + to no limit by passing the string `unlimited`, rather than the + integer 0. + **Feature level 99** * [`PATCH /realm/user_settings_defaults`](/api/update-realm-user-settings-defaults), diff --git a/tools/linter_lib/pyflakes.py b/tools/linter_lib/pyflakes.py index 080425a98c..e9090d3c66 100644 --- a/tools/linter_lib/pyflakes.py +++ b/tools/linter_lib/pyflakes.py @@ -15,6 +15,10 @@ def check_pyflakes(files: List[str], options: argparse.Namespace) -> bool: "zerver/views/realm.py", "local variable 'message_retention_days' is assigned to but never used", ), + ( + "zerver/views/realm.py", + "local variable 'message_content_delete_limit_seconds' is assigned to but never used", + ), ("settings.py", "settings import *' used; unable to detect undefined names"), ( "settings.py", diff --git a/version.py b/version.py index d8bed47cba..41a3ce3ac1 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 = 99 +API_FEATURE_LEVEL = 100 # 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/lib/message.py b/zerver/lib/message.py index cd2775272b..fb557cdc2f 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -2,7 +2,7 @@ import copy import datetime import zlib from dataclasses import dataclass, field -from typing import Any, Dict, List, Optional, Sequence, Set, Tuple +from typing import Any, Dict, List, Mapping, Optional, Sequence, Set, Tuple, Union import ahocorasick import orjson @@ -28,6 +28,7 @@ from zerver.lib.exceptions import JsonableError, MissingAuthenticationError from zerver.lib.markdown import MessageRenderingResult, markdown_convert, topic_links from zerver.lib.markdown import version as markdown_version from zerver.lib.mention import MentionData +from zerver.lib.request import RequestVariableConversionError from zerver.lib.stream_subscription import ( get_stream_subscriptions_for_user, get_subscribed_stream_recipient_ids_for_user, @@ -1470,3 +1471,15 @@ def wildcard_mention_allowed(sender: UserProfile, stream: Stream) -> bool: return not sender.is_guest raise AssertionError("Invalid wildcard mention policy") + + +def parse_message_content_delete_limit( + value: Union[int, str], + special_values_map: Mapping[str, Optional[int]], +) -> Optional[int]: + if isinstance(value, str) and value in special_values_map.keys(): + return special_values_map[value] + if isinstance(value, str) or value <= 0: + raise RequestVariableConversionError("message_content_delete_limit_seconds", value) + assert isinstance(value, int) + return value diff --git a/zerver/migrations/0354_alter_realm_message_content_delete_limit_seconds.py b/zerver/migrations/0354_alter_realm_message_content_delete_limit_seconds.py new file mode 100644 index 0000000000..0f1368ea63 --- /dev/null +++ b/zerver/migrations/0354_alter_realm_message_content_delete_limit_seconds.py @@ -0,0 +1,58 @@ +# Generated by Django 3.2.4 on 2021-06-14 12:12 + +from django.db import migrations, models +from django.db.backends.postgresql.schema import DatabaseSchemaEditor +from django.db.migrations.state import StateApps + + +def make_zero_invalid_for_message_delete_limit( + apps: StateApps, schema_editor: DatabaseSchemaEditor +) -> None: + Realm = apps.get_model("zerver", "Realm") + Realm.DEFAULT_MESSAGE_CONTENT_DELETE_LIMIT_SECONDS = 600 + + Realm.objects.filter( + allow_message_deleting=True, message_content_delete_limit_seconds=0 + ).update(message_content_delete_limit_seconds=None) + + Realm.objects.filter( + allow_message_deleting=False, message_content_delete_limit_seconds=0 + ).update( + message_content_delete_limit_seconds=Realm.DEFAULT_MESSAGE_CONTENT_DELETE_LIMIT_SECONDS + ) + + +def reverse_make_zero_invalid_for_message_delete_limit( + apps: StateApps, schema_editor: DatabaseSchemaEditor +) -> None: + Realm = apps.get_model("zerver", "Realm") + Realm.DEFAULT_MESSAGE_CONTENT_DELETE_LIMIT_SECONDS = 600 + + Realm.objects.filter( + allow_message_deleting=True, message_content_delete_limit_seconds=None + ).update(message_content_delete_limit_seconds=0) + + +class Migration(migrations.Migration): + + dependencies = [ + ("zerver", "0353_remove_realm_default_twenty_four_hour_time"), + ] + + operations = [ + migrations.AlterField( + model_name="realm", + name="message_content_delete_limit_seconds", + field=models.IntegerField(default=600, null=True), + ), + migrations.RunPython( + make_zero_invalid_for_message_delete_limit, + reverse_code=reverse_make_zero_invalid_for_message_delete_limit, + elidable=True, + ), + migrations.AlterField( + model_name="realm", + name="message_content_delete_limit_seconds", + field=models.PositiveIntegerField(default=600, null=True), + ), + ] diff --git a/zerver/models.py b/zerver/models.py index cce4c6f343..b5bc14d85e 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -371,8 +371,11 @@ class Realm(models.Model): DEFAULT_MESSAGE_CONTENT_DELETE_LIMIT_SECONDS = ( 600 # if changed, also change in admin.js, setting_org.js ) - message_content_delete_limit_seconds: int = models.IntegerField( - default=DEFAULT_MESSAGE_CONTENT_DELETE_LIMIT_SECONDS, + MESSAGE_CONTENT_DELETE_LIMIT_SPECIAL_VALUES_MAP = { + "unlimited": None, + } + message_content_delete_limit_seconds: int = models.PositiveIntegerField( + default=DEFAULT_MESSAGE_CONTENT_DELETE_LIMIT_SECONDS, null=True ) allow_message_editing: bool = models.BooleanField(default=True) @@ -629,7 +632,7 @@ class Realm(models.Model): private_message_policy=int, user_group_edit_policy=int, default_code_block_language=(str, type(None)), - message_content_delete_limit_seconds=int, + message_content_delete_limit_seconds=(int, type(None)), wildcard_mention_policy=int, ) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 15f3888970..71d675b320 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3556,10 +3556,17 @@ paths: in this organization. Null if no default has been set. message_content_delete_limit_seconds: type: integer + nullable: true description: | Messages sent more than this many seconds ago cannot be deleted with this organization's [message deletion policy](/help/configure-message-editing-and-deletion). + + A 'null' value means no limit: messages can be deleted + regardless of how long ago they were sent. + + **Changes**: No limit was represented using the + special value `0` before Zulip 5.0 (feature level 100). authentication_methods: type: object additionalProperties: @@ -10599,12 +10606,19 @@ paths: in this organization. Null if no default has been set. realm_message_content_delete_limit_seconds: type: integer + nullable: true description: | Present if `realm` is present in `fetch_event_types`. Messages sent more than this many seconds ago cannot be deleted with this organization's [message deletion policy](/help/configure-message-editing-and-deletion). + + A 'null' value means no limit: messages can be deleted + regardless of how long ago they were sent. + + **Changes**: No limit was represented using the + special value `0` before Zulip 5.0 (feature level 100). realm_authentication_methods: type: object additionalProperties: diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index d8c4e371f0..d3ec5bd0a8 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -1,6 +1,6 @@ import datetime from operator import itemgetter -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple, Union from unittest import mock import orjson @@ -2142,14 +2142,16 @@ class DeleteMessageTest(ZulipTestCase): def test_delete_message_by_user(self) -> None: def set_message_deleting_params( - allow_message_deleting: bool, message_content_delete_limit_seconds: int + allow_message_deleting: bool, message_content_delete_limit_seconds: Union[int, str] ) -> None: self.login("iago") result = self.client_patch( "/json/realm", { "allow_message_deleting": orjson.dumps(allow_message_deleting).decode(), - "message_content_delete_limit_seconds": message_content_delete_limit_seconds, + "message_content_delete_limit_seconds": orjson.dumps( + message_content_delete_limit_seconds + ).decode(), }, ) self.assert_json_success(result) @@ -2170,7 +2172,7 @@ class DeleteMessageTest(ZulipTestCase): return result # Test if message deleting is not allowed(default). - set_message_deleting_params(False, 0) + set_message_deleting_params(False, "unlimited") hamlet = self.example_user("hamlet") self.login_user(hamlet) msg_id = self.send_stream_message(hamlet, "Scotland") @@ -2185,8 +2187,8 @@ class DeleteMessageTest(ZulipTestCase): self.assert_json_success(result) # Test if message deleting is allowed. - # Test if time limit is zero(no limit). - set_message_deleting_params(True, 0) + # Test if time limit is None(no limit). + set_message_deleting_params(True, "unlimited") msg_id = self.send_stream_message(hamlet, "Scotland") message = Message.objects.get(id=msg_id) message.date_sent = message.date_sent - datetime.timedelta(seconds=600) diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 2f92670f69..488619de0f 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -1005,18 +1005,34 @@ class RealmAPITest(ZulipTestCase): def test_update_realm_allow_message_deleting(self) -> None: """Tests updating the realm property 'allow_message_deleting'.""" self.set_up_db("allow_message_deleting", True) - self.set_up_db("message_content_delete_limit_seconds", 0) realm = self.update_with_api("allow_message_deleting", False) self.assertEqual(realm.allow_message_deleting, False) - self.assertEqual(realm.message_content_delete_limit_seconds, 0) + self.assertEqual(realm.message_content_delete_limit_seconds, 600) realm = self.update_with_api("allow_message_deleting", True) realm = self.update_with_api("message_content_delete_limit_seconds", 100) self.assertEqual(realm.allow_message_deleting, True) self.assertEqual(realm.message_content_delete_limit_seconds, 100) + realm = self.update_with_api( + "message_content_delete_limit_seconds", orjson.dumps("unlimited").decode() + ) + self.assertEqual(realm.allow_message_deleting, True) + self.assertEqual(realm.message_content_delete_limit_seconds, None) realm = self.update_with_api("message_content_delete_limit_seconds", 600) self.assertEqual(realm.allow_message_deleting, True) self.assertEqual(realm.message_content_delete_limit_seconds, 600) + # Test that 0 is invalid value. + req = dict(message_content_delete_limit_seconds=orjson.dumps(0).decode()) + result = self.client_patch("/json/realm", req) + self.assert_json_error(result, "Bad value for 'message_content_delete_limit_seconds': 0") + + # Test that only "unlimited" string is valid and others are invalid. + req = dict(message_content_delete_limit_seconds=orjson.dumps("invalid").decode()) + result = self.client_patch("/json/realm", req) + self.assert_json_error( + result, "Bad value for 'message_content_delete_limit_seconds': invalid" + ) + def test_change_invite_to_realm_policy_by_owners_only(self) -> None: self.login("iago") req = {"invite_to_realm_policy": Realm.POLICY_ADMINS_ONLY} diff --git a/zerver/views/message_edit.py b/zerver/views/message_edit.py index 09f00c02e6..7e0ddb6b3e 100644 --- a/zerver/views/message_edit.py +++ b/zerver/views/message_edit.py @@ -137,9 +137,9 @@ def validate_can_delete_message(user_profile: UserProfile, message: Message) -> # User can not delete message, if message deleting is not allowed in realm. raise JsonableError(_("You don't have permission to delete this message")) - deadline_seconds = user_profile.realm.message_content_delete_limit_seconds - if deadline_seconds == 0: - # 0 for no time limit to delete message + deadline_seconds: Optional[int] = user_profile.realm.message_content_delete_limit_seconds + if deadline_seconds is None: + # None means no time limit to delete message return if (timezone_now() - message.date_sent) > datetime.timedelta(seconds=deadline_seconds): # User can not delete message after deadline time of realm diff --git a/zerver/views/realm.py b/zerver/views/realm.py index 2b71c1d49f..3dfc701bb6 100644 --- a/zerver/views/realm.py +++ b/zerver/views/realm.py @@ -21,6 +21,7 @@ from zerver.lib.actions import ( ) from zerver.lib.exceptions import JsonableError, OrganizationOwnerRequired from zerver.lib.i18n import get_available_language_codes +from zerver.lib.message import parse_message_content_delete_limit from zerver.lib.request import REQ, has_request_variables from zerver.lib.response import json_success from zerver.lib.retention import parse_message_retention_days @@ -67,8 +68,8 @@ def update_realm( json_validator=check_int_in(Realm.COMMON_POLICY_TYPES), default=None ), allow_message_deleting: Optional[bool] = REQ(json_validator=check_bool, default=None), - message_content_delete_limit_seconds: Optional[int] = REQ( - converter=to_non_negative_int, default=None + message_content_delete_limit_seconds_raw: Optional[Union[int, str]] = REQ( + "message_content_delete_limit_seconds", json_validator=check_string_or_int, default=None ), allow_message_editing: Optional[bool] = REQ(json_validator=check_bool, default=None), edit_topic_policy: Optional[int] = REQ( @@ -157,6 +158,22 @@ def update_realm( if invite_to_realm_policy is not None and not user_profile.is_realm_owner: raise OrganizationOwnerRequired() + data: Dict[str, Any] = {} + + message_content_delete_limit_seconds: Optional[int] = None + if message_content_delete_limit_seconds_raw is not None: + message_content_delete_limit_seconds = parse_message_content_delete_limit( + message_content_delete_limit_seconds_raw, + Realm.MESSAGE_CONTENT_DELETE_LIMIT_SPECIAL_VALUES_MAP, + ) + do_set_realm_property( + realm, + "message_content_delete_limit_seconds", + message_content_delete_limit_seconds, + acting_user=user_profile, + ) + data["message_content_delete_limit_seconds"] = message_content_delete_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. # @@ -164,7 +181,6 @@ def update_realm( # further by some more advanced usage of the # `REQ/has_request_variables` extraction. req_vars = {k: v for k, v in list(locals().items()) if k in realm.property_types} - data: Dict[str, Any] = {} for k, v in list(req_vars.items()): if v is not None and getattr(realm, k) != v: