diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 29bdd4f2db..c0f22b8285 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,11 @@ below features are supported. ## Changes in Zulip 2.2 +**Feature level 18** + +* [`POST /register`](/api/register-queue): Added + `user_avatar_url_field_optional` to supported `client_capabilities`. + **Feature level 17** * [`GET users/me/subscriptions`](/api/get-subscribed-streams), [`GET /streams`] diff --git a/version.py b/version.py index c99df3fd7c..c80a9a5988 100644 --- a/version.py +++ b/version.py @@ -29,7 +29,7 @@ DESKTOP_WARNING_VERSION = "5.2.0" # # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md. -API_FEATURE_LEVEL = 17 +API_FEATURE_LEVEL = 18 # 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 diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 5acf094604..6f54851c12 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -504,8 +504,10 @@ def notify_created_user(user_profile: UserProfile) -> None: person = format_user_row(user_profile.realm, user_profile, user_row, # Since we don't know what the client # supports at this point in the code, we - # just assume client_gravatar=False :( + # just assume client_gravatar and + # user_avatar_url_field_optional = False :( client_gravatar=False, + user_avatar_url_field_optional=False, # We assume there's no custom profile # field data for a new user; initial # values are expected to be added in a diff --git a/zerver/lib/events.py b/zerver/lib/events.py index cbfb8c36d9..7641de5e32 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -88,6 +88,7 @@ def always_want(msg_type: str) -> bool: def fetch_initial_state_data(user_profile: UserProfile, event_types: Optional[Iterable[str]], queue_id: str, client_gravatar: bool, + user_avatar_url_field_optional: bool, slim_presence: bool = False, include_subscribers: bool = True) -> Dict[str, Any]: state: Dict[str, Any] = {'queue_id': queue_id} @@ -208,7 +209,8 @@ def fetch_initial_state_data(user_profile: UserProfile, if want('realm_user'): state['raw_users'] = get_raw_user_data(realm, user_profile, - client_gravatar=client_gravatar) + client_gravatar=client_gravatar, + user_avatar_url_field_optional=user_avatar_url_field_optional) # For the user's own avatar URL, we force # client_gravatar=False, since that saves some unnecessary @@ -848,6 +850,7 @@ def do_events_register(user_profile: UserProfile, user_client: Client, notification_settings_null = client_capabilities.get('notification_settings_null', False) bulk_message_deletion = client_capabilities.get('bulk_message_deletion', False) + user_avatar_url_field_optional = client_capabilities.get('user_avatar_url_field_optional', False) if user_profile.realm.email_address_visibility != Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE: # If real email addresses are not available to the user, their @@ -877,6 +880,7 @@ def do_events_register(user_profile: UserProfile, user_client: Client, ret = fetch_initial_state_data(user_profile, event_types_set, queue_id, client_gravatar=client_gravatar, + user_avatar_url_field_optional=user_avatar_url_field_optional, slim_presence=slim_presence, include_subscribers=include_subscribers) diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 5c9ebda2f3..181b635dd2 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -303,7 +303,7 @@ def compute_show_invites_and_add_streams(user_profile: Optional[UserProfile]) -> return True, True def format_user_row(realm: Realm, acting_user: UserProfile, row: Dict[str, Any], - client_gravatar: bool, + client_gravatar: bool, user_avatar_url_field_optional: bool, custom_profile_field_data: Optional[Dict[str, Any]] = None) -> Dict[str, Any]: """Formats a user row returned by a database fetch using .values(*realm_user_dict_fields) into a dictionary representation @@ -311,23 +311,13 @@ def format_user_row(realm: Realm, acting_user: UserProfile, row: Dict[str, Any], argument is used for permissions checks. """ - avatar_url = get_avatar_field(user_id=row['id'], - realm_id=realm.id, - email=row['delivery_email'], - avatar_source=row['avatar_source'], - avatar_version=row['avatar_version'], - medium=False, - client_gravatar=client_gravatar) - is_admin = is_administrator_role(row['role']) is_owner = row['role'] == UserProfile.ROLE_REALM_OWNER is_guest = row['role'] == UserProfile.ROLE_GUEST is_bot = row['is_bot'] - # This format should align with get_cross_realm_dicts() and notify_created_user result = dict( email=row['email'], user_id=row['id'], - avatar_url=avatar_url, avatar_version=row['avatar_version'], is_admin=is_admin, is_owner=is_owner, @@ -338,6 +328,35 @@ def format_user_row(realm: Realm, acting_user: UserProfile, row: Dict[str, Any], is_active = row['is_active'], date_joined = row['date_joined'].isoformat(), ) + + # Zulip clients that support using `GET /avatar/{user_id}` as a + # fallback if we didn't send an avatar URL in the user object pass + # user_avatar_url_field_optional in client_capabilities. + # + # This is a major network performance optimization for + # organizations with 10,000s of users where we would otherwise + # send avatar URLs in the payload (either because most users have + # uploaded avatars or because EMAIL_ADDRESS_VISIBILITY_ADMINS + # prevents the older client_gravatar optimization from helping). + # The performance impact is large largely because the hashes in + # avatar URLs structurally cannot compress well. + # + # The user_avatar_url_field_optional gives the server sole + # discretion in deciding for which users we want to send the + # avatar URL (Which saves clients an RTT at the cost of some + # bandwidth). At present, the server looks at `long_term_idle` to + # decide which users to include avatars for, piggy-backing on a + # different optimization for organizations with 10,000s of users. + include_avatar_url = not user_avatar_url_field_optional or not row['long_term_idle'] + if include_avatar_url: + result['avatar_url'] = get_avatar_field(user_id=row['id'], + realm_id=realm.id, + email=row['delivery_email'], + avatar_source=row['avatar_source'], + avatar_version=row['avatar_version'], + medium=False, + client_gravatar=client_gravatar) + if (realm.email_address_visibility == Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS and acting_user.is_realm_admin): result['delivery_email'] = row['delivery_email'] @@ -396,6 +415,7 @@ def get_cross_realm_dicts() -> List[Dict[str, Any]]: acting_user=user, row=user_row, client_gravatar=False, + user_avatar_url_field_optional=False, custom_profile_field_data=None)) return result @@ -416,8 +436,8 @@ def get_custom_profile_field_values(custom_profile_field_values: } return profiles_by_user_id -def get_raw_user_data(realm: Realm, acting_user: UserProfile, client_gravatar: bool, - target_user: Optional[UserProfile]=None, +def get_raw_user_data(realm: Realm, acting_user: UserProfile, *, target_user: Optional[UserProfile]=None, + client_gravatar: bool, user_avatar_url_field_optional: bool, include_custom_profile_fields: bool=True) -> Dict[int, Dict[str, str]]: """Fetches data about the target user(s) appropriate for sending to acting_user via the standard format for the Zulip API. If @@ -447,9 +467,10 @@ def get_raw_user_data(realm: Realm, acting_user: UserProfile, client_gravatar: b custom_profile_field_data = profiles_by_user_id.get(row['id'], {}) result[row['id']] = format_user_row(realm, - acting_user = acting_user, + acting_user=acting_user, row=row, - client_gravatar= client_gravatar, - custom_profile_field_data = custom_profile_field_data, + client_gravatar=client_gravatar, + user_avatar_url_field_optional=user_avatar_url_field_optional, + custom_profile_field_data=custom_profile_field_data, ) return result diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index ce0a5eda8c..b163ae8f21 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3207,6 +3207,15 @@ paths: in a loop. New in Zulip 2.2 (feature level 13). This capability is for backwards-compatibility; it will be required in a future server release. + + * `user_avatar_url_field_optional`: Boolean for whether the + client required avatar URLs for all users, or supports + using `GET /avatar/{user_id}` to access user avatars. If the + client has this capability, the server may skip sending a + `avatar_url` field in the `realm_user` at its sole discretion + to optimize network performance. This is an important optimization + in organizations with 10,000s of users. + New in Zulip 2.2 (feature level 18). schema: type: object example: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 3b9b2608ea..065022d335 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -503,8 +503,8 @@ class EventsRegisterTest(ZulipTestCase): def do_test(self, action: Callable[[], object], event_types: Optional[List[str]]=None, include_subscribers: bool=True, state_change_expected: bool=True, - notification_settings_null: bool=False, - client_gravatar: bool=True, slim_presence: bool=False, + notification_settings_null: bool=False, client_gravatar: bool=True, + user_avatar_url_field_optional: bool=False, slim_presence: bool=False, num_events: int=1, bulk_message_deletion: bool=True) -> List[Dict[str, Any]]: ''' Make sure we have a clean slate of client descriptors for these tests. @@ -536,6 +536,7 @@ class EventsRegisterTest(ZulipTestCase): hybrid_state = fetch_initial_state_data( self.user_profile, event_types, "", client_gravatar=client_gravatar, + user_avatar_url_field_optional=user_avatar_url_field_optional, slim_presence=slim_presence, include_subscribers=include_subscribers, ) @@ -567,6 +568,7 @@ class EventsRegisterTest(ZulipTestCase): normal_state = fetch_initial_state_data( self.user_profile, event_types, "", client_gravatar=client_gravatar, + user_avatar_url_field_optional=user_avatar_url_field_optional, slim_presence=slim_presence, include_subscribers=include_subscribers, ) @@ -2052,7 +2054,7 @@ class EventsRegisterTest(ZulipTestCase): def test_realm_update_plan_type(self) -> None: realm = self.user_profile.realm - state_data = fetch_initial_state_data(self.user_profile, None, "", False) + state_data = fetch_initial_state_data(self.user_profile, None, "", False, False) self.assertEqual(state_data['realm_plan_type'], Realm.SELF_HOSTED) self.assertEqual(state_data['zulip_plan_is_not_limited'], True) @@ -2069,7 +2071,7 @@ class EventsRegisterTest(ZulipTestCase): error = schema_checker('events[0]', events[0]) self.assert_on_error(error) - state_data = fetch_initial_state_data(self.user_profile, None, "", False) + state_data = fetch_initial_state_data(self.user_profile, None, "", False, False) self.assertEqual(state_data['realm_plan_type'], Realm.LIMITED) self.assertEqual(state_data['zulip_plan_is_not_limited'], False) @@ -2846,7 +2848,7 @@ class EventsRegisterTest(ZulipTestCase): lambda: do_delete_messages(self.user_profile.realm, [message]), state_change_expected=True, ) - result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) + result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False) self.assertEqual(result['max_message_id'], -1) def test_add_attachment(self) -> None: @@ -3069,7 +3071,7 @@ class FetchInitialStateDataTest(ZulipTestCase): def test_realm_bots_non_admin(self) -> None: user_profile = self.example_user('cordelia') self.assertFalse(user_profile.is_realm_admin) - result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) + result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False) self.assert_length(result['realm_bots'], 0) # additionally the API key for a random bot is not present in the data @@ -3081,14 +3083,14 @@ class FetchInitialStateDataTest(ZulipTestCase): user_profile = self.example_user('hamlet') do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR) self.assertTrue(user_profile.is_realm_admin) - result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) + result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False) self.assertTrue(len(result['realm_bots']) > 2) def test_max_message_id_with_no_history(self) -> None: user_profile = self.example_user('aaron') # Delete all historical messages for this user UserMessage.objects.filter(user_profile=user_profile).delete() - result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) + result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False) self.assertEqual(result['max_message_id'], -1) def test_delivery_email_presence_for_non_admins(self) -> None: @@ -3097,13 +3099,13 @@ class FetchInitialStateDataTest(ZulipTestCase): do_set_realm_property(user_profile.realm, "email_address_visibility", Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE) - result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) + result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False) for key, value in result['raw_users'].items(): self.assertNotIn('delivery_email', value) do_set_realm_property(user_profile.realm, "email_address_visibility", Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS) - result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) + result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False) for key, value in result['raw_users'].items(): self.assertNotIn('delivery_email', value) @@ -3113,16 +3115,62 @@ class FetchInitialStateDataTest(ZulipTestCase): do_set_realm_property(user_profile.realm, "email_address_visibility", Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE) - result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) + result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False) for key, value in result['raw_users'].items(): self.assertNotIn('delivery_email', value) do_set_realm_property(user_profile.realm, "email_address_visibility", Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS) - result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False) + result = fetch_initial_state_data(user_profile, None, "", client_gravatar=False, user_avatar_url_field_optional=False) for key, value in result['raw_users'].items(): self.assertIn('delivery_email', value) + def test_user_avatar_url_field_optional(self) -> None: + hamlet = self.example_user('hamlet') + users = [ + self.example_user('iago'), + self.example_user('cordelia'), + self.example_user('ZOE'), + self.example_user('othello'), + ] + + for user in users: + user.long_term_idle = True + user.save() + + long_term_idle_users_ids = [user.id for user in users] + + result = fetch_initial_state_data(user_profile=hamlet, + event_types=None, + queue_id='', + client_gravatar=False, + user_avatar_url_field_optional=True) + + raw_users = result['raw_users'] + + for user_dict in raw_users.values(): + if user_dict['user_id'] in long_term_idle_users_ids: + self.assertFalse('avatar_url' in user_dict) + else: + self.assertIsNotNone(user_dict['avatar_url']) + + gravatar_users_id = [user_dict['user_id'] for user_dict in raw_users.values() + if 'avatar_url' in user_dict and 'gravatar.com' in user_dict['avatar_url']] + + # Test again with client_gravatar = True + result = fetch_initial_state_data(user_profile=hamlet, + event_types=None, + queue_id='', + client_gravatar=True, + user_avatar_url_field_optional=True) + + raw_users = result['raw_users'] + + for user_dict in raw_users.values(): + if user_dict['user_id'] in gravatar_users_id: + self.assertIsNone(user_dict['avatar_url']) + else: + self.assertFalse('avatar_url' in user_dict) class GetUnreadMsgsTest(ZulipTestCase): def mute_stream(self, user_profile: UserProfile, stream: Stream) -> None: @@ -3836,6 +3884,7 @@ class FetchQueriesTest(ZulipTestCase): event_types=None, queue_id='x', client_gravatar=False, + user_avatar_url_field_optional=False ) self.assert_length(queries, 30) @@ -3892,6 +3941,7 @@ class FetchQueriesTest(ZulipTestCase): event_types=event_types, queue_id='x', client_gravatar=False, + user_avatar_url_field_optional=False ) self.assert_length(queries, count) @@ -3971,7 +4021,8 @@ class TestEventsRegisterNarrowDefaults(ZulipTestCase): class TestGetRawUserDataSystemBotRealm(ZulipTestCase): def test_get_raw_user_data_on_system_bot_realm(self) -> None: - result = get_raw_user_data(get_realm("zulipinternal"), self.example_user('hamlet'), True) + result = get_raw_user_data(get_realm("zulipinternal"), self.example_user('hamlet'), + client_gravatar=True, user_avatar_url_field_optional=True) for bot_email in settings.CROSS_REALM_BOT_EMAILS: bot_profile = get_system_bot(bot_email) diff --git a/zerver/views/events_register.py b/zerver/views/events_register.py index d48bd0f821..b91e44629e 100644 --- a/zerver/views/events_register.py +++ b/zerver/views/events_register.py @@ -40,6 +40,7 @@ def events_register_backend( ], [ # Any new fields of `client_capabilities` should be optional. Add them here. ("bulk_message_deletion", check_bool), + ('user_avatar_url_field_optional', check_bool), ]), default=None), event_types: Optional[Iterable[str]]=REQ(validator=check_list(check_string), default=None), fetch_event_types: Optional[Iterable[str]]=REQ(validator=check_list(check_string), default=None), diff --git a/zerver/views/users.py b/zerver/views/users.py index aa2efe4858..a3ffd42407 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -451,8 +451,9 @@ def get_members_backend(request: HttpRequest, user_profile: UserProfile, user_id target_user = access_user_by_id(user_profile, user_id, allow_deactivated=True, allow_bots=True, read_only=True) - members = get_raw_user_data(realm, user_profile, client_gravatar=client_gravatar, - target_user=target_user, + members = get_raw_user_data(realm, user_profile, target_user=target_user, + client_gravatar=client_gravatar, + user_avatar_url_field_optional=False, include_custom_profile_fields=include_custom_profile_fields) if target_user is not None: @@ -501,7 +502,9 @@ def create_user_backend(request: HttpRequest, user_profile: UserProfile, def get_profile_backend(request: HttpRequest, user_profile: UserProfile) -> HttpResponse: raw_user_data = get_raw_user_data(user_profile.realm, user_profile, - client_gravatar=False, target_user=user_profile) + target_user=user_profile, + client_gravatar=False, + user_avatar_url_field_optional=False) result: Dict[str, Any] = raw_user_data[user_profile.id] result['max_message_id'] = -1