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

Allow for dependent nullify to happen on soft destroy #117

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Braden-077
Copy link
Member

@Braden-077 Braden-077 commented Apr 18, 2024

Why?

This PR adds the cascade effect to dependencies with dependent: :nullify by nullifying the dependency on soft destroy.

Note: this is an opt-in behavior, pulled from a specific use case within RoleModel. It's a draft for the moment just to see if this is worth pulling in.

What Changed

  • Add nullify to soft destroy (following what Rails nullify does)

@Braden-077 Braden-077 self-assigned this Apr 18, 2024
@Braden-077 Braden-077 changed the title Allow for nullify to cascade on soft destroy Allow for dependent nullify to happen on soft destroy Apr 18, 2024
@Braden-077 Braden-077 marked this pull request as ready for review April 19, 2024 16:02
@@ -70,9 +70,20 @@ def restore!
restore(deleted_at) || raise_validation_errors
end

def cascade_soft_destroy_nullify

Choose a reason for hiding this comment

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

Hmm, this would come in handy for a specific case in Gimme Credit https://github.com/RoleModel/gimme-credit/pull/495/files

Is there a reason you aren't checking belongs_to and has_one associations?

@BlaineIrvin perhaps we should use this pattern in Gimme Credit?

Copy link
Member Author

Choose a reason for hiding this comment

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

@justwiebe No, the has_one would be another good one to include!

I'm curious why we'd need to do anything to our belongs_to, could you explain that one?

Choose a reason for hiding this comment

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

belongs_to might be an even bigger edge case, but for our situation, we've got a chain of reports that "replace" the previous one. We disallow soft destroying a report in the middle of the chain to side step complications, but we do allow soft destroying the latest report. To keep the chain linear, we decided to remove the soft destroyed one from it altogether be removing the belongs_to association

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds like you're having the child class nullify its own association. There could CERTAINLY be value in that one!

Copy link
Member Author

@Braden-077 Braden-077 Apr 22, 2024

Choose a reason for hiding this comment

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

That sounds like an after_soft_destroy :nullify_self

Choose a reason for hiding this comment

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

I'm not sure that belongs_to would fit into this pattern, since it looks like it can't be dependent nullify, only delete, destroy, and destroy_async.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well sure, maybe not out of the box. You could use an arbitrary value on that dependent call. 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

@justwiebe @BlaineIrvin Thinking back on this - I'm going to add the has_one update. I think the specific case of the parent record might be best solved by touching the parent record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants