Skip to content

Commit

Permalink
registerForEvents: Start sending user_avatar_url_field_optional.
Browse files Browse the repository at this point in the history
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: zulip#4157
  • Loading branch information
chrisbobbe committed Aug 19, 2020
1 parent 912e8ee commit edcf172
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 17 deletions.
5 changes: 3 additions & 2 deletions src/api/registerForEvents.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,19 @@ type RegisterForEventsParams = {|
client_capabilities?: {|
notification_settings_null: boolean,
bulk_message_deletion: boolean,
user_avatar_url_field_optional: boolean,
|},
|};

export const migrateUserOrBot = <T: User | CrossRealmBot>(
rawUserOrBot: $FlowFixMe, // server data pre-transformation
realm: string,
): T => {
const { avatar_url: rawAvatarUrl, email } = rawUserOrBot;
const { avatar_url: rawAvatarUrl, user_id: userId, email } = rawUserOrBot;

return {
...rawUserOrBot,
avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm }),
avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm }),
};
};

Expand Down
9 changes: 8 additions & 1 deletion src/boot/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,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';
Expand Down Expand Up @@ -279,6 +279,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);
};
Expand All @@ -293,6 +298,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
}
Expand Down
10 changes: 6 additions & 4 deletions src/events/eventToAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,21 +132,22 @@ 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,
// TODO: Validate and rebuild `event.person`.
person: {
...event.person,
avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm }),
avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm }),
},
};
}
case 'update': {
const { user_id: userId } = event.person;
let existingUser: UserOrBot;
try {
existingUser = getUserForId(state, event.person.user_id);
existingUser = getUserForId(state, userId);
} catch (e) {
// We don't store deactivated users. We don't know whether
// these events are actually emitted for deactivated users
Expand All @@ -161,7 +162,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
Expand All @@ -172,6 +173,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => {
? {
avatar_url: AvatarURL.fromUserOrBotData({
rawAvatarUrl: event.person.avatar_url,
userId,
email: existingUser.email,
realm,
}),
Expand Down
1 change: 1 addition & 0 deletions src/message/fetchActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,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,
},
}),
),
Expand Down
37 changes: 31 additions & 6 deletions src/utils/__tests__/avatar-test.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,42 @@
/* @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 `GravatarURL` if `rawAvatarURL` is null', () => {
const rawAvatarUrl = null;
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf(
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf(
GravatarURL,
);
});

test('gives a `GravatarURL` if `rawAvatarURL` is a URL string on Gravatar origin', () => {
const rawAvatarUrl =
'https://secure.gravatar.com/avatar/2efaec12efd9bea8a089299208117786?d=identicon&version=3';
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf(
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf(
GravatarURL,
);
});

test('gives an `UploadedAvatarURL` if `rawAvatarURL` is a non-Gravatar absolute URL string', () => {
const rawAvatarUrl =
'https://zulip-avatars.s3.amazonaws.com/13/430713047f2cffed661f84e139a64f864f17f286?x=x&version=5';
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf(
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf(
UploadedAvatarURL,
);
});

test('gives an `UploadedAvatarURL` if `rawAvatarURL` is a relative URL string', () => {
const rawAvatarUrl =
'/user_avatars/2/08fb6d007eb10a56efee1d64760fbeb6111c4352.png?x=x&version=2';
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, email, realm })).toBeInstanceOf(
expect(AvatarURL.fromUserOrBotData({ rawAvatarUrl, userId, email, realm })).toBeInstanceOf(
UploadedAvatarURL,
);
});
Expand Down Expand Up @@ -181,3 +181,28 @@ describe('UploadedAvatarURL', () => {
});
});
});

describe('FallbackAvatarURL', () => {
test('serializes/deserializes correctly', () => {
const instance = new FallbackAvatarURL({
realm: new URL(eg.realm),
userId: eg.selfUser.user_id,
});

const roundTripped = FallbackAvatarURL.deserialize(FallbackAvatarURL.serialize(instance));

SIZES_WE_USE.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 = new FallbackAvatarURL({
realm: new URL('https://chat.zulip.org'),
userId,
});

expect(instance.get().toString()).toEqual(`https://chat.zulip.org/avatar/${userId.toString()}`);
});
});
74 changes: 70 additions & 4 deletions src/utils/avatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ export class AvatarURL {
* defined.
*/
static fromUserOrBotData: (args: {|
rawAvatarUrl: string | null,
rawAvatarUrl: string | void | null,
userId: number,
email: string,
realm: string,
|}) => AvatarURL;
Expand Down Expand Up @@ -100,6 +101,57 @@ 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 {
_url: URL;

/**
* Private; only stored for use by serialize/deserialize.
*/
_constructorArgs: {| realm: URL, userId: number |};

/**
* Serialize to a special string; reversible with `deserialize`.
*/
static serialize(instance: FallbackAvatarURL): string {
// eslint-disable-next-line no-underscore-dangle
const instanceConstructorArgs = instance._constructorArgs;
return JSON.stringify({
...instanceConstructorArgs,
realm: instanceConstructorArgs.realm.toString(),
});
}

/**
* Use a special string from `serialize` to make a new instance.
*/
static deserialize(serialized: string): FallbackAvatarURL {
const parsedArgs = JSON.parse(serialized);
return new FallbackAvatarURL({
...parsedArgs,
realm: new URL(parsedArgs.realm),
});
}

constructor(args: typeof FallbackAvatarURL.prototype._constructorArgs) {
super();
this._constructorArgs = args;
const { realm, userId } = args;
this._url = new URL(`/avatar/${userId.toString()}`, realm.origin);
}

get(size: number | void): URL {
return this._url;
}
}

/**
* An avatar that was uploaded locally, or to an S3 backend.
*
Expand Down Expand Up @@ -167,12 +219,26 @@ export class UploadedAvatarURL extends AvatarURL {
}

AvatarURL.fromUserOrBotData = (args: {|
rawAvatarUrl: string | null,
rawAvatarUrl: string | void | null,
userId: number,
email: string,
realm: string,
|}): AvatarURL => {
const { rawAvatarUrl, email, realm } = args;
if (rawAvatarUrl === null) {
const { rawAvatarUrl, userId, email, realm } = args;
if (rawAvatarUrl === undefined) {
// New in Zulip 2.2, feature level 18, the field may be missing 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 new FallbackAvatarURL({ realm: new URL(realm), userId });
} else if (rawAvatarUrl === null) {
// If we announce `client_gravatar`, which we do, `rawAvatarUrl`
// might be null. In that case, we take responsibility for
// computing a hash for the user's email and using it to form a
Expand Down

0 comments on commit edcf172

Please sign in to comment.