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

feat: add client-side RemoveMultipleOrgUsersMutation #10805

Open

tianrunhe wants to merge 8 commits into master from feat/removeMultipleOrgUsersMutation

Contributor

tianrunhe commented Feb 6, 2025 •

edited

Loading

Description

Following up #10675, this PR is the client-side mutation

Final checklist

I checked the code review guidelines
I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
I have performed a self-review of my code, the same way I'd do it for any other team member
I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
Whenever I took a non-obvious choice I added a comment explaining why I did it this way
I added the label Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
PR title is human readable and could be used in changelog

parabol-release-bot bot and others added 3 commits

February 6, 2025 15:46


          chore(release): release v8.23.8 (#10792)

e3d7b6e

Co-authored-by: parabol-release-bot[bot] <150284312+parabol-release-bot[bot]@users.noreply.github.com>


          Wire up related subscriptions

752d3f6


          Remove demo UI element for RemoveMultipleOrgUsersMutation

3d5b1a2

github-actions bot added the size/m label


          Merge branch 'master' into feat/removeMultipleOrgUsersMutation

3c160cf

tianrunhe marked this pull request as ready for review

February 6, 2025 14:06

tianrunhe requested a review from Dschoordsch

February 6, 2025 14:06


          Merge branch 'master' into feat/removeMultipleOrgUsersMutation

c63e19e

Dschoordsch requested changes

View reviewed changes

Contributor

Dschoordsch left a comment

Did not test

packages/server/graphql/public/mutations/removeMultipleOrgUsers.ts Outdated Show resolved Hide resolved

packages/server/graphql/public/mutations/removeMultipleOrgUsers.ts Outdated Show resolved Hide resolved

packages/client/mutations/RemoveMultipleOrgUsersMutation.ts Outdated Show resolved Hide resolved

packages/server/graphql/public/mutations/removeMultipleOrgUsers.ts Outdated Show resolved Hide resolved

packages/server/graphql/public/mutations/removeMultipleOrgUsers.ts Outdated Show resolved Hide resolved

packages/server/graphql/public/mutations/removeMultipleOrgUsers.ts Outdated Show resolved Hide resolved

tianrunhe added 2 commits

February 12, 2025 13:29


          Clean up subscription publish in removeMultipleOrgUsers

60a3462


          Handle task updates for removed org users properly

73c64d6

tianrunhe requested a review from Dschoordsch

February 18, 2025 20:46

Dschoordsch requested changes

View reviewed changes

Contributor

Dschoordsch left a comment

Wow, that's quite convoluted. From reviewing this I get the impression that removeOrgUser should be half-broken currently.

packages/client/mutations/RemoveMultipleOrgUsersMutation.ts

Comment on lines +43 to +46

+                  organization {
+                    id
+                    name
+                  }

Contributor

Dschoordsch Feb 20, 2025

-1 organization is currently unused. It also might not be able to resolve all fields for kicked out users as they're not a member anymore. As we need this for the kicked out users for their updates, add an organizationId field.

packages/server/graphql/public/mutations/removeMultipleOrgUsers.ts Show resolved Hide resolved

packages/client/mutations/RemoveMultipleOrgUsersMutation.ts Show resolved Hide resolved

packages/client/mutations/RemoveMultipleOrgUsersMutation.ts Show resolved Hide resolved

packages/client/mutations/RemoveMultipleOrgUsersMutation.ts

+                const removedOrgMembers = payload.getLinkedRecords('removedOrgMembers')
+                const orgId = payload.getLinkedRecord('organization').getValue('id')
+                if (users.some((user) => user.getValue('id') === viewerId)) {

Contributor

Dschoordsch Feb 20, 2025

-1 if we handle refreshing the auth token correctly, this update should not reach the kicked out users. Instead all the updates for the kicked should be done in the notification subscription.

packages/client/mutations/RemoveMultipleOrgUsersMutation.ts

+                    team {
+                      id
+                      name
+                      activeMeetings {

Contributor

Dschoordsch Feb 20, 2025

-1 This will be empty for the kicked out use because they are not a team member anymore:

parabol/packages/server/graphql/types/Team.ts

Line 218 in 73c64d6

if (!isTeamMember(authToken, teamId)) return []

I think it would be better to have a flat kickOutNotifications object with just constant fields and no nested objects.

packages/client/mutations/RemoveMultipleOrgUsersMutation.ts

+                const users = payload.getLinkedRecords('users')
+                const {viewerId} = atmosphere
+                if (users.some((user) => user.getValue('id') === viewerId)) {

Contributor

Dschoordsch Feb 20, 2025

-1 This should not reach the kicked out user anymore


          Merge branch 'master' into feat/removeMultipleOrgUsersMutation

0cbf402

Contributor Author

tianrunhe commented Feb 21, 2025

Hey @Dschoordsch , thanks so much for taking time to review this. I agree it's quite complicated and it also makes me wonder if removeOrgUser ever worked correctly. Here's my ongoing plan:

For this PR, I'd be focusing on making RemoveMultipleOrgUsersMutation work perfectly. I'll leave RemoveOrgUserMutation as-is.
When this PR is merged, I'd open a new PR to rename removeMultipleOrgUsers to removeOrgUsers
Finally, I'll remove removeOrgUser and replace all the client side call with the plural version removeOrgUsers

Sounds good?

Contributor Author

tianrunhe commented Feb 21, 2025

@Dschoordsch now with regards to this PR specifically, I have a doubt on this statement you made:

With the new auth token the kicked off users will need to update their subscriptions and then won't receive TEAM and ORGANIZATION updates for teams and orgs they're not a part of anymore

The unsubscribe operation comes with 1 second delay. In my test, the removed users would still get all the updates from the team/org they were removed from?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels