Skip to content

Commit

Permalink
Support Django's save method update_fields kwarg (#336)
Browse files Browse the repository at this point in the history
  • Loading branch information
abdullahkady authored Jan 7, 2022
1 parent 05c2805 commit 54dc20e
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 7 deletions.
8 changes: 7 additions & 1 deletion auditlog/diff.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def get_field_value(obj, field):
return value


def model_instance_diff(old, new):
def model_instance_diff(old, new, fields_to_check=None):
"""
Calculates the differences between two model instances. One of the instances may be ``None`` (i.e., a newly
created model or deleted model). This will cause all fields with a value to have changed (from ``None``).
Expand All @@ -85,6 +85,9 @@ def model_instance_diff(old, new):
:type old: Model
:param new: The new state of the model instance.
:type new: Model
:param fields_to_check: An iterable of the field names to restrict the diff to, while ignoring the rest of
the model's fields. This is used to pass the `update_fields` kwarg from the model's `save` method.
:type fields_to_check: Iterable
:return: A dictionary with the names of the changed fields as keys and a two tuple of the old and new field values
as value.
:rtype: dict
Expand All @@ -111,6 +114,9 @@ def model_instance_diff(old, new):
fields = set()
model_fields = None

if fields_to_check:
fields = set([field for field in fields if field.name in fields_to_check])

# Check if fields must be filtered
if (
model_fields
Expand Down
4 changes: 2 additions & 2 deletions auditlog/receivers.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ def log_update(sender, instance, **kwargs):
pass
else:
new = instance

changes = model_instance_diff(old, new)
update_fields = kwargs.get("update_fields", None)
changes = model_instance_diff(old, new, fields_to_check=update_fields)

# Log an entry only if there are changes
if changes:
Expand Down
81 changes: 77 additions & 4 deletions auditlog_tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,49 @@ def test_update(self):
msg="The change is correctly logged",
)

def test_update_specific_field_supplied_via_save_method(self):
obj = self.obj

# Change 2 fields, but save one only.
obj.boolean = True
obj.text = "Short text"
obj.save(update_fields=["boolean"])

# This implicitly asserts there is only one UPDATE change since the `.get` would fail otherwise.
self.assertJSONEqual(
obj.history.get(action=LogEntry.Action.UPDATE).changes,
'{"boolean": ["False", "True"]}',
msg="Object modifications that are not saved to DB are not logged when using the `update_fields`.",
)

def test_django_update_fields_edge_cases(self):
"""
The test ensures that if Django's `update_fields` behavior ever changes for special values `(None, [])`, the
package should too. https://docs.djangoproject.com/en/3.2/ref/models/instances/#specifying-which-fields-to-save
"""
obj = self.obj

# Change boolean, but save no changes by passing an empty list.
obj.boolean = True
obj.save(update_fields=[])

self.assertTrue(
obj.history.filter(action=LogEntry.Action.UPDATE).count() == 0,
msg="There is no log entries created",
)
obj.refresh_from_db()
self.assertFalse(obj.boolean) # Change didn't persist in DB as expected.

# Passing `None` should save both fields according to Django.
obj.integer = 1
obj.boolean = True
obj.save(update_fields=None)
self.assertJSONEqual(
obj.history.get(action=LogEntry.Action.UPDATE).changes,
'{"boolean": ["False", "True"], "integer": ["None", "1"]}',
msg="The 2 fields changed are correctly logged",
)

def test_delete(self):
"""Deletion is logged correctly."""
# Get the object to work with
Expand Down Expand Up @@ -235,9 +278,29 @@ def test_exception(self):
self.assertFalse(pre_save.has_listeners(LogEntry))


class SimpeIncludeModelTest(TestCase):
class SimpleIncludeModelTest(TestCase):
"""Log only changes in include_fields"""

def test_specified_save_fields_are_ignored_if_not_included(self):
obj = SimpleIncludeModel.objects.create(label="Initial label", text="Text")
obj.text = "New text"
obj.save(update_fields=["text"])

self.assertTrue(
obj.history.filter(action=LogEntry.Action.UPDATE).count() == 0,
msg="Text change was not logged, even when passed explicitly",
)

obj.label = "New label"
obj.text = "Newer text"
obj.save(update_fields=["text", "label"])

self.assertJSONEqual(
obj.history.get(action=LogEntry.Action.UPDATE).changes,
'{"label": ["Initial label", "New label"]}',
msg="Only the label was logged, regardless of multiple entries in `update_fields`",
)

def test_register_include_fields(self):
sim = SimpleIncludeModel(label="Include model", text="Looong text")
sim.save()
Expand All @@ -254,20 +317,30 @@ def test_register_include_fields(self):
self.assertTrue(sim.history.count() == 2, msg="There are two log entries")


class SimpeExcludeModelTest(TestCase):
class SimpleExcludeModelTest(TestCase):
"""Log only changes that are not in exclude_fields"""

def test_specified_save_fields_are_excluded_normally(self):
obj = SimpleExcludeModel.objects.create(label="Exclude model", text="Text")
obj.text = "New text"
obj.save(update_fields=["text"])

self.assertTrue(
obj.history.filter(action=LogEntry.Action.UPDATE).count() == 0,
msg="Text change was not logged, even when passed explicitly",
)

def test_register_exclude_fields(self):
sem = SimpleExcludeModel(label="Exclude model", text="Looong text")
sem.save()
self.assertTrue(sem.history.count() == 1, msg="There is one log entry")

# Change label, ignore
# Change label, record it.
sem.label = "Changed label"
sem.save()
self.assertTrue(sem.history.count() == 2, msg="There are two log entries")

# Change text, record
# Change text, ignore it.
sem.text = "Short text"
sem.save()
self.assertTrue(sem.history.count() == 2, msg="There are two log entries")
Expand Down

0 comments on commit 54dc20e

Please sign in to comment.