Allow alternative user model for tracking history_user #371
Conversation
Codecov Report
@@ Coverage Diff @@
## master #371 +/- ##
==========================================
+ Coverage 97.16% 97.18% +0.01%
==========================================
Files 14 14
Lines 635 639 +4
Branches 89 89
==========================================
+ Hits 617 621 +4
Misses 9 9
Partials 9 9
Continue to review full report at Codecov.
|
|
@rwlogel sorry for the delay on this. It touches a lot of the codebase and I want to go through it thoroughly before merging. Hopefully will have it looked at by EOW |
|
@rwlogel checking in on this. Is this still something you want to merge? |
|
@rossmechanic definitely, we are currently using a forked repo for our released product. It would be great if we could use the version from pip |
|
@rwlogel cool. Want to rebase this? I'll look at this week or early next week. I want to merge #381 today or tomorrow and then release 2.1.1. Then I'd be happy to look at this thoroughly and merge it into 2.2.0 next week (which will also include support for bulk_create and queryset updates that include history) |
|
@rossmechanic rebase complete. |
|
Thanks @rwlogel . I'll check this out early next week |
simple_history/models.py
Outdated
| excluded_fields=None, | ||
| history_id_field=None, | ||
| excluded_fields=None, history_id_field=None, | ||
| user_model=None, get_user=default_get_user, |
There was a problem hiding this comment.
Want to add user_model and get_user after history_change_reason_field in case anyone is just using ordered args?
There was a problem hiding this comment.
Sure I can re-order them
| inherited = (self.inherit and issubclass(sender, hint_class)) | ||
| if not inherited: | ||
| return # set in abstract | ||
| if self.cls is not sender: # set in concrete |
There was a problem hiding this comment.
Will this still work when you call register?
There was a problem hiding this comment.
Yes I updated the register method to store this and the unit tests have been updated to verify it.
There was a problem hiding this comment.
What about populate_history? I noticed this removed adding the model to registered_models which is used there
There was a problem hiding this comment.
The register function calls calls records.finalize(model) on line 34. The finalize method calls create_history_model which also updates registered_models on line 148.
simple_history/tests/models.py
Outdated
| return None | ||
|
|
||
|
|
||
| class BucketDataRegister2(models.Model): |
There was a problem hiding this comment.
I see the precedent for the ..1 ..2 naming already in the file but it's not very communicative. It looks like they're differentiated by how the history user is fetched - can we make the naming reflect that?
simple_history/models.py
Outdated
| return None | ||
| pass | ||
|
|
||
| return self.get_user(history=self, instance=instance, request=request) |
There was a problem hiding this comment.
Why include the HistoricalRecords instance in self.get_user? What might need to use that?
There was a problem hiding this comment.
Once I hear back about this history here, I'm happy to merge.
There was a problem hiding this comment.
I thought it may be useful to give the historical object to the method since it is a generic API that the user is providing. It could be removed.
There was a problem hiding this comment.
Yea I don't see a reason for it to be included. Let's get rid of it for now and if someone ends up needing it, we can reevaluate. Just remove that and then I'm happy to merge.
| @@ -301,6 +303,21 @@ def test_deleteting_user(self): | |||
| historical_poll = poll.history.all()[0] | |||
| self.assertEqual(historical_poll.history_user, None) | |||
|
|
|||
| def test_deleteting_member(self): | |||
@rossmechanic I improved the ability to set an alternative user model so it will also work with register and middleware as discussed in #351.