diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 8f7a2f3d48d..5eb81dcce71 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -14,6 +14,7 @@ import type { } from '../../api/modelTypes'; import type { Action, GlobalState, RealmState } from '../../reduxTypes'; import type { Auth, Account, Outbox } from '../../types'; +import { UploadedAvatarURL } from '../../utils/avatar'; import { ZulipVersion } from '../../utils/zulipVersion'; import { ACCOUNT_SWITCH, @@ -88,7 +89,10 @@ const userOrBotProperties = ({ name: _name }) => { const name = _name ?? randString(); const capsName = name.substring(0, 1).toUpperCase() + name.substring(1); return deepFreeze({ - avatar_url: `https://zulip.example.org/yo/avatar-${name}.png`, + avatar_url: UploadedAvatarURL.validateAndConstructInstance({ + realm: new URL('https://zulip.example.org'), + absoluteOrRelativeUrl: `/yo/avatar-${name}.png`, + }), date_joined: `2014-04-${randInt(30) .toString() @@ -266,12 +270,11 @@ const messagePropertiesBase = deepFreeze({ }); const messagePropertiesFromSender = (user: User) => { - const { avatar_url, user_id: sender_id, email: sender_email, full_name: sender_full_name } = user; + const { user_id: sender_id, email: sender_email, full_name: sender_full_name } = user; return deepFreeze({ sender_domain: '', - - avatar_url, + avatar_url: user.avatar_url, client: 'ExampleClient', gravatar_hash: 'd3adb33f', sender_email, @@ -366,7 +369,6 @@ export const streamMessage = (args?: {| const outboxMessageBase: $Diff = deepFreeze({ isOutbox: true, isSent: false, - avatar_url: selfUser.avatar_url, content: '

Test.

', display_recipient: stream.name, diff --git a/src/account-info/AccountDetails.js b/src/account-info/AccountDetails.js index a565bcbee08..7d9fc31055f 100644 --- a/src/account-info/AccountDetails.js +++ b/src/account-info/AccountDetails.js @@ -6,10 +6,9 @@ import type { UserOrBot, Dispatch } from '../types'; import styles, { createStyleSheet } from '../styles'; import { connect } from '../react-redux'; import { UserAvatar, ComponentList, RawLabel } from '../common'; -import { getCurrentRealm, getUserStatusTextForUser } from '../selectors'; +import { getUserStatusTextForUser } from '../selectors'; import PresenceStatusIndicator from '../common/PresenceStatusIndicator'; import ActivityText from '../title/ActivityText'; -import { getAvatarFromUser } from '../utils/avatar'; import { nowInTimeZone } from '../utils/date'; const componentStyles = createStyleSheet({ @@ -30,10 +29,7 @@ const componentStyles = createStyleSheet({ }, }); -const AVATAR_SIZE = 200; - type SelectorProps = {| - realm: URL, userStatusText: string | void, |}; @@ -46,7 +42,7 @@ type Props = $ReadOnly<{| class AccountDetails extends PureComponent { render() { - const { realm, user, userStatusText } = this.props; + const { user, userStatusText } = this.props; let localTime: string | null = null; // See comments at CrossRealmBot and User at src/api/modelTypes.js. @@ -62,7 +58,7 @@ class AccountDetails extends PureComponent { return ( - + { } export default connect((state, props) => ({ - realm: getCurrentRealm(state), userStatusText: getUserStatusTextForUser(state, props.user.user_id), }))(AccountDetails); diff --git a/src/actionTypes.js b/src/actionTypes.js index cd51ddc49de..01eb2e3d29a 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -414,9 +414,11 @@ type EventUserRemoveAction = {| |}; type EventUserUpdateAction = {| + ...ServerEvent, type: typeof EVENT_USER_UPDATE, - // In reality there's more -- but this will prevent accidentally using - // the type before going and adding those other properties here properly. + userId: number, + // Include only the fields that should be overwritten. + person: $Shape, |}; type EventMutedTopicsAction = {| diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index 5ff6a77fc58..81718952b87 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -100,19 +100,26 @@ export type InitialDataRealmFilters = {| realm_filters: RealmFilter[], |}; -export type InitialDataRealmUser = {| +export type RawInitialDataRealmUser = {| avatar_source: 'G', avatar_url: string | null, avatar_url_medium: string, can_create_streams: boolean, - cross_realm_bots: CrossRealmBot[], + cross_realm_bots: Array<{| ...CrossRealmBot, avatar_url?: string | null |}>, email: string, enter_sends: boolean, full_name: string, is_admin: boolean, + realm_users: Array<{| ...User, avatar_url?: string | null |}>, + realm_non_active_users: Array<{| ...User, avatar_url?: string | null |}>, + user_id: number, +|}; + +export type InitialDataRealmUser = {| + ...RawInitialDataRealmUser, + cross_realm_bots: CrossRealmBot[], realm_non_active_users: User[], realm_users: User[], - user_id: number, |}; export type InitialDataRealmUserGroups = {| @@ -280,7 +287,8 @@ export type InitialDataUserStatus = {| user_status?: UserStatusMapObject, |}; -// Initial data snapshot sent in response to a `/register` request. +// Initial data snapshot sent in response to a `/register` request, +// after validation and transformation. export type InitialData = {| // The server sends different subsets of the full available data, // depending on what event types the client subscribes to with the @@ -306,3 +314,10 @@ export type InitialData = {| ...InitialDataUpdateMessageFlags, ...InitialDataUserStatus, |}; + +// Initial data snapshot sent in response to a `/register` request, +// before validation and transformation. +export type RawInitialData = {| + ...InitialData, + ...RawInitialDataRealmUser, +|}; diff --git a/src/api/messages/__tests__/migrateMessages-test.js b/src/api/messages/__tests__/migrateMessages-test.js index 05f1c80ded7..7f77f435df4 100644 --- a/src/api/messages/__tests__/migrateMessages-test.js +++ b/src/api/messages/__tests__/migrateMessages-test.js @@ -4,6 +4,7 @@ import { identityOfAuth } from '../../../account/accountMisc'; import * as eg from '../../../__tests__/lib/exampleData'; import type { ServerMessage, ServerReaction } from '../getMessages'; import type { Message } from '../../modelTypes'; +import { GravatarURL } from '../../../utils/avatar'; describe('migrateMessages', () => { const reactingUser = eg.makeUser(); @@ -22,6 +23,7 @@ describe('migrateMessages', () => { const serverMessage: ServerMessage = { ...eg.streamMessage(), reactions: [serverReaction], + avatar_url: null, }; const input: ServerMessage[] = [serverMessage]; @@ -37,12 +39,17 @@ describe('migrateMessages', () => { emoji_code: serverReaction.emoji_code, }, ], + avatar_url: GravatarURL.validateAndConstructInstance({ email: serverMessage.sender_email }), }, ]; const actualOutput: Message[] = migrateMessages(input, identityOfAuth(eg.selfAuth)); - test('Replace user object with `user_id`', () => { + test('In reactions, replace user object with `user_id`', () => { expect(actualOutput.map(m => m.reactions)).toEqual(expectedOutput.map(m => m.reactions)); }); + + test('Converts avatar_url correctly', () => { + expect(actualOutput.map(m => m.avatar_url)).toEqual(expectedOutput.map(m => m.avatar_url)); + }); }); diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index 9af24c7cae8..3e0ebc93589 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -5,6 +5,7 @@ import type { Message, Narrow } from '../apiTypes'; import type { Reaction } from '../modelTypes'; import { apiGet } from '../apiFetch'; import { identityOfAuth } from '../../account/accountMisc'; +import { AvatarURL } from '../../utils/avatar'; type ApiResponseMessages = {| ...ApiResponseSuccess, @@ -32,6 +33,7 @@ export type ServerReaction = $ReadOnly<{| export type ServerMessage = $ReadOnly<{| ...$Exact, + avatar_url: string | null, reactions: $ReadOnlyArray, |}>; @@ -45,9 +47,16 @@ type ServerApiResponseMessages = {| /** Exported for tests only. */ export const migrateMessages = (messages: ServerMessage[], identity: Identity): Message[] => messages.map(message => { - const { reactions, ...restMessage } = message; + const { reactions, avatar_url: rawAvatarUrl, ...restMessage } = message; + return { ...restMessage, + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl, + email: message.sender_email, + userId: message.sender_id, + realm: identity.realm, + }), reactions: reactions.map(reaction => { const { user, ...restReaction } = reaction; return { @@ -92,6 +101,7 @@ export default async ( num_after: numAfter, apply_markdown: true, use_first_unread_anchor: useFirstUnread, + client_gravatar: true, }); return migrateResponse(response, identityOfAuth(auth)); }; diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index a1900bff23d..638c6195e3d 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -12,6 +12,8 @@ // // +import type { AvatarURL } from '../utils/avatar'; + export type ImageEmojiType = $ReadOnly<{| author?: $ReadOnly<{| email: string, @@ -106,8 +108,22 @@ export type User = {| // instead of an empty string. timezone?: string, - // avatar_url is synthesized on the server by `get_avatar_field`. - avatar_url: string | null, + /** + * Present under EVENT_USER_ADD, EVENT_USER_UPDATE (if change + * indicated), under REALM_INIT, and in `state.users`, all as an + * AvatarURL, because we translate into that form at the edge. + * + * For how it appears at the edge (and how we translate) see + * AvatarURL.fromUserOrBotData. + */ + avatar_url: AvatarURL, + + // These properties appear in data from the server, but we ignore + // them. If we add these, we should try to avoid `avatar_url` + // falling out of sync with them. + // avatar_source: mixed, + // avatar_url_medium: mixed, + // avatar_version: mixed, // profile_data added in commit 02b845336 (in 1.8.0); // see also e3aed0f7b (in 2.0.0) @@ -128,9 +144,10 @@ export type User = {| * * `UserOrBot`, a convenience union */ export type CrossRealmBot = {| - // avatar_url included since commit 58ee3fa8c (in 1.9.0) - // TODO(crunchy): convert missing -> null - avatar_url?: string | null, + /** + * See note for this property on User. + */ + avatar_url: AvatarURL, // date_joined included since commit 58ee3fa8c (in 1.9.0) date_joined?: string, @@ -480,10 +497,18 @@ export type Message = $ReadOnly<{| */ flags?: $ReadOnlyArray, - // + /** + * Present under EVENT_NEW_MESSAGE and under MESSAGE_FETCH_COMPLETE, + * and in `state.messages`, all as an AvatarURL, because we + * translate into that form at the edge. + * + * For how it appears at the edge (and how we translate) see + * AvatarURL.fromUserOrBotData. + */ + avatar_url: AvatarURL, + // The rest are believed to really appear in `message` events. - avatar_url: string | null, client: string, content: string, content_type: 'text/html' | 'text/markdown', diff --git a/src/api/registerForEvents.js b/src/api/registerForEvents.js index 1d87b2b73ab..4f23f32b1f7 100644 --- a/src/api/registerForEvents.js +++ b/src/api/registerForEvents.js @@ -1,8 +1,10 @@ /* @flow strict-local */ -import type { InitialData } from './initialDataTypes'; +import type { RawInitialData, InitialData } from './initialDataTypes'; import type { Auth } from './transportTypes'; import type { Narrow } from './apiTypes'; +import type { CrossRealmBot, User } from './modelTypes'; import { apiPost } from './apiFetch'; +import { AvatarURL } from '../utils/avatar'; type RegisterForEventsParams = {| apply_markdown?: boolean, @@ -16,17 +18,56 @@ type RegisterForEventsParams = {| client_capabilities?: {| notification_settings_null: boolean, bulk_message_deletion: boolean, + user_avatar_url_field_optional: boolean, |}, |}; +const transformUser = (rawUser: {| ...User, avatar_url?: string | null |}, realm: URL): User => { + const { avatar_url: rawAvatarUrl, email } = rawUser; + + return { + ...rawUser, + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl, + email, + userId: rawUser.user_id, + realm, + }), + }; +}; + +const transformCrossRealmBot = ( + rawCrossRealmBot: {| ...CrossRealmBot, avatar_url?: string | null |}, + realm: URL, +): CrossRealmBot => { + const { avatar_url: rawAvatarUrl, user_id: userId, email } = rawCrossRealmBot; + + return { + ...rawCrossRealmBot, + avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm }), + }; +}; + +const transform = (rawInitialData: RawInitialData, auth: Auth): InitialData => ({ + ...rawInitialData, + realm_users: rawInitialData.realm_users.map(rawUser => transformUser(rawUser, auth.realm)), + realm_non_active_users: rawInitialData.realm_non_active_users.map(rawNonActiveUser => + transformUser(rawNonActiveUser, auth.realm), + ), + cross_realm_bots: rawInitialData.cross_realm_bots.map(rawCrossRealmBot => + transformCrossRealmBot(rawCrossRealmBot, auth.realm), + ), +}); + /** See https://zulip.com/api/register-queue */ export default async (auth: Auth, params: RegisterForEventsParams): Promise => { const { narrow, event_types, fetch_event_types, client_capabilities } = params; - return apiPost(auth, 'register', { + const rawInitialData = await apiPost(auth, 'register', { ...params, narrow: JSON.stringify(narrow), event_types: JSON.stringify(event_types), fetch_event_types: JSON.stringify(fetch_event_types), client_capabilities: JSON.stringify(client_capabilities), }); + return transform(rawInitialData, auth); }; diff --git a/src/api/users/getUsers.js b/src/api/users/getUsers.js index 53d0b545e10..33feb0b7ce0 100644 --- a/src/api/users/getUsers.js +++ b/src/api/users/getUsers.js @@ -5,9 +5,13 @@ import { apiGet } from '../apiFetch'; type ApiResponseUsers = {| ...ApiResponseSuccess, - members: User[], + members: $ReadOnlyArray<{| ...User, avatar_url: string | null |}>, |}; +// TODO: If we start to use this, we need to convert `.avatar_url` to +// an AvatarURL instance, like we do in `registerForEvents` and +// `EVENT_USER_ADD` and `EVENT_USER_UPDATE`. + /** See https://zulip.com/api/get-all-users */ export default (auth: Auth): Promise => apiGet(auth, 'users', { client_gravatar: true }); diff --git a/src/boot/store.js b/src/boot/store.js index e978ff1da1c..d8a71347a3a 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -10,6 +10,7 @@ import { persistStore, autoRehydrate } from '../third/redux-persist'; import type { Config } from '../third/redux-persist'; import { ZulipVersion } from '../utils/zulipVersion'; +import { GravatarURL, UploadedAvatarURL, FallbackAvatarURL } from '../utils/avatar'; import type { Action, GlobalState } from '../types'; import config from '../config'; import { REHYDRATE } from '../actionConstants'; @@ -217,6 +218,13 @@ const migrations: { [string]: (GlobalState) => GlobalState } = { narrows: Immutable.Map(state.narrows), }), + // Convert messages[].avatar_url from `string | null` to `AvatarURL`. + '17': dropCache, + + // Convert `UserOrBot.avatar_url` from raw server data to + // `AvatarURL`. + '18': dropCache, + // TIP: When adding a migration, consider just using `dropCache`. }; @@ -303,6 +311,18 @@ const customReplacer = (key, value, defaultReplacer) => { return { data: value.raw(), [SERIALIZED_TYPE_FIELD_NAME]: 'ZulipVersion' }; } else if (value instanceof URL) { return { data: value.toString(), [SERIALIZED_TYPE_FIELD_NAME]: 'URL' }; + } else if (value instanceof GravatarURL) { + return { data: GravatarURL.serialize(value), [SERIALIZED_TYPE_FIELD_NAME]: 'GravatarURL' }; + } else if (value instanceof UploadedAvatarURL) { + return { + data: UploadedAvatarURL.serialize(value), + [SERIALIZED_TYPE_FIELD_NAME]: 'UploadedAvatarURL', + }; + } else if (value instanceof FallbackAvatarURL) { + return { + data: FallbackAvatarURL.serialize(value), + [SERIALIZED_TYPE_FIELD_NAME]: 'FallbackAvatarURL', + }; } return defaultReplacer(key, value); }; @@ -315,6 +335,12 @@ const customReviver = (key, value, defaultReviver) => { return new ZulipVersion(data); case 'URL': return new URL(data); + case 'GravatarURL': + return GravatarURL.deserialize(data); + case 'UploadedAvatarURL': + return UploadedAvatarURL.deserialize(data); + case 'FallbackAvatarURL': + return FallbackAvatarURL.deserialize(data); default: // Fall back to defaultReviver, below } diff --git a/src/common/GroupAvatar.js b/src/common/GroupAvatar.js index 92505dfa7f4..29193e3d7cf 100644 --- a/src/common/GroupAvatar.js +++ b/src/common/GroupAvatar.js @@ -42,7 +42,7 @@ export const initialsForGroupIcon = (names: string[]): string => { * We are currently using it only for group chats. * * @prop names - The name of one or more users, used to extract their initials. - * @prop size - Sets width and height in pixels. + * @prop size - Sets width and height in logical pixels. * @prop [shape] * @prop children - If provided, will render inside the component body. * @prop onPress - Event fired on pressing the component. diff --git a/src/common/OwnAvatar.js b/src/common/OwnAvatar.js index d767ea8cea1..8cedec48fcc 100644 --- a/src/common/OwnAvatar.js +++ b/src/common/OwnAvatar.js @@ -3,31 +3,27 @@ import React, { PureComponent } from 'react'; import type { User, Dispatch } from '../types'; import { connect } from '../react-redux'; -import { getCurrentRealm, getSelfUserDetail } from '../selectors'; +import { getSelfUserDetail } from '../selectors'; import UserAvatar from './UserAvatar'; -import { getAvatarFromUser } from '../utils/avatar'; type Props = $ReadOnly<{| dispatch: Dispatch, user: User, size: number, - realm: URL, |}>; /** * Renders an image of the current user's avatar * - * @prop size - Sets width and height in pixels. + * @prop size - Sets width and height in logical pixels. */ class OwnAvatar extends PureComponent { render() { - const { user, size, realm } = this.props; - const fullAvatarUrl = getAvatarFromUser(user, realm, size); - return ; + const { user, size } = this.props; + return ; } } export default connect(state => ({ - realm: getCurrentRealm(state), user: getSelfUserDetail(state), }))(OwnAvatar); diff --git a/src/common/UserAvatar.js b/src/common/UserAvatar.js index 7ee485cdf2f..00e4fa73b9d 100644 --- a/src/common/UserAvatar.js +++ b/src/common/UserAvatar.js @@ -1,12 +1,12 @@ /* @flow strict-local */ -import React, { PureComponent } from 'react'; -import { ImageBackground, View } from 'react-native'; +import React, { PureComponent, type Node as React$Node } from 'react'; +import { ImageBackground, View, PixelRatio } from 'react-native'; -import type { Node as React$Node } from 'react'; import Touchable from './Touchable'; +import { AvatarURL } from '../utils/avatar'; type Props = $ReadOnly<{| - avatarUrl: string, + avatarUrl: AvatarURL, size: number, shape: 'rounded' | 'square', children?: React$Node, @@ -16,8 +16,8 @@ type Props = $ReadOnly<{| /** * Renders an image of the user's avatar. * - * @prop avatarUrl - Absolute or relative url to an avatar image. - * @prop size - Sets width and height in pixels. + * @prop avatarUrl + * @prop size - Sets width and height in logical pixels. * @prop [shape] - 'rounded' (default) means a square with rounded corners. * @prop [children] - If provided, will render inside the component body. * @prop [onPress] - Event fired on pressing the component. @@ -41,7 +41,9 @@ export default class UserAvatar extends PureComponent { void, |}>; @@ -31,33 +26,27 @@ type Props = $ReadOnly<{| /** * Renders a user avatar with a PresenceStatusIndicator in the corner * - * @prop [avatarUrl] - Absolute or relative url to an avatar image. - * @prop [email] - User's' email address, to calculate Gravatar URL if not given `avatarUrl`. - * @prop [size] - Sets width and height in pixels. - * @prop [realm] - Current realm url, used if avatarUrl is relative. + * @prop [avatarUrl] + * @prop [email] - Sender's / user's email address, for the presence dot. + * @prop [size] - Sets width and height in logical pixels. * @prop [shape] - 'rounded' (default) means a square with rounded corners. * @prop [onPress] - Event fired on pressing the component. */ -class UserAvatarWithPresence extends PureComponent { +export default class UserAvatarWithPresence extends PureComponent { static defaultProps = { - avatarUrl: '', + // TODO: An empty-string `email` is probably up to no good. Remove + // this default. email: '', - size: 32, shape: 'rounded', }; render() { - const { avatarUrl, email, size, onPress, realm, shape } = this.props; - const fullAvatarUrl = getAvatarUrl(avatarUrl, email, realm); + const { avatarUrl, email, size, onPress, shape } = this.props; return ( - + ); } } - -export default connect(state => ({ - realm: getCurrentRealm(state), -}))(UserAvatarWithPresence); diff --git a/src/emoji/__tests__/data-test.js b/src/emoji/__tests__/data-test.js index 44a622f56dc..9365cbec05c 100644 --- a/src/emoji/__tests__/data-test.js +++ b/src/emoji/__tests__/data-test.js @@ -7,7 +7,7 @@ import { codeToEmojiMap, getFilteredEmojis } from '../data'; /* eslint-disable no-multi-spaces */ describe('codeToEmojiMap', () => { - // Tell Jest to recognize `check` as a helper function that runs + // Tell ESLint to recognize `check` as a helper function that runs // assertions. /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "check"] }] */ const check = (name, string1, string2) => { diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index ddc74f49e72..e198780e7d9 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -1,6 +1,7 @@ /* @flow strict-local */ import { EventTypes } from '../api/eventTypes'; +import * as logging from '../utils/logging'; import type { GlobalState, EventAction } from '../types'; import { EVENT_ALERT_WORDS, @@ -31,7 +32,9 @@ import { EVENT_SUBSCRIPTION, EVENT, } from '../actionConstants'; -import { getOwnUser, getOwnUserId } from '../users/userSelectors'; +import { getOwnUser, getOwnUserId, getAllUsersById } from '../users/userSelectors'; +import { AvatarURL } from '../utils/avatar'; +import { getCurrentRealm } from '../account/accountsSelectors'; const opToActionUserGroup = { add: EVENT_USER_GROUP_ADD, @@ -83,6 +86,12 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { // Move `flags` key from `event` to `event.message` for consistency, and // default to an empty array if `event.flags` is not set. flags: event.message.flags ?? event.flags ?? [], + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl: event.message.avatar_url, + email: event.message.sender_email, + userId: event.message.sender_id, + realm: getCurrentRealm(state), + }), }, local_message_id: event.local_message_id, caughtUp: state.caughtUp, @@ -120,21 +129,64 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { }; case 'realm_user': { + const realm = getCurrentRealm(state); + switch (event.op) { - case 'add': + case 'add': { + const { avatar_url: rawAvatarUrl, user_id: userId, email } = event.person; return { type: EVENT_USER_ADD, id: event.id, // TODO: Validate and rebuild `event.person`. - person: event.person, + person: { + ...event.person, + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl, + userId, + email, + realm, + }), + }, }; + } + + case 'update': { + const { user_id: userId } = event.person; + const existingUser = getAllUsersById(state).get(userId); + if (!existingUser) { + // If we get one of these events and don't have + // information on the user, there's nothing to do about + // it. But it's probably a bug, so, tell Sentry. + logging.warn( + "`realm_user` event with op `update` received for a user we don't know about", + { userId }, + ); + return { type: 'ignore' }; + } - case 'update': - // In an upcoming commit, we'll add `person`, with the - // fields we wish to update. return { type: EVENT_USER_UPDATE, + id: event.id, + userId, + // Just the fields we want to overwrite. + person: { + // Note: The `avatar_url` field will be out of sync with + // some related, documented properties, but we don't + // currently use them: `avatar_source`, + // `avatar_url_medium`, and `avatar_version`. + ...(event.person.avatar_url !== undefined + ? { + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl: event.person.avatar_url, + userId, + email: existingUser.email, + realm, + }), + } + : undefined), + }, }; + } case 'remove': // TODO: Handle this event and properly form this action. @@ -148,6 +200,9 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { } case 'realm_bot': + // If implementing, don't forget to convert `avatar_url` on + // `op: 'add'`, and (where `avatar_url` is present) on + // `op: 'update'`. return { type: 'ignore' }; case 'reaction': diff --git a/src/lightbox/Lightbox.js b/src/lightbox/Lightbox.js index 64c650c243e..35a13382b00 100644 --- a/src/lightbox/Lightbox.js +++ b/src/lightbox/Lightbox.js @@ -16,7 +16,6 @@ import LightboxHeader from './LightboxHeader'; import LightboxFooter from './LightboxFooter'; import { constructActionSheetButtons, executeActionSheetAction } from './LightboxActionSheet'; import { NAVBAR_SIZE, createStyleSheet } from '../styles'; -import { getAvatarFromMessage } from '../utils/avatar'; import { navigateBack } from '../actions'; import { streamNameOfStreamMessage } from '../utils/recipient'; @@ -118,7 +117,7 @@ class Lightbox extends PureComponent { diff --git a/src/lightbox/LightboxHeader.js b/src/lightbox/LightboxHeader.js index 3f3824ce474..d70fad84ae3 100644 --- a/src/lightbox/LightboxHeader.js +++ b/src/lightbox/LightboxHeader.js @@ -6,6 +6,7 @@ import { shortTime, humanDate } from '../utils/date'; import { createStyleSheet } from '../styles'; import { UserAvatarWithPresence, Touchable } from '../common'; import { Icon } from '../common/Icons'; +import { AvatarURL } from '../utils/avatar'; const styles = createStyleSheet({ text: { @@ -41,10 +42,18 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| senderName: string, timestamp: number, - avatarUrl: string, + avatarUrl: AvatarURL, onPressBack: () => void, |}>; +/** + * Shows sender's name and date of photo being displayed. + * + * @prop [senderName] - The sender's full name. + * @prop [avatarUrl] + * @prop [timestamp] + * @prop [onPressBack] + */ export default class LightboxHeader extends PureComponent { render() { const { onPressBack, senderName, timestamp, avatarUrl } = this.props; diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 04251957e77..99444fc192a 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -18,6 +18,7 @@ import { FIRST_UNREAD_ANCHOR } from '../../anchor'; import type { Message } from '../../api/modelTypes'; import type { ServerMessage } from '../../api/messages/getMessages'; import { streamNarrow, HOME_NARROW, HOME_NARROW_STR } from '../../utils/narrow'; +import { GravatarURL } from '../../utils/avatar'; import * as eg from '../../__tests__/lib/exampleData'; const mockStore = configureStore([thunk]); @@ -112,7 +113,26 @@ describe('fetchActions', () => { }); describe('fetchMessages', () => { - const message1 = eg.streamMessage({ id: 1 }); + const email = 'abc123@example.com'; + const sender = { + ...eg.makeUser(), + email, + avatar_url: GravatarURL.validateAndConstructInstance({ email }), + }; + const message1 = eg.streamMessage({ id: 1, sender }); + + type CommonFields = $Diff; + + // message1 exactly as we receive it from the server, before our + // own transformations. + // + // TODO: Deduplicate this logic with similar logic in + // migrateMessages-test.js. + const serverMessage1: ServerMessage = { + ...(omit(message1, 'reactions', 'avatar_url'): CommonFields), + reactions: [], + avatar_url: null, // Null in server data will be transformed to a GravatarURL + }; const baseState = eg.reduxState({ accounts: [eg.makeAccount()], @@ -123,18 +143,6 @@ describe('fetchActions', () => { describe('success', () => { beforeEach(() => { - type CommonFields = $Diff; - - // A message exactly as we receive it from the server, before - // our own transformations. - // - // TODO: Deduplicate this logic with similar logic in - // migrateMessages-test.js. - const serverMessage1: ServerMessage = { - ...(omit(message1, 'reactions'): CommonFields), - reactions: [], - }; - const response = { messages: [serverMessage1], result: 'success', @@ -181,7 +189,6 @@ describe('fetchActions', () => { // [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/928778 expect(actions[1].type).toBe('MESSAGE_FETCH_COMPLETE'); - expect(returnValue).toEqual([message1]); }); }); @@ -243,11 +250,12 @@ describe('fetchActions', () => { emoji_code: '1f44d', emoji_name: 'thumbs_up', }; + const response = { // Flow would complain at `faultyReaction` if it // type-checked `response`, but we should ignore it if that // day comes. It's badly shaped on purpose. - messages: [{ ...message1, reactions: [faultyReaction] }], + messages: [{ ...serverMessage1, reactions: [faultyReaction] }], result: 'success', }; fetch.mockResponseSuccess(JSON.stringify(response)); diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index a610c11eae3..7f1c2bcc410 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -320,6 +320,7 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat client_capabilities: { notification_settings_null: true, bulk_message_deletion: true, + user_avatar_url_field_optional: true, }, }), ), diff --git a/src/nullObjects.js b/src/nullObjects.js index 17b52f6795b..a7ae5766370 100644 --- a/src/nullObjects.js +++ b/src/nullObjects.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import type { User, Subscription } from './types'; +import { GravatarURL } from './utils/avatar'; export const NULL_OBJECT = Object.freeze({}); @@ -31,7 +32,7 @@ export const NULL_ARRAY = Object.freeze([]); /** DEPRECATED; don't add new uses. See block comment above definition. */ export const NULL_USER: User = { - avatar_url: '', + avatar_url: GravatarURL.validateAndConstructInstance({ email: '' }), email: '', full_name: '', is_admin: false, diff --git a/src/user-picker/AvatarItem.js b/src/user-picker/AvatarItem.js index 856281a19b6..4edfa0fcd92 100644 --- a/src/user-picker/AvatarItem.js +++ b/src/user-picker/AvatarItem.js @@ -5,6 +5,7 @@ import { Animated, Easing, View } from 'react-native'; import { UserAvatarWithPresence, ComponentWithOverlay, RawLabel, Touchable } from '../common'; import { createStyleSheet } from '../styles'; import { IconCancel } from '../common/Icons'; +import { AvatarURL } from '../utils/avatar'; const styles = createStyleSheet({ wrapper: { @@ -27,11 +28,19 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| email: string, - avatarUrl: ?string, + avatarUrl: AvatarURL, fullName: string, onPress: (email: string) => void, |}>; +/** + * Pressable avatar for items in the user-picker card. + * + * @prop [email] + * @prop [avatarUrl] + * @prop [fullName] + * @prop [onPress] + */ export default class AvatarItem extends PureComponent { animatedValue = new Animated.Value(0); diff --git a/src/users/__tests__/usersReducer-test.js b/src/users/__tests__/usersReducer-test.js index 8c5e3aa50e5..9926d331385 100644 --- a/src/users/__tests__/usersReducer-test.js +++ b/src/users/__tests__/usersReducer-test.js @@ -2,7 +2,9 @@ import deepFreeze from 'deep-freeze'; import * as eg from '../../__tests__/lib/exampleData'; -import { EVENT_USER_ADD, ACCOUNT_SWITCH } from '../../actionConstants'; +import type { User } from '../../types'; +import { UploadedAvatarURL } from '../../utils/avatar'; +import { EVENT_USER_ADD, EVENT_USER_UPDATE, ACCOUNT_SWITCH } from '../../actionConstants'; import usersReducer from '../usersReducer'; describe('usersReducer', () => { @@ -46,6 +48,87 @@ describe('usersReducer', () => { }); }); + describe('EVENT_USER_UPDATE', () => { + const theUser = eg.makeUser(); + const prevState = deepFreeze([theUser]); + + /** + * Check that an update event with supplied `person` works. + * + * May omit `user_id` to avoid repetition. + */ + // Tell ESLint to recognize `check` as a helper function that runs + // assertions. + /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "check"] }] */ + const check = (personMaybeWithoutId: $Shape) => { + const person = { + user_id: theUser.user_id, + ...personMaybeWithoutId, + }; + const action = deepFreeze({ + id: 1, + type: EVENT_USER_UPDATE, + userId: person.user_id, + person, + }); + + expect(usersReducer(prevState, action)).toEqual([{ ...theUser, ...person }]); + }; + + /* + * Should match REALM_USER OP: UPDATE in the doc. + * + * See https://zulip.com/api/get-events#realm_user-update. + * + * A few properties that we don't handle are commented out. + */ + + test('When a user changes their full name.', () => { + check({ full_name: eg.randString() }); + }); + + test('When a user changes their avatar.', () => { + check({ + avatar_url: UploadedAvatarURL.validateAndConstructInstance({ + realm: new URL('https://zulip.example.org'), + absoluteOrRelativeUrl: `/yo/avatar-${eg.randString()}.png`, + }), + // avatar_source: user1.avatar_source === 'G' ? 'U' : 'G', + // avatar_url_medium: eg.randString(), + // avatar_version: user1.avatar_version + 1, + }); + }); + + test('When a user changes their timezone setting.', () => { + check({ timezone: eg.randString() }); + }); + + // Excluded: "When the owner of a bot changes." The `users` state + // doesn't include cross-realm bots. + + test('When the role of a user changes.', () => { + check({ + // role: user1.role + 1, + }); + }); + + test('When the delivery email of a user changes.', () => { + check({ + // delivery_email: eg.randString(), + }); + }); + + test('When the user updates one of their custom profile fields.', () => { + check({ + // custom_profile_field: { + // id: 4, + // value: eg.randString(), + // rendered_value: eg.randString(), + // }, + }); + }); + }); + describe('ACCOUNT_SWITCH', () => { const user1 = eg.makeUser(); diff --git a/src/users/usersReducer.js b/src/users/usersReducer.js index 5870bf62e34..efd0bb38ba0 100644 --- a/src/users/usersReducer.js +++ b/src/users/usersReducer.js @@ -30,7 +30,9 @@ export default (state: UsersState = initialState, action: Action): UsersState => return state; // TODO case EVENT_USER_UPDATE: - return state; // TODO + return state.map(user => + user.user_id === action.userId ? { ...user, ...action.person } : user, + ); default: return state; diff --git a/src/utils/__tests__/avatar-test.js b/src/utils/__tests__/avatar-test.js index 0fe2bd35ceb..18da866abba 100644 --- a/src/utils/__tests__/avatar-test.js +++ b/src/utils/__tests__/avatar-test.js @@ -1,38 +1,187 @@ /* @flow strict-local */ -import { getMediumAvatar, getGravatarFromEmail } from '../avatar'; +import md5 from 'blueimp-md5'; -// avatarUrl can be converted to retrieve medium sized avatars(mediumAvatarUrl) if and only if -// avatarUrl contains avatar image name with a .png extension (e.g. AVATAR_IMAGE_NAME.png). +import { AvatarURL, GravatarURL, FallbackAvatarURL, UploadedAvatarURL } from '../avatar'; +import * as eg from '../../__tests__/lib/exampleData'; -describe('getMediumAvatar', () => { - test('if avatarUrl can be converted into mediumAvatarUrl, return mediumAvatarUrl', () => { - const avatarUrl = '/user_avatars/AVATAR_IMAGE_NAME.png/'; - const mediumAvatarUrl = '/user_avatars/AVATAR_IMAGE_NAME-medium.png/'; +describe('AvatarURL', () => { + describe('fromUserOrBotData', () => { + const user = eg.makeUser(); + const { email, user_id: userId } = user; + const realm = eg.realm; - const resultUrl = getMediumAvatar(avatarUrl); + test('gives a `FallbackAvatarURL` if `rawAvatarURL` is undefined', () => { + const rawAvatarUrl = undefined; + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( + FallbackAvatarURL, + ); + }); - expect(resultUrl).toEqual(mediumAvatarUrl); + test('gives a `GravatarURL` if `rawAvatarURL` is null', () => { + const rawAvatarUrl = null; + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( + GravatarURL, + ); + }); + + test('gives a `GravatarURL` if `rawAvatarURL` is a URL string on Gravatar origin', () => { + const rawAvatarUrl = + 'https://secure.gravatar.com/avatar/2efaec12efd9bea8a089299208117786?d=identicon&version=3'; + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( + GravatarURL, + ); + }); + + test('gives an `UploadedAvatarURL` if `rawAvatarURL` is a non-Gravatar absolute URL string', () => { + const rawAvatarUrl = + 'https://zulip-avatars.s3.amazonaws.com/13/430713047f2cffed661f84e139a64f864f17f286?x=x&version=5'; + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( + UploadedAvatarURL, + ); + }); + + test('gives an `UploadedAvatarURL` if `rawAvatarURL` is a relative URL string', () => { + const rawAvatarUrl = + '/user_avatars/2/08fb6d007eb10a56efee1d64760fbeb6111c4352.png?x=x&version=2'; + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( + UploadedAvatarURL, + ); + }); + }); +}); + +const SIZES_TO_TEST = [24, 32, 48, 80, 200]; + +describe('GravatarURL', () => { + test('serializes/deserializes correctly', () => { + const instance = GravatarURL.validateAndConstructInstance({ email: eg.selfUser.email }); + + const roundTripped = GravatarURL.deserialize(GravatarURL.serialize(instance)); + + SIZES_TO_TEST.forEach(size => { + expect(instance.get(size).toString()).toEqual(roundTripped.get(size).toString()); + }); }); - test('if avatarUrl cannot be converted into mediumAvatarUrl, return avatarUrl itself', () => { - const avatarUrl = '/avatar/AVATAR_IMAGE_NAME/'; + test('lowercases email address before hashing', () => { + const email = 'uNuSuAlCaPs@example.com'; + const instance = GravatarURL.validateAndConstructInstance({ email }); + SIZES_TO_TEST.forEach(size => { + expect(instance.get(size).toString()).toContain(md5('unusualcaps@example.com')); + }); + }); - const resultUrl = getMediumAvatar(avatarUrl); + test('uses hash from server, if provided', () => { + const email = 'user13313@chat.zulip.org'; + const hash = md5('cbobbe@zulip.com'); + const instance = GravatarURL.validateAndConstructInstance({ email, hash }); + SIZES_TO_TEST.forEach(size => { + expect(instance.get(size).toString()).toContain(hash); + }); + }); - expect(resultUrl).toEqual(avatarUrl); + test('produces corresponding URLs for all sizes', () => { + const instance = GravatarURL.validateAndConstructInstance({ email: eg.selfUser.email }); + + SIZES_TO_TEST.forEach(size => { + expect(instance.get(size).toString()).toContain(`s=${size.toString()}`); + }); }); }); -describe('getGravatarFromEmail', () => { - test('given an email return gravatar url', () => { - expect(getGravatarFromEmail('test@example.com', 80)).toEqual( - 'https://secure.gravatar.com/avatar/55502f40dc8b7c769880b10874abc9d0?d=identicon&s=80', - ); +describe('UploadedAvatarURL', () => { + test('serializes/deserializes correctly', () => { + const instance = UploadedAvatarURL.validateAndConstructInstance({ + realm: eg.realm, + absoluteOrRelativeUrl: + 'https://zulip-avatars.s3.amazonaws.com/13/430713047f2cffed661f84e139a64f864f17f286?x=x&version=5', + }); + + const roundTripped = UploadedAvatarURL.deserialize(UploadedAvatarURL.serialize(instance)); + + SIZES_TO_TEST.forEach(size => { + expect(instance.get(size).toString()).toEqual(roundTripped.get(size).toString()); + }); }); - test('given a case-sensitive email canonize and return gravatar url', () => { - expect(getGravatarFromEmail('Test@example.com', 80)).toEqual( - 'https://secure.gravatar.com/avatar/55502f40dc8b7c769880b10874abc9d0?d=identicon&s=80', - ); + test('if a relative URL, gives a URL on the given realm', () => { + const instance = UploadedAvatarURL.validateAndConstructInstance({ + realm: new URL('https://chat.zulip.org'), + absoluteOrRelativeUrl: + '/user_avatars/2/e35cdbc4771c5e4b94e705bf6ff7cca7fa1efcae.png?x=x&version=2', + }); + + SIZES_TO_TEST.forEach(size => { + const result = instance.get(size).toString(); + expect( + result + === 'https://chat.zulip.org/user_avatars/2/e35cdbc4771c5e4b94e705bf6ff7cca7fa1efcae.png?x=x&version=2' + || result + === 'https://chat.zulip.org/user_avatars/2/e35cdbc4771c5e4b94e705bf6ff7cca7fa1efcae-medium.png?x=x&version=2', + ).toBeTrue(); + }); + }); + + test('if an absolute URL, just use it', () => { + const instance = UploadedAvatarURL.validateAndConstructInstance({ + realm: new URL('https://chat.zulip.org'), + absoluteOrRelativeUrl: + 'https://zulip-avatars.s3.amazonaws.com/13/430713047f2cffed661f84e139a64f864f17f286?x=x&version=5', + }); + SIZES_TO_TEST.forEach(size => { + expect(instance.get(size).toString()).toMatch( + 'https://zulip-avatars.s3.amazonaws.com/13/430713047f2cffed661f84e139a64f864f17f286?x=x&version=5', + ); + }); + }); + + test('converts *.png to *-medium.png for sizes over 100', () => { + const realm = new URL('https://chat.zulip.org'); + const instance = UploadedAvatarURL.validateAndConstructInstance({ + realm, + absoluteOrRelativeUrl: + '/user_avatars/2/e35cdbc4771c5e4b94e705bf6ff7cca7fa1efcae.png?x=x&version=2', + }); + const sizesOver100 = SIZES_TO_TEST.filter(s => s > 100); + const sizesAtMost100 = SIZES_TO_TEST.filter(s => s <= 100); + sizesOver100.forEach(size => { + expect(instance.get(size).toString()).toEqual( + 'https://chat.zulip.org/user_avatars/2/e35cdbc4771c5e4b94e705bf6ff7cca7fa1efcae-medium.png?x=x&version=2', + ); + }); + sizesAtMost100.forEach(size => { + expect(instance.get(size).toString()).toEqual( + 'https://chat.zulip.org/user_avatars/2/e35cdbc4771c5e4b94e705bf6ff7cca7fa1efcae.png?x=x&version=2', + ); + }); + }); +}); + +describe('FallbackAvatarURL', () => { + test('serializes/deserializes correctly', () => { + const instance = FallbackAvatarURL.validateAndConstructInstance({ + realm: eg.realm, + userId: eg.selfUser.user_id, + }); + + const roundTripped = FallbackAvatarURL.deserialize(FallbackAvatarURL.serialize(instance)); + + SIZES_TO_TEST.forEach(size => { + expect(instance.get(size).toString()).toEqual(roundTripped.get(size).toString()); + }); + }); + + test('gives the `/avatar/{user_id}` URL, on the provided realm', () => { + const userId = eg.selfUser.user_id; + const instance = FallbackAvatarURL.validateAndConstructInstance({ + realm: new URL('https://chat.zulip.org'), + userId, + }); + + SIZES_TO_TEST.forEach(size => { + expect(instance.get(size).toString()).toEqual( + `https://chat.zulip.org/avatar/${userId.toString()}`, + ); + }); }); }); diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index b961d7ef124..a72b3b1fd30 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -145,7 +145,7 @@ describe('getNarrowFromLink', () => { }); describe('on stream links', () => { - // Tell Jest to recognize `expectStream` as a helper function that + // Tell ESLint to recognize `expectStream` as a helper function that // runs assertions. /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectStream"] }] */ const expectStream = (operand, streams, expectedName: null | string) => { diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 87d553374d3..f8d5e7fe4d1 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -1,46 +1,341 @@ /* @flow strict-local */ +/* eslint-disable no-underscore-dangle */ +/* eslint-disable no-use-before-define */ import md5 from 'blueimp-md5'; -import type { Message, Outbox, UserOrBot } from '../types'; +import { tryParseUrl } from './url'; +import * as logging from './logging'; +import { ensureUnreachable } from '../types'; /** - * When selecting the size of a gravatar we can pick any arbitrary - * size we want. For server-uploaded avatars this is not the case. - * We have only: - * * default - image is 100x100 - * * medium - image is 500x500 + * A way to get a standard avatar URL, or a sized one if available * - * This function converts a normal avatar to medium-sized one. + * This class is abstract. Only instantiate its subclasses. */ -export const getMediumAvatar = (avatarUrl: string): string => { - const matches = new RegExp(/(\w+)\.png/g).exec(avatarUrl); +export class AvatarURL { + /** + * From info on a user or bot, make the right subclass instance. + */ + static fromUserOrBotData(args: {| + rawAvatarUrl: string | void | null, + userId: number, + email: string, + realm: URL, + |}): AvatarURL { + const { rawAvatarUrl, userId, email, realm } = args; + if (rawAvatarUrl === undefined) { + // New in Zulip 3.0, feature level 18, the field may be missing + // on user objects in the register response, at the server's + // discretion, if we announce the + // `user_avatar_url_field_optional` client capability, which we + // do. See the note about `user_avatar_url_field_optional` at + // https://zulipchat.com/api/register-queue. + // + // It will also be absent on cross-realm bots from servers prior + // to 58ee3fa8c (1.9.0). The effect of using FallbackAvatarURL for + // this case isn't thoroughly considered, but at worst, it means a + // 404. We could plumb through the server version and + // conditionalize on that. + return FallbackAvatarURL.validateAndConstructInstance({ realm, userId }); + } else if (rawAvatarUrl === null) { + // If we announce `client_gravatar`, which we do, `rawAvatarUrl` + // might be null. In that case, we take responsibility for + // computing a hash for the user's email and using it to form a + // URL for an avatar served by Gravatar. + return GravatarURL.validateAndConstructInstance({ email }); + } else if (typeof rawAvatarUrl === 'string') { + // If we don't announce `client_gravatar` (which we do), or if + // the server doesn't have EMAIL_ADDRESS_VISIBILITY_EVERYONE + // set, then `rawAvatarUrl` will be the absolute Gravatar URL + // string. + // + // (In the latter case, we won't have real email addresses with + // which to generate the correct hash; see + // https://github.com/zulip/zulip/issues/15287. Implemented at + // `do_events_register` in zerver/lib/events.py on the server.) + if (tryParseUrl(rawAvatarUrl)?.origin === GravatarURL.ORIGIN) { + const hashMatch = /[0-9a-fA-F]{32}$/.exec(new URL(rawAvatarUrl).pathname); + if (hashMatch === null) { + const msg = 'Unexpected Gravatar URL shape from server.'; + logging.error(msg, { value: rawAvatarUrl }); + throw new Error(msg); + } + return GravatarURL.validateAndConstructInstance({ email, hash: hashMatch[0] }); + } - return matches ? avatarUrl.replace(matches[0], `${matches[1]}-medium.png`) : avatarUrl; -}; + // Otherwise, it's a realm-uploaded avatar, either absolute or + // relative, depending on how uploads are stored. + return UploadedAvatarURL.validateAndConstructInstance({ + realm, + absoluteOrRelativeUrl: rawAvatarUrl, + }); + } else { + ensureUnreachable(rawAvatarUrl); + const msg = 'Unexpected value for `rawAvatarUrl` in `AvatarURL.fromUserOrBotData`'; + logging.error(msg, { value: rawAvatarUrl }); + throw new Error(msg); + } + } + + /* eslint-disable-next-line class-methods-use-this */ + get(sizePhysicalPx: number): URL { + throw new Error('unimplemented'); + } +} + +/** + * A Gravatar URL with a hash we compute from an email address. + * + * See http://secure.gravatar.com/site/implement/images/, which covers + * the size options. + */ +export class GravatarURL extends AvatarURL { + /** + * Serialize to a special string; reversible with `deserialize`. + */ + static serialize(instance: GravatarURL): string { + return instance._standardUrl instanceof URL + ? instance._standardUrl.toString() + : instance._standardUrl; + } + + /** + * Use a special string from `serialize` to make a new instance. + */ + static deserialize(serialized: string): GravatarURL { + return new GravatarURL(serialized); + } + + /** + * Construct from raw server data, or throw an error. + * + * Pass the hash if the server provides it, to avoid computing it + * unnecessarily. + */ + static validateAndConstructInstance(args: {| email: string, hash?: string |}): GravatarURL { + const { email, hash = md5(email.toLowerCase()) } = args; + + const standardSizeUrl = new URL(`/avatar/${hash}`, GravatarURL.ORIGIN); + standardSizeUrl.searchParams.set('d', 'identicon'); + + return new GravatarURL(standardSizeUrl); + } + + static ORIGIN = 'https://secure.gravatar.com'; + + /** + * Standard URL from which to generate others. PRIVATE. + * + * May be a string if the instance was constructed at rehydrate + * time, when URL validation is unnecessary. + */ + _standardUrl: string | URL; + + /** + * PRIVATE: Make an instance from already-validated data. + * + * Not part of the public interface; use the static methods instead. + * + * It's private because we need a path to constructing an instance + * without constructing URL objects, which takes more time than is + * acceptable when we can avoid it, e.g., during rehydration. + * Constructing URL objects is a necessary part of validating data + * from the server, but we only need to validate the data once, when + * it's first received. + */ + constructor(standardUrl: string | URL) { + super(); + this._standardUrl = standardUrl; + } + + /** + * Get a URL object for the given size. + * + * `sizePhysicalPx` must be an integer. (Gravatar doesn't advertise + * the ability to specify noninteger values for the size.) + */ + get(sizePhysicalPx: number): URL { + // `this._standardUrl` may have begun its life as a string, to + // avoid computing a URL object during rehydration + if (typeof this._standardUrl === 'string') { + this._standardUrl = new URL(this._standardUrl); + } + + // Make a new URL to mutate + // $FlowFixMe - https://github.com/zulip/zulip-mobile/pull/4230#discussion_r512351202 + const result: URL = new URL(this._standardUrl); + result.searchParams.set('s', sizePhysicalPx.toString()); + return result; + } +} + +/** + * The /avatar/{user_id} redirect. + * + * See the point on `user_avatar_url_field_optional` at + * https://zulipchat.com/api/register-queue. + * + * This endpoint does not currently support size customization. + */ +export class FallbackAvatarURL extends AvatarURL { + /** + * Serialize to a special string; reversible with `deserialize`. + */ + static serialize(instance: FallbackAvatarURL): string { + return instance._standardUrl instanceof URL + ? instance._standardUrl.toString() + : instance._standardUrl; + } -export const getGravatarFromEmail = (email: string = '', size: number): string => - `https://secure.gravatar.com/avatar/${md5(email.toLowerCase())}?d=identicon&s=${size}`; + /** + * Use a special string from `serialize` to make a new instance. + */ + static deserialize(serialized: string): FallbackAvatarURL { + return new FallbackAvatarURL(serialized); + } -export const getAvatarUrl = ( - avatarUrl: ?string, - email: string, - realm: URL, - size: number = 80, -): string => { - if (typeof avatarUrl !== 'string') { - return getGravatarFromEmail(email, size); + /** + * Construct from raw server data, or throw an error. + */ + static validateAndConstructInstance(args: {| realm: URL, userId: number |}): FallbackAvatarURL { + const { realm, userId } = args; + return new FallbackAvatarURL(new URL(`/avatar/${userId.toString()}`, realm)); } - const fullUrl = new URL(avatarUrl, realm).toString(); + /** + * Standard URL from which to generate others. PRIVATE. + * + * May be a string if the instance was constructed at rehydrate + * time, when URL validation is unnecessary. + */ + _standardUrl: string | URL; + + /** + * PRIVATE: Make an instance from already-validated data. + * + * Not part of the public interface; use the static methods instead. + * + * It's private because we need a path to constructing an instance + * without constructing URL objects, which takes more time than is + * acceptable when we can avoid it, e.g., during rehydration. + * Constructing URL objects is a necessary part of validating data + * from the server, but we only need to validate the data once, when + * it's first received. + */ + constructor(standardUrl: string | URL) { + super(); + this._standardUrl = standardUrl; + } - return size > 100 ? getMediumAvatar(fullUrl) : fullUrl; -}; + /** + * Get a URL object for the given size. + * + * Size customization isn't currently supported for + * FallbackAvatarURLs. + * + * Still, we'll take `sizePhysicalPx` (it should be an integer), to + * make it easy to support in the future. + */ + get(sizePhysicalPx: number): URL { + // `this._standardUrl` may have begun its life as a string, to + // avoid computing a URL object during rehydration + if (typeof this._standardUrl === 'string') { + this._standardUrl = new URL(this._standardUrl); + } -export const getAvatarFromUser = (user: UserOrBot, realm: URL, size?: number): string => - getAvatarUrl(user.avatar_url, user.email, realm, size); + return this._standardUrl; + } +} -export const getAvatarFromMessage = ( - message: Message | Outbox, - realm: URL, - size?: number, -): string => getAvatarUrl(message.avatar_url, message.sender_email, realm, size); +/** + * An avatar that was uploaded to the Zulip server. + * + * There are two size options; if `sizePhysicalPx` is greater than + * 100, medium is chosen: + * * default: 100x100 + * * medium: 500x500 + */ +export class UploadedAvatarURL extends AvatarURL { + /** + * Serialize to a special string; reversible with `deserialize`. + */ + static serialize(instance: UploadedAvatarURL): string { + return instance._standardUrl instanceof URL + ? instance._standardUrl.toString() + : instance._standardUrl; + } + + /** + * Use a special string from `serialize` to make a new instance. + */ + static deserialize(serialized: string): UploadedAvatarURL { + return new UploadedAvatarURL(serialized); + } + + /** + * Construct from raw server data, or throw an error. + * + * Expects a relative URL plus the realm for a local upload; + * otherwise, an absolute URL of the avatar on the S3 backend. + */ + static validateAndConstructInstance(args: {| + realm: URL, + absoluteOrRelativeUrl: string, + |}): UploadedAvatarURL { + const { realm, absoluteOrRelativeUrl } = args; + // If `absoluteOrRelativeUrl` is absolute, the second argument + // is ignored. + return new UploadedAvatarURL(new URL(absoluteOrRelativeUrl, realm)); + } + + /** + * Standard URL from which to generate others. PRIVATE. + * + * May be a string if the instance was constructed at rehydrate + * time, when URL validation is unnecessary. + */ + _standardUrl: string | URL; + + /** + * PRIVATE: Make an instance from already-validated data. + * + * Not part of the public interface; use the static methods instead. + * + * It's private because we need a path to constructing an instance + * without constructing URL objects, which takes more time than is + * acceptable when we can avoid it, e.g., during rehydration. + * Constructing URL objects is a necessary part of validating data + * from the server, but we only need to validate the data once, when + * it's first received. + */ + constructor(standardUrl: string | URL) { + super(); + this._standardUrl = standardUrl; + } + + /** + * Get a URL object for the given size. + * + * `sizePhysicalPx` should be an integer. + */ + get(sizePhysicalPx: number): URL { + // `this._standardUrl` may have begun its life as a string, to + // avoid computing a URL object during rehydration + if (typeof this._standardUrl === 'string') { + this._standardUrl = new URL(this._standardUrl); + } + + let result: URL = this._standardUrl; + if (sizePhysicalPx > 100) { + // Make a new URL to mutate, instead of mutating this._standardUrl + // $FlowFixMe - https://github.com/zulip/zulip-mobile/pull/4230#discussion_r512351202 + result = new URL(this._standardUrl); + + const match = new RegExp(/(\w+)\.png/g).exec(result.pathname); + if (match !== null) { + result.pathname = result.pathname.replace(match[0], `${match[1]}-medium.png`); + } + } + return result; + } +} diff --git a/src/webview/html/__tests__/render-test.js b/src/webview/html/__tests__/render-test.js index 365af1b6fa9..7212d0cb7c6 100644 --- a/src/webview/html/__tests__/render-test.js +++ b/src/webview/html/__tests__/render-test.js @@ -7,7 +7,6 @@ describe('typing', () => { const name = '&
diff --git a/src/webview/html/messageTypingAsHtml.js b/src/webview/html/messageTypingAsHtml.js index 8360767b1d2..94926e192c5 100644 --- a/src/webview/html/messageTypingAsHtml.js +++ b/src/webview/html/messageTypingAsHtml.js @@ -1,14 +1,21 @@ /* @flow strict-local */ +import { PixelRatio } from 'react-native'; + import template from './template'; import type { UserOrBot } from '../../types'; -import { getAvatarFromUser } from '../../utils/avatar'; const typingAvatar = (realm: URL, user: UserOrBot): string => template`
+ src="${user.avatar_url + .get( + // 48 logical pixels; see `.avatar` and `.avatar img` in + // src/webview/static/base.css. + PixelRatio.getPixelSizeForLayoutSize(48), + ) + .toString()}">
`; diff --git a/src/webview/static/base.css b/src/webview/static/base.css index 91d459a895d..2f75e135689 100644 --- a/src/webview/static/base.css +++ b/src/webview/static/base.css @@ -291,7 +291,12 @@ body { transition-timing-function: ease-out; } -/* The sender's avatar. */ +/* + * The sender's avatar. If changing the size here, be sure to change + * the size we request for the avatar image file in the corresponding + * HTML-generating code (see messageTypingAsHtml.js and + * messageAsHtml.js). + */ .avatar, .loading-avatar { min-width: 48px;