-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Improve soft delete mixin #290
Conversation
the restore method should just turn the query into an update method: should run this query: builder.update({"deleted_at": None}) Force delete should just delete the record using a DELETE query |
If you pull in 0.9 it will update some of those broken tests |
Changed this to be based on new 1.0 branch |
Added force delete |
Added restore method |
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 like I finished the last 2 methods required to close this ticket out! Great job!
return builder.where_not_null("deleted_at") | ||
|
||
def _force_delete(self, model, builder): | ||
return builder.remove_global_scope(self).set_action("delete") |
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.
@josephmancuso I am just not sure to understand why we should call remove_global_scope(self)
before setting action type ? it is to avoid other scope to apply so that force_delete
really deletes ? If this was not there, the force_delete would be modified to an update of deleted_at with a timestamp through _query_set_null_on_delete
?
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 am just not sure to understand why we should call remove_global_scope(self) before setting action type
doesn't matter which order we do it in.
builder.set_action("delete").remove_global_scope(self)
Works just as well. We do have to remove the scopes because yeah we don't want other scopes changing the action type back to update. We might not even have to set the action here, just removing the scope might actually work ..
Implements #276