Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

user-groups: Remove deactivated users from groups #5905

Merged
merged 2 commits into from
Nov 13, 2024
Merged

user-groups: Remove deactivated users from groups #5905

merged 2 commits into from
Nov 13, 2024

Conversation

chrisbobbe
Copy link
Contributor

Also move users between state.users and state.realm.nonActiveUsers in response to realm_user/update events that have is_active.

Fixes: #5899

@chrisbobbe chrisbobbe requested a review from gnprice November 12, 2024 01:06
gnprice
gnprice reviewed Nov 12, 2024
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this! Comments below.

src/user-groups/__tests__/userGroupsReducer-test.js Outdated
Comment on lines 208 to 211
const person = { userToDeactivate: user1 };
const action = deepFreeze({
type: EVENT,
event: { id: 1, type: EventTypes.realm_user, op: 'update', person },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write these tests instead in terms of the event we get from the server? That's the actual API we're implementing (that's external to this codebase itself), so it's the interface we'd ideally be testing at.

The existing tests in this file don't really work that way — and we're not going to convert them, in this legacy codebase — but I think it should be straightforward to write a new test that way: just call eventToAction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, good idea.

src/api/eventTypes.js Outdated
Comment on lines 458 to 463
| {|
// Digested from `is_active: true`. We add the full user details
// from our data structures; the event only carries the user ID.
+userToActivate: User,
|}
| {| +userToDeactivate: User |}, // from `is_active: false`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this ridiculousness is driven by the fact that we keep deactivated users in a different part of the state from most users, and so those two sub-reducers need to get the data from each other.

How about this, though:

Suggested change
| {|
// Digested from `is_active: true`. We add the full user details
// from our data structures; the event only carries the user ID.
+userToActivate: User,
|}
| {| +userToDeactivate: User |}, // from `is_active: false`
| {| +user_id: UserOrBot['user_id'], +is_active: boolean, +existingUser: UserOrBot |},

That way it's a bit clearer that our processing in eventToAction is just adding information from the state to accompany the information from the server's event.

Also then the reducers only have to consult the added property existingUser in the two cases that actually need information from outside the server event, and can stick to the properties from the server in the three other cases that don't need that extra context.

Copy link
Contributor Author

@chrisbobbe chrisbobbe Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense.

How about existingUser: User, and still having eventToAction look for it only in users or realm.nonActiveUsers as appropriate for is_active?

If we also look for it in realm.crossRealmBots (as tryGetUserForId does), then it might be a CrossRealmBot, and both users and realm.nonActiveUsers are $ReadOnlyArray<User>. I don't think we've written logic that checks if a UserOrBot is a User or a CrossRealmBot, but I guess we could do that if needed:

const isCrossRealmBot = (userOrBot: UserOrBot): boolean =>
  userOrBot.is_system_bot === true || userOrBot.is_cross_realm_bot === true;

It also seems nice to avoid adding a user to users or realm.nonActiveUsers if the user is already there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, User is fine. I typed UserOrBot out of habit because that's what we almost always want; but here since we're actually updating these data structures that have a difference, User is accurate.

@chrisbobbe chrisbobbe force-pushed the pr-remove-deactivated-users-from-groups branch from c5d0795 to 54998e7 Compare November 12, 2024 21:36
@chrisbobbe
Copy link
Contributor Author

Thanks for the review! Revision pushed.

gnprice
gnprice reviewed Nov 12, 2024
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One nit below; otherwise all LGTM.

src/realm/realmReducer.js Outdated
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const userId = person.existingUser.user_id;
const userId = person.user_id;

(or further simplify)

That way we're using the existingUser oddity only in the cases it's needed.

src/users/usersReducer.js Outdated
Comment on lines 79 to 85
// 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 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoying, yeah. Well, not our problem soon. 🤷 😄

@chrisbobbe
users: On update-user with is_active, users ↔ realm.nonActiveUsers
c63407f 
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.
@chrisbobbe
user-groups: Remove deactivated users from groups
f0cd0d5 
As newly required to support Zulip Server 10+; see zulip#5899.

Fixes: zulip#5899
@chrisbobbe chrisbobbe force-pushed the pr-remove-deactivated-users-from-groups branch from 54998e7 to f0cd0d5 Compare November 13, 2024 00:15
@chrisbobbe
Copy link
Contributor Author

Thanks for the review! Revision pushed.

@gnprice gnprice merged commit f0cd0d5 into zulip:main Nov 13, 2024
1 check passed
@gnprice
Copy link
Member

gnprice commented Nov 13, 2024

Thanks for the revisions! Looks good; merging.

@chrisbobbe chrisbobbe deleted the pr-remove-deactivated-users-from-groups branch November 13, 2024 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

Assignees

@gnprice gnprice

Labels
P1 high-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove deactivated users from groups
2 participants
@chrisbobbe @gnprice