Add history diff column to admin change history table

[raunaq-sailo]

Description

Main changes:

• Added a "Changes" column to SimpleHistoryAdmin's object history table, listing the changes between each historical record of the object; see the docs under "Customizing the History Admin Templates" for overriding its template context (c9963aaeaddd9d66619a9996aa6bb36a70f0d779)
  • Related: da87859a325a01b256b9890674d0503202e54863, b5e627c70da94a8f960a27a35d6b57c9854e5124, 04eefadf187670b018cbfd3306d5001447eb0c52, 733f4e0a6046917848ef46277079cefcf6c87c98
• Renamed the (previously internal) admin template simple_history/_object_history_list.html to simple_history/object_history_list.html, and added the field SimpleHistoryAdmin.object_history_list_template for overriding it (16b7de7482d0898c9f83c9042c01442fe35ba66e)
• Deprecated the undocumented template tag simple_history_admin_list.display_list(); it will be removed in version 3.8 (16b7de7482d0898c9f83c9042c01442fe35ba66e)
• Added SimpleHistoryAdmin.get_history_queryset() for overriding which QuerySet is used to list the historical records (18f00bddbfdd0b0d65757982197d670315174ad1)
• Added SimpleHistoryAdmin.get_history_list_display() which returns history_list_display by default, and made the latter into an actual field (dc3a34eea0a65bbcb3b64865ab500c5896c34e65)
• ModelDelta and ModelChange (in simple_history.models) are now immutable dataclasses; their signatures remain unchanged (28afb4dc8486a7c4563070e25139a107036569b3)
• ModelDelta's changes and changed_fields are now sorted alphabetically by 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. (a842b98a2e36c6ab217831769a1817a28cd59f60)
• diff_against() has a new keyword argument, foreign_keys_are_objs; see usage in the docs under "History Diffing" (a842b98a2e36c6ab217831769a1817a28cd59f60)
  • Related: a5439d2d1e274b3b619abacb2700ef6a95f921d0, 0589ac2e637760e5365a023798a7aa0e034aa68f

Related Issue

Closes #886, closes #1030, closes #1308.

Motivation and Context

See the linked issues.

How Has This Been Tested?

Through the tests added in the changed test files.

Screenshots (if appropriate):

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] 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.
• [x] 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.
• [ ] I have added my name and/or github handle to AUTHORS.rst
• [x] I have added my change to CHANGES.rst
• [x] All new and existing tests passed.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 97.59615% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 96.83%. Comparing base (4d39103) to head (733f4e0).

Files	Patch %	Lines
simple_history/models.py	94.87%	3 Missing and 1 partial :warning:
simple_history/admin.py	97.05%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1128      +/-   ##
==========================================
- Coverage   96.88%   96.83%   -0.06%     
==========================================
  Files          23       24       +1     
  Lines        1285     1452     +167     
  Branches      212      237      +25     
==========================================
+ Hits         1245     1406     +161     
- Misses         21       24       +3     
- Partials       19       22       +3     

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

[PetrDlouhy]

I tested this PR and it seems to me, that it works correctly and is very helpful. Here is a preview of what it does:

I checked, that it does show the diff values for my models and also that it doesn't make unnecessary DB queries (the number of queries is constant and not dependent on number of history records).

When I look at the code, i think there is added unnecessary comment line:

<!-- {{ action.history_diff }}-->

which is probably some relic from development.

And obviously the failed test would need to be corrected. @raunaq-sailo Could you please remove the comment and fix the failing tests?

[ddabble]

@raunaq-sailo Just as a heads-up, I'm considering implementing the changes I suggested myself, if no activity has happened within a couple weeks :)

[ddabble]

I think this is fine, but I would like to raise a slight change in implementation. Rather than always showing the change at the end, would it make more sense to include this as a default option in history_list_display? That way, it would allow the user to decide if they want to even include the changes or move it to a different column?

One of the problems with doing that is that we'd have to define some name for this column that would shadow potential fields on the actual historical model. Maybe something like Historical changes? I'm thinking Changes by itself might be too generic.

Sure, so make history_list_display into a field (like Django's list_display) with a default value of something like ["historical_changes"], where historical_changes is the name of a method that returns the changes for each history record/row? If so, we could also do the same for the other default columns, to allow for a similar customization as list_display does (so that the current default column layout would be roughly translated to history_list_display = ["history_object", "history_date", "history_type", "history_user", "history_change_reason", "historical_changes"]) - though that would have to be in another PR, of course 🙂 Otherwise, I'm not entirely sure how a user would "move [Changes] to a different column" 🤔

[tim-schilling]

Oh, I thought history_list_display was already a thing. I don't want to have perfect get in the way of good though, so let's make this improvement.

[ddabble]

@tim-schilling I realized some additional changes needed to be made to properly display foreign keys and M2M objects, and so most of the recent commits were made to facilitate that - incl. making some optimizations and usage improvements along the way. It's absolutely possible, however, to display them without some of those changes, so let me know if I should split it into multiple PRs 🙂 (I'll squash some of the commits before merging.)

I also realized that making history_list_display contain the current default column layout - like we discussed - would require several extra changes, which I think is better to include in a separate PR that I can open after this one. Is that okay with you?

[ddabble]

@tim-schilling I mainly added some (presumably final) polish commits - including fd72e7a5f9943a3d93be4066a97ed6d946b0c4d8 and 38ec0c2049bc2388408d0701a69cefed1bbad45d, which improve how really long strings are displayed - and added tests for how safe strings are handled. Again, please let me know if I should split it into multiple PRs 🙂

(The failing tests are unrelated to this PR; I'm looking into them.)

[tim-schilling]

@ddabble I think it looks good. There was a lot in those changes and admittedly I didn't do a great job of reviewing. It looks like the code is well written, there are tests and they are passing. Not having code coverage hurts us here because that would confirm that the code is actually running in the tests.

Let's make this the last of the large, multi-commit PRs though. They're really tough to do a good job when I'm not super-duper familiar with the codebase.

[tim-schilling]

@ddabble when you think it's ready, I'll integrate it with our application to see how things perform.

[ddabble]

Not having code coverage hurts us here because that would confirm that the code is actually running in the tests.

True.. From the workflow logs (under "Upload coverage"), it looks like we're being rate-limited by Codecov, since the repo doesn't have any upload token 😕 (see #1305)

Let's make this the last of the large, multi-commit PRs though. They're really tough to do a good job when I'm not super-duper familiar with the codebase.

Alright, that's fair 😅 Thanks a bunch for reviewing regardless! I'll clean up the commits for merging, as mentioned above (followed by re-requesting a review) 🙂

when you think it's ready, I'll integrate it with our application to see how things perform.

I'm assuming you mean as a final system test before merging..?

[tim-schilling]

I'm assuming you mean as a final system test before merging..?

Yeah, that's what I was thinking. Let me do that now.

[tim-schilling]

Dang this is cool! So amazing :rocket:

[tim-schilling]

Also, I didn't run into any issues. All good.

[ddabble]

Finished cleaning up the commits, so the PR is ready for merging :) (A few minor changes done while rebasing)

[hfroot]

Not to spam, but I think I love you 😍 this feature is awesome