Skip to content
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

Stop deleting pusher_id on versions when the parent user is deleted #3766

Conversation

JackMcDataGrail
Copy link

Summary

This PR deletes the dependent: :nullify option that, when a User was deleted, was setting the pusher_id of all their Version objects to nil.

Risks

This approach does not add any risk (since the IDs are opaque), but it does create the somewhat strange condition that there will be Versions pointing at non-existent users. Rails handles this for optional relationships by simply returning nil as if there isn't an ID at all, which is fine. The alternative approach would be to switch to soft-deletion for users. However, this could present its own set of problems due to the retention of "private" data past the user's explicit request to delete it.

For now, I propose we stop nulling out the ID, and leave soft-deletion to another PR.

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #3766 (6b037f0) into master (77e3421) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3766   +/-   ##
=======================================
  Coverage   98.80%   98.80%           
=======================================
  Files         212      212           
  Lines        5198     5199    +1     
=======================================
+ Hits         5136     5137    +1     
  Misses         62       62           
Impacted Files Coverage Δ
app/models/user.rb 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@simi simi self-requested a review May 5, 2023 20:18
@simi
Copy link
Member

simi commented May 13, 2023

Hello!

I was thinking about this for some time and I don't think this is good idea. It will leave database in inconsistent state with broken relations. Soft deletion could be the way to keep at least something about original pusher when user got deleted.

@segiddins
Copy link
Member

@simi I think this is a step in the right direction, we can implement soft deletion separately

@simi
Copy link
Member

simi commented Jan 25, 2024

ℹ️ soft deletion happening at #4376 🙏

@segiddins segiddins closed this Feb 3, 2024
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.

None yet

3 participants