django-simple-history icon indicating copy to clipboard operation
django-simple-history copied to clipboard

fix(diff_against): support diffing against deleted records

Open euriostigue opened this issue 1 year ago • 3 comments

This modifies diff_against to compare against records when history_type is -.

Description

When determining the record value for comparison, check if the history_type corresponds to a deletion. Use the value None if the record corresponds to a deletion record.

Related Issue

Closes #1307

Motivation and Context

In our application, we use diff_against to identify changes between records. A deletion was not detected as a change by diff_against.

How Has This Been Tested?

An added unit test checks comparisons in both directions:

  • comparing a deleted record to a created record
  • comparing a creation record to a deleted record

Screenshots (if appropriate):

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] I have run the pre-commit run command to format and lint.
  • [ ] My change requires a change to the documentation.
  • [x] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes.
  • [x] I have added my name and/or github handle to AUTHORS.rst
  • [x] I have added my change to CHANGES.rst
  • [ ] All new and existing tests passed.

euriostigue avatar Feb 20 '24 21:02 euriostigue

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.87%. Comparing base (c39ef2a) to head (9e3be9d). Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1312   +/-   ##
=======================================
  Coverage   96.87%   96.87%           
=======================================
  Files          23       23           
  Lines        1282     1282           
  Branches      211      211           
=======================================
  Hits         1242     1242           
  Misses         21       21           
  Partials       19       19           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 15 '24 20:03 codecov[bot]

@ddabble I noticed that you ran CI on this PR. Thank you! Do you mind reviewing this PR?

Only the CI jobs running against Django's main branch are failing, due to seemingly unrelated failing uniqueness constraints. What else do I need to do to get this PR in mergeable shape?

euriostigue avatar Apr 30 '24 16:04 euriostigue

Sure, thank you for the PR! 😊

I'm not sure this is the best way of solving the issue, as how would a user know that the value None means that the object has been deleted, and not that a nullable field has simply been changed to None?

I agree that changes and changed_fields being empty when diffing a deleted record against an earlier one, is slightly unintuitive, but having them contain None for all fields, also introduces ambiguity between diffs of deleted objects and diffs of objects whose fields have all been set to None. One could argue that the ambiguity is removed by simply checking the nullability of the objects' fields, but it's just as easy for the user to check the history_type of the historical record...

Any thoughts on this? 🙂 (In any case, it's clear that the docs should be updated to explain what to expect when diffing a deleted object, at the very least :))

ddabble avatar May 03 '24 18:05 ddabble