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

Expose change reason in admin form #1232

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

Conversation

mjsir911
Copy link
Contributor

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

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

@mjsir911 mjsir911 force-pushed the msirabella/admin_change_reason branch from 12ca6c9 to 989d52e Compare August 19, 2023 01:39
@mjsir911 mjsir911 changed the title Expose change reason to admin form Expose change reason in admin form Aug 19, 2023
@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #1232 (ba35004) into master (29108a5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            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              
Files Coverage Δ
simple_history/admin.py 98.70% <100.00%> (+0.07%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

theryanwalker
theryanwalker previously approved these changes Aug 27, 2023
Copy link
Member

@theryanwalker theryanwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Member

@ddabble ddabble left a 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):

  1. 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:
    1. dynamically detecting this and making sure that this code's change_reason field has a non-colliding name
    2. renaming this code's change_reason to e.g. history_change_reason (since it's already listed as a reserved field name)
  2. A subclass of SimpleHistoryAdmin that overrides the form field will have a FieldError raised after get_fieldsets() adds the change_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.
  3. 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.
  4. 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 the change_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! 😊

simple_history/admin.py Outdated Show resolved Hide resolved
simple_history/admin.py Outdated Show resolved Hide resolved
simple_history/tests/tests/test_admin.py Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
simple_history/admin.py Outdated Show resolved Hide resolved
simple_history/tests/tests/test_admin.py Show resolved Hide resolved
@mjsir911
Copy link
Contributor Author

mjsir911 commented Oct 4, 2023

4. 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 the change_reason field appears in the revert form as well).

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

@mjsir911 mjsir911 force-pushed the msirabella/admin_change_reason branch from 1fb1401 to ba35004 Compare October 6, 2023 02:28
@mjsir911
Copy link
Contributor Author

mjsir911 commented Oct 6, 2023

All that's left to be done is documentation!

@mjsir911
Copy link
Contributor Author

@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

@jkaeske
Copy link

jkaeske commented Feb 29, 2024

Any news on this?

@al-beton
Copy link

al-beton commented May 4, 2024

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
Copy link
Contributor

@jkaeske or @al-beton would you like to take this over?

@al-beton
Copy link

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

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.

Can't add change_reason field to the admin form
6 participants