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

Deprecate changereason #655

Merged
merged 2 commits into from
Apr 26, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ Unreleased
------------
- Added `bulk_update_with_history` utility function (gh-650)
- Add default user and default change reason to `bulk_create_with_history` and `bulk_update_with_history`
- Start using `_change_reason` instead of `changeReason` to add change reasons to historical
objects. `changeReason` is deprecated and will be removed in version `3.0.0`

2.9.0 (2020-04-23)
------------------
Expand Down
6 changes: 3 additions & 3 deletions docs/common_issues.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,19 @@ history:
1000

If you want to specify a change reason or history user for each record in the bulk create,
you can add `changeReason` or `_history_user` on each instance:
you can add `_change_reason` or `_history_user` on each instance:

.. code-block:: pycon

>>> for poll in data:
poll.changeReason = 'reason'
poll._change_reason = 'reason'
poll._history_user = my_user
>>> objs = bulk_create_with_history(data, Poll, batch_size=500)
>>> Poll.history.get(id=data[0].id).history_change_reason
'reason'

You can also specify a default user or default change reason responsible for the change
(`_history_user` and `changeReason` take precedence).
(`_history_user` and `_change_reason` take precedence).

.. code-block:: pycon

Expand Down
10 changes: 5 additions & 5 deletions docs/historical_model.rst
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,10 @@ As an example using the callable mechanism, the below changes the default prefix
opinion = models.CharField(max_length=2000)

register(Opinion, custom_model_name=lambda x:f'Audit{x}')

The resulting history class names would be `AuditPoll` and `AuditOpinion`.
If the app the models are defined in is `yoda` then the corresponding history table names would be `yoda_auditpoll` and `yoda_auditopinion`

IMPORTANT: Setting `custom_model_name` to `lambda x:f'{x}'` is not permitted.
An error will be generated and no history model created if they are the same.

Expand Down Expand Up @@ -287,8 +287,8 @@ Change Reason
Change reason is a message to explain why the change was made in the instance. It is stored in the
field ``history_change_reason`` and its default value is ``None``.

By default, the django-simple-history gets the change reason in the field ``changeReason`` of the instance. Also, is possible to pass
the ``changeReason`` explicitly. For this, after a save or delete in an instance, is necessary call the
By default, the django-simple-history gets the change reason in the field ``_change_reason`` of the instance. Also, is possible to pass
the ``_change_reason`` explicitly. For this, after a save or delete in an instance, is necessary call the
function ``utils.update_change_reason``. The first argument of this function is the instance and the second
is the message that represents the change reason.

Expand All @@ -308,7 +308,7 @@ You can create an instance with an implicit change reason.
.. code-block:: python

poll = Poll(question='Question 1')
poll.changeReason = 'Add a question'
poll._change_reason = 'Add a question'
poll.save()

Or you can pass the change reason explicitly:
Expand Down
7 changes: 4 additions & 3 deletions simple_history/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
from django.db import models
from django.utils import timezone

from simple_history.utils import get_change_reason_from_object


class HistoryDescriptor(object):
def __init__(self, model):
Expand Down Expand Up @@ -121,9 +123,8 @@ def bulk_history_create(
row = self.model(
history_date=getattr(instance, "_history_date", timezone.now()),
history_user=getattr(instance, "_history_user", default_user),
history_change_reason=getattr(
instance, "changeReason", default_change_reason
),
history_change_reason=get_change_reason_from_object(instance)
or default_change_reason,
history_type=history_type,
**{
field.attname: getattr(instance, field.attname)
Expand Down
3 changes: 2 additions & 1 deletion simple_history/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from . import exceptions
from .manager import HistoryDescriptor
from .signals import post_create_historical_record, pre_create_historical_record
from .utils import get_change_reason_from_object

if django.VERSION < (2,):
from django.utils.translation import ugettext_lazy as _
Expand Down Expand Up @@ -479,7 +480,7 @@ def create_historical_record(self, instance, history_type, using=None):
using = using if self.use_base_model_db else None
history_date = getattr(instance, "_history_date", timezone.now())
history_user = self.get_history_user(instance)
history_change_reason = getattr(instance, "changeReason", None)
history_change_reason = get_change_reason_from_object(instance)
manager = getattr(instance, self.manager_name)

attrs = {}
Expand Down
2 changes: 1 addition & 1 deletion simple_history/tests/tests/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def test_history_list(self):
self.assertEqual(model_name, "customuser")
self.login()
poll = Poll(question="why?", pub_date=today)
poll.changeReason = "A random test reason"
poll._change_reason = "A random test reason"
poll._history_user = self.user
poll.save()

Expand Down
6 changes: 3 additions & 3 deletions simple_history/tests/tests/test_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def test_simple_bulk_history_create(self):

def test_bulk_history_create_with_change_reason(self):
for poll in self.data:
poll.changeReason = "reason"
poll._change_reason = "reason"

Poll.history.bulk_history_create(self.data)

Expand Down Expand Up @@ -162,7 +162,7 @@ def test_bulk_history_create_history_user_overrides_default(self):

def test_bulk_history_create_change_reason_overrides_default(self):
for data in self.data:
data.changeReason = "my_reason"
data._change_reason = "my_reason"

Poll.history.bulk_history_create(self.data, default_change_reason="test")

Expand Down Expand Up @@ -236,7 +236,7 @@ def test_simple_bulk_history_create(self):

def test_bulk_history_create_with_change_reason(self):
for poll in self.data:
poll.changeReason = "reason"
poll._change_reason = "reason"

Poll.history.bulk_history_create(self.data)

Expand Down
2 changes: 1 addition & 1 deletion simple_history/tests/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def test_update(self):
def test_delete_verify_change_reason_implicitly(self):
p = Poll.objects.create(question="what's up?", pub_date=today)
poll_id = p.id
p.changeReason = "wrongEntry"
p._change_reason = "wrongEntry"
p.delete()
delete_record, create_record = Poll.history.all()
self.assertRecordValues(
Expand Down
18 changes: 18 additions & 0 deletions simple_history/utils.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import warnings

import django
from django.db import transaction
from django.forms.models import model_to_dict
Expand Down Expand Up @@ -123,3 +125,19 @@ def bulk_update_with_history(
default_user=default_user,
default_change_reason=default_change_reason,
)


def get_change_reason_from_object(obj):
if hasattr(obj, "_change_reason"):
return getattr(obj, "_change_reason")
Copy link

Choose a reason for hiding this comment

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

This could be return obj._change_reason!
(or if this is called many times in loops, even more efficient would be try: return obj.change_reason except AttributeError: pass)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree on the latter - though this will be removed in next version anyway. How could this be return obj._change_reason though? If that doesn't exist, it needs to check changeReason

Copy link

Choose a reason for hiding this comment

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

I was saying two things at once!

1: not reason to use getattr + string after you’ve already tested that the attribute exists

if hasattr(obj, "_change_reason"):
    return obj._change_reason

2: if performance matters, you can avoid getting the attribute multiple times (hasattr under the covers tries to get the attr and catches AttributeError):

try:
    return object._change_reason
except AttributeError:
   pass

# rest of the code that tries changeReason

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, good points.


if hasattr(obj, "changeReason"):
warning_msg = (
"Using the attr changeReason to populate history_change_reason is"
" deprecated in 2.10.0 and will be removed in 3.0.0. Use "
"_change_reason instead. "
)
warnings.warn(warning_msg, DeprecationWarning)
Copy link

Choose a reason for hiding this comment

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

warnings most of the time need a stacklevel parameter (usually with value 2, but depends on the calling pattern) so that the message shows the calling code rather that the line with warnings.warn.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

didn't know that. thanks for the feedback

return getattr(obj, "changeReason")

return None