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

@@ -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,

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;
     }

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 = true;
-                        data.allow_message_deleting = Boolean(
-                            data.message_content_delete_limit_seconds,
-                        );
 
                         break;
                     }

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),

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",

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

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

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),
+        ),
+    ]

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(
+    MESSAGE_CONTENT_DELETE_LIMIT_SPECIAL_VALUES_MAP = {
-        default=DEFAULT_MESSAGE_CONTENT_DELETE_LIMIT_SECONDS,
+        "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,
     )
 

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:

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).
+        # Test if time limit is None(no limit).
-        set_message_deleting_params(True, 0)
+        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)

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}

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
+    deadline_seconds: Optional[int] = user_profile.realm.message_content_delete_limit_seconds
-    if deadline_seconds == 0:
+    if deadline_seconds is None:
-        # 0 for no time limit to delete message
+        # 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

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(
+    message_content_delete_limit_seconds_raw: Optional[Union[int, str]] = REQ(
-        converter=to_non_negative_int, default=None
+        "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: