From bd9b74436c6f24425a284c1a366f355d3d7bdb6f Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Thu, 7 May 2020 16:49:54 +0530 Subject: [PATCH] org settings: Enable message_retention_days in org settings UI. Since production testing of `message_retention_days` is finished, we can enable this feature in the organization settings page. We already had this setting in frontend but it was bit rotten and not rendered in templates. Here we replaced our past text-input based setting with a dropdown-with-text-input setting approach which is more consistent with our existing UI. Along with frontend changes, we also incorporated a backend change to handle making retention period forever. This change introduces a new convertor `to_positive_or_allowed_int` which only allows positive integers and an allowed value for settings like `message_retention_days` which can be a positive integer or has the value `Realm.RETAIN_MESSAGE_FOREVER` when we change the setting to retain message forever. This change made `to_not_negative_int_or_none` redundant so removed it as well. Fixes: #14854 --- analytics/views.py | 3 +- frontend_tests/node_tests/settings_org.js | 2 + static/js/settings_config.js | 1 + static/js/settings_org.js | 42 ++++++++++++----- .../settings/organization_settings_admin.hbs | 45 ++++++++++++++----- zerver/decorator.py | 8 ---- zerver/lib/validator.py | 11 +++++ zerver/models.py | 1 + zerver/tests/test_decorators.py | 15 ++++--- zerver/tornado/views.py | 7 +-- zerver/views/messages.py | 5 +-- zerver/views/pointer.py | 2 +- zerver/views/realm.py | 8 ++-- zerver/views/report.py | 5 +-- zerver/views/streams.py | 4 +- 15 files changed, 107 insertions(+), 52 deletions(-) diff --git a/analytics/views.py b/analytics/views.py index 53611f1667..2145127a6a 100644 --- a/analytics/views.py +++ b/analytics/views.py @@ -31,7 +31,7 @@ from analytics.models import BaseCount, InstallationCount, \ RealmCount, StreamCount, UserCount, last_successful_fill, installation_epoch from confirmation.models import Confirmation, confirmation_url, _properties from zerver.decorator import require_server_admin, require_server_admin_api, \ - to_non_negative_int, to_utc_datetime, zulip_login_required, require_non_guest_user + to_utc_datetime, zulip_login_required, require_non_guest_user from zerver.lib.exceptions import JsonableError from zerver.lib.request import REQ, has_request_variables from zerver.lib.response import json_success @@ -41,6 +41,7 @@ from zerver.views.invite import get_invitee_emails_set from zerver.lib.subdomains import get_subdomain_from_hostname from zerver.lib.actions import do_change_plan_type, do_deactivate_realm, \ do_send_realm_reactivation_email, do_scrub_realm +from zerver.lib.validator import to_non_negative_int from confirmation.settings import STATUS_ACTIVE if settings.BILLING_ENABLED: diff --git a/frontend_tests/node_tests/settings_org.js b/frontend_tests/node_tests/settings_org.js index e9f0b18486..a1c4842eba 100644 --- a/frontend_tests/node_tests/settings_org.js +++ b/frontend_tests/node_tests/settings_org.js @@ -804,6 +804,8 @@ run_test('set_up', () => { }; $("#id_realm_message_content_edit_limit_minutes").set_parent($.create('')); $("#id_realm_message_content_delete_limit_minutes").set_parent($.create('')); + $("#id_realm_message_retention_days").set_parent($.create('')); + $("#id_realm_message_retention_setting").change = noop; $("#message_content_in_email_notifications_label").set_parent($.create('')); $("#enable_digest_emails_label").set_parent($.create('')); $("#id_realm_digest_weekday").set_parent($.create('')); diff --git a/static/js/settings_config.js b/static/js/settings_config.js index 0bd9c4c6b4..54b88a3bac 100644 --- a/static/js/settings_config.js +++ b/static/js/settings_config.js @@ -156,6 +156,7 @@ const time_limit_dropdown_values = new Map([ exports.msg_edit_limit_dropdown_values = time_limit_dropdown_values; exports.msg_delete_limit_dropdown_values = time_limit_dropdown_values; +exports.retain_message_forever = -1; // NOTIFICATIONS diff --git a/static/js/settings_org.js b/static/js/settings_org.js index 2583f91430..fac808c431 100644 --- a/static/js/settings_org.js +++ b/static/js/settings_org.js @@ -119,6 +119,14 @@ function get_property_value(property_name) { return "custom_limit"; } + if (property_name === 'realm_message_retention_setting') { + if (page_params.realm_message_retention_days === null || + page_params.realm_message_retention_days === settings_config.retain_message_forever) { + return "retain_forever"; + } + return "retain_for_period"; + } + if (property_name === 'realm_msg_delete_limit_setting') { if (!page_params.realm_allow_message_deleting) { return "never"; @@ -234,6 +242,13 @@ function set_msg_delete_limit_dropdown() { value === "custom_limit"); } +function set_message_retention_setting_dropdown() { + const value = get_property_value("realm_message_retention_setting"); + $("#id_realm_message_retention_setting").val(value); + change_element_block_display_property('id_realm_message_retention_days', + value === "retain_for_period"); +} + function set_org_join_restrictions_dropdown() { const value = get_property_value("realm_org_join_restrictions"); $("#id_realm_org_join_restrictions").val(value); @@ -327,6 +342,8 @@ function update_dependent_subsettings(property_name) { } else if (property_name === 'realm_msg_edit_limit_setting' || property_name === 'realm_message_content_edit_limit_minutes') { set_msg_edit_limit_dropdown(); + } else if (property_name === 'realm_message_retention_days') { + set_message_retention_setting_dropdown(); } else if (property_name === 'realm_msg_delete_limit_setting' || property_name === 'realm_message_content_delete_limit_minutes') { set_msg_delete_limit_dropdown(); @@ -576,6 +593,7 @@ exports.build_page = function () { set_video_chat_provider_dropdown(); set_msg_edit_limit_dropdown(); set_msg_delete_limit_dropdown(); + set_message_retention_setting_dropdown(); set_org_join_restrictions_dropdown(); set_message_content_in_email_notifications_visiblity(); set_digest_emails_weekday_visibility(); @@ -671,19 +689,17 @@ exports.build_page = function () { parseInt(exports.notifications_stream_widget.value(), 10)); data.signup_notifications_stream_id = JSON.stringify( parseInt(exports.signup_notifications_stream_widget.value(), 10)); - } else if (subsection === 'other_settings') { - let new_message_retention_days = $("#id_realm_message_retention_days").val(); - - if (parseInt(new_message_retention_days, 10).toString() !== new_message_retention_days - && new_message_retention_days !== "") { - new_message_retention_days = ""; + } else if (subsection === 'message_retention') { + const message_retention_setting_value = $('#id_realm_message_retention_setting').val(); + if (message_retention_setting_value === 'retain_forever') { + data.message_retention_days = settings_config.retain_message_forever; + } else { + data.message_retention_days = exports.get_input_element_value( + $('#id_realm_message_retention_days')); } - + } else if (subsection === 'other_settings') { const code_block_language_value = exports.default_code_language_widget.value(); data.default_code_block_language = JSON.stringify(code_block_language_value); - - data.message_retention_days = new_message_retention_days !== "" ? - JSON.stringify(parseInt(new_message_retention_days, 10)) : null; } else if (subsection === 'other_permissions') { const add_emoji_permission = $("#id_realm_add_emoji_by_admins_only").val(); @@ -791,6 +807,12 @@ exports.build_page = function () { msg_delete_limit_dropdown_value === 'custom_limit'); }); + $("#id_realm_message_retention_setting").change(function (e) { + const message_retention_setting_dropdown_value = e.target.value; + change_element_block_display_property('id_realm_message_retention_days', + message_retention_setting_dropdown_value === 'retain_for_period'); + }); + $("#id_realm_waiting_period_setting").change(function () { const waiting_period_threshold = this.value; change_element_block_display_property('id_realm_waiting_period_threshold', diff --git a/static/templates/settings/organization_settings_admin.hbs b/static/templates/settings/organization_settings_admin.hbs index 3cd0c9c3e6..5405ea57af 100644 --- a/static/templates/settings/organization_settings_admin.hbs +++ b/static/templates/settings/organization_settings_admin.hbs @@ -142,6 +142,39 @@ +
+
+

