From 24f527cb599abf4da9b52e6dcbea29716436c45b Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 3 May 2022 14:22:41 -0700 Subject: [PATCH] home: Don't send /register response for spectators. In very large communities, computing page_params can be quite expensive. Because we've moved the homepage for communities with web public streams enabled to be the Zulip app, and it's common for automation to frequently poll the homepage of a Zulip organization, we'd like to keep those homepages cheap (as the login pages are). We address this by prototyping something we may end up wanting to do anyway -- having the web application do a `POST /register` API call in order to fetch most page_params, and merging those with the mostly webapp configuration page_params that we leave in the / response for convenience. This exact implementation is messy in a few ways: * We rely on the assumption that ui_init.initialize_everything happens before all code that needs to inspect the page_params properties we are fetching via /register. This is likely mostly true, but nothing in the implementation enforces it. * The bundle of ~25 keys that are in page_params ideally would be considered individually, with some moved to the /register API response and perhaps others eliminated or namespaced inside a webapp_settings object. * It's weird to have the spectators network sequence different that from logged-in users, and potentially a maintainability risk. * We might be able to arrange that the initial `/` response be cacheable, now that we're no longer embedding our metadata inside it. We've made no effort to do that as of yet. Despite those issues, this commit solves an immediate problem and will give us helpful experience with a model closer to the one we'll want in order to happily support a web client that can be run locally against a production Zulip server's data. Co-authored-by: Anders Kaseorg --- .eslintrc.json | 3 +- static/js/ui_init.js | 21 +++++++++++++- zerver/lib/home.py | 29 ++++++------------- zerver/tests/test_home.py | 60 +++++++++++++++++++-------------------- 4 files changed, 60 insertions(+), 53 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 802f87aea0..3041aa2963 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -93,6 +93,7 @@ "no-undef-init": "error", "no-unneeded-ternary": ["error", {"defaultAssignment": false}], "no-unused-expressions": "error", + "no-unused-vars": ["error", {"ignoreRestSiblings": true}], "no-use-before-define": ["error", {"functions": false}], "no-useless-concat": "error", "no-useless-constructor": "error", @@ -188,7 +189,7 @@ "@typescript-eslint/no-non-null-assertion": "off", "@typescript-eslint/no-parameter-properties": "error", "@typescript-eslint/no-unnecessary-qualifier": "error", - "@typescript-eslint/no-unused-vars": ["error", {"varsIgnorePattern": "^_"}], + "@typescript-eslint/no-unused-vars": ["error", {"ignoreRestSiblings": true}], "@typescript-eslint/no-unsafe-argument": "off", "@typescript-eslint/no-unsafe-assignment": "off", "@typescript-eslint/no-unsafe-call": "off", diff --git a/static/js/ui_init.js b/static/js/ui_init.js index a28a929daa..62d1eac689 100644 --- a/static/js/ui_init.js +++ b/static/js/ui_init.js @@ -16,6 +16,7 @@ import * as activity from "./activity"; import * as alert_words from "./alert_words"; import * as blueslip from "./blueslip"; import * as bot_data from "./bot_data"; +import * as channel from "./channel"; import * as click_handlers from "./click_handlers"; import * as common from "./common"; import * as compose from "./compose"; @@ -683,7 +684,25 @@ export function initialize_everything() { $("#app-loading").addClass("loaded"); } -$(() => { +$(async () => { + if (page_params.is_spectator) { + const data = { + apply_markdown: true, + client_capabilities: JSON.stringify({ + notification_settings_null: true, + bulk_message_deletion: true, + user_avatar_url_field_optional: true, + // Set this to true when stream typing notifications are implemented. + stream_typing_notifications: false, + user_settings_object: true, + }), + client_gravatar: false, + }; + const {result, msg, ...state} = await new Promise((success, error) => { + channel.post({url: "/json/register", data, success, error}); + }); + Object.assign(page_params, state); + } blueslip.measure_time("initialize_everything", () => { initialize_everything(); }); diff --git a/zerver/lib/home.py b/zerver/lib/home.py index d009ab1dc2..76ab1aa250 100644 --- a/zerver/lib/home.py +++ b/zerver/lib/home.py @@ -153,32 +153,21 @@ def build_page_params_for_home_page_load( narrow=narrow, include_streams=False, ) + default_language = register_ret["user_settings"]["default_language"] else: - # Since events for spectator is not implemented, we only fetch the data - # at the time of request and don't register for any events. - # TODO: Implement events for spectator. - from zerver.lib.events import fetch_initial_state_data, post_process_state - - register_ret = fetch_initial_state_data( - user_profile, - realm=realm, - event_types=None, - queue_id=None, - client_gravatar=False, - user_avatar_url_field_optional=client_capabilities["user_avatar_url_field_optional"], - user_settings_object=client_capabilities["user_settings_object"], - slim_presence=False, - include_subscribers=False, - include_streams=False, - ) - - post_process_state(user_profile, register_ret, False) + # The spectator client will be fetching the /register response + # for spectators via the API. But we still need to set the + # values not presence in that object. + register_ret = { + "queue_id": None, + } + default_language = realm.default_language furthest_read_time = get_furthest_read_time(user_profile) request_language = get_and_set_request_language( request, - register_ret["user_settings"]["default_language"], + default_language, translation.get_language_from_path(request.path_info), ) diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 632ad8f640..dc9056862c 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -329,38 +329,36 @@ class HomeTest(ZulipTestCase): page_params = self._get_page_params(result) self.assertEqual(page_params["is_spectator"], True) actual_keys = sorted(str(k) for k in page_params.keys()) - removed_keys = [ - "custom_profile_field_types", - "custom_profile_fields", - "last_event_id", - "narrow", - "narrow_stream", + expected_keys = [ + "apps_page_url", + "bot_types", + "corporate_enabled", + "development_environment", + "first_in_realm", + "furthest_read_time", + "insecure_desktop_app", + "is_spectator", + "language_list", + "login_page", + "needs_tutorial", + "no_event_queue", + "promote_sponsoring_zulip", + "prompt_for_invites", + "queue_id", + "realm_rendered_description", + "request_language", + "search_pills_enabled", + "show_billing", + "show_plans", + "show_webathena", + "test_suite", + "translation_data", + "two_fa_enabled", + "two_fa_enabled_user", + "warn_no_email", + "webpack_public_path", ] - expected_keys = set(self.expected_page_params_keys + ["realm_rendered_description"]) - set( - removed_keys - ) - self.assertEqual(set(actual_keys), set(expected_keys)) - - # Test information passed to client about users. - page_params = self._get_page_params(result) - self.assertEqual( - sorted(page_params["realm_users"][0].keys()), - [ - "avatar_url", - "avatar_version", - "date_joined", - "email", - "full_name", - "is_admin", - "is_bot", - "is_guest", - "is_owner", - "role", - "user_id", - ], - ) - date_length = len("YYYY-MM-DD") - self.assert_length(page_params["realm_users"][0]["date_joined"], date_length) + self.assertEqual(actual_keys, expected_keys) def test_home_under_2fa_without_otp_device(self) -> None: with self.settings(TWO_FACTOR_AUTHENTICATION_ENABLED=True):