settings: Merge settings API endpoints.

This API change removes unnecessary complexity from a client that
wants to change a user's personal settings, and also saves developers
from needing to make decisions about what sort of setting something is
at the API level.

We preserve the old settings endpoints as mapping to the same function
as the new one for backwards-compatibility. We delete the
documentation for the old endpoints, though the documentation for the
merged /settings endpoint mentions how to use the old endpoints when
needed.

We migrate all backend tests to the new endpoints, except for
individual tests for each legacy endpoint to verify they still work.

Co-authored-by: sahil839 <sahilbatra839@gmail.com>

templates/zerver/api/changelog.md

@@ -11,6 +11,14 @@ below features are supported.
 
 ## Changes in Zulip 5.0
 
+**Feature level 80**
+
+* [`PATCH /settings`](/api/update-settings): The
+  `/settings/notifications` and `/settings/display` endpoints were
+  merged into the main `/settings` endpoint; now all personal settings
+  should be edited using that single endpoint. The old URLs are
+  preserved as deprecated aliases for backwards compatibility.
+
 **Feature level 79**
 
 * [`GET /users/me/subscriptions`](/api/get-subscriptions): The
@@ -22,12 +30,13 @@ below features are supported.
 
 **Feature level 78**
 
-* `PATCH /settings`: Added `ignored_parameters_unsupported` field,
-  which is a list of parameters that were ignored by the endpoint,
-  to the response object.
+* [`PATCH /settings`](/api/update-settings): Added
+  `ignored_parameters_unsupported` field, which is a list of
+  parameters that were ignored by the endpoint, to the response
+  object.
 
-* `PATCH /settings`: Removed `full_name` and `account_email` fields
-  from the response object.
+* [`PATCH /settings`](/api/update-settings): Removed `full_name` and
+  `account_email` fields from the response object.
 
 **Feature level 77**
 

templates/zerver/help/include/rest-endpoints.md

@@ -44,8 +44,7 @@
 * [Set "typing" status](/api/set-typing-status)
 * [Get user presence](/api/get-user-presence)
 * [Get attachments](/api/get-attachments)
-* [Update display settings](/api/update-display-settings)
-* [Update notification settings](/api/update-notification-settings)
+* [Update settings](/api/update-settings)
 * [Get user groups](/api/get-user-groups)
 * [Create a user group](/api/create-user-group)
 * [Update a user group](/api/update-user-group)

tools/linter_lib/custom_check.py

