Skip to content

Commit

Permalink
users: On update-user with is_active, usersrealm.nonActiveUsers
Browse files Browse the repository at this point in the history
The `is_active` property of realm_user/update events is new in Zulip
Server 8.0 (FL 222):
  https://zulip.com/api/get-events#realm_user-update

Start handling it by moving the user between `state.users` (where we
keep active users) and `state.realm.nonActiveUsers`.

For servers 10.0+ (starting at FL 303), the client has more to do to
handle user deactivation; that's zulip#5899 "Remove deactivated users
from groups". Best to do this more basic thing first.
  • Loading branch information
chrisbobbe committed Nov 12, 2024
1 parent 715d60a commit ad3a26f
Show file tree
Hide file tree
Showing 6 changed files with 151 additions and 4 deletions.
10 changes: 8 additions & 2 deletions src/api/eventTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {
UserSettings,
ClientPresence,
UserTopic,
User,
} from './modelTypes';
import type { RealmDataForUpdate } from './realmDataTypes';

Expand Down Expand Up @@ -422,7 +423,11 @@ export type RealmUserUpdateEventRaw = {|
// the collections of users and bots in the initial data. Ignore.
// avatar_source: string,
// avatar_url_medium: string,
|},
|}
// TODO(flow) should be just one branch with `boolean` instead of
// separate `true` and `false`; investigate errors
| {| +user_id: UserOrBot['user_id'], +is_active: true |}
| {| +user_id: UserOrBot['user_id'], +is_active: false |},
|};

/** A realm_user update event, after we've processed it at the edge. */
Expand All @@ -449,7 +454,8 @@ export type RealmUserUpdateEvent = {|
// the collections of users and bots in the initial data. Ignore.
// avatar_source: string,
// avatar_url_medium: string,
|},
|}
| {| +user_id: User['user_id'], +is_active: boolean, +existingUser: User |},
|};

