Skip to content

Commit

Permalink
usersReducer: Handle EVENT_USER_UPDATE a little bit.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
chrisbobbe committed Dec 15, 2020
1 parent 3b76e5b commit 7296551
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 8 deletions.
6 changes: 4 additions & 2 deletions src/actionTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<User>,
|};

type EventMutedTopicsAction = {|
Expand Down
34 changes: 30 additions & 4 deletions src/events/eventToAction.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
81 changes: 80 additions & 1 deletion src/users/__tests__/usersReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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<User>) => {
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();

Expand Down
4 changes: 3 additions & 1 deletion src/users/usersReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 7296551

Please sign in to comment.