Add history diff column to admin change history table #1128
Conversation
Codecov ReportAttention: Patch coverage is
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 ☔ View full report in Codecov by Sentry. |
|
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: which is probably some relic from development. And obviously the failed test would need to be corrected. |
simple_history/templates/simple_history/_object_history_list.html
Outdated
Show resolved
Hide resolved
simple_history/templates/simple_history/_object_history_list.html
Outdated
Show resolved
Hide resolved
ddabble
changed the title
|
@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
force-pushed
the
feat/adding-history-diff
branch
from
2e39d74 to
afb3827
Compare
There was a problem hiding this comment.
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.
I don't feel super strong about it, but it seems like a reasonable customization we could implement. And it would avoid having an explicit call to set_history_delta_changes.
Sure, so make |
|
Oh, I thought |
|
@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 |
See discussion: jazzband#1128 (comment) Also prefixed the method with `_`, to mark it as not being part of the public API.
|
@tim-schilling I mainly added some (presumably final) polish commits - including fd72e7a and 38ec0c2, 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.) |
|
@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. |
|
@ddabble when you think it's ready, I'll integrate it with our application to see how things perform. |
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)
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) 🙂
I'm assuming you mean as a final system test before merging..? |
Yeah, that's what I was thinking. Let me do that now. |
|
Dang this is cool! So amazing 🚀 |
|
Also, I didn't run into any issues. All good. |
This is a more accurate name for the contents of the queryset. Also did a small cleanup of `_object_history_list.html`, incl. removing the `scope` attribute of a `<td>` tag, as it's been deprecated for those tags (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/td#scope).
Including renaming it to remove the `_` prefix (indicating that it's internal / not meant for usage by library users), and adding an `object_history_list_template` field to the admin class. This rename also changed the order of messages in the translation files. Also deprecated the template tag `simple_history_admin_list.display_list`, as it's now entirely unused within the project.
These should help with often having to check which values are returned by `m2m_field_name()` and `m2m_reverse_field_name()` - since they're both undocumented. Also, if Django changes the mentioned internal methods, it'll be easier to only update these two functions instead of all the places they are / will be used.
This gives them useful methods like `__eq__()`, `__repr__()` and `__hash__()` for free :) Note that they have `frozen=True` (mainly to allow safe `__hash__()` implementations), as I think the vast majority of users are in practice treating these classes as read-only; modifying them serve no purpose within this library, and should therefore be an incredibly niche use case (except when comparing against expected mock objects while testing, but then `dataclasses.replace()` can be used).
* Refactored building the change lists
* Extracted the code for M2M fields and other fields into two helper
methods
* Derive `changed_fields` from `changes`
* Refactored getting M2M fields to avoid querying the DB (1 or 2 times)
for a `reference_history_m2m_item`
This makes it easier to get the related objects from the diff objects, and facilitates prefetching the related objects when listing the diffs - which is needed for the upcoming changes to the admin history page adding a "Changes" column. The returned `ModelDelta` fields are now alphabetically ordered, to prevent flakiness of one of the added tests. The `old` and `new` M2M lists of `ModelChange` are now also sorted, which will prevent inconsistent ordering of the `Place` objects in `AdminSiteTest.test_history_list_contains_diff_changes_for_m2m_fields()` (will be added as part of the previously mentioned upcoming admin history changes). Lastly, cleaned up some `utils` imports in `models.py`.
...when called with `foreign_keys_are_objs=True`. This special dataclass (`DeletedObject`) will signal to the user that the related object was deleted, and provides useful info - i.e. the object's PK and model - and a verbose `__str__()` implementation. (An alternative solution of returning `None` for deleted objects, would introduce ambiguity if the `ForeignKey` field is nullable.)
...to `HistoricalQuerySet`. This convenience method will make it easier to avoid database queries - e.g. when diffing and listing the results, which will be done as part of the upcoming changes to the admin history page adding a "Changes" column. No documentation was added, after the following discussion: jazzband#1128 (comment) Also prefixed the method with `_`, to mark it as not being part of the public API.
...i.e. the code that's used to display the difference between the actual and expected values for most of the `assert[...]` methods of `TestCase` when the assertion fails. This will be used to better display the diffs on the admin history page in an upcoming commit - with some modifications. The code was copied from https://github.com/python/cpython/blob/v3.12.0/Lib/unittest/util.py#L8-L52, with the following changes: * Placed the code inside a class, to group the functions and their "setting" variables from other code - which also lets them easily be overridden by users * Removed the `_` prefix from the functions and variables * Added type hints * Formatted with Black Lastly, the code was copied instead of simply imported from `unittest`, because the functions are undocumented and underscore-prefixed, which means that they're prone to being changed (drastically) or even removed, and so I think maintaining it will be easier and more stable by copy-pasting it - which additionally facilitates modification.
This lists the changes between each historical record of the object. Effort has been made to make it easy for library users to override most parts of this, and to let them customize only the details they want to without having to copy or replace our implementation. An overview of how to override it has been added to `admin.rst`. Most of the code was added to `template_utils.py`, to avoid cluttering `SimpleHistoryAdmin`. Also updated the Norwegian Bokmål translations, and updated all the docs screenshots, to make them have Django 5.0's UI and to showcase the new column. Co-authored-by: Gur Raunaq Singh <[email protected]> Co-authored-by: Tim Schilling <[email protected]>
This removes the need for the `nosec` comments in files inside those directories. Also, the existing `-x` arg doesn't seem to work anymore, so this fixes that.
ddabble
force-pushed
the
feat/adding-history-diff
branch
from
5a772dd to
733f4e0
Compare
|
Finished cleaning up the commits, so the PR is ready for merging :) |
|
Not to spam, but I think I love you 😍 this feature is awesome |
Description
Main changes:
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 (c9963aa)simple_history/_object_history_list.htmltosimple_history/object_history_list.html, and added the fieldSimpleHistoryAdmin.object_history_list_templatefor overriding it (16b7de7)simple_history_admin_list.display_list(); it will be removed in version 3.8 (16b7de7)SimpleHistoryAdmin.get_history_queryset()for overriding whichQuerySetis used to list the historical records (18f00bd)SimpleHistoryAdmin.get_history_list_display()which returnshistory_list_displayby default, and made the latter into an actual field (dc3a34e)ModelDeltaandModelChange(insimple_history.models) are now immutable dataclasses; their signatures remain unchanged (28afb4d)ModelDelta'schangesandchanged_fieldsare now sorted alphabetically by field name. Also, ifModelChangeis for an M2M field, itsoldandnewlists are sorted by the related object. This should help prevent flaky tests. (a842b98)diff_against()has a new keyword argument,foreign_keys_are_objs; see usage in the docs under "History Diffing" (a842b98)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
Checklist:
pre-commit runcommand to format and lint.AUTHORS.rstCHANGES.rst