-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Remove direct chat channels when banishing user #5784
Conversation
4e707d7
to
37716ef
Compare
37716ef
to
bc57690
Compare
@@ -19,6 +19,7 @@ def banish | |||
delete_user_activity | |||
delete_comments | |||
delete_articles | |||
cleanup_chat_channels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we really need this level of indirection, I was just following the established pattern here. Will we also be calling delete_chat_channels
in other contexts than banishing users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there aren't other contexts where we'd deleted chat channels; maybe an opportunity to refactor in a different PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I just added this method, I'd rather update this PR and move it right away.
ccm.destroy! | ||
end | ||
|
||
# We only destroy direct message channels, not open and invite-only ones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zhao-Andy Your snippet in #3601 did not include a check list this, but I guess it's better to err on the side of caution here. Running the specs with the next line commented out would remove all open channels the banished user was in, which is probably not what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, we should only destroy
direct chat channels. I think we might also want to only remove_from_index!
for direct chat channels as well. Most banished users won't be a part of any open channels, but better to be safe here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I thought.
Also, this snippet:
chat_channel.chat_channel_memberships.each do |ccm|
ccm.remove_from_index!
ccm.destroy!
end
This would remove channel memberships for all other users if a banished account was part of an open channel, I just confirmed this via a spec.
I'd rather play this safe, so I rewrote the whole method to do this in a two step process:
- Get all direct channels and clean them up.
- Iterate over the user's remaining memberships and clean them up, leaving the channels alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one change about only completing the action for direct chat channels.
Thanks for the PR! Looks great overall.
@@ -19,6 +19,7 @@ def banish | |||
delete_user_activity | |||
delete_comments | |||
delete_articles | |||
cleanup_chat_channels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there aren't other contexts where we'd deleted chat channels; maybe an opportunity to refactor in a different PR?
ccm.destroy! | ||
end | ||
|
||
# We only destroy direct message channels, not open and invite-only ones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, we should only destroy
direct chat channels. I think we might also want to only remove_from_index!
for direct chat channels as well. Most banished users won't be a part of any open channels, but better to be safe here.
@Zhao-Andy Thanks for the feedback. I actually found another problem, so I rewrote the whole process little bit, see comment above. Please review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for the fix + refactor!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! LGTM
Closes #3601
What type of PR is this? (check all applicable)
Description
Users who got banished left some dangling associations, most notably direct chat channels. This PR adds code for cleaning them up.
Related Tickets & Documents
#3601
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
n/a
Added to documentation?