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

skip updating paranoia_destroy_attributes for records while really_destroy! #535

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

kortirso
Copy link
Contributor

There is issue with destroying records when, for example:

  • record had nil attribute, or optional belongs_to (database didn't have not null constaint)
  • record was soft-deleted
  • some validation was added, or belongs_to was changed to required state
  • calling record.really_destroy! will fail during update_columns, because record can't be saved with some nil values

and there is some additional profit, really_destroy! will work faster without updating timestamps

@mathieujobin
Copy link
Collaborator

if I understand your patch properly, its opt-out by default, so we need to specify really_destroy!(update_destroy_attributes: false) explicitly, otherwise, it makes no difference, am I reading this right?

Also, can you update the Changelog and bump the version, please?

@kortirso
Copy link
Contributor Author

if I understand your patch properly, its opt-out by default, so we need to specify really_destroy!(update_destroy_attributes: false) explicitly, otherwise, it makes no difference, am I reading this right?

Also, can you update the Changelog and bump the version, please?

yes, by default everything will work as usually, and only who wants to skip updating in the middle of really_destroy - they will call really_destroy!(update_destroy_attributes: false)

@kortirso
Copy link
Contributor Author

if I understand your patch properly, its opt-out by default, so we need to specify really_destroy!(update_destroy_attributes: false) explicitly, otherwise, it makes no difference, am I reading this right?

Also, can you update the Changelog and bump the version, please?

changelog and readme are updated

@kortirso
Copy link
Contributor Author

@mathieujobin is it mergeable?

@mathieujobin mathieujobin merged commit 09e3a9f into rubysherpas:core Nov 15, 2022
@mathieujobin
Copy link
Collaborator

Thanks for the ping, looks good

@mathieujobin
Copy link
Collaborator

released

@kortirso
Copy link
Contributor Author

thank you

karunkumar1ly pushed a commit to edcast/paranoia that referenced this pull request Feb 6, 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.

2 participants