From f277cf8625e4f5dc49b5c9793482cf8c7829c35f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 20 Nov 2020 07:48:00 -0800 Subject: [PATCH 01/23] eslint [nfc]: Correct a few errors in comments. As Greg points out at https://github.com/zulip/zulip-mobile/pull/4230#discussion_r521615562. --- src/emoji/__tests__/data-test.js | 2 +- src/utils/__tests__/internalLinks-test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/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) => { From 6c28206da16d482d8058e696cc6a71f580cfb168 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Nov 2020 12:50:35 -0800 Subject: [PATCH 02/23] UserAvatarWithPresence: Stop double-processing avatar URLs. Before this commit, `getAvatarFromMessage`, which is a thin wrapper on `getAvatarUrl`, had a callsite in `Lightbox`; the output of that call then made its way to `UserAvatarWithPresence`, and the latter passed it to `getAvatarUrl` again. This situation of passing the `.avatar_url` field on a message, user, or cross-realm bot through `getAvatarUrl` twice, instead of just once, has so far been harmless; the second time has just returned its input unmodified. But if we were to do the same thing with a `size` over 100, we could end up with something like `.../foo-medium-medium.png`, for an uploaded avatar, which would be no good. So, stop the double-processing by removing the `getAvatarFromMessage` call. And try to prevent the same thing from happening again by changing the contract of `UserAvatarWithPresence` (since it calls `getAvatarUrl`), so that it clearly asks for the `.avatar_url` property on a message, user, or cross-realm bot object, and propagate the change of contract to a few callers, adding and changing some JSDocs. The change of contract is safe because `getAvatarUrl` knows how to handle the `.avatar_url` field on message, user, and cross-realm bot objects. --- src/common/UserAvatarWithPresence.js | 8 +++++--- src/lightbox/Lightbox.js | 3 +-- src/lightbox/LightboxHeader.js | 13 ++++++++++++- src/user-picker/AvatarItem.js | 13 ++++++++++++- 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index 2b5cae6d049..bce8e7a5ac5 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -2,7 +2,7 @@ import React, { PureComponent } from 'react'; -import type { Dispatch } from '../types'; +import type { Dispatch, Message, User, CrossRealmBot } from '../types'; import { createStyleSheet } from '../styles'; import { connect } from '../react-redux'; import { getCurrentRealm } from '../selectors'; @@ -20,7 +20,9 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| dispatch: Dispatch, - avatarUrl: ?string, + avatarUrl: | $PropertyType + | $PropertyType + | $PropertyType, email: string, size: number, realm: URL, @@ -31,7 +33,7 @@ type Props = $ReadOnly<{| /** * Renders a user avatar with a PresenceStatusIndicator in the corner * - * @prop [avatarUrl] - Absolute or relative url to an avatar image. + * @prop [avatarUrl] - `.avatar_url` on a `Message` or a `UserOrBot` * @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. 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..2990aeb2ec3 100644 --- a/src/lightbox/LightboxHeader.js +++ b/src/lightbox/LightboxHeader.js @@ -2,6 +2,7 @@ import React, { PureComponent } from 'react'; import { View, Text } from 'react-native'; +import type { Message, User, CrossRealmBot } from '../types'; import { shortTime, humanDate } from '../utils/date'; import { createStyleSheet } from '../styles'; import { UserAvatarWithPresence, Touchable } from '../common'; @@ -41,10 +42,20 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| senderName: string, timestamp: number, - avatarUrl: string, + avatarUrl: | $PropertyType + | $PropertyType + | $PropertyType, onPressBack: () => void, |}>; +/** + * Shows sender's name and date of photo being displayed. + * + * @prop [senderName] - The sender's full name. + * @prop [avatarUrl] - `.avatar_url` on a `Message` or a `UserOrBot` + * @prop [timestamp] + * @prop [onPressBack] + */ export default class LightboxHeader extends PureComponent { render() { const { onPressBack, senderName, timestamp, avatarUrl } = this.props; diff --git a/src/user-picker/AvatarItem.js b/src/user-picker/AvatarItem.js index 856281a19b6..3b100285106 100644 --- a/src/user-picker/AvatarItem.js +++ b/src/user-picker/AvatarItem.js @@ -2,6 +2,7 @@ import React, { PureComponent } from 'react'; import { Animated, Easing, View } from 'react-native'; +import type { Message, User, CrossRealmBot } from '../types'; import { UserAvatarWithPresence, ComponentWithOverlay, RawLabel, Touchable } from '../common'; import { createStyleSheet } from '../styles'; import { IconCancel } from '../common/Icons'; @@ -27,11 +28,21 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| email: string, - avatarUrl: ?string, + avatarUrl: | $PropertyType + | $PropertyType + | $PropertyType, fullName: string, onPress: (email: string) => void, |}>; +/** + * Pressable avatar for items in the user-picker card. + * + * @prop [email] + * @prop [avatarUrl] - `.avatar_url` on a `Message` or a `UserOrBot` + * @prop [fullName] + * @prop [onPress] + */ export default class AvatarItem extends PureComponent { animatedValue = new Animated.Value(0); From 5c1668bf9b9cb016b83058fcd38cd33cea965d51 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 23 Nov 2020 14:06:00 -0500 Subject: [PATCH 03/23] UserAvatarWithPresence [nfc]: Remove unused default prop for `size`. --- src/common/UserAvatarWithPresence.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index bce8e7a5ac5..d87cd669359 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -44,7 +44,6 @@ class UserAvatarWithPresence extends PureComponent { static defaultProps = { avatarUrl: '', email: '', - size: 32, shape: 'rounded', }; From e8f6bc0ebe4e3b62a3a464d45a0de1ba7f6148e1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 23 Nov 2020 14:20:44 -0500 Subject: [PATCH 04/23] avatar [nfc]: Move default size of 80 to `getAvatar*` functions' callsites. And make that param required. It appears that this value of 80 was taken from Gravatar, which they use as a default size if no size is provided [1]. [1] https://en.gravatar.com/site/implement/images/#size --- src/common/UserAvatarWithPresence.js | 2 +- src/utils/avatar.js | 11 ++++------- src/webview/html/messageAsHtml.js | 2 +- src/webview/html/messageTypingAsHtml.js | 2 +- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index d87cd669359..5f164c41d16 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -49,7 +49,7 @@ class UserAvatarWithPresence extends PureComponent { render() { const { avatarUrl, email, size, onPress, realm, shape } = this.props; - const fullAvatarUrl = getAvatarUrl(avatarUrl, email, realm); + const fullAvatarUrl = getAvatarUrl(avatarUrl, email, realm, 80); return ( diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 87d553374d3..c360c9a8038 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -25,7 +25,7 @@ export const getAvatarUrl = ( avatarUrl: ?string, email: string, realm: URL, - size: number = 80, + size: number, ): string => { if (typeof avatarUrl !== 'string') { return getGravatarFromEmail(email, size); @@ -36,11 +36,8 @@ export const getAvatarUrl = ( return size > 100 ? getMediumAvatar(fullUrl) : fullUrl; }; -export const getAvatarFromUser = (user: UserOrBot, realm: URL, size?: number): string => +export const getAvatarFromUser = (user: UserOrBot, realm: URL, size: number): string => getAvatarUrl(user.avatar_url, user.email, realm, size); -export const getAvatarFromMessage = ( - message: Message | Outbox, - realm: URL, - size?: number, -): string => getAvatarUrl(message.avatar_url, message.sender_email, realm, size); +export const getAvatarFromMessage = (message: Message | Outbox, realm: URL, size: number): string => + getAvatarUrl(message.avatar_url, message.sender_email, realm, size); diff --git a/src/webview/html/messageAsHtml.js b/src/webview/html/messageAsHtml.js index 4de8c3ef133..b6d54c63009 100644 --- a/src/webview/html/messageAsHtml.js +++ b/src/webview/html/messageAsHtml.js @@ -118,7 +118,7 @@ $!${divOpenHtml} const { sender_full_name } = message; const sender_id = message.isOutbox ? backgroundData.ownUser.user_id : message.sender_id; - const avatarUrl = getAvatarFromMessage(message, backgroundData.auth.realm); + const avatarUrl = getAvatarFromMessage(message, backgroundData.auth.realm, 80); const subheaderHtml = template`
diff --git a/src/webview/html/messageTypingAsHtml.js b/src/webview/html/messageTypingAsHtml.js index 8360767b1d2..0c7ef746e68 100644 --- a/src/webview/html/messageTypingAsHtml.js +++ b/src/webview/html/messageTypingAsHtml.js @@ -8,7 +8,7 @@ const typingAvatar = (realm: URL, user: UserOrBot): string => template` + src="${getAvatarFromUser(user, realm, 80)}">
`; From 947fad028ec36e4f66362b9bcc1c4eb879678b0d Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 20 Nov 2020 17:47:50 -0500 Subject: [PATCH 05/23] avatar: Clearly differentiate physical from layout pixels for avatars. This isn't a total fix for #4305, but it goes a long way. As Greg says there, """ The basic issue is that our avatars code has confused [logical pixels vs. physical pixels](https://github.com/zulip/zulip-mobile/blob/d6254ad75ee0c327e37b382190f2dd041e858213/docs/background/layout.md). """ First, we decide that `getAvatarUrl` and its friends (which will soon turn into `AvatarURL#get`, etc.) will take its size parameter in physical pixels. For extra clarity, rename it to `sizePhysicalPx`. For `sizePhysicalPx` at all call sites, we multiply the logical-pixel size of the image (the layout size) by the appropriate ratio (i.e., the ratio of physical to logical pixels), which React Native conveniently gives us. - In a few places, we permanently abandon an arbitrary value of 80 that had been used for the physical size. The physical size is only correctly determined by both the layout and the device, so a constant will never work. In `UserAvatarWithPresence`, the logical-pixel size is right there; it's the `size` prop. In two HTML-generation functions, the layout size isn't quite so easy to find, but we find it in `base.css` and make a code comment pointing there. We know that '48px' in `base.css` means 48 logical pixels because we've written it down in our WebView doc [1]: "A length in `px` is exactly that many logical pixels". The remainder for #4305, we believe, is what's done in the next commit, handling a few so-far-neglected cases where we get a Gravatar URL from the server. [1] https://github.com/zulip/zulip-mobile/blob/master/docs/background/webview.md#lengths-px-vs-rem-no-em --- src/account-info/AccountDetails.js | 11 +++++++++-- src/common/GroupAvatar.js | 2 +- src/common/OwnAvatar.js | 9 +++++++-- src/common/UserAvatar.js | 2 +- src/common/UserAvatarWithPresence.js | 10 ++++++++-- src/utils/avatar.js | 21 ++++++++++++--------- src/webview/html/messageAsHtml.js | 9 ++++++++- src/webview/html/messageTypingAsHtml.js | 10 +++++++++- src/webview/static/base.css | 7 ++++++- 9 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/account-info/AccountDetails.js b/src/account-info/AccountDetails.js index a565bcbee08..390ab3add21 100644 --- a/src/account-info/AccountDetails.js +++ b/src/account-info/AccountDetails.js @@ -1,6 +1,6 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; -import { View } from 'react-native'; +import { View, PixelRatio } from 'react-native'; import type { UserOrBot, Dispatch } from '../types'; import styles, { createStyleSheet } from '../styles'; @@ -62,7 +62,14 @@ class AccountDetails extends PureComponent { return ( - + { * 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..2b0507b996c 100644 --- a/src/common/OwnAvatar.js +++ b/src/common/OwnAvatar.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; +import { PixelRatio } from 'react-native'; import type { User, Dispatch } from '../types'; import { connect } from '../react-redux'; @@ -17,12 +18,16 @@ type Props = $ReadOnly<{| /** * 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); + const fullAvatarUrl = getAvatarFromUser( + user, + realm, + PixelRatio.getPixelSizeForLayoutSize(size), + ); return ; } } diff --git a/src/common/UserAvatar.js b/src/common/UserAvatar.js index 7ee485cdf2f..afd3aaa7f2e 100644 --- a/src/common/UserAvatar.js +++ b/src/common/UserAvatar.js @@ -17,7 +17,7 @@ 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 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. diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index 5f164c41d16..4c45fc7726c 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -1,6 +1,7 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; +import { PixelRatio } from 'react-native'; import type { Dispatch, Message, User, CrossRealmBot } from '../types'; import { createStyleSheet } from '../styles'; @@ -35,7 +36,7 @@ type Props = $ReadOnly<{| * * @prop [avatarUrl] - `.avatar_url` on a `Message` or a `UserOrBot` * @prop [email] - User's' email address, to calculate Gravatar URL if not given `avatarUrl`. - * @prop [size] - Sets width and height in pixels. + * @prop [size] - Sets width and height in logical pixels. * @prop [realm] - Current realm url, used if avatarUrl is relative. * @prop [shape] - 'rounded' (default) means a square with rounded corners. * @prop [onPress] - Event fired on pressing the component. @@ -49,7 +50,12 @@ class UserAvatarWithPresence extends PureComponent { render() { const { avatarUrl, email, size, onPress, realm, shape } = this.props; - const fullAvatarUrl = getAvatarUrl(avatarUrl, email, realm, 80); + const fullAvatarUrl = getAvatarUrl( + avatarUrl, + email, + realm, + PixelRatio.getPixelSizeForLayoutSize(size), + ); return ( diff --git a/src/utils/avatar.js b/src/utils/avatar.js index c360c9a8038..cf4bf8c0cba 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -18,26 +18,29 @@ export const getMediumAvatar = (avatarUrl: string): string => { return matches ? avatarUrl.replace(matches[0], `${matches[1]}-medium.png`) : avatarUrl; }; -export const getGravatarFromEmail = (email: string = '', size: number): string => - `https://secure.gravatar.com/avatar/${md5(email.toLowerCase())}?d=identicon&s=${size}`; +export const getGravatarFromEmail = (email: string = '', sizePhysicalPx: number): string => + `https://secure.gravatar.com/avatar/${md5(email.toLowerCase())}?d=identicon&s=${sizePhysicalPx}`; export const getAvatarUrl = ( avatarUrl: ?string, email: string, realm: URL, - size: number, + sizePhysicalPx: number, ): string => { if (typeof avatarUrl !== 'string') { - return getGravatarFromEmail(email, size); + return getGravatarFromEmail(email, sizePhysicalPx); } const fullUrl = new URL(avatarUrl, realm).toString(); - return size > 100 ? getMediumAvatar(fullUrl) : fullUrl; + return sizePhysicalPx > 100 ? getMediumAvatar(fullUrl) : fullUrl; }; -export const getAvatarFromUser = (user: UserOrBot, realm: URL, size: number): string => - getAvatarUrl(user.avatar_url, user.email, realm, size); +export const getAvatarFromUser = (user: UserOrBot, realm: URL, sizePhysicalPx: number): string => + getAvatarUrl(user.avatar_url, user.email, realm, sizePhysicalPx); -export const getAvatarFromMessage = (message: Message | Outbox, realm: URL, size: number): string => - getAvatarUrl(message.avatar_url, message.sender_email, realm, size); +export const getAvatarFromMessage = ( + message: Message | Outbox, + realm: URL, + sizePhysicalPx: number, +): string => getAvatarUrl(message.avatar_url, message.sender_email, realm, sizePhysicalPx); diff --git a/src/webview/html/messageAsHtml.js b/src/webview/html/messageAsHtml.js index b6d54c63009..4b96e08da3b 100644 --- a/src/webview/html/messageAsHtml.js +++ b/src/webview/html/messageAsHtml.js @@ -1,4 +1,5 @@ /* @flow strict-local */ +import { PixelRatio } from 'react-native'; import distanceInWordsToNow from 'date-fns/distance_in_words_to_now'; import template from './template'; import type { @@ -118,7 +119,13 @@ $!${divOpenHtml} const { sender_full_name } = message; const sender_id = message.isOutbox ? backgroundData.ownUser.user_id : message.sender_id; - const avatarUrl = getAvatarFromMessage(message, backgroundData.auth.realm, 80); + const avatarUrl = getAvatarFromMessage( + message, + backgroundData.auth.realm, + // 48 logical pixels; see `.avatar` and `.avatar img` in + // src/webview/static/base.css. + PixelRatio.getPixelSizeForLayoutSize(48), + ); const subheaderHtml = template`
diff --git a/src/webview/html/messageTypingAsHtml.js b/src/webview/html/messageTypingAsHtml.js index 0c7ef746e68..c9dbecc05e7 100644 --- a/src/webview/html/messageTypingAsHtml.js +++ b/src/webview/html/messageTypingAsHtml.js @@ -1,4 +1,6 @@ /* @flow strict-local */ +import { PixelRatio } from 'react-native'; + import template from './template'; import type { UserOrBot } from '../../types'; import { getAvatarFromUser } from '../../utils/avatar'; @@ -8,7 +10,13 @@ const typingAvatar = (realm: URL, user: UserOrBot): string => template` + src="${getAvatarFromUser( + user, + realm, + // 48 logical pixels; see `.avatar` and `.avatar img` in + // src/webview/static/base.css. + PixelRatio.getPixelSizeForLayoutSize(48), + )}">
`; 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; From faadedb03462be42fa7c1d0c3cd673e7c8593b18 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 20 Nov 2020 15:21:43 -0500 Subject: [PATCH 06/23] avatar: Handle neglected cases where server sends a Gravatar URL. This fixes a bug stemming from the assumption that, if `avatar_url` from the server is a string, it won't be a Gravatar URL. There are in fact two cases where we'll get a string that represents a Gravatar URL: - If we don't announce `client_gravatar`. Although we plan to fix it soon, we don't currently announce it in `getMessages`. - If the server doesn't have EMAIL_ADDRESS_VISIBILITY_EVERYONE set. When that's set, real email addresses don't get sent to the client. But real email addresses are still supposed to determine Gravatar URLs, since I might have told Gravatar what my email address is, and what image to show for me. The server can't expect the client to compute the Gravatar URL for the real email address, since the server isn't sending real email addresses. So they just give the client the Gravatar URL for the real email address. Reading past the first conditional in `getAvatarUrl`, we appear to have assumed that the `avatar_url` string would correspond to a relative or absolute URL for an uploaded avatar. This code was mostly harmless to a Gravatar URL string that had snuck through for one of the above reasons. Gravatar URLs are absolute URLs, so `realm` in `new URL(avatarUrl, realm)` would have been ignored. If `size` was greater than 100, it would have been run through a regex, but that's a no-op because Gravatar URLs don't have `.png` in them. So, no runtime errors. I say it was "mostly harmless" because there is some behavior we didn't intend: a Gravatar URL string sneaking through in these cases wouldn't ever be set to the desired or the Zulip-default size (80). The way to set a Gravatar URL to a certain size is to pass a query param `s`, like `s=100`. And that just wasn't being done in these cases because the code wasn't written to be aware that it might be working with a Gravatar URL. So, fix that by recognizing when we're dealing with a Gravatar URL the server gave us and sizing it appropriately. With these Gravatar URLs being sized now -- with numbers chosen correctly in the previous commit -- we can say #4305 is fixed. More discussion of this is at at that issue and at https://github.com/zulip/zulip-mobile/pull/4230#discussion_r523383345. Fixes: #4305 --- src/utils/avatar.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/utils/avatar.js b/src/utils/avatar.js index cf4bf8c0cba..6235f4b8650 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -2,6 +2,8 @@ import md5 from 'blueimp-md5'; import type { Message, Outbox, UserOrBot } from '../types'; +import { tryParseUrl } from './url'; +import * as logging from './logging'; /** * When selecting the size of a gravatar we can pick any arbitrary @@ -28,9 +30,30 @@ export const getAvatarUrl = ( sizePhysicalPx: number, ): string => { if (typeof avatarUrl !== 'string') { + // It's our job to compute the Gravatar hash. return getGravatarFromEmail(email, sizePhysicalPx); } + // If we don't announce `client_gravatar` to the server (we + // sometimes don't), or if the server doesn't have + // `EMAIL_ADDRESS_VISIBILITY_EVERYONE` set, `avatarUrl` will be a + // Gravatar URL, and we should size it correctly with the `size` + // parameter. + const GRAVATAR_ORIGIN = 'https://secure.gravatar.com'; + if (tryParseUrl(avatarUrl)?.origin === GRAVATAR_ORIGIN) { + const hashMatch = /[0-9a-fA-F]{32}$/.exec(new URL(avatarUrl).pathname); + if (hashMatch === null) { + const msg = 'Unexpected Gravatar URL shape from server.'; + logging.error(msg, { value: avatarUrl }); + throw new Error(msg); + } + + const url = new URL(`/avatar/${hashMatch[0]}`, GRAVATAR_ORIGIN); + url.searchParams.set('d', 'identicon'); + url.searchParams.set('s', sizePhysicalPx.toString()); + return url.toString(); + } + const fullUrl = new URL(avatarUrl, realm).toString(); return sizePhysicalPx > 100 ? getMediumAvatar(fullUrl) : fullUrl; From 05dc392cd63cead367dc3c82ccd3f9f24a914034 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 23 Nov 2020 12:21:10 -0500 Subject: [PATCH 07/23] UserAvatar [nfc]: Take `.avatar_url` on a `Message` or a `UserOrBot`. Instead of making its callers do it, have `UserAvatar` compute the actual URL from the pattern the server provides in `avatar_url`. An important point toward this being NFC is that both the layout size, in logical pixels, and the image dimensions, in physical pixels, are unchanged: - In all three of `UserAvatar`'s callsites, the value passed for the `size` prop (for the layout size) doesn't change in this commit. - Before this commit, the image's physical-pixel dimensions were all calculated to be the layout size multiplied by the pixel ratio, near all the callsites. After this commit, the same arithmetic is done; it's just done in `UserAvatar` itself, instead. --- src/account-info/AccountDetails.js | 18 +++---------- src/common/OwnAvatar.js | 15 +++-------- src/common/UserAvatar.js | 40 ++++++++++++++++++++++------ src/common/UserAvatarWithPresence.js | 27 ++++--------------- 4 files changed, 44 insertions(+), 56 deletions(-) diff --git a/src/account-info/AccountDetails.js b/src/account-info/AccountDetails.js index 390ab3add21..5625e2f5f18 100644 --- a/src/account-info/AccountDetails.js +++ b/src/account-info/AccountDetails.js @@ -1,15 +1,14 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; -import { View, PixelRatio } from 'react-native'; +import { View } from 'react-native'; 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({ @@ -33,7 +32,6 @@ const componentStyles = createStyleSheet({ const AVATAR_SIZE = 200; type SelectorProps = {| - realm: URL, userStatusText: string | void, |}; @@ -46,7 +44,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,14 +60,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/common/OwnAvatar.js b/src/common/OwnAvatar.js index 2b0507b996c..489c7a653b3 100644 --- a/src/common/OwnAvatar.js +++ b/src/common/OwnAvatar.js @@ -1,18 +1,15 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; -import { PixelRatio } from 'react-native'; 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, |}>; /** @@ -22,17 +19,11 @@ type Props = $ReadOnly<{| */ class OwnAvatar extends PureComponent { render() { - const { user, size, realm } = this.props; - const fullAvatarUrl = getAvatarFromUser( - user, - realm, - PixelRatio.getPixelSizeForLayoutSize(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 afd3aaa7f2e..557e24f0da1 100644 --- a/src/common/UserAvatar.js +++ b/src/common/UserAvatar.js @@ -1,34 +1,47 @@ /* @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 { connect } from '../react-redux'; -import type { Node as React$Node } from 'react'; +import { getCurrentRealm } from '../selectors'; +import { getAvatarUrl } from '../utils/avatar'; import Touchable from './Touchable'; +import type { Dispatch, Message, User, CrossRealmBot } from '../types'; + +type SelectorProps = {| + realm: URL, +|}; type Props = $ReadOnly<{| - avatarUrl: string, + avatarUrl: | $PropertyType + | $PropertyType + | $PropertyType, + email: string, size: number, shape: 'rounded' | 'square', children?: React$Node, onPress?: () => void, + + dispatch: Dispatch, + ...SelectorProps, |}>; /** * Renders an image of the user's avatar. * - * @prop avatarUrl - Absolute or relative url to an avatar image. + * @prop avatarUrl - `.avatar_url` on a `Message` or a `UserOrBot` * @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. */ -export default class UserAvatar extends PureComponent { +class UserAvatar extends PureComponent { static defaultProps = { shape: 'rounded', }; render() { - const { avatarUrl, children, size, shape, onPress } = this.props; + const { avatarUrl, email, realm, children, size, shape, onPress } = this.props; const borderRadius = shape === 'rounded' ? size / 8 : 0; const style = { height: size, @@ -41,7 +54,14 @@ export default class UserAvatar extends PureComponent { { ); } } + +export default connect(state => ({ + realm: getCurrentRealm(state), +}))(UserAvatar); diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index 4c45fc7726c..9ac64d9177b 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -1,14 +1,10 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; -import { PixelRatio } from 'react-native'; -import type { Dispatch, Message, User, CrossRealmBot } from '../types'; +import type { Message, User, CrossRealmBot } from '../types'; import { createStyleSheet } from '../styles'; -import { connect } from '../react-redux'; -import { getCurrentRealm } from '../selectors'; import UserAvatar from './UserAvatar'; -import { getAvatarUrl } from '../utils/avatar'; import PresenceStatusIndicator from './PresenceStatusIndicator'; const styles = createStyleSheet({ @@ -20,13 +16,11 @@ const styles = createStyleSheet({ }); type Props = $ReadOnly<{| - dispatch: Dispatch, avatarUrl: | $PropertyType | $PropertyType | $PropertyType, email: string, size: number, - realm: URL, shape: 'rounded' | 'square', onPress?: () => void, |}>; @@ -35,13 +29,12 @@ type Props = $ReadOnly<{| * Renders a user avatar with a PresenceStatusIndicator in the corner * * @prop [avatarUrl] - `.avatar_url` on a `Message` or a `UserOrBot` - * @prop [email] - User's' email address, to calculate Gravatar URL if not given `avatarUrl`. + * @prop [email] - Sender's / user's email address, for the presence dot. * @prop [size] - Sets width and height in logical pixels. - * @prop [realm] - Current realm url, used if avatarUrl is relative. * @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: '', email: '', @@ -49,22 +42,12 @@ class UserAvatarWithPresence extends PureComponent { }; render() { - const { avatarUrl, email, size, onPress, realm, shape } = this.props; - const fullAvatarUrl = getAvatarUrl( - avatarUrl, - email, - realm, - PixelRatio.getPixelSizeForLayoutSize(size), - ); + const { avatarUrl, email, size, onPress, shape } = this.props; return ( - + ); } } - -export default connect(state => ({ - realm: getCurrentRealm(state), -}))(UserAvatarWithPresence); From 3b76e5b23af52b03b4fc3f016ee4dd96770e44a3 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 23 Nov 2020 13:32:55 -0500 Subject: [PATCH 08/23] AccountDetails [nfc]: Inline `AVATAR_SIZE`. Now that it's only being consumed in one place, it doesn't really make sense to keep it in a variable. --- src/account-info/AccountDetails.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/account-info/AccountDetails.js b/src/account-info/AccountDetails.js index 5625e2f5f18..d1196dc6a16 100644 --- a/src/account-info/AccountDetails.js +++ b/src/account-info/AccountDetails.js @@ -29,8 +29,6 @@ const componentStyles = createStyleSheet({ }, }); -const AVATAR_SIZE = 200; - type SelectorProps = {| userStatusText: string | void, |}; @@ -60,7 +58,7 @@ class AccountDetails extends PureComponent { return ( - + Date: Mon, 17 Aug 2020 16:20:29 -0700 Subject: [PATCH 09/23] usersReducer: Handle `EVENT_USER_UPDATE` a little bit. I'm wary of suddenly starting to clobber any arbitrary property in users, so I'm starting small with just `avatar_url`. I chose this in particular because we're about to use our own new data type here, the `AvatarURL`. But, in the tests, lay the groundwork for handling all other documented updates. Now, when someone updates their avatar, it will be live-updated in `state.users`; see discussion, where Tim said to check that this is being done [1]. Note that it's not yet updated in `state.messages`, so the message list won't be live-updated when someone changes their avatar. See discussion [2] on implementing this. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/909056. In retrospect, I think he actually meant to make sure the server was behaving correctly -- but we might as well start doing the right thing here too. [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/992469 --- src/actionTypes.js | 6 +- src/events/eventToAction.js | 34 ++++++++-- src/users/__tests__/usersReducer-test.js | 81 +++++++++++++++++++++++- src/users/usersReducer.js | 4 +- 4 files changed, 117 insertions(+), 8 deletions(-) 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/events/eventToAction.js b/src/events/eventToAction.js index ddc74f49e72..dcae0301c1b 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,7 @@ import { EVENT_SUBSCRIPTION, EVENT, } from '../actionConstants'; -import { getOwnUser, getOwnUserId } from '../users/userSelectors'; +import { getOwnUser, getOwnUserId, getAllUsersById } from '../users/userSelectors'; const opToActionUserGroup = { add: EVENT_USER_GROUP_ADD, @@ -129,12 +130,37 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { person: event.person, }; - case 'update': - // In an upcoming commit, we'll add `person`, with the - // fields we wish to update. + case 'update': { + // We'll use this in an upcoming commit. + // eslint-disable-next-line no-unused-vars + const existingUser = getAllUsersById(state).get(event.person.user_id); + 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: event.person.user_id }, + ); + return { type: 'ignore' }; + } + return { type: EVENT_USER_UPDATE, + id: event.id, + userId: event.person.user_id, + // 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: event.person.avatar_url } + : undefined), + }, }; + } case 'remove': // TODO: Handle this event and properly form this action. diff --git a/src/users/__tests__/usersReducer-test.js b/src/users/__tests__/usersReducer-test.js index 8c5e3aa50e5..10c4b3f82bd 100644 --- a/src/users/__tests__/usersReducer-test.js +++ b/src/users/__tests__/usersReducer-test.js @@ -2,7 +2,8 @@ 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 { EVENT_USER_ADD, EVENT_USER_UPDATE, ACCOUNT_SWITCH } from '../../actionConstants'; import usersReducer from '../usersReducer'; describe('usersReducer', () => { @@ -46,6 +47,84 @@ 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: eg.randString(), + // 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; From c0d8a0496ddaeea48938ccfe3e39b08994608ca5 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 14 Aug 2020 13:34:12 -0700 Subject: [PATCH 10/23] avatar: Add `AvatarURL` abstract class, and two subclasses. We'll soon start instantiating these from raw data at the edge, in the "crunchy shell" pattern [1]. This will help us clean up lots of confusing UI logic in the "soft center" of the app that's in charge of displaying avatars, and we'll be able to uproot several helper functions and simplify some props (often ambiguously `string`-typed) of several React components. This doesn't yet add support for the `user_avatar_url_field_optional` client capability, but it will make it much more straightforward to do so in an upcoming commit. Some of the boilerplate in the subclasses is due to a performance optimization [2] based on an observed ~1s added to the rehydrate time on CZO when URL objects are constructed, vs. about 200ms (and maybe a bit more in the upper tail) when they aren't. During rehydration, we don't need to expensively construct URL objects to validate raw URL strings, as long as we do so at the edge when we receive them from the network. See discussion [3]. [1] https://github.com/zulip/zulip-mobile/blob/master/docs/architecture/crunchy-shell.md [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/993660 [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/908946 --- src/utils/__tests__/avatar-test.js | 157 +++++++++++++++++- src/utils/avatar.js | 248 +++++++++++++++++++++++++++++ 2 files changed, 404 insertions(+), 1 deletion(-) diff --git a/src/utils/__tests__/avatar-test.js b/src/utils/__tests__/avatar-test.js index 0fe2bd35ceb..5e71958b9cf 100644 --- a/src/utils/__tests__/avatar-test.js +++ b/src/utils/__tests__/avatar-test.js @@ -1,5 +1,160 @@ /* @flow strict-local */ -import { getMediumAvatar, getGravatarFromEmail } from '../avatar'; +import md5 from 'blueimp-md5'; + +import { + AvatarURL, + GravatarURL, + UploadedAvatarURL, + getMediumAvatar, + getGravatarFromEmail, +} from '../avatar'; +import * as eg from '../../__tests__/lib/exampleData'; + +describe('AvatarURL', () => { + describe('fromUserOrBotData', () => { + const user = eg.makeUser(); + const { email } = user; + const realm = eg.realm; + + test('gives a `GravatarURL` if `rawAvatarURL` is null', () => { + const rawAvatarUrl = null; + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, 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, 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, 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, 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('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')); + }); + }); + + 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); + }); + }); + + 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('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('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', + ); + }); + }); +}); // 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). diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 6235f4b8650..3a76b73f20b 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -1,9 +1,257 @@ /* @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'; + +/** + * A way to get a standard avatar URL, or a sized one if available + * + * This class is abstract. Only instantiate its subclasses. + */ +export class AvatarURL { + /** + * From info on a user or bot, make the right subclass instance. + */ + static fromUserOrBotData(args: {| + rawAvatarUrl: string | void | null, + email: string, + realm: URL, + |}): AvatarURL { + const { rawAvatarUrl, email, realm } = args; + if (rawAvatarUrl === undefined) { + // `rawAvatarUrl` will be undefined for cross-realm bots from + // servers prior to 58ee3fa8c (1.9.0). Fall back to Gravatar for + // this case; it should be pretty rare. + return GravatarURL.validateAndConstructInstance({ email }); + } else if (rawAvatarUrl === null) { + // If we announce `client_gravatar`, which we sometimes 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 sometimes + // 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] }); + } + + // 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; + } +} + +/** + * 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; + } +} /** * When selecting the size of a gravatar we can pick any arbitrary From fecfcc84c7be07f3a48a347d6ad5ac71ef2e61c0 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 27 Oct 2020 18:38:56 -0700 Subject: [PATCH 11/23] initialDataTypes: Add `RawInitialData`. Greg points out [1] that it's time we start keeping track of a type for our initial data pre-transformation, since we're about to start transforming it. Here, we say what we expect `realm_users` and two of its friends to look like. We grab those expectations from the `avatar_url` field on `User` and on `CrossRealmBot`. [1] https://github.com/zulip/zulip-mobile/pull/4230#discussion_r513100837 --- src/api/initialDataTypes.js | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index 5ff6a77fc58..e1315d44b37 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, +|}; From f177462cbf95c8e862ceace1f89f638ee62daefc Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 29 Jul 2020 17:46:24 -0700 Subject: [PATCH 12/23] register response [nfc]: Prepare to transform users and bots. This is setup for when we'll soon be converting `avatar_url` on objects in `realm_users` and `cross_realm_bots` on the register response. For motivation, see a recent commit where we added the `AvatarURL` abstract class. --- src/api/registerForEvents.js | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/api/registerForEvents.js b/src/api/registerForEvents.js index 1d87b2b73ab..a19fdb81b30 100644 --- a/src/api/registerForEvents.js +++ b/src/api/registerForEvents.js @@ -1,7 +1,8 @@ /* @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'; type RegisterForEventsParams = {| @@ -19,14 +20,39 @@ type RegisterForEventsParams = {| |}, |}; +const transformUser = (rawUser: {| ...User, avatar_url: string | null |}, realm: URL): User => + // In an upcoming commit, we'll convert the raw-data `avatar_url` + // field to an AvatarURL instance. + rawUser; + +const transformCrossRealmBot = ( + rawCrossRealmBot: {| ...CrossRealmBot, avatar_url: string | void | null |}, + realm: URL, +): CrossRealmBot => + // In an upcoming commit, we'll convert the raw-data `avatar_url` + // field to an AvatarURL instance. + rawCrossRealmBot; + +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); }; From a8f30c59664b9df4ad43e2ad133edd1a63be20f6 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 4 Aug 2020 13:38:06 -0700 Subject: [PATCH 13/23] redux: Replace/revive AvatarURL subclass instances. The instances aren't stored in Redux in this commit. This just handles the replace/revive logic for when they will be, in an upcoming commit. Like we did in bfe794955 for ZulipVersion instances. --- src/boot/store.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/boot/store.js b/src/boot/store.js index e978ff1da1c..d325b38420d 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 } from '../utils/avatar'; import type { Action, GlobalState } from '../types'; import config from '../config'; import { REHYDRATE } from '../actionConstants'; @@ -303,6 +304,13 @@ 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', + }; } return defaultReplacer(key, value); }; @@ -315,6 +323,10 @@ 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); default: // Fall back to defaultReviver, below } From 7044bb0a235c762a2d9e882b42a5514c338660d2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 14 Aug 2020 10:53:35 -0700 Subject: [PATCH 14/23] fetchActions tests [nfc]: Move `serverMessage1` up a level. We'll use this soon in a test in the `failure` block. --- src/message/__tests__/fetchActions-test.js | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 04251957e77..a25d252d4d0 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -114,6 +114,18 @@ describe('fetchActions', () => { describe('fetchMessages', () => { const message1 = eg.streamMessage({ id: 1 }); + 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'): CommonFields), + reactions: [], + }; + const baseState = eg.reduxState({ accounts: [eg.makeAccount()], narrows: Immutable.Map({ @@ -123,18 +135,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', From fd0f9fdccfb45c078da038da8536c5c7cbcc7c14 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 20 Nov 2020 08:22:10 -0800 Subject: [PATCH 15/23] UserAvatarWithPresence [nfc]: Remove default prop for `avatarUrl`. We weren't ever actually passing a void `avatarUrl`, and an empty string is nonsense for an avatar URL. Also make a note about another default prop that would be good to remove [1], but that is still operating in one case. [1] https://github.com/zulip/zulip-mobile/pull/4230#discussion_r521641673 --- src/common/UserAvatarWithPresence.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index 9ac64d9177b..580041b2341 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -36,7 +36,8 @@ type Props = $ReadOnly<{| */ export default class UserAvatarWithPresence extends PureComponent { static defaultProps = { - avatarUrl: '', + // TODO: An empty-string `email` is probably up to no good. Remove + // this default. email: '', shape: 'rounded', }; From b902744345c559b6f80fea1e94ad96ec7e073788 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 20 Nov 2020 15:34:26 -0500 Subject: [PATCH 16/23] avatar [nfc]: Make `getAvatarUrl` use the AvatarURL class. At this point, we switch out the old string-heavy, no-classes implementation of "compute the actual URL from the pattern the server provides in `avatar_url`" for the new one we've just introduced. We expect this to be NFC; we've combed through the possible things we could get from the server and ironed out various wrinkles [1]. [1] https://github.com/zulip/zulip-mobile/pull/4230#discussion_r513075744 --- src/utils/__tests__/avatar-test.js | 44 +------------------------ src/utils/avatar.js | 52 +++--------------------------- 2 files changed, 5 insertions(+), 91 deletions(-) diff --git a/src/utils/__tests__/avatar-test.js b/src/utils/__tests__/avatar-test.js index 5e71958b9cf..3ed083b0001 100644 --- a/src/utils/__tests__/avatar-test.js +++ b/src/utils/__tests__/avatar-test.js @@ -1,13 +1,7 @@ /* @flow strict-local */ import md5 from 'blueimp-md5'; -import { - AvatarURL, - GravatarURL, - UploadedAvatarURL, - getMediumAvatar, - getGravatarFromEmail, -} from '../avatar'; +import { AvatarURL, GravatarURL, UploadedAvatarURL } from '../avatar'; import * as eg from '../../__tests__/lib/exampleData'; describe('AvatarURL', () => { @@ -155,39 +149,3 @@ describe('UploadedAvatarURL', () => { }); }); }); - -// 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). - -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/'; - - const resultUrl = getMediumAvatar(avatarUrl); - - expect(resultUrl).toEqual(mediumAvatarUrl); - }); - - test('if avatarUrl cannot be converted into mediumAvatarUrl, return avatarUrl itself', () => { - const avatarUrl = '/avatar/AVATAR_IMAGE_NAME/'; - - const resultUrl = getMediumAvatar(avatarUrl); - - expect(resultUrl).toEqual(avatarUrl); - }); -}); - -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', - ); - }); - - 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', - ); - }); -}); diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 3a76b73f20b..00a6ed00290 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -253,59 +253,15 @@ export class UploadedAvatarURL extends AvatarURL { } } -/** - * 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 - * - * This function converts a normal avatar to medium-sized one. - */ -export const getMediumAvatar = (avatarUrl: string): string => { - const matches = new RegExp(/(\w+)\.png/g).exec(avatarUrl); - - return matches ? avatarUrl.replace(matches[0], `${matches[1]}-medium.png`) : avatarUrl; -}; - -export const getGravatarFromEmail = (email: string = '', sizePhysicalPx: number): string => - `https://secure.gravatar.com/avatar/${md5(email.toLowerCase())}?d=identicon&s=${sizePhysicalPx}`; - export const getAvatarUrl = ( avatarUrl: ?string, email: string, realm: URL, sizePhysicalPx: number, -): string => { - if (typeof avatarUrl !== 'string') { - // It's our job to compute the Gravatar hash. - return getGravatarFromEmail(email, sizePhysicalPx); - } - - // If we don't announce `client_gravatar` to the server (we - // sometimes don't), or if the server doesn't have - // `EMAIL_ADDRESS_VISIBILITY_EVERYONE` set, `avatarUrl` will be a - // Gravatar URL, and we should size it correctly with the `size` - // parameter. - const GRAVATAR_ORIGIN = 'https://secure.gravatar.com'; - if (tryParseUrl(avatarUrl)?.origin === GRAVATAR_ORIGIN) { - const hashMatch = /[0-9a-fA-F]{32}$/.exec(new URL(avatarUrl).pathname); - if (hashMatch === null) { - const msg = 'Unexpected Gravatar URL shape from server.'; - logging.error(msg, { value: avatarUrl }); - throw new Error(msg); - } - - const url = new URL(`/avatar/${hashMatch[0]}`, GRAVATAR_ORIGIN); - url.searchParams.set('d', 'identicon'); - url.searchParams.set('s', sizePhysicalPx.toString()); - return url.toString(); - } - - const fullUrl = new URL(avatarUrl, realm).toString(); - - return sizePhysicalPx > 100 ? getMediumAvatar(fullUrl) : fullUrl; -}; +): string => + AvatarURL.fromUserOrBotData({ rawAvatarUrl: avatarUrl, email, realm }) + .get(sizePhysicalPx) + .toString(); export const getAvatarFromUser = (user: UserOrBot, realm: URL, sizePhysicalPx: number): string => getAvatarUrl(user.avatar_url, user.email, realm, sizePhysicalPx); From 1fa78a1a3b67b21a8b94bf29d8383fb312591c92 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 17:36:28 -0700 Subject: [PATCH 17/23] crunchy shell: Use AvatarURL for Message objects. And have the UI logic that handles avatars for messages accept the new form. We'll do the same for UserOrBot objects in the next commit. There's a small amount of churn that comes from not converting users and messages in the same commit; I've added temporary code comments where this happens. For the `GET /messages` response, the natural place to do the conversion is in `migrateMessages`. For events, it's in `eventToAction`. --- src/__tests__/lib/exampleData.js | 19 +++++++++++++++--- .../__tests__/migrateMessages-test.js | 9 ++++++++- src/api/messages/getMessages.js | 10 +++++++++- src/api/modelTypes.js | 14 +++++++++++-- src/boot/store.js | 3 +++ src/common/UserAvatar.js | 20 ++++++++++++------- src/events/eventToAction.js | 7 +++++++ src/message/__tests__/fetchActions-test.js | 16 +++++++++++---- src/outbox/outboxActions.js | 12 ++++++++++- src/utils/avatar.js | 8 +------- src/webview/html/messageAsHtml.js | 15 +++++++------- 11 files changed, 99 insertions(+), 34 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 8f7a2f3d48d..463907302ee 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -15,6 +15,7 @@ import type { import type { Action, GlobalState, RealmState } from '../../reduxTypes'; import type { Auth, Account, Outbox } from '../../types'; import { ZulipVersion } from '../../utils/zulipVersion'; +import { AvatarURL } from '../../utils/avatar'; import { ACCOUNT_SWITCH, LOGIN_SUCCESS, @@ -266,12 +267,18 @@ 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, + // In the next commit, we'll just use `user.avatar_url`, since + // it'll already be an `AvatarURL`. + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl: user.avatar_url, + email: user.email, + realm, + }), client: 'ExampleClient', gravatar_hash: 'd3adb33f', sender_email, @@ -367,7 +374,13 @@ const outboxMessageBase: $Diff = deep isOutbox: true, isSent: false, - avatar_url: selfUser.avatar_url, + // In the next commit, we'll just use `selfUser.avatar_url`, since + // it'll already be an `AvatarURL`. + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl: selfUser.avatar_url, + email: selfUser.email, + realm, + }), content: '

Test.

', display_recipient: stream.name, // id: ..., 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..ca73fc5f4c8 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,15 @@ 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, + realm: identity.realm, + }), reactions: reactions.map(reaction => { const { user, ...restReaction } = reaction; return { diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index a1900bff23d..474caf18ebe 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, @@ -480,10 +482,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/boot/store.js b/src/boot/store.js index d325b38420d..07533652ca7 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -218,6 +218,9 @@ const migrations: { [string]: (GlobalState) => GlobalState } = { narrows: Immutable.Map(state.narrows), }), + // Convert messages[].avatar_url from `string | null` to `AvatarURL`. + '17': dropCache, + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/common/UserAvatar.js b/src/common/UserAvatar.js index 557e24f0da1..6068da417e8 100644 --- a/src/common/UserAvatar.js +++ b/src/common/UserAvatar.js @@ -4,7 +4,7 @@ import { ImageBackground, View, PixelRatio } from 'react-native'; import { connect } from '../react-redux'; import { getCurrentRealm } from '../selectors'; -import { getAvatarUrl } from '../utils/avatar'; +import { getAvatarUrl, AvatarURL } from '../utils/avatar'; import Touchable from './Touchable'; import type { Dispatch, Message, User, CrossRealmBot } from '../types'; @@ -55,12 +55,18 @@ class UserAvatar extends PureComponent { { // 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, + realm: getCurrentRealm(state), + }), }, local_message_id: event.local_message_id, caughtUp: state.caughtUp, diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index a25d252d4d0..576b70eda48 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -112,9 +112,10 @@ describe('fetchActions', () => { }); describe('fetchMessages', () => { - const message1 = eg.streamMessage({ id: 1 }); + const sender = eg.makeUser(); + const message1 = eg.streamMessage({ id: 1, sender }); - type CommonFields = $Diff; + type CommonFields = $Diff; // message1 exactly as we receive it from the server, before our // own transformations. @@ -122,8 +123,14 @@ describe('fetchActions', () => { // TODO: Deduplicate this logic with similar logic in // migrateMessages-test.js. const serverMessage1: ServerMessage = { - ...(omit(message1, 'reactions'): CommonFields), + ...(omit(message1, 'reactions', 'avatar_url'): CommonFields), reactions: [], + // `User.avatar_url` is still in its raw form, which is what we + // want for raw message data from the server. In the next + // commit, we won't be able to use `User.avatar_url` as raw data + // because it'll be an `AvatarURL` instance. We'll do something + // else to simulate raw data here. + avatar_url: sender.avatar_url, }; const baseState = eg.reduxState({ @@ -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/outbox/outboxActions.js b/src/outbox/outboxActions.js index 1a1d06f5617..017d2c60cbe 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -15,6 +15,7 @@ import * as api from '../api'; import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors'; import { getUsersAndWildcards } from '../users/userHelpers'; import { caseNarrowPartial } from '../utils/narrow'; +import { AvatarURL } from '../utils/avatar'; import { BackoffMachine } from '../utils/async'; import { recipientsOfPrivateMessage, streamNameOfStreamMessage } from '../utils/recipient'; @@ -162,6 +163,7 @@ export const addToOutbox = (narrow: Narrow, content: string) => async ( ) => { const state = getState(); const ownUser = getOwnUser(state); + const auth = getAuth(state); const localTime = Math.round(new Date().getTime() / 1000); dispatch( @@ -175,7 +177,15 @@ export const addToOutbox = (narrow: Narrow, content: string) => async ( sender_full_name: ownUser.full_name, sender_email: ownUser.email, sender_id: ownUser.user_id, - avatar_url: ownUser.avatar_url, + + // In the next commit, we'll just use `ownUser.avatar_url`, + // since it'll already be an `AvatarURL`. + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl: ownUser.avatar_url, + email: ownUser.email, + realm: auth.realm, + }), + isOutbox: true, reactions: [], }), diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 00a6ed00290..b9f498da7bd 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -3,7 +3,7 @@ /* eslint-disable no-use-before-define */ import md5 from 'blueimp-md5'; -import type { Message, Outbox, UserOrBot } from '../types'; +import type { UserOrBot } from '../types'; import { tryParseUrl } from './url'; import * as logging from './logging'; import { ensureUnreachable } from '../types'; @@ -265,9 +265,3 @@ export const getAvatarUrl = ( export const getAvatarFromUser = (user: UserOrBot, realm: URL, sizePhysicalPx: number): string => getAvatarUrl(user.avatar_url, user.email, realm, sizePhysicalPx); - -export const getAvatarFromMessage = ( - message: Message | Outbox, - realm: URL, - sizePhysicalPx: number, -): string => getAvatarUrl(message.avatar_url, message.sender_email, realm, sizePhysicalPx); diff --git a/src/webview/html/messageAsHtml.js b/src/webview/html/messageAsHtml.js index 4b96e08da3b..b11b48b572f 100644 --- a/src/webview/html/messageAsHtml.js +++ b/src/webview/html/messageAsHtml.js @@ -12,7 +12,6 @@ import type { ImageEmojiType, } from '../../types'; import type { BackgroundData } from '../MessageList'; -import { getAvatarFromMessage } from '../../utils/avatar'; import { shortTime } from '../../utils/date'; import aggregateReactions from '../../reactions/aggregateReactions'; import { codeToEmojiMap } from '../../emoji/data'; @@ -119,13 +118,13 @@ $!${divOpenHtml} const { sender_full_name } = message; const sender_id = message.isOutbox ? backgroundData.ownUser.user_id : message.sender_id; - const avatarUrl = getAvatarFromMessage( - message, - backgroundData.auth.realm, - // 48 logical pixels; see `.avatar` and `.avatar img` in - // src/webview/static/base.css. - PixelRatio.getPixelSizeForLayoutSize(48), - ); + const avatarUrl = message.avatar_url + .get( + // 48 logical pixels; see `.avatar` and `.avatar img` in + // src/webview/static/base.css. + PixelRatio.getPixelSizeForLayoutSize(48), + ) + .toString(); const subheaderHtml = template`
From 8bef380be9765a72b3d19a99b4bcf2f26da54927 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 18 Jun 2020 15:27:49 -0700 Subject: [PATCH 18/23] crunchy shell: Use AvatarURL for User and CrossRealmBot objects. For the `/register` response, we added a `transform` function with a `migrateUserOrBot` helper in a recent commit, to be used here. For events (for added and updated users), the natural place to do this is in `eventToAction`. We don't reuse `migrateUserOrBot` from `registerForEvents` because users and bots are likely to have different shapes in the `/register` response and in events, and we'll want to handle those shapes separately. --- src/__tests__/lib/exampleData.js | 25 +++++----------- src/account-info/AccountDetails.js | 2 +- src/api/modelTypes.js | 25 ++++++++++++---- src/api/registerForEvents.js | 25 +++++++++++----- src/api/users/getUsers.js | 6 +++- src/boot/store.js | 4 +++ src/common/OwnAvatar.js | 2 +- src/common/UserAvatar.js | 34 +++------------------- src/common/UserAvatarWithPresence.js | 2 +- src/events/eventToAction.js | 28 ++++++++++++++---- src/message/__tests__/fetchActions-test.js | 16 +++++----- src/nullObjects.js | 3 +- src/outbox/outboxActions.js | 12 +------- src/users/__tests__/usersReducer-test.js | 6 +++- src/utils/avatar.js | 4 --- src/webview/html/__tests__/render-test.js | 1 - src/webview/html/messageTypingAsHtml.js | 15 +++++----- 17 files changed, 106 insertions(+), 104 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 463907302ee..5eb81dcce71 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -14,8 +14,8 @@ 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 { AvatarURL } from '../../utils/avatar'; import { ACCOUNT_SWITCH, LOGIN_SUCCESS, @@ -89,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() @@ -271,14 +274,7 @@ const messagePropertiesFromSender = (user: User) => { return deepFreeze({ sender_domain: '', - - // In the next commit, we'll just use `user.avatar_url`, since - // it'll already be an `AvatarURL`. - avatar_url: AvatarURL.fromUserOrBotData({ - rawAvatarUrl: user.avatar_url, - email: user.email, - realm, - }), + avatar_url: user.avatar_url, client: 'ExampleClient', gravatar_hash: 'd3adb33f', sender_email, @@ -373,14 +369,7 @@ export const streamMessage = (args?: {| const outboxMessageBase: $Diff = deepFreeze({ isOutbox: true, isSent: false, - - // In the next commit, we'll just use `selfUser.avatar_url`, since - // it'll already be an `AvatarURL`. - avatar_url: AvatarURL.fromUserOrBotData({ - rawAvatarUrl: selfUser.avatar_url, - email: selfUser.email, - realm, - }), + avatar_url: selfUser.avatar_url, content: '

Test.

', display_recipient: stream.name, // id: ..., diff --git a/src/account-info/AccountDetails.js b/src/account-info/AccountDetails.js index d1196dc6a16..7d9fc31055f 100644 --- a/src/account-info/AccountDetails.js +++ b/src/account-info/AccountDetails.js @@ -58,7 +58,7 @@ class AccountDetails extends PureComponent { return ( - + 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, diff --git a/src/api/registerForEvents.js b/src/api/registerForEvents.js index a19fdb81b30..ebc77097b70 100644 --- a/src/api/registerForEvents.js +++ b/src/api/registerForEvents.js @@ -4,6 +4,7 @@ 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, @@ -20,18 +21,26 @@ type RegisterForEventsParams = {| |}, |}; -const transformUser = (rawUser: {| ...User, avatar_url: string | null |}, realm: URL): User => - // In an upcoming commit, we'll convert the raw-data `avatar_url` - // field to an AvatarURL instance. - rawUser; +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, realm }), + }; +}; const transformCrossRealmBot = ( rawCrossRealmBot: {| ...CrossRealmBot, avatar_url: string | void | null |}, realm: URL, -): CrossRealmBot => - // In an upcoming commit, we'll convert the raw-data `avatar_url` - // field to an AvatarURL instance. - rawCrossRealmBot; +): CrossRealmBot => { + const { avatar_url: rawAvatarUrl, email } = rawCrossRealmBot; + + return { + ...rawCrossRealmBot, + avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm }), + }; +}; const transform = (rawInitialData: RawInitialData, auth: Auth): InitialData => ({ ...rawInitialData, 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 07533652ca7..b300d490c8a 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -221,6 +221,10 @@ const migrations: { [string]: (GlobalState) => GlobalState } = { // 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`. }; diff --git a/src/common/OwnAvatar.js b/src/common/OwnAvatar.js index 489c7a653b3..8cedec48fcc 100644 --- a/src/common/OwnAvatar.js +++ b/src/common/OwnAvatar.js @@ -20,7 +20,7 @@ type Props = $ReadOnly<{| class OwnAvatar extends PureComponent { render() { const { user, size } = this.props; - return ; + return ; } } diff --git a/src/common/UserAvatar.js b/src/common/UserAvatar.js index 6068da417e8..4d8beab39ad 100644 --- a/src/common/UserAvatar.js +++ b/src/common/UserAvatar.js @@ -1,29 +1,18 @@ /* @flow strict-local */ import React, { PureComponent, type Node as React$Node } from 'react'; import { ImageBackground, View, PixelRatio } from 'react-native'; -import { connect } from '../react-redux'; -import { getCurrentRealm } from '../selectors'; -import { getAvatarUrl, AvatarURL } from '../utils/avatar'; import Touchable from './Touchable'; -import type { Dispatch, Message, User, CrossRealmBot } from '../types'; - -type SelectorProps = {| - realm: URL, -|}; +import type { Message, User, CrossRealmBot } from '../types'; type Props = $ReadOnly<{| avatarUrl: | $PropertyType | $PropertyType | $PropertyType, - email: string, size: number, shape: 'rounded' | 'square', children?: React$Node, onPress?: () => void, - - dispatch: Dispatch, - ...SelectorProps, |}>; /** @@ -35,13 +24,13 @@ type Props = $ReadOnly<{| * @prop [children] - If provided, will render inside the component body. * @prop [onPress] - Event fired on pressing the component. */ -class UserAvatar extends PureComponent { +export default class UserAvatar extends PureComponent { static defaultProps = { shape: 'rounded', }; render() { - const { avatarUrl, email, realm, children, size, shape, onPress } = this.props; + const { avatarUrl, children, size, shape, onPress } = this.props; const borderRadius = shape === 'rounded' ? size / 8 : 0; const style = { height: size, @@ -55,18 +44,7 @@ class UserAvatar extends PureComponent { { ); } } - -export default connect(state => ({ - realm: getCurrentRealm(state), -}))(UserAvatar); diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index 580041b2341..9d679ae57fb 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -46,7 +46,7 @@ export default class UserAvatarWithPresence extends PureComponent { const { avatarUrl, email, size, onPress, shape } = this.props; return ( - + ); diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index 3cf74e10e08..05432b5b618 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -128,18 +128,27 @@ 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, 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, + email, + realm, + }), + }, }; + } case 'update': { - // We'll use this in an upcoming commit. - // eslint-disable-next-line no-unused-vars const existingUser = getAllUsersById(state).get(event.person.user_id); if (!existingUser) { // If we get one of these events and don't have @@ -163,7 +172,13 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { // currently use them: `avatar_source`, // `avatar_url_medium`, and `avatar_version`. ...(event.person.avatar_url !== undefined - ? { avatar_url: event.person.avatar_url } + ? { + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl: event.person.avatar_url, + email: existingUser.email, + realm, + }), + } : undefined), }, }; @@ -181,6 +196,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/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 576b70eda48..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,12 @@ describe('fetchActions', () => { }); describe('fetchMessages', () => { - const sender = eg.makeUser(); + 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; @@ -125,12 +131,7 @@ describe('fetchActions', () => { const serverMessage1: ServerMessage = { ...(omit(message1, 'reactions', 'avatar_url'): CommonFields), reactions: [], - // `User.avatar_url` is still in its raw form, which is what we - // want for raw message data from the server. In the next - // commit, we won't be able to use `User.avatar_url` as raw data - // because it'll be an `AvatarURL` instance. We'll do something - // else to simulate raw data here. - avatar_url: sender.avatar_url, + avatar_url: null, // Null in server data will be transformed to a GravatarURL }; const baseState = eg.reduxState({ @@ -188,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]); }); }); 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/outbox/outboxActions.js b/src/outbox/outboxActions.js index 017d2c60cbe..1a1d06f5617 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -15,7 +15,6 @@ import * as api from '../api'; import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors'; import { getUsersAndWildcards } from '../users/userHelpers'; import { caseNarrowPartial } from '../utils/narrow'; -import { AvatarURL } from '../utils/avatar'; import { BackoffMachine } from '../utils/async'; import { recipientsOfPrivateMessage, streamNameOfStreamMessage } from '../utils/recipient'; @@ -163,7 +162,6 @@ export const addToOutbox = (narrow: Narrow, content: string) => async ( ) => { const state = getState(); const ownUser = getOwnUser(state); - const auth = getAuth(state); const localTime = Math.round(new Date().getTime() / 1000); dispatch( @@ -177,15 +175,7 @@ export const addToOutbox = (narrow: Narrow, content: string) => async ( sender_full_name: ownUser.full_name, sender_email: ownUser.email, sender_id: ownUser.user_id, - - // In the next commit, we'll just use `ownUser.avatar_url`, - // since it'll already be an `AvatarURL`. - avatar_url: AvatarURL.fromUserOrBotData({ - rawAvatarUrl: ownUser.avatar_url, - email: ownUser.email, - realm: auth.realm, - }), - + avatar_url: ownUser.avatar_url, isOutbox: true, reactions: [], }), diff --git a/src/users/__tests__/usersReducer-test.js b/src/users/__tests__/usersReducer-test.js index 10c4b3f82bd..9926d331385 100644 --- a/src/users/__tests__/usersReducer-test.js +++ b/src/users/__tests__/usersReducer-test.js @@ -3,6 +3,7 @@ import deepFreeze from 'deep-freeze'; import * as eg from '../../__tests__/lib/exampleData'; 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'; @@ -88,7 +89,10 @@ describe('usersReducer', () => { test('When a user changes their avatar.', () => { check({ - avatar_url: eg.randString(), + 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, diff --git a/src/utils/avatar.js b/src/utils/avatar.js index b9f498da7bd..b93e054d3bf 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -3,7 +3,6 @@ /* eslint-disable no-use-before-define */ import md5 from 'blueimp-md5'; -import type { UserOrBot } from '../types'; import { tryParseUrl } from './url'; import * as logging from './logging'; import { ensureUnreachable } from '../types'; @@ -262,6 +261,3 @@ export const getAvatarUrl = ( AvatarURL.fromUserOrBotData({ rawAvatarUrl: avatarUrl, email, realm }) .get(sizePhysicalPx) .toString(); - -export const getAvatarFromUser = (user: UserOrBot, realm: URL, sizePhysicalPx: number): string => - getAvatarUrl(user.avatar_url, user.email, realm, sizePhysicalPx); 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 = '& template`
+ src="${user.avatar_url + .get( + // 48 logical pixels; see `.avatar` and `.avatar img` in + // src/webview/static/base.css. + PixelRatio.getPixelSizeForLayoutSize(48), + ) + .toString()}">
`; From c7a240b8928524df87a6b10820ddffb554a276d3 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Nov 2020 13:27:11 -0800 Subject: [PATCH 19/23] UserAvatarWithPresence types [nfc]: Spell `avatarUrl`'s type more simply. Now that the three unioned types are all `AvatarURL`. Also, make a slight change to the contract for this prop: although it probably will remain so in practice, the value doesn't have to come from `.avatar_url` on a `Message` or `UserOrBot`; any `AvatarURL` will be handled appropriately. --- src/common/UserAvatarWithPresence.js | 8 +++----- src/lightbox/LightboxHeader.js | 8 +++----- src/user-picker/AvatarItem.js | 8 +++----- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index 9d679ae57fb..d648a70d2b8 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -2,10 +2,10 @@ import React, { PureComponent } from 'react'; -import type { Message, User, CrossRealmBot } from '../types'; import { createStyleSheet } from '../styles'; import UserAvatar from './UserAvatar'; import PresenceStatusIndicator from './PresenceStatusIndicator'; +import { AvatarURL } from '../utils/avatar'; const styles = createStyleSheet({ status: { @@ -16,9 +16,7 @@ const styles = createStyleSheet({ }); type Props = $ReadOnly<{| - avatarUrl: | $PropertyType - | $PropertyType - | $PropertyType, + avatarUrl: AvatarURL, email: string, size: number, shape: 'rounded' | 'square', @@ -28,7 +26,7 @@ type Props = $ReadOnly<{| /** * Renders a user avatar with a PresenceStatusIndicator in the corner * - * @prop [avatarUrl] - `.avatar_url` on a `Message` or a `UserOrBot` + * @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. diff --git a/src/lightbox/LightboxHeader.js b/src/lightbox/LightboxHeader.js index 2990aeb2ec3..d70fad84ae3 100644 --- a/src/lightbox/LightboxHeader.js +++ b/src/lightbox/LightboxHeader.js @@ -2,11 +2,11 @@ import React, { PureComponent } from 'react'; import { View, Text } from 'react-native'; -import type { Message, User, CrossRealmBot } from '../types'; 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: { @@ -42,9 +42,7 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| senderName: string, timestamp: number, - avatarUrl: | $PropertyType - | $PropertyType - | $PropertyType, + avatarUrl: AvatarURL, onPressBack: () => void, |}>; @@ -52,7 +50,7 @@ type Props = $ReadOnly<{| * Shows sender's name and date of photo being displayed. * * @prop [senderName] - The sender's full name. - * @prop [avatarUrl] - `.avatar_url` on a `Message` or a `UserOrBot` + * @prop [avatarUrl] * @prop [timestamp] * @prop [onPressBack] */ diff --git a/src/user-picker/AvatarItem.js b/src/user-picker/AvatarItem.js index 3b100285106..4edfa0fcd92 100644 --- a/src/user-picker/AvatarItem.js +++ b/src/user-picker/AvatarItem.js @@ -2,10 +2,10 @@ import React, { PureComponent } from 'react'; import { Animated, Easing, View } from 'react-native'; -import type { Message, User, CrossRealmBot } from '../types'; 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: { @@ -28,9 +28,7 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| email: string, - avatarUrl: | $PropertyType - | $PropertyType - | $PropertyType, + avatarUrl: AvatarURL, fullName: string, onPress: (email: string) => void, |}>; @@ -39,7 +37,7 @@ type Props = $ReadOnly<{| * Pressable avatar for items in the user-picker card. * * @prop [email] - * @prop [avatarUrl] - `.avatar_url` on a `Message` or a `UserOrBot` + * @prop [avatarUrl] * @prop [fullName] * @prop [onPress] */ From b7caedb624c92d7ef9c8a3787636cfac2edf08ba Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Nov 2020 15:54:55 -0800 Subject: [PATCH 20/23] UserAvatar types [nfc]: Spell `avatarUrl`'s type more simply. Like we did in the previous commit, for `UserAvatarWithPresence`. --- src/common/UserAvatar.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/common/UserAvatar.js b/src/common/UserAvatar.js index 4d8beab39ad..00e4fa73b9d 100644 --- a/src/common/UserAvatar.js +++ b/src/common/UserAvatar.js @@ -3,12 +3,10 @@ import React, { PureComponent, type Node as React$Node } from 'react'; import { ImageBackground, View, PixelRatio } from 'react-native'; import Touchable from './Touchable'; -import type { Message, User, CrossRealmBot } from '../types'; +import { AvatarURL } from '../utils/avatar'; type Props = $ReadOnly<{| - avatarUrl: | $PropertyType - | $PropertyType - | $PropertyType, + avatarUrl: AvatarURL, size: number, shape: 'rounded' | 'square', children?: React$Node, @@ -18,7 +16,7 @@ type Props = $ReadOnly<{| /** * Renders an image of the user's avatar. * - * @prop avatarUrl - `.avatar_url` on a `Message` or a `UserOrBot` + * @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. From 2ba3b54823e6f0182109fec26760705d562bff88 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 16:34:32 -0700 Subject: [PATCH 21/23] avatar [nfc]: Remove `getAvatarUrl`, which is now unused. It was confusing to rely on this instead of putting avatar URLs into a standard form at the edge. Now, it has no call sites, so they can disappear! :) --- src/utils/avatar.js | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/utils/avatar.js b/src/utils/avatar.js index b93e054d3bf..01a34fafcaf 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -251,13 +251,3 @@ export class UploadedAvatarURL extends AvatarURL { return result; } } - -export const getAvatarUrl = ( - avatarUrl: ?string, - email: string, - realm: URL, - sizePhysicalPx: number, -): string => - AvatarURL.fromUserOrBotData({ rawAvatarUrl: avatarUrl, email, realm }) - .get(sizePhysicalPx) - .toString(); From 10935c441693f8f9654e8a411adba8d14451f2bf Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 18 Aug 2020 11:52:27 -0700 Subject: [PATCH 22/23] getMessages: Send `client_gravatar`. I'd thought we were already doing this, but apparently not. See doc at https://zulipchat.com/api/get-messages#parameters. --- src/api/messages/getMessages.js | 1 + src/utils/avatar.js | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index ca73fc5f4c8..a37a4859ba7 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -100,6 +100,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/utils/avatar.js b/src/utils/avatar.js index 01a34fafcaf..cb2838c77e6 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -28,16 +28,16 @@ export class AvatarURL { // this case; it should be pretty rare. return GravatarURL.validateAndConstructInstance({ email }); } else if (rawAvatarUrl === null) { - // If we announce `client_gravatar`, which we sometimes 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. + // 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 sometimes - // do), or if the server doesn't have - // EMAIL_ADDRESS_VISIBILITY_EVERYONE set, then `rawAvatarUrl` - // will be the absolute Gravatar URL 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 From c6426776853bb68496c4f3ca67f8e45819a379b6 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 17:58:01 -0700 Subject: [PATCH 23/23] registerForEvents: Start sending `user_avatar_url_field_optional`. See the point on `user_avatar_url_field_optional` at https://zulipchat.com/api/register-queue. This will reduce the volume of data that will have to be sent in the `/register` response, which should speed up the initial load. Now, the server may choose, at its own discretion, to omit the `avatar_url` field for users in the `/register` response [1], and we'll handle it by using the `/avatar/{user_id}` endpoint. [1] As of the introduction of the client capability (Zulip 3.0, feature level 18), it makes this decision based on whether the user has been inactive for a long time (using the `long_term_idle` field), but this is an implementation detail and should be expected to change without notice. Fixes: #4157 --- src/api/initialDataTypes.js | 4 +- src/api/messages/getMessages.js | 1 + src/api/registerForEvents.js | 16 +++-- src/boot/store.js | 9 ++- src/events/eventToAction.js | 12 ++-- src/message/fetchActions.js | 1 + src/utils/__tests__/avatar-test.js | 48 +++++++++++++-- src/utils/avatar.js | 98 ++++++++++++++++++++++++++++-- 8 files changed, 166 insertions(+), 23 deletions(-) diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index e1315d44b37..81718952b87 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -110,8 +110,8 @@ export type RawInitialDataRealmUser = {| 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 |}>, + realm_users: Array<{| ...User, avatar_url?: string | null |}>, + realm_non_active_users: Array<{| ...User, avatar_url?: string | null |}>, user_id: number, |}; diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index a37a4859ba7..3e0ebc93589 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -54,6 +54,7 @@ export const migrateMessages = (messages: ServerMessage[], identity: Identity): avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email: message.sender_email, + userId: message.sender_id, realm: identity.realm, }), reactions: reactions.map(reaction => { diff --git a/src/api/registerForEvents.js b/src/api/registerForEvents.js index ebc77097b70..4f23f32b1f7 100644 --- a/src/api/registerForEvents.js +++ b/src/api/registerForEvents.js @@ -18,27 +18,33 @@ 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 transformUser = (rawUser: {| ...User, avatar_url?: string | null |}, realm: URL): User => { const { avatar_url: rawAvatarUrl, email } = rawUser; return { ...rawUser, - avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm }), + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl, + email, + userId: rawUser.user_id, + realm, + }), }; }; const transformCrossRealmBot = ( - rawCrossRealmBot: {| ...CrossRealmBot, avatar_url: string | void | null |}, + rawCrossRealmBot: {| ...CrossRealmBot, avatar_url?: string | null |}, realm: URL, ): CrossRealmBot => { - const { avatar_url: rawAvatarUrl, email } = rawCrossRealmBot; + const { avatar_url: rawAvatarUrl, user_id: userId, email } = rawCrossRealmBot; return { ...rawCrossRealmBot, - avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm }), + avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm }), }; }; diff --git a/src/boot/store.js b/src/boot/store.js index b300d490c8a..d8a71347a3a 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -10,7 +10,7 @@ import { persistStore, autoRehydrate } from '../third/redux-persist'; import type { Config } from '../third/redux-persist'; import { ZulipVersion } from '../utils/zulipVersion'; -import { GravatarURL, UploadedAvatarURL } from '../utils/avatar'; +import { GravatarURL, UploadedAvatarURL, FallbackAvatarURL } from '../utils/avatar'; import type { Action, GlobalState } from '../types'; import config from '../config'; import { REHYDRATE } from '../actionConstants'; @@ -318,6 +318,11 @@ const customReplacer = (key, value, defaultReplacer) => { 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); }; @@ -334,6 +339,8 @@ const customReviver = (key, value, defaultReviver) => { 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/events/eventToAction.js b/src/events/eventToAction.js index 05432b5b618..e198780e7d9 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -89,6 +89,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl: event.message.avatar_url, email: event.message.sender_email, + userId: event.message.sender_id, realm: getCurrentRealm(state), }), }, @@ -132,7 +133,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { switch (event.op) { case 'add': { - const { avatar_url: rawAvatarUrl, email } = event.person; + const { avatar_url: rawAvatarUrl, user_id: userId, email } = event.person; return { type: EVENT_USER_ADD, id: event.id, @@ -141,6 +142,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { ...event.person, avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, + userId, email, realm, }), @@ -149,14 +151,15 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { } case 'update': { - const existingUser = getAllUsersById(state).get(event.person.user_id); + 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: event.person.user_id }, + { userId }, ); return { type: 'ignore' }; } @@ -164,7 +167,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { return { type: EVENT_USER_UPDATE, id: event.id, - userId: event.person.user_id, + userId, // Just the fields we want to overwrite. person: { // Note: The `avatar_url` field will be out of sync with @@ -175,6 +178,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { ? { avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl: event.person.avatar_url, + userId, email: existingUser.email, realm, }), 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/utils/__tests__/avatar-test.js b/src/utils/__tests__/avatar-test.js index 3ed083b0001..18da866abba 100644 --- a/src/utils/__tests__/avatar-test.js +++ b/src/utils/__tests__/avatar-test.js @@ -1,18 +1,25 @@ /* @flow strict-local */ import md5 from 'blueimp-md5'; -import { AvatarURL, GravatarURL, UploadedAvatarURL } from '../avatar'; +import { AvatarURL, GravatarURL, FallbackAvatarURL, UploadedAvatarURL } from '../avatar'; import * as eg from '../../__tests__/lib/exampleData'; describe('AvatarURL', () => { describe('fromUserOrBotData', () => { const user = eg.makeUser(); - const { email } = user; + const { email, user_id: userId } = user; const realm = eg.realm; + test('gives a `FallbackAvatarURL` if `rawAvatarURL` is undefined', () => { + const rawAvatarUrl = undefined; + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( + FallbackAvatarURL, + ); + }); + test('gives a `GravatarURL` if `rawAvatarURL` is null', () => { const rawAvatarUrl = null; - expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf( + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( GravatarURL, ); }); @@ -20,7 +27,7 @@ describe('AvatarURL', () => { 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, email, realm })).toBeInstanceOf( + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( GravatarURL, ); }); @@ -28,7 +35,7 @@ describe('AvatarURL', () => { 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, email, realm })).toBeInstanceOf( + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( UploadedAvatarURL, ); }); @@ -36,7 +43,7 @@ describe('AvatarURL', () => { 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, email, realm })).toBeInstanceOf( + expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf( UploadedAvatarURL, ); }); @@ -149,3 +156,32 @@ describe('UploadedAvatarURL', () => { }); }); }); + +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/avatar.js b/src/utils/avatar.js index cb2838c77e6..f8d5e7fe4d1 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -18,15 +18,25 @@ export class AvatarURL { */ static fromUserOrBotData(args: {| rawAvatarUrl: string | void | null, + userId: number, email: string, realm: URL, |}): AvatarURL { - const { rawAvatarUrl, email, realm } = args; + const { rawAvatarUrl, userId, email, realm } = args; if (rawAvatarUrl === undefined) { - // `rawAvatarUrl` will be undefined for cross-realm bots from - // servers prior to 58ee3fa8c (1.9.0). Fall back to Gravatar for - // this case; it should be pretty rare. - return GravatarURL.validateAndConstructInstance({ email }); + // 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 @@ -159,6 +169,84 @@ export class GravatarURL extends AvatarURL { } } +/** + * 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; + } + + /** + * Use a special string from `serialize` to make a new instance. + */ + static deserialize(serialized: string): FallbackAvatarURL { + return new FallbackAvatarURL(serialized); + } + + /** + * 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)); + } + + /** + * 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. + * + * 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); + } + + return this._standardUrl; + } +} + /** * An avatar that was uploaded to the Zulip server. *