-
-
Notifications
You must be signed in to change notification settings - Fork 483
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
Expose change reason in admin form #1232
base: master
Are you sure you want to change the base?
Expose change reason in admin form #1232
Conversation
12ca6c9
to
989d52e
Compare
Codecov Report
@@ Coverage Diff @@
## master #1232 +/- ##
==========================================
+ Coverage 96.87% 96.88% +0.01%
==========================================
Files 23 23
Lines 1278 1286 +8
Branches 211 212 +1
==========================================
+ Hits 1238 1246 +8
Misses 21 21
Partials 19 19
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea, but I think some changes are needed (in addition to the minor ones in the comments below this one):
- If a history-tracked model happens to already have a
change_reason
field, this code will cause problems.. I guess this can be solved by either:- dynamically detecting this and making sure that this code's
change_reason
field has a non-colliding name - renaming this code's
change_reason
to e.g.history_change_reason
(since it's already listed as a reserved field name)
- dynamically detecting this and making sure that this code's
- A subclass of
SimpleHistoryAdmin
that overrides theform
field will have aFieldError
raised afterget_fieldsets()
adds thechange_reason
field, which means that multiple existing codebases will likely break. This should be solved by either detecting it in the code, or by documenting it - see the point below. - All of this should be documented so that users know that the
change_reason
form field exists and is added by default. Instructions on how to disable/override the behavior should also be documented - and potentially implemented. - It would be nice to have a test checking that the
change_reason
field is correctly set when reverting an object to a previous history record (since thechange_reason
field appears in the revert form as well).
Some tests for the problem scenarios mentioned in the points above would also be nice 🙂
Lastly, thank you for having taken the initiative to implement this! 😊
Will add tests but also this functions as expected, reverting the object with a change history reason as the supplied reason. The field does not get pre-filled |
989d52e
to
1fb1401
Compare
1fb1401
to
ba35004
Compare
All that's left to be done is documentation! |
@ddabble ready for re-review, I'm a bit confused as to what "overriding the behaviour" would look like wrt documenting it, but other than that |
Any news on this? |
Any news? This change is a really nice feature that I feel that would add to the codebase. It feels like it is an intended behaviour that was never quite finished. |
@tim-schilling Yes I would be glad to take it over, I have some time to bring the project up to the latest python + django versions. If @jkaeske want's to help out also that would be a plus! |
Description
Based off of #853 (comment), add a new collapsed field on admin forms to provide the
_change_reason
text on a change.Related Issue
Fixes #853
Motivation and Context
Lets change reason be set in admin form
How Has This Been Tested?
Automated tests included in PR
Screenshots (if appropriate):
Types of changes
Checklist:
pre-commit run
command to format and lint.AUTHORS.rst
CHANGES.rst