Skip to content

Commit

Permalink
Sort ModelChange's old and new M2M lists
Browse files Browse the repository at this point in the history
This should help prevent flaky tests, like the order of the `Place`
objects in
`AdminSiteTest.test_history_list_contains_diff_changes_for_m2m_fields()`.
  • Loading branch information
ddabble committed Mar 9, 2024
1 parent 8acfbc0 commit 4fe8463
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 11 deletions.
3 changes: 2 additions & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ Unreleased
- ``ModelDelta`` and ``ModelChange`` (in ``simple_history.models``) are now immutable
dataclasses; their signatures remain unchanged (gh-1128)
- ``ModelDelta``'s ``changes`` and ``changed_fields`` are now sorted alphabetically by
field name, to prevent flaky tests by making their order consistent (gh-1128)
field name. Also, if ``ModelChange`` is for an M2M field, its ``old`` and ``new``
lists are sorted by the related object. This should help prevent flaky tests. (gh-1128)
- ``diff_against()`` has a new keyword argument, ``foreign_keys_are_objs``;
see usage in the docs under "History Diffing" (gh-1128)
- Added ``HistoricalQuerySet.select_related_history_tracked_objs()``; see usage in the
Expand Down
1 change: 1 addition & 0 deletions docs/history_diffing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ This will return a ``ModelDelta`` object with the following attributes:

- For many-to-many fields, these values will be lists of dictionaries from the
through model fields to the primary keys of the through model's related objects.
The lists are sorted by the value of the many-to-many related object.

This may be useful when you want to construct timelines and need to get only
the model modifications.
Expand Down
23 changes: 13 additions & 10 deletions simple_history/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
pre_create_historical_m2m_records,
pre_create_historical_record,
)
from .utils import get_change_reason_from_object
from .utils import get_change_reason_from_object, get_m2m_reverse_field_name

try:
from asgiref.local import Local as LocalContext
Expand Down Expand Up @@ -1047,9 +1047,12 @@ def _get_m2m_field_changes_for_diff(
changes = []

for field in m2m_fields:
old_m2m_manager = getattr(old_history, field)
new_m2m_manager = getattr(self, field)
m2m_through_model_opts = new_m2m_manager.model._meta
original_field_meta = self.instance_type._meta.get_field(field)
reverse_field_name = get_m2m_reverse_field_name(original_field_meta)
# Sort the M2M rows by the related object, to ensure a consistent order
old_m2m_qs = getattr(old_history, field).order_by(reverse_field_name)
new_m2m_qs = getattr(self, field).order_by(reverse_field_name)
m2m_through_model_opts = new_m2m_qs.model._meta

# Create a list of field names to compare against.
# The list is generated without the PK of the intermediate (through)
Expand All @@ -1060,8 +1063,8 @@ def _get_m2m_field_changes_for_diff(
for f in m2m_through_model_opts.fields
if f.editable and f.name not in ["id", "m2m_history_id", "history"]
]
old_rows = list(old_m2m_manager.values(*through_model_fields))
new_rows = list(new_m2m_manager.values(*through_model_fields))
old_rows = list(old_m2m_qs.values(*through_model_fields))
new_rows = list(new_m2m_qs.values(*through_model_fields))

if old_rows != new_rows:
if foreign_keys_are_objs:
Expand All @@ -1073,7 +1076,7 @@ def _get_m2m_field_changes_for_diff(

# Set the through fields to their related model objects instead of
# the raw PKs from `values()`
def rows_with_foreign_key_objs(m2m_manager):
def rows_with_foreign_key_objs(m2m_qs):
def get_value(obj, through_field):
try:
value = getattr(obj, through_field)
Expand All @@ -1094,11 +1097,11 @@ def get_value(obj, through_field):
through_field: get_value(through_obj, through_field)
for through_field in through_model_fields
}
for through_obj in m2m_manager.select_related(*fk_fields)
for through_obj in m2m_qs.select_related(*fk_fields)
]

old_rows = rows_with_foreign_key_objs(old_m2m_manager)
new_rows = rows_with_foreign_key_objs(new_m2m_manager)
old_rows = rows_with_foreign_key_objs(old_m2m_qs)
new_rows = rows_with_foreign_key_objs(new_m2m_qs)

change = ModelChange(field, old_rows, new_rows)
changes.append(change)
Expand Down

0 comments on commit 4fe8463

Please sign in to comment.