{{t "Message retention" }}

+ {{> settings_save_discard_widget section_name="message-retention" }} +
+ + {{> upgrade_tip_widget }} + +
+
+ + + +
+ + +
+
+
+
+

{{t "Other settings" }}

@@ -209,18 +242,6 @@ is_checked=realm_message_content_allowed_in_email_notifications label=admin_settings_label.realm_message_content_allowed_in_email_notifications}} - {{#if false}} -
- - -
- {{/if}} {{> settings_checkbox setting_name="realm_mandatory_topics" prefix="id_" diff --git a/zerver/decorator.py b/zerver/decorator.py index b6c2796876..f7802dfa7d 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -26,7 +26,6 @@ from zerver.lib.exceptions import JsonableError, ErrorCode, \ InvalidJSONError, InvalidAPIKeyError, InvalidAPIKeyFormatError, \ OrganizationAdministratorRequired from zerver.lib.types import ViewFuncT -from zerver.lib.validator import to_non_negative_int from zerver.lib.rate_limiter import RateLimitedUser from zerver.lib.request import REQ, has_request_variables @@ -716,13 +715,6 @@ def internal_notify_view(is_tornado_view: bool) -> Callable[[ViewFuncT], ViewFun return _wrapped_func_arguments return _wrapped_view_func - -def to_not_negative_int_or_none(s: str) -> Optional[int]: - if s: - return to_non_negative_int(s) - return None - - def to_utc_datetime(timestamp: str) -> datetime.datetime: return timestamp_to_datetime(float(timestamp)) diff --git a/zerver/lib/validator.py b/zerver/lib/validator.py index aa53f1eb3e..15dc93b075 100644 --- a/zerver/lib/validator.py +++ b/zerver/lib/validator.py @@ -418,6 +418,17 @@ def to_non_negative_int(s: str, max_int_size: int=2**32-1) -> int: raise ValueError('%s is too large (max %s)' % (x, max_int_size)) return x +def to_positive_or_allowed_int(allowed_integer: int) -> Callable[[str], int]: + @set_type_structure('int') + def convertor(s: str) -> int: + x = int(s) + if x == allowed_integer: + return x + if x == 0: + raise ValueError("argument is 0") + return to_non_negative_int(s) + return convertor + @set_type_structure('any(List[int], str)]') def check_string_or_int_list(var_name: str, val: object) -> Optional[str]: if isinstance(val, str): diff --git a/zerver/models.py b/zerver/models.py index 062393eb08..82f57801c2 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -264,6 +264,7 @@ class Realm(models.Model): "Stream", related_name="+", null=True, blank=True, on_delete=CASCADE ) + RETAIN_MESSAGE_FOREVER = -1 # For old messages being automatically deleted message_retention_days: Optional[int] = models.IntegerField(null=True) diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index ebb35d1db6..7784a9a824 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -37,7 +37,7 @@ from zerver.decorator import ( authenticate_notify, cachify, get_client_name, internal_notify_view, is_local_addr, rate_limit, validate_api_key, - return_success_on_head_request, to_not_negative_int_or_none, + return_success_on_head_request, zulip_login_required ) from zerver.lib.cache import ignore_unhashable_lru_cache, dict_to_items_tuple, items_tuple_to_dict @@ -45,7 +45,8 @@ from zerver.lib.validator import ( check_string, check_dict, check_dict_only, check_bool, check_float, check_int, check_list, Validator, check_variable_type, equals, check_none_or, check_url, check_short_string, check_string_fixed_length, check_capped_string, check_color, to_non_negative_int, - check_string_or_int_list, check_string_or_int, check_int_in, check_string_in + check_string_or_int_list, check_string_or_int, check_int_in, check_string_in, + to_positive_or_allowed_int ) from zerver.models import \ get_realm, get_user, UserProfile, Realm @@ -773,11 +774,13 @@ class ValidatorTestCase(TestCase): with self.assertRaisesRegex(ValueError, re.escape('%s is too large (max %s)' % (2**32, 2**32-1))): self.assertEqual(to_non_negative_int(str(2**32))) - def test_check_to_not_negative_int_or_none(self) -> None: - self.assertEqual(to_not_negative_int_or_none('5'), 5) - self.assertEqual(to_not_negative_int_or_none(None), None) + def test_to_positive_or_allowed_int(self) -> None: + self.assertEqual(to_positive_or_allowed_int(-1)('5'), 5) + self.assertEqual(to_positive_or_allowed_int(-1)('-1'), -1) + with self.assertRaisesRegex(ValueError, 'argument is negative'): + to_positive_or_allowed_int(-1)('-5') with self.assertRaises(ValueError): - to_not_negative_int_or_none('-5') + to_positive_or_allowed_int(-1)('0') def test_check_float(self) -> None: x: Any = 5.5 diff --git a/zerver/tornado/views.py b/zerver/tornado/views.py index 94d0aa7bdc..701ffe02df 100644 --- a/zerver/tornado/views.py +++ b/zerver/tornado/views.py @@ -5,10 +5,11 @@ import ujson from django.http import HttpRequest, HttpResponse from django.utils.translation import ugettext as _ -from zerver.decorator import REQ, to_non_negative_int, \ - has_request_variables, internal_notify_view, process_client +from zerver.decorator import REQ, has_request_variables, internal_notify_view, \ + process_client from zerver.lib.response import json_error, json_success -from zerver.lib.validator import check_bool, check_int, check_list, check_string +from zerver.lib.validator import check_bool, check_int, check_list, check_string, \ + to_non_negative_int from zerver.models import Client, UserProfile, get_client, get_user_profile_by_id from zerver.tornado.event_queue import fetch_events, \ get_client_descriptor, process_notification diff --git a/zerver/views/messages.py b/zerver/views/messages.py index b4a89df6f8..4a76fa8024 100644 --- a/zerver/views/messages.py +++ b/zerver/views/messages.py @@ -9,8 +9,7 @@ from typing import Dict, List, Set, Any, Iterable, \ Optional, Tuple, Union, Sequence, cast from zerver.lib.exceptions import JsonableError, ErrorCode from zerver.lib.html_diff import highlight_html_differences -from zerver.decorator import has_request_variables, \ - REQ, to_non_negative_int +from zerver.decorator import has_request_variables, REQ from django.utils.html import escape as escape_html from zerver.lib import bugdown from zerver.lib.zcommand import process_zcommands @@ -50,7 +49,7 @@ from zerver.lib.utils import statsd from zerver.lib.validator import \ check_list, check_int, check_dict, check_string, check_bool, \ check_string_or_int_list, check_string_or_int, check_string_in, \ - check_required_string + check_required_string, to_non_negative_int from zerver.lib.zephyr import compute_mit_user_fullname from zerver.models import Message, UserProfile, Stream, Subscription, Client,\ Realm, RealmDomain, Recipient, UserMessage, \ diff --git a/zerver/views/pointer.py b/zerver/views/pointer.py index 9b5b9ffcfd..c01ba35489 100644 --- a/zerver/views/pointer.py +++ b/zerver/views/pointer.py @@ -1,10 +1,10 @@ from django.http import HttpRequest, HttpResponse from django.utils.translation import ugettext as _ -from zerver.decorator import to_non_negative_int from zerver.lib.actions import do_update_pointer from zerver.lib.request import has_request_variables, JsonableError, REQ from zerver.lib.response import json_success +from zerver.lib.validator import to_non_negative_int from zerver.models import UserProfile, get_usermessage_by_message_id def get_pointer_backend(request: HttpRequest, user_profile: UserProfile) -> HttpResponse: diff --git a/zerver/views/realm.py b/zerver/views/realm.py index ed0d1165fe..5b978d818e 100644 --- a/zerver/views/realm.py +++ b/zerver/views/realm.py @@ -5,7 +5,7 @@ from django.utils.translation import ugettext as _ from django.core.exceptions import ValidationError from django.views.decorators.http import require_safe -from zerver.decorator import require_realm_admin, to_non_negative_int, to_not_negative_int_or_none +from zerver.decorator import require_realm_admin from zerver.lib.actions import ( do_set_realm_message_editing, do_set_realm_message_deleting, @@ -19,7 +19,8 @@ from zerver.lib.actions import ( from zerver.lib.i18n import get_available_language_codes from zerver.lib.request import has_request_variables, REQ, JsonableError from zerver.lib.response import json_success, json_error -from zerver.lib.validator import check_string, check_dict, check_bool, check_int, check_int_in +from zerver.lib.validator import check_string, check_dict, check_bool, check_int, \ + check_int_in, to_positive_or_allowed_int, to_non_negative_int from zerver.lib.streams import access_stream_by_id from zerver.lib.domains import validate_domain from zerver.lib.video_calls import request_zoom_video_call_url @@ -55,7 +56,8 @@ def update_realm( authentication_methods: Optional[Dict[Any, Any]]=REQ(validator=check_dict([]), default=None), notifications_stream_id: Optional[int]=REQ(validator=check_int, default=None), signup_notifications_stream_id: Optional[int]=REQ(validator=check_int, default=None), - message_retention_days: Optional[int]=REQ(converter=to_not_negative_int_or_none, default=None), + message_retention_days: Optional[int] = REQ(converter=to_positive_or_allowed_int( + Realm.RETAIN_MESSAGE_FOREVER), default=None), send_welcome_emails: Optional[bool]=REQ(validator=check_bool, default=None), digest_emails_enabled: Optional[bool]=REQ(validator=check_bool, default=None), message_content_allowed_in_email_notifications: Optional[bool]=REQ( diff --git a/zerver/views/report.py b/zerver/views/report.py index b802027278..3ce197d914 100644 --- a/zerver/views/report.py +++ b/zerver/views/report.py @@ -6,8 +6,7 @@ from django.conf import settings from django.http import HttpRequest, HttpResponse from django.views.decorators.csrf import csrf_exempt from django.views.decorators.http import require_POST -from zerver.decorator import human_users_only, \ - to_non_negative_int +from zerver.decorator import human_users_only from zerver.lib.bugdown import privacy_clean_markdown from zerver.lib.request import has_request_variables, REQ from zerver.lib.response import json_success @@ -15,7 +14,7 @@ from zerver.lib.queue import queue_json_publish from zerver.lib.storage import static_path from zerver.lib.unminify import SourceMap from zerver.lib.utils import statsd, statsd_key -from zerver.lib.validator import check_bool, check_dict +from zerver.lib.validator import check_bool, check_dict, to_non_negative_int from zerver.models import UserProfile import subprocess diff --git a/zerver/views/streams.py b/zerver/views/streams.py index e744da406d..b10b356a90 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -9,7 +9,7 @@ from django.http import HttpRequest, HttpResponse from zerver.lib.exceptions import JsonableError, ErrorCode from zerver.lib.request import REQ, has_request_variables from zerver.decorator import authenticated_json_post_view, \ - require_realm_admin, to_non_negative_int, require_non_guest_user + require_realm_admin, require_non_guest_user from zerver.lib.actions import bulk_remove_subscriptions, \ do_change_subscription_property, internal_prep_private_message, \ internal_prep_stream_message, \ @@ -28,7 +28,7 @@ from zerver.lib.streams import access_stream_by_id, access_stream_by_name, \ from zerver.lib.topic import get_topic_history_for_stream, messages_for_topic from zerver.lib.validator import check_string, check_int, check_list, check_dict, \ check_bool, check_variable_type, check_capped_string, check_color, check_dict_only, \ - check_int_in + check_int_in, to_non_negative_int from zerver.models import UserProfile, Stream, Realm, UserMessage, \ get_system_bot, get_active_user