Remove direct chat channels when banishing user #5784
Conversation
citizen428
requested review from
jacobherrington and
Zhao-Andy
as code owners
citizen428
force-pushed
the
citizen428/remove-banished-user-data
branch
3 times, most recently
from
4e707d7
to
37716ef
Compare
citizen428
force-pushed
the
citizen428/remove-banished-user-data
branch
from
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.
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.
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.
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.
@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.
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.
That's what I thought.
Also, this snippet:
chat_channel.chat_channel_memberships.each do |ccm|
ccm.remove_from_index!
ccm.destroy!
endThis 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.
| @@ -19,6 +19,7 @@ def banish | |||
| delete_user_activity | |||
| delete_comments | |||
| delete_articles | |||
| cleanup_chat_channels | |||
There was a problem hiding this comment.
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.
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.
pr-triage
bot
added
PR: reviewed-changes-requested
pr-triage
bot
added
PR: unreviewed
|
@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. |
pr-triage
bot
added
PR: reviewed-approved
pr-triage
bot
added
PR: merged
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?