Skip to content

Add support for m2m changes in AbstractLogEntry.changes_str#798

Merged
hramezani merged 8 commits intojazzband:masterfrom
marco-thirona:changes-str-add-support-for-m2m-changes
Jan 29, 2026
Merged

Add support for m2m changes in AbstractLogEntry.changes_str#798
hramezani merged 8 commits intojazzband:masterfrom
marco-thirona:changes-str-add-support-for-m2m-changes

Conversation

@marco-thirona
Copy link
Contributor

Fixes #585

@marco-thirona marco-thirona marked this pull request as ready for review January 12, 2026 10:09
@codecov
Copy link

codecov bot commented Jan 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.33%. Comparing base (ede4d10) to head (42b4ea5).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #798      +/-   ##
==========================================
+ Coverage   95.92%   96.33%   +0.41%     
==========================================
  Files          35       35              
  Lines        1252     1256       +4     
==========================================
+ Hits         1201     1210       +9     
+ Misses         51       46       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hramezani hramezani requested a review from 2ykwang January 12, 2026 10:32
@hramezani
Copy link
Member

Thanks @marco-thirona for the PR.

Could you please fix the failing test and add a change log entry?

@2ykwang Could you please review the PR?

@2ykwang
Copy link
Member

2ykwang commented Jan 14, 2026

Thanks for the PR! I'll review this over the weekend. @marco-thirona

before that, could you please:

  1. Check the failing CI test (PostgreSQL, Python 3.13)
  2. Add a CHANGELOG.md

@marco-thirona marco-thirona marked this pull request as draft January 15, 2026 09:04
@marco-thirona
Copy link
Contributor Author

before that, could you please:

  1. Check the failing CI test (PostgreSQL, Python 3.13)
  2. Add a CHANGELOG.md

@2ykwang The test that was failing in the previous build is flaky, I'm afraid:

  ======================================================================
  ERROR: test_log_entry_deleted_fk_changes_to_string_objects_in_display_dict (auditlog_tests.tests.TestRelatedDiffs.test_log_entry_deleted_fk_changes_to_string_objects_in_display_dict)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/home/runner/work/django-auditlog/django-auditlog/auditlog_tests/tests.py", line 2393, in test_log_entry_deleted_fk_changes_to_string_objects_in_display_dict
      display_dict["related"][0], f"Deleted 'SimpleModel' ({one_simple_id})"
      ~~~~~~~~~~~~^^^^^^^^^^^
  KeyError: 'related'
  
  ----------------------------------------------------------------------

I just added a changelog note, and now the builds are fine again.

@marco-thirona marco-thirona marked this pull request as ready for review January 15, 2026 09:17
Copy link
Member

@2ykwang 2ykwang left a comment

Choose a reason for hiding this comment

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

Thanks @marco-thirona
sorry for the late review. just a quick comment.

Copy link
Member

Choose a reason for hiding this comment

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

@marco-thirona
small suggestion: Sequence includes str as well (though unlikely to cause issues in practice).
How about isinstance(value, (list, tuple)) to make the intent clearer?

also, adding a length check like len(value) == 2 might be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestions @2ykwang, I'll make these changes

@marco-thirona marco-thirona requested a review from 2ykwang January 22, 2026 10:29
Copy link
Member

@2ykwang 2ykwang left a comment

Choose a reason for hiding this comment

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

Sorry for the late review! Left a small suggestion.
let me know what you think 😃

Copy link
Member

Choose a reason for hiding this comment

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

for field, value in sorted(self.changes_dict.items()):
    if isinstance(value, (list, tuple)) and len(value) == 2:
        # handle regular field change
    elif isinstance(value, dict) and value.get("type") == "m2m":
        # handle m2m change

What do you think about restructuring it this way? Given the current design, mixed types won't occur,
but this would iterate only once and improve readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True that log entries do not currently mix regular and m2m changes, though this approach looks cleaner indeed, so I'll restructure as you proposed. Thanks for the suggestion

@2ykwang
Copy link
Member

2ykwang commented Jan 27, 2026

and thank you for working on this! @marco-thirona

@marco-thirona marco-thirona requested a review from 2ykwang January 28, 2026 13:29
Copy link
Member

@2ykwang 2ykwang left a comment

Choose a reason for hiding this comment

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

thanks! @marco-thirona

@2ykwang
Copy link
Member

2ykwang commented Jan 29, 2026

@hramezani could you double-check this when you have a moment?

@hramezani hramezani merged commit 6d170da into jazzband:master Jan 29, 2026
15 checks passed
@hramezani
Copy link
Member

Thanks both

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LogEntry.changes_str fails with KeyError when using m2m_fields

3 participants

Comments