From 5db4fe865224a18bb11e0f3e67e0f7fe9f52665e Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Bodas Date: Thu, 22 Jul 2021 13:35:04 +0530 Subject: [PATCH] api: Allow setting email_notifications_batching_period_seconds. We allow a maximum value of one week to make sure there aren't a huge number of rows in the table for any user (this could happen if stream notifications are enabled). This commit also fixes a small error in the user_settings test. --- templates/zerver/api/changelog.md | 7 +++++++ zerver/openapi/zulip.yaml | 18 ++++++++++++++++++ zerver/tests/test_home.py | 1 + zerver/tests/test_settings.py | 28 ++++++++++++++++++++++++++-- zerver/views/user_settings.py | 14 ++++++++++++++ 5 files changed, 66 insertions(+), 2 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 6f127bb7c7..f9dbe5315a 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -11,6 +11,13 @@ below features are supported. ## Changes in Zulip 5.0 +**Feature level 82** + +* [`PATCH /settings`](/api/update-settings) now accepts a new + `email_notifications_batching_period_seconds` field for setting the + time duration for which the server will collect email notifications + before sending them. + **Feature level 81** * `POST /users/me/enter-sends` has been removed. The `enter_sends` diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 9cbcc309c4..6e72287376 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -8446,6 +8446,14 @@ paths: description: | Present if `update_global_notifications` is present in `fetch_event_types`. + The current value of this global notification setting for the user. + See [PATCH /settings](/api/update-settings) for details on + the meaning of this setting. + email_notifications_batching_period_seconds: + type: integer + description: | + Present if `update_global_notifications` is present in `fetch_event_types`. + The current value of this global notification setting for the user. See [PATCH /settings](/api/update-settings) for details on the meaning of this setting. @@ -10253,6 +10261,16 @@ paths: schema: type: boolean example: true + - name: email_notifications_batching_period_seconds + in: query + description: | + The duration (in seconds) for which the server should wait to batch + email notifications before sending them. + + **Changes**: New in Zulip 5.0 (feature level 82) + schema: + type: integer + example: 120 - name: enable_offline_email_notifications in: query description: | diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 204c63be55..c4e89dfec2 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -65,6 +65,7 @@ class HomeTest(ZulipTestCase): "desktop_icon_count_display", "development_environment", "email", + "email_notifications_batching_period_seconds", "emojiset", "emojiset_choices", "enable_desktop_notifications", diff --git a/zerver/tests/test_settings.py b/zerver/tests/test_settings.py index 390d11d425..a51e69e9c2 100644 --- a/zerver/tests/test_settings.py +++ b/zerver/tests/test_settings.py @@ -135,12 +135,12 @@ class ChangeSettingsTest(ZulipTestCase): # This is basically a don't-explode test. def test_notify_settings(self) -> None: - for notification_setting in UserProfile.notification_setting_types: + for notification_setting in UserProfile.notification_setting_types.keys(): # `notification_sound` is a string not a boolean, so this test # doesn't work for it. # # TODO: Make this work more like do_test_realm_update_api - if notification_setting != "notification_sound": + if UserProfile.notification_setting_types[notification_setting] is bool: self.check_for_toggle_param_patch("/json/settings", notification_setting) def test_change_notification_sound(self) -> None: @@ -166,6 +166,30 @@ class ChangeSettingsTest(ZulipTestCase): user_profile = self.example_user("hamlet") self.assertEqual(getattr(user_profile, param), "zulip") + def test_change_email_batching_period(self) -> None: + hamlet = self.example_user("hamlet") + self.login_user(hamlet) + + # Default is two minutes + self.assertEqual(hamlet.email_notifications_batching_period_seconds, 120) + + result = self.client_patch( + "/json/settings", {"email_notifications_batching_period_seconds": -1} + ) + self.assert_json_error(result, "Invalid email batching period: -1 seconds") + + result = self.client_patch( + "/json/settings", {"email_notifications_batching_period_seconds": 7 * 24 * 60 * 60 + 10} + ) + self.assert_json_error(result, "Invalid email batching period: 604810 seconds") + + result = self.client_patch( + "/json/settings", {"email_notifications_batching_period_seconds": 5 * 60} + ) + self.assert_json_success(result) + hamlet = self.example_user("hamlet") + self.assertEqual(hamlet.email_notifications_batching_period_seconds, 300) + def test_toggling_boolean_user_display_settings(self) -> None: """Test updating each boolean setting in UserProfile property_types""" boolean_settings = ( diff --git a/zerver/views/user_settings.py b/zerver/views/user_settings.py index ce51010d41..1ad008f2f9 100644 --- a/zerver/views/user_settings.py +++ b/zerver/views/user_settings.py @@ -121,6 +121,9 @@ def json_change_settings( timezone: Optional[str] = REQ( str_validator=check_string_in(pytz.all_timezones_set), default=None ), + email_notifications_batching_period_seconds: Optional[int] = REQ( + json_validator=check_int, default=None + ), enable_stream_desktop_notifications: Optional[bool] = REQ( json_validator=check_bool, default=None ), @@ -169,6 +172,17 @@ def json_change_settings( ): raise JsonableError(_("Invalid notification sound '{}'").format(notification_sound)) + if email_notifications_batching_period_seconds is not None and ( + email_notifications_batching_period_seconds <= 0 + or email_notifications_batching_period_seconds > 7 * 24 * 60 * 60 + ): + # We set a limit of one week for the batching period + raise JsonableError( + _("Invalid email batching period: {} seconds").format( + email_notifications_batching_period_seconds + ) + ) + if new_password != "": return_data: Dict[str, Any] = {} if email_belongs_to_ldap(user_profile.realm, user_profile.delivery_email):