@@ -820,7 +820,6 @@ markdown_docs_length_exclude = {
     "templates/zerver/api/get-messages.md",
     # This macro has a long indented URL
     "templates/zerver/help/include/git-webhook-url-with-branches-indented.md",
-    "templates/zerver/api/update-notification-settings.md",
     # These two are the same file and have some too-long lines for GitHub badges
     "README.md",
     "docs/overview/readme.md",

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 = 79
+API_FEATURE_LEVEL = 80
 
 # 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/openapi/openapi.py

@@ -28,9 +28,7 @@ EXCLUDE_UNDOCUMENTED_ENDPOINTS = {
 }
 # Consists of endpoints with some documentation remaining.
 # These are skipped but return true as the validator cannot exclude objects
-EXCLUDE_DOCUMENTED_ENDPOINTS = {
-    ("/settings/notifications", "patch"),
-}
+EXCLUDE_DOCUMENTED_ENDPOINTS: Set[Tuple[str, str]] = set([])
 
 # Most of our code expects allOf to be preprocessed away because that is what
 # yamole did.  Its algorithm for doing so is not standards compliant, but we

zerver/openapi/python_examples.py

@@ -1132,35 +1132,20 @@ def get_server_settings(client: Client) -> None:
     validate_against_openapi_schema(result, "/server_settings", "get", "200")
 
 
-@openapi_test_function("/settings/notifications:patch")
-def update_notification_settings(client: Client) -> None:
+@openapi_test_function("/settings:patch")
+def update_settings(client: Client) -> None:
 
     # {code_example|start}
-    # Enable push notifications even when online
+    # Enable push notifications even when online and change emojiset
     request = {
         "enable_offline_push_notifications": True,
         "enable_online_push_notifications": True,
-    }
-    result = client.update_notification_settings(request)
-    # {code_example|end}
-
-    validate_against_openapi_schema(result, "/settings/notifications", "patch", "200")
-
-
-@openapi_test_function("/settings/display:patch")
-def update_display_settings(client: Client) -> None:
-
-    # {code_example|start}
-    # Show user list on left sidebar in narrow windows.
-    # Change emoji set used for display to Google modern.
-    request = {
-        "left_side_userlist": True,
         "emojiset": "google",
     }
-    result = client.call_endpoint("settings/display", method="PATCH", request=request)
+    result = client.call_endpoint("/settings", method="PATCH", request=request)
     # {code_example|end}
 
-    validate_against_openapi_schema(result, "/settings/display", "patch", "200")
+    validate_against_openapi_schema(result, "/settings", "patch", "200")
 
 
 @openapi_test_function("/user_uploads:post")
@@ -1488,8 +1473,7 @@ def test_users(client: Client, owner_client: Client) -> None:
     get_user_by_email(client)
     get_subscription_status(client)
     get_profile(client)
-    update_notification_settings(client)
-    update_display_settings(client)
+    update_settings(client)
     upload_file(client)
     get_attachments(client)
     set_typing_status(client)
@@ -1520,7 +1504,6 @@ def test_streams(client: Client, nonadmin_client: Client) -> None:
     remove_subscriptions(client)
     toggle_mute_topic(client)
     update_subscription_settings(client)
-    update_notification_settings(client)
     get_stream_topics(client, 1)
     archive_stream(client, stream_id)
 

zerver/tests/test_openapi.py

@@ -253,7 +253,6 @@ class OpenAPIArgumentsTest(ZulipTestCase):
         "/users/me/android_gcm_reg_id",
         "/users/me/apns_device_token",
         #### These personal settings endpoints have modest value to document:
-        "/settings",
         "/users/me/avatar",
         "/users/me/api_key/regenerate",
         # Much more valuable would be an org admin bulk-upload feature.

zerver/tests/test_settings.py

@@ -158,12 +158,10 @@ class ChangeSettingsTest(ZulipTestCase):
             #
             # TODO: Make this work more like do_test_realm_update_api
             if notification_setting != "notification_sound":
-                self.check_for_toggle_param_patch(
-                    "/json/settings/notifications", notification_setting
-                )
+                self.check_for_toggle_param_patch("/json/settings", notification_setting)
 
     def test_change_notification_sound(self) -> None:
-        pattern = "/json/settings/notifications"
+        pattern = "/json/settings"
         param = "notification_sound"
         user_profile = self.example_user("hamlet")
         self.login_user(user_profile)
@@ -191,7 +189,7 @@ class ChangeSettingsTest(ZulipTestCase):
             s for s in UserProfile.property_types if UserProfile.property_types[s] is bool
         )
         for display_setting in boolean_settings:
-            self.check_for_toggle_param_patch("/json/settings/display", display_setting)
+            self.check_for_toggle_param_patch("/json/settings", display_setting)
 
     def test_enter_sends_setting(self) -> None:
         self.check_for_toggle_param("/json/users/me/enter-sends", "enter_sends")
@@ -344,7 +342,7 @@ class ChangeSettingsTest(ZulipTestCase):
         else:
             data = {setting_name: orjson.dumps(test_value).decode()}
 
-        result = self.client_patch("/json/settings/display", data)
+        result = self.client_patch("/json/settings", data)
         self.assert_json_success(result)
         user_profile = self.example_user("hamlet")
         self.assertEqual(getattr(user_profile, setting_name), test_value)
@@ -356,7 +354,7 @@ class ChangeSettingsTest(ZulipTestCase):
         else:
             data = {setting_name: orjson.dumps(invalid_value).decode()}
 
-        result = self.client_patch("/json/settings/display", data)
+        result = self.client_patch("/json/settings", data)
         # the json error for multiple word setting names (ex: default_language)
         # displays as 'Invalid language'. Using setting_name.split('_') to format.
         self.assert_json_error(result, f"Invalid {setting_name}")
@@ -376,7 +374,7 @@ class ChangeSettingsTest(ZulipTestCase):
     def do_change_emojiset(self, emojiset: str) -> HttpResponse:
         self.login("hamlet")
         data = {"emojiset": emojiset}
-        result = self.client_patch("/json/settings/display", data)
+        result = self.client_patch("/json/settings", data)
         return result
 
     def test_emojiset(self) -> None:
@@ -415,6 +413,35 @@ class ChangeSettingsTest(ZulipTestCase):
         self.assertIn("ignored_parameters_unsupported", result)
         self.assertEqual(result["ignored_parameters_unsupported"], ["invalid_setting"])
 
+    def test_changing_setting_using_display_setting_endpoint(self) -> None:
+        """
+        This test is just for adding coverage for `/settings/display` endpoint which is
+        now depreceated.
+        """
+        self.login("hamlet")
+
+        result = self.client_patch(
+            "/json/settings/display", dict(color_scheme=UserProfile.COLOR_SCHEME_NIGHT)
+        )
+        self.assert_json_success(result)
+        hamlet = self.example_user("hamlet")
+        self.assertEqual(hamlet.color_scheme, UserProfile.COLOR_SCHEME_NIGHT)
+
+    def test_changing_setting_using_notification_setting_endpoint(self) -> None:
+        """
+        This test is just for adding coverage for `/settings/notifications` endpoint which is
+        now depreceated.
+        """
+        self.login("hamlet")
+
+        result = self.client_patch(
+            "/json/settings/notifications",
+            dict(enable_stream_desktop_notifications=orjson.dumps(True).decode()),
+        )
+        self.assert_json_success(result)
+        hamlet = self.example_user("hamlet")
+        self.assertEqual(hamlet.enable_stream_desktop_notifications, True)
+
 
 class UserChangesTest(ZulipTestCase):
     def test_update_api_key(self) -> None:

zerver/views/user_settings.py

@@ -87,6 +87,10 @@ def confirm_email_change(request: HttpRequest, confirmation_key: str) -> HttpRes
     return render(request, "confirmation/confirm_email_change.html", context=ctx)
 
 
+emojiset_choices = {emojiset["key"] for emojiset in UserProfile.emojiset_choices()}
+default_view_options = ["recent_topics", "all_messages"]
+
+
 @human_users_only
 @has_request_variables
 def json_change_settings(
@@ -96,7 +100,74 @@ def json_change_settings(
     email: str = REQ(default=""),
     old_password: str = REQ(default=""),
     new_password: str = REQ(default=""),
+    twenty_four_hour_time: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    dense_mode: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    starred_message_counts: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    fluid_layout_width: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    high_contrast_mode: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    color_scheme: Optional[int] = REQ(
+        json_validator=check_int_in(UserProfile.COLOR_SCHEME_CHOICES), default=None
+    ),
+    translate_emoticons: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    default_language: Optional[str] = REQ(default=None),
+    default_view: Optional[str] = REQ(
+        str_validator=check_string_in(default_view_options), default=None
+    ),
+    left_side_userlist: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    emojiset: Optional[str] = REQ(str_validator=check_string_in(emojiset_choices), default=None),
+    demote_inactive_streams: Optional[int] = REQ(
+        json_validator=check_int_in(UserProfile.DEMOTE_STREAMS_CHOICES), default=None
+    ),
+    timezone: Optional[str] = REQ(
+        str_validator=check_string_in(pytz.all_timezones_set), default=None
+    ),
+    enable_stream_desktop_notifications: Optional[bool] = REQ(
+        json_validator=check_bool, default=None
+    ),
+    enable_stream_email_notifications: Optional[bool] = REQ(
+        json_validator=check_bool, default=None
+    ),
+    enable_stream_push_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    enable_stream_audible_notifications: Optional[bool] = REQ(
+        json_validator=check_bool, default=None
+    ),
+    wildcard_mentions_notify: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    notification_sound: Optional[str] = REQ(default=None),
+    enable_desktop_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    enable_sounds: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    enable_offline_email_notifications: Optional[bool] = REQ(
+        json_validator=check_bool, default=None
+    ),
+    enable_offline_push_notifications: Optional[bool] = REQ(
+        json_validator=check_bool, default=None
+    ),
+    enable_online_push_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    enable_digest_emails: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    enable_login_emails: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    enable_marketing_emails: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    message_content_in_email_notifications: Optional[bool] = REQ(
+        json_validator=check_bool, default=None
+    ),
+    pm_content_in_desktop_notifications: Optional[bool] = REQ(
+        json_validator=check_bool, default=None
+    ),
+    desktop_icon_count_display: Optional[int] = REQ(json_validator=check_int, default=None),
+    realm_name_in_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
+    presence_enabled: Optional[bool] = REQ(json_validator=check_bool, default=None),
 ) -> HttpResponse:
+    # We can't use REQ for this widget because
+    # get_available_language_codes requires provisioning to be
+    # complete.
+    if default_language is not None and default_language not in get_available_language_codes():
+        raise JsonableError(_("Invalid default_language"))
+
+    if (
+        notification_sound is not None
+        and notification_sound not in get_available_notification_sounds()
+        and notification_sound != "none"
+    ):
+        raise JsonableError(_("Invalid notification sound '{}'").format(notification_sound))
+
     if new_password != "":
         return_data: Dict[str, Any] = {}
         if email_belongs_to_ldap(user_profile.realm, user_profile.delivery_email):
@@ -172,6 +243,23 @@ def json_change_settings(
             # Note that check_change_full_name strips the passed name automatically
             check_change_full_name(user_profile, full_name, user_profile)
 
+    # Loop over user_profile.property_types
+    request_settings = {k: v for k, v in list(locals().items()) if k in user_profile.property_types}
+    for k, v in list(request_settings.items()):
+        if v is not None and getattr(user_profile, k) != v:
+            do_set_user_display_setting(user_profile, k, v)
+
+    req_vars = {
+        k: v for k, v in list(locals().items()) if k in user_profile.notification_setting_types
+    }
+
+    for k, v in list(req_vars.items()):
+        if v is not None and getattr(user_profile, k) != v:
+            do_change_notification_settings(user_profile, k, v, acting_user=user_profile)
+
+    if timezone is not None and user_profile.timezone != timezone:
+        do_set_user_display_setting(user_profile, "timezone", timezone)
+
     # TODO: Do this more generally.
     from zerver.lib.request import get_request_notes
 
@@ -186,120 +274,6 @@ def json_change_settings(
     return json_success(result)
 
 
-emojiset_choices = {emojiset["key"] for emojiset in UserProfile.emojiset_choices()}
-default_view_options = ["recent_topics", "all_messages"]
-
-
-@human_users_only
-@has_request_variables
-def update_display_settings_backend(
-    request: HttpRequest,
-    user_profile: UserProfile,
-    twenty_four_hour_time: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    dense_mode: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    starred_message_counts: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    fluid_layout_width: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    high_contrast_mode: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    color_scheme: Optional[int] = REQ(
-        json_validator=check_int_in(UserProfile.COLOR_SCHEME_CHOICES), default=None
-    ),
-    translate_emoticons: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    default_language: Optional[str] = REQ(default=None),
-    default_view: Optional[str] = REQ(
-        str_validator=check_string_in(default_view_options), default=None
-    ),
-    left_side_userlist: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    emojiset: Optional[str] = REQ(str_validator=check_string_in(emojiset_choices), default=None),
-    demote_inactive_streams: Optional[int] = REQ(
-        json_validator=check_int_in(UserProfile.DEMOTE_STREAMS_CHOICES), default=None
-    ),
-    timezone: Optional[str] = REQ(
-        str_validator=check_string_in(pytz.all_timezones_set), default=None
-    ),
-) -> HttpResponse:
-
-    # We can't use REQ for this widget because
-    # get_available_language_codes requires provisioning to be
-    # complete.
-    if default_language is not None and default_language not in get_available_language_codes():
-        raise JsonableError(_("Invalid default_language"))
-
-    request_settings = {k: v for k, v in list(locals().items()) if k in user_profile.property_types}
-    result: Dict[str, Any] = {}
-    for k, v in list(request_settings.items()):
-        if v is not None and getattr(user_profile, k) != v:
-            do_set_user_display_setting(user_profile, k, v)
-            result[k] = v
-
-    if timezone is not None and user_profile.timezone != timezone:
-        do_set_user_display_setting(user_profile, "timezone", timezone)
-        result["timezone"] = timezone
-
-    return json_success(result)
-
-
-@human_users_only
-@has_request_variables
-def json_change_notify_settings(
-    request: HttpRequest,
-    user_profile: UserProfile,
-    enable_stream_desktop_notifications: Optional[bool] = REQ(
-        json_validator=check_bool, default=None
-    ),
-    enable_stream_email_notifications: Optional[bool] = REQ(
-        json_validator=check_bool, default=None
-    ),
-    enable_stream_push_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    enable_stream_audible_notifications: Optional[bool] = REQ(
-        json_validator=check_bool, default=None
-    ),
-    wildcard_mentions_notify: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    notification_sound: Optional[str] = REQ(default=None),
-    enable_desktop_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    enable_sounds: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    enable_offline_email_notifications: Optional[bool] = REQ(
-        json_validator=check_bool, default=None
-    ),
-    enable_offline_push_notifications: Optional[bool] = REQ(
-        json_validator=check_bool, default=None
-    ),
-    enable_online_push_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    enable_digest_emails: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    enable_login_emails: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    enable_marketing_emails: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    message_content_in_email_notifications: Optional[bool] = REQ(
-        json_validator=check_bool, default=None
-    ),
-    pm_content_in_desktop_notifications: Optional[bool] = REQ(
-        json_validator=check_bool, default=None
-    ),
-    desktop_icon_count_display: Optional[int] = REQ(json_validator=check_int, default=None),
-    realm_name_in_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    presence_enabled: Optional[bool] = REQ(json_validator=check_bool, default=None),
-) -> HttpResponse:
-    result = {}
-
-    # Stream notification settings.
-
-    if (
-        notification_sound is not None
-        and notification_sound not in get_available_notification_sounds()
-        and notification_sound != "none"
-    ):
-        raise JsonableError(_("Invalid notification sound '{}'").format(notification_sound))
-
-    req_vars = {
-        k: v for k, v in list(locals().items()) if k in user_profile.notification_setting_types
-    }
-
-    for k, v in list(req_vars.items()):
-        if v is not None and getattr(user_profile, k) != v:
-            do_change_notification_settings(user_profile, k, v, acting_user=user_profile)
-            result[k] = v
-
-    return json_success(result)
-
-
 def set_avatar_backend(request: HttpRequest, user_profile: UserProfile) -> HttpResponse:
     if len(request.FILES) != 1:
         raise JsonableError(_("You must upload exactly one avatar."))

zproject/urls.py

@@ -182,11 +182,9 @@ from zerver.views.user_settings import (
     change_enter_sends,
     confirm_email_change,
     delete_avatar_backend,
-    json_change_notify_settings,
     json_change_settings,
     regenerate_api_key,
     set_avatar_backend,
-    update_display_settings_backend,
 )
 from zerver.views.users import (
     add_bot_backend,
@@ -414,8 +412,13 @@ v1_api_and_json_patterns = [
     ),
     # settings -> zerver.views.user_settings
     rest_path("settings", PATCH=json_change_settings),
-    rest_path("settings/display", PATCH=update_display_settings_backend),
-    rest_path("settings/notifications", PATCH=json_change_notify_settings),
+    # These next two are legacy aliases for /settings, from before
+    # we merged the endpoints. They are documented in the `/json/settings`
+    # documentation, rather than having dedicated pages.
+    rest_path("settings/display", PATCH=(json_change_settings, {"intentionally_undocumented"})),
+    rest_path(
+        "settings/notifications", PATCH=(json_change_settings, {"intentionally_undocumented"})
+    ),
     # users/me/alert_words -> zerver.views.alert_words
     rest_path(
         "users/me/alert_words",