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

fix(diff_against): support diffing against deleted records #1312

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

euriostigue
Copy link

@euriostigue euriostigue commented Feb 20, 2024

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

  • 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:

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

@euriostigue euriostigue force-pushed the 1307-diff-against-deleted-record branch from af19ddf to f98fec8 Compare February 20, 2024 21:14
@euriostigue euriostigue force-pushed the 1307-diff-against-deleted-record branch from f98fec8 to c39ef2a Compare February 29, 2024 22:09
@euriostigue euriostigue reopened this Feb 29, 2024
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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           

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

@euriostigue
Copy link
Author

@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?

@ddabble
Copy link
Member

ddabble commented May 3, 2024

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 :))

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.

diff_against does not detect changes on deletion history type
2 participants