export type UserTopicEvent = {|
Expand Down
26 changes: 26 additions & 0 deletions src/events/eventToAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,32 @@ export default (state: PerAccountState, event: $FlowFixMe): EventAction | null =
},
},
};
} else if (person.is_active === true) {
const { is_active, user_id } = person;
const nonActiveUser = state.realm.nonActiveUsers.find(u => u.user_id === user_id);
if (nonActiveUser == null) {
logging.warn(
'Got realm_user/update event to reactivate a user not found in state.realm.nonActiveUsers',
);
return null;
}
return {
type: EVENT,
event: { ...rawEvent, person: { user_id, is_active, existingUser: nonActiveUser } },
};
} else if (person.is_active === false) {
const { is_active, user_id } = person;
const activeUser = state.users.find(u => u.user_id === user_id);
if (activeUser == null) {
logging.warn(
'Got realm_user/update event to deactivate a user not found in state.users',
);
return null;
}
return {
type: EVENT,
event: { ...rawEvent, person: { user_id, is_active, existingUser: activeUser } },
};
} else {
return {
type: EVENT,
Expand Down
44 changes: 44 additions & 0 deletions src/realm/__tests__/realmReducer-test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* @flow strict-local */
import deepFreeze from 'deep-freeze';
import invariant from 'invariant';

import type { RealmState } from '../../types';
import realmReducer from '../realmReducer';
Expand All @@ -19,6 +20,7 @@ import {
import { CustomProfileFieldType } from '../../api/modelTypes';
import { EventTypes } from '../../api/eventTypes';
import * as eg from '../../__tests__/lib/exampleData';
import eventToAction from '../../events/eventToAction';

const labelFromValue = value =>
/* $FlowFixMe[incompatible-use] - CreateWebPublicStreamPolicy is a
Expand Down Expand Up @@ -576,5 +578,47 @@ describe('realmReducer', () => {
check(false, false);
});
});

describe('type `realm_user`, op `update`', () => {
test('User is deactivated', () => {
const user = eg.makeUser();

const event = {
id: 0,
type: 'realm_user',
op: 'update',
person: { user_id: user.user_id, is_active: false },
};

const prevRealmState = { ...eg.plusReduxState.realm, nonActiveUsers: [] };
const prevPerAccountState = eg.reduxStatePlus({ users: [user], realm: prevRealmState });
const action = eventToAction(prevPerAccountState, event);
expect(action).not.toBeNull();
invariant(action != null, 'action not null');

const actualState = realmReducer(prevRealmState, action);
expect(actualState.nonActiveUsers).toEqual([user]);
});

test('User is reactivated', () => {
const user = eg.makeUser();

const event = {
id: 0,
type: 'realm_user',
op: 'update',
person: { user_id: user.user_id, is_active: true },
};

const prevRealmState = { ...eg.plusReduxState.realm, nonActiveUsers: [user] };
const prevPerAccountState = eg.reduxStatePlus({ users: [], realm: prevRealmState });
const action = eventToAction(prevPerAccountState, event);
expect(action).not.toBeNull();
invariant(action != null, 'action not null');

const actualState = realmReducer(prevRealmState, action);
expect(actualState.nonActiveUsers).toEqual([]);
});
});
});
});
15 changes: 15 additions & 0 deletions src/realm/realmReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,21 @@ export default (
// `op: 'update_dict'` events near the edge.)
return state;

case EventTypes.realm_user: {
const { person } = event;

// TODO(flow) teach Flow that the `person.existingUser != null`s are redundant
if (person.is_active === false && person.existingUser != null) {
return { ...state, nonActiveUsers: [...state.nonActiveUsers, person.existingUser] };
} else if (person.is_active === true && person.existingUser != null) {
const userId = person.existingUser.user_id;
const nonActiveUsers = state.nonActiveUsers.filter(u => u.user_id !== userId);
return { ...state, nonActiveUsers };
}

return state;
}

case EventTypes.user_settings:
if (event.op === 'update') {
const { property, value } = event;
Expand Down
44 changes: 44 additions & 0 deletions src/users/__tests__/usersReducer-test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* @flow strict-local */
import deepFreeze from 'deep-freeze';
import invariant from 'invariant';

import * as eg from '../../__tests__/lib/exampleData';
import { UploadedAvatarURL } from '../../utils/avatar';
Expand All @@ -9,6 +10,7 @@ import type { User } from '../../types';
import { Role } from '../../api/permissionsTypes';
import usersReducer from '../usersReducer';
import { randString } from '../../utils/misc';
import eventToAction from '../../events/eventToAction';

describe('usersReducer', () => {
describe('REGISTER_COMPLETE', () => {
Expand Down Expand Up @@ -144,6 +146,48 @@ describe('usersReducer', () => {
const new_email = randString();
check({ new_email }, { ...theUser, email: new_email });
});

test('When a user is deactivated', () => {
const event = {
id: 0,
type: 'realm_user',
op: 'update',
person: { user_id: theUser.user_id, is_active: false },
};

const prevUsersState = [theUser];
const prevPerAccountState = eg.reduxStatePlus({
users: prevUsersState,
realm: { ...eg.plusReduxState.realm, nonActiveUsers: [] },
});
const action = eventToAction(prevPerAccountState, event);
expect(action).not.toBeNull();
invariant(action != null, 'action not null');

const actualState = usersReducer(prevUsersState, action);
expect(actualState).toEqual([]);
});

test('When a user is activated', () => {
const event = {
id: 0,
type: 'realm_user',
op: 'update',
person: { user_id: theUser.user_id, is_active: true },
};

const prevUsersState = [];
const prevPerAccountState = eg.reduxStatePlus({
users: prevUsersState,
realm: { ...eg.plusReduxState.realm, nonActiveUsers: [theUser] },
});
const action = eventToAction(prevPerAccountState, event);
expect(action).not.toBeNull();
invariant(action != null, 'action not null');

const actualState = usersReducer(prevUsersState, action);
expect(actualState).toEqual([theUser]);
});
});

describe('RESET_ACCOUNT_DATA', () => {
Expand Down
16 changes: 14 additions & 2 deletions src/users/usersReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,14 @@ export default (
case EventTypes.realm_user: {
switch (event.op) {
case 'update': {
const { person } = event;
// TODO(flow) teach Flow that the `person.existingUser != null`s are redundant
if (person.is_active === true && person.existingUser != null) {
return [...state, person.existingUser];
} else if (person.is_active === false && person.existingUser != null) {
return state.filter(u => u.user_id !== person.user_id);
}
return state.map(user => {
const { person } = event;
if (user.user_id !== person.user_id) {
return user;
}
Expand Down Expand Up @@ -70,7 +76,13 @@ export default (
} else if (person.new_email !== undefined) {
return { ...user, email: person.new_email };
} else {
return { ...user, ...person };
// eslint-disable-next-line no-unused-vars
const { existingUser, is_active, ...rest } = person;

// TODO(flow) Use `...person`, not `...rest`; teach Flow that
// that existingUser and is_active are absent in `person`;
// see early-returns before the loop-through-users.
return { ...user, ...rest };
}
});
}
Expand Down

0 comments on commit ad3a26f

Please sign in to comment.