Skip to content

[FIX] No warning message is sent when user is removed from a team's main channel#21949

Merged
sampaiodiego merged 3 commits intodevelopfrom
teams-remove-member-message
May 7, 2021
Merged

[FIX] No warning message is sent when user is removed from a team's main channel#21949
sampaiodiego merged 3 commits intodevelopfrom
teams-remove-member-message

Conversation

@matheusbsilva137
Copy link
Contributor

Proposed changes (including videos or screenshots)

  • Send a warning message to a team's main channel when a user is removed from the team;
  • Trigger events while removing a user from a team's main channel;
  • Fix usersCount field in the team's main room when a user is removed from the team (usersCount is now decreased by 1).

Issue(s)

Task - ClickUp (no warning messages are sent)
Task - ClickUp (usersCount field is decreased by 2)

Steps to test or reproduce

Create a new team and insert/remove a member. When a user is removed or leaves a team, its main channel's usersCount field will be decreased by 1 and a warning message will be sent.

Further comments

Fixes #21901 (includes its changes)

@matheusbsilva137 matheusbsilva137 requested a review from a team May 4, 2021 20:31
milton-rucks
milton-rucks previously approved these changes May 5, 2021
KevLehman
KevLehman previously approved these changes May 5, 2021

await this.unsubscribeFromMain(team.roomId, member.userId);
const removedUser = await this.Users.findOneById(member.userId, { projection: { _id: 1, username: 1 } });
const byUser = uid !== member.userId ? await this.Users.findOneById(uid, { projection: { _id: 1, username: 1 } }) : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

my only issue with this is that if a user is removing some other users, for every user being removed it will execute the same find for uid..

to avoid this I'd suggest to do a find for uid outside the loop and use it when needed.

Copy link
Member

Choose a reason for hiding this comment

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

Following this line, it would be better to extract all the userIds before the loop, and then doing a find for all, instead of one by one 🤔 (then inside of this loop you can use arr.find)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I've just applied both changes (the find for uid is now performed only once, and I grouped the findOneById for each userId in a single find).

}

const membersIds = members.map((m) => m.userId);
const usersToRemove = await this.Users.find({ _id: { $in: membersIds } }, { projection: { _id: 1, username: 1 } }).toArray();
Copy link
Member

Choose a reason for hiding this comment

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

the new changes are looking good, it is definitely better finding all members at once 👏

we just need to avoid using custom queries, so in this case I'd recommend using this.Users.findByIds.. looks like it doesn't exists, so you'd to create it.. 😉

@sampaiodiego sampaiodiego merged commit 645b119 into develop May 7, 2021
@sampaiodiego sampaiodiego deleted the teams-remove-member-message branch May 7, 2021 17:18
@sampaiodiego sampaiodiego mentioned this pull request May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments