-
Notifications
You must be signed in to change notification settings - Fork 479
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
Handle deleted user accounts for code review #46520
Conversation
comments_written = CodeReviewNote.where(commenter_id: user_id) | ||
comments_written_count = comments_written.count | ||
comments_written.each do |comment| | ||
comment.commenter_id = nil |
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 see this is how the Jira ticket is written, but surprised we're not deleting the contents of the comment? For future reference, @maureensturgeon could you clarify the reasoning here? Do we need to keep comments for legal reasons or something?
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 what was requested by Mike, I think so that if someone looks back at their code review they can still see the comments that they were given
# soft delete the code reviews of the user. This also soft deletes any comments on those reviews. | ||
code_reviews = CodeReview.where(user_id: user_id) | ||
code_reviews_count = code_reviews.count | ||
code_reviews.destroy_all |
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.
How do the comments (associated code review notes) get deleted?
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.
They get deleted via the dependent: :destroy
in code_review
Sorry for approving then raising a bunch of noise on this :) FWIW I think its fine to merge and then get clarity on the privacy question, but I don't think this would really block any other work so maybe just better to wait to clear things up. |
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 good, just pending the privacy conversation
after further discussion I created this PR to allow the comment to be null. After that's in I will update this PR to null out comments on user delete. |
@maureensturgeon @bencodeorg updated with new logic after discussion. |
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.
LGTM!
# Table: dashboard.code_review_requests | ||
# Table: dashboard.code_review_notes | ||
# | ||
test "soft deletes comments for purged user" do |
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.
Nit: maybe an update to this test name that notes there's some hard deleting going on this case, and aligns with the naming in the next test? Maybe "deletes code review comments on reviews owned by purged user"
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.
good call
refute_nil comment.commenter | ||
end | ||
|
||
test "anonymizes and deletes code review comments written by user" do |
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 guess we don't need to anonymize them anymore since they're just getting deleted
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 we are soft-deleting I'm still anonymizing
On user account delete, do the following:
Links
Testing story
Tested via unit tests.
PR Checklist: