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 alternative user model for tracking history_user #371

Merged
merged 5 commits into from
Jun 19, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
Changes
=======

Unreleased
----------
- Add ability to specify alternative user_model for tracking

2.1.1 (2018-06-15)
------------------
- Fixed out-of-memory exception when running populate_history management command (gh-408)
Expand Down
61 changes: 60 additions & 1 deletion docs/advanced.rst
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,66 @@ referencing the ``changed_by`` field:
def _history_user(self, value):
self.changed_by = value

Admin integration requires that you use a ``_history_user.setter`` attribute with your custom ``_history_user`` property (see :ref:`admin_integration`).
Admin integration requires that you use a ``_history_user.setter`` attribute with
your custom ``_history_user`` property (see :ref:`admin_integration`).

Another option for identifying the change user is by providing a function via ``get_user``.
If provided it will be called everytime that the ``history_user`` needs to be
identified with the following key word arguments:

* ``history``: The current ``HistoricalRecords`` instance
* ``instance``: The current instance being modified
* ``request``: If using the middleware the current request object will be provided if they are authenticated.

This is very helpful when using ``register``:

.. code-block:: python

from django.db import models
from simple_history.models import HistoricalRecords

class Poll(models.Model):
question = models.CharField(max_length=200)
pub_date = models.DateTimeField('date published')
changed_by = models.ForeignKey('auth.User')


def get_poll_user(instance, **kwargs):
return instance.changed_by

register(Poll, get_user=get_poll_user)


Change User Model
------------------------------------

If you need to use a different user model then ``settings.AUTH_USER_MODEL``,
pass in the required model to ``user_model``. Doing this requires ``_history_user``
or ``get_user`` is provided as detailed above.

.. code-block:: python

from django.db import models
from simple_history.models import HistoricalRecords

class PollUser(models.Model):
user_id = models.ForeignKey('auth.User')


# Only PollUsers should be modifying a Poll
class Poll(models.Model):
question = models.CharField(max_length=200)
pub_date = models.DateTimeField('date published')
changed_by = models.ForeignKey(PollUser)
history = HistoricalRecords(user_model=PollUser)

@property
def _history_user(self):
return self.changed_by

@_history_user.setter
def _history_user(self, value):
self.changed_by = value

Custom ``history_id``
---------------------
Expand Down
2 changes: 1 addition & 1 deletion simple_history/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,6 @@ def register(
records.manager_name = manager_name
records.table_name = table_name
records.module = app and ("%s.models" % app) or model.__module__
records.cls = model
records.add_extra_methods(model)
records.finalize(model)
models.registered_models[model._meta.db_table] = model
41 changes: 25 additions & 16 deletions simple_history/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,29 @@
registered_models = {}


def default_get_user(request, **kwargs):
try:
return request.user
except AttributeError:
return None


class HistoricalRecords(object):
thread = threading.local()

def __init__(self, verbose_name=None, bases=(models.Model,),
user_related_name='+', table_name=None, inherit=False,
excluded_fields=None,
history_id_field=None,
history_change_reason_field=None):
excluded_fields=None, history_id_field=None,
history_change_reason_field=None,
user_model=None, get_user=default_get_user):
self.user_set_verbose_name = verbose_name
self.user_related_name = user_related_name
self.table_name = table_name
self.inherit = inherit
self.history_id_field = history_id_field
self.history_change_reason_field = history_change_reason_field
self.user_model = user_model
self.get_user = get_user
if excluded_fields is None:
excluded_fields = []
self.excluded_fields = excluded_fields
Expand Down Expand Up @@ -74,15 +83,11 @@ def save_without_historical_record(self, *args, **kwargs):

def finalize(self, sender, **kwargs):
inherited = False
try:
hint_class = self.cls
except AttributeError: # called via `register`
pass
else:
if hint_class is not sender: # set in concrete
inherited = (self.inherit and issubclass(sender, hint_class))
if not inherited:
return # set in abstract
if self.cls is not sender: # set in concrete
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this still work when you call register?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I updated the register method to store this and the unit tests have been updated to verify it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about populate_history? I noticed this removed adding the model to registered_models which is used there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

inherited = (self.inherit and issubclass(sender, self.cls))
if not inherited:
return # set in abstract

if hasattr(sender._meta, 'simple_history_manager_attribute'):
raise exceptions.MultipleRegistrationsError(
'{}.{} registered multiple times for history tracking.'.format(
Expand Down Expand Up @@ -207,7 +212,9 @@ def copy_fields(self, model):
def get_extra_fields(self, model, fields):
"""Return dict of extra fields added to the historical record model"""

user_model = getattr(settings, 'AUTH_USER_MODEL', 'auth.User')
user_model = self.user_model or getattr(
settings, 'AUTH_USER_MODEL', 'auth.User'
)

def revert_url(self):
"""URL for this change in the default admin site."""
Expand Down Expand Up @@ -349,12 +356,14 @@ def get_history_user(self, instance):
try:
return instance._history_user
except AttributeError:
request = None
try:
if self.thread.request.user.is_authenticated:
return self.thread.request.user
return None
request = self.thread.request
except AttributeError:
return None
pass

return self.get_user(history=self, instance=instance, request=request)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why include the HistoricalRecords instance in self.get_user? What might need to use that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Once I hear back about this history here, I'm happy to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.



def transform_field(field):
Expand Down
65 changes: 65 additions & 0 deletions simple_history/tests/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,71 @@ class InheritTracking4(TrackedAbstractBaseA):
pass


class BucketMember(models.Model):
name = models.CharField(max_length=30)
user = models.OneToOneField(
User,
related_name="bucket_member",
on_delete=models.CASCADE
)


class BucketData(models.Model):
changed_by = models.ForeignKey(
BucketMember,
on_delete=models.SET_NULL,
null=True, blank=True,
)
history = HistoricalRecords(user_model=BucketMember)

@property
def _history_user(self):
return self.changed_by


def get_bucket_member1(instance, **kwargs):
try:
return instance.changed_by
except AttributeError:
return None


class BucketDataRegister1(models.Model):
changed_by = models.ForeignKey(
BucketMember,
on_delete=models.SET_NULL,
null=True, blank=True,
)


register(
BucketDataRegister1,
user_model=BucketMember,
get_user=get_bucket_member1
)


def get_bucket_member2(request, **kwargs):
try:
return request.user.bucket_member
except AttributeError:
return None


class BucketDataRegister2(models.Model):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

data = models.CharField(max_length=30)

def get_absolute_url(self):
return reverse('bucket_data-detail', kwargs={'pk': self.pk})


register(
BucketDataRegister2,
user_model=BucketMember,
get_user=get_bucket_member2
)


class UUIDModel(models.Model):
id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
history = HistoricalRecords(
Expand Down
17 changes: 17 additions & 0 deletions simple_history/tests/tests/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
from simple_history.tests.tests.utils import middleware_override_settings
from ..models import (
Book,
BucketData,
BucketMember,
Choice,
ConcreteExternal,
Employee,
Expand Down Expand Up @@ -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):
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

"""Test deletes of a BucketMember doesn't cascade delete the history"""
self.login()
member = BucketMember.objects.create(name="member1", user=self.user)
bucket_data = BucketData(changed_by=member)
bucket_data.save()

historical_poll = bucket_data.history.all()[0]
self.assertEqual(historical_poll.history_user, member)

member.delete()

historical_poll = bucket_data.history.all()[0]
self.assertEqual(historical_poll.history_user, None)

def test_missing_one_to_one(self):
"""A relation to a missing one-to-one model should still show
history"""
Expand Down
21 changes: 20 additions & 1 deletion simple_history/tests/tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
from django.urls import reverse

from simple_history.tests.custom_user.models import CustomUser
from simple_history.tests.models import Poll
from simple_history.tests.models import (
BucketDataRegister2,
BucketMember,
Poll
)
from simple_history.tests.tests.utils import middleware_override_settings


Expand Down Expand Up @@ -160,3 +164,18 @@ def test_user_is_not_set_on_delete_view_when_not_logged_in(self):

self.assertListEqual([ph.history_user_id for ph in poll_history],
[None, None])

def test_bucket_member_is_set_on_create_view_when_logged_in(self):
self.client.force_login(self.user)
member1 = BucketMember.objects.create(name="member1", user=self.user)
data = {
'data': 'Test Data',
}
self.client.post(reverse('bucket_data-add'), data=data)
bucket_datas = BucketDataRegister2.objects.all()
self.assertEqual(bucket_datas.count(), 1)

history = bucket_datas.first().history.all()

self.assertListEqual([h.history_user_id for h in history],
[member1.id])
29 changes: 29 additions & 0 deletions simple_history/tests/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
AdminProfile,
Book,
Bookcase,
BucketData,
BucketDataRegister1,
BucketMember,
Choice,
City,
ConcreteAttr,
Expand Down Expand Up @@ -375,6 +378,32 @@ def test_model_with_excluded_fields(self):
self.assertIn('question', all_fields_names)
self.assertNotIn('pub_date', all_fields_names)

def test_user_model_override(self):
user1 = User.objects.create_user('user1', '1@example.com')
user2 = User.objects.create_user('user2', '1@example.com')
member1 = BucketMember.objects.create(name="member1", user=user1)
member2 = BucketMember.objects.create(name="member2", user=user2)
bucket_data = BucketData.objects.create(changed_by=member1)
bucket_data.changed_by = member2
bucket_data.save()
bucket_data.changed_by = None
bucket_data.save()
self.assertEqual([d.history_user for d in bucket_data.history.all()],
[None, member2, member1])

def test_user_model_override_registered(self):
user1 = User.objects.create_user('user1', '1@example.com')
user2 = User.objects.create_user('user2', '1@example.com')
member1 = BucketMember.objects.create(name="member1", user=user1)
member2 = BucketMember.objects.create(name="member2", user=user2)
bucket_data = BucketDataRegister1.objects.create(changed_by=member1)
bucket_data.changed_by = member2
bucket_data.save()
bucket_data.changed_by = None
bucket_data.save()
self.assertEqual([d.history_user for d in bucket_data.history.all()],
[None, member2, member1])

def test_uuid_history_id(self):
entry = UUIDModel.objects.create()

Expand Down
6 changes: 6 additions & 0 deletions simple_history/tests/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from django.contrib import admin

from simple_history.tests.view import (
BucketDataRegister2Create,
BucketDataRegister2Detail,
PollCreate,
PollDelete,
PollDetail,
Expand All @@ -17,6 +19,10 @@
urlpatterns = [
url(r'^admin/', admin.site.urls),
url(r'^other-admin/', other_admin.site.urls),
url(r'^bucket_data/add/$', BucketDataRegister2Create.as_view(),
name='bucket_data-add'),
url(r'^bucket_data/(?P<pk>[0-9]+)/$', BucketDataRegister2Detail.as_view(),
name='bucket_data-detail'),
url(r'^poll/add/$', PollCreate.as_view(), name='poll-add'),
url(r'^poll/(?P<pk>[0-9]+)/$', PollUpdate.as_view(), name='poll-update'),
url(r'^poll/(?P<pk>[0-9]+)/delete/$', PollDelete.as_view(),
Expand Down
12 changes: 11 additions & 1 deletion simple_history/tests/view.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
UpdateView
)

from simple_history.tests.models import Poll
from simple_history.tests.models import BucketDataRegister2, Poll


class PollCreate(CreateView):
Expand All @@ -33,3 +33,13 @@ class PollList(ListView):
class PollDetail(DetailView):
model = Poll
fields = ['question', 'pub_date']


class BucketDataRegister2Create(CreateView):
model = BucketDataRegister2
fields = ['data']


class BucketDataRegister2Detail(DetailView):
model = BucketDataRegister2
fields = ['data']