-
Notifications
You must be signed in to change notification settings - Fork 531
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
Introduce #paranoia_destroy_attributes and #paranoia_restore_attributes extension points #245
Conversation
#paranoia_restore_attributes
and #paranoia_restore_attributes
as extension points
The alternative we're taking advantage of at the moment is to override the behavior of |
Failure looks spurious to me. |
Looks good to me. Could you please add some documentation for this to the README? |
I updated the branch here: https://github.com/brigade/paranoia/tree/update_attributes doesn't seem to be showing up in the diff here. Changes are that I update the readme, add some testing, and fixed an issue in .only_deleted, which would miss records when a null value signifies deletion. |
@radar I think this is ready to go |
@@ -1,5 +1,11 @@ | |||
# paranoia Changelog | |||
|
|||
## Unreleased |
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've done a bad job of keeping this up to date. Normally what I would do is just point people at the changes between tags.
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.
Would you rather I drop it then?
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.
Yup, that'd be preferable.
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 aside from the comments I've left. |
a92bed8
to
68576fd
Compare
This PR does not merge cleanly to master. Please update. |
…tension points Use update_columns rather than touch to update the record, for generality Unlike touch, update_columns does not create a transaction for itself, so we need to add the record to the transaction, if present. If there is not a current_transaction, the add is a no-op. This all means that delete will not invoke a transaction or run the after_commit callbacks unless called from within one, which is consistent with the Rails docs and the behavior of ActiveRecord::Base#delete.
68576fd
to
b2b8d19
Compare
@radar Done |
Introduce #paranoia_destroy_attributes and #paranoia_restore_attributes extension points
Thanks! |
We maintain a second
active
column for the sake of enforcing uniqueness among un-deleted rows - this is a sort of more general solution to the same problem https://github.com/radar/paranoia/wiki/Custom-sentinel-values attempts to solve (incidentally, a problem more elegantly solved by http://www.postgresql.org/docs/9.4/static/indexes-expressional.html, if one has access to them).As an alternative to maintaining this extra value via a separate callback, it would be nice to update it at the same time paranoia updates the
deleted_at
column. We can do that and perhaps serve other needs by introducing these 2 extension points. With them, getting the behavior we need is a simple matter of adding a value to each of the hashes from these methods.One significant difference brought on by this change is that
#touch
occurs inside a transaction, whereas#update_columns
creates no such transaction. This only affects#delete
because#destroy
already creates its own transaction. To my eye, this behavior is more in line with the activerecord documentation and default behavior, which makes no mention of transactions for#delete
but it is a change.