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

Admin revert enabled/disabled, view only permissions #631

Closed
wants to merge 13 commits into from
Closed

Admin revert enabled/disabled, view only permissions #631

wants to merge 13 commits into from

Conversation

erikvw
Copy link
Contributor

@erikvw erikvw commented Feb 16, 2020

In some scenarios, it is not ideal to allow users to revert a model instance to a previous version.
Django 2.1 introduced view permissions which, if integrated into DSH, would give an administrator the ability to allow a user to add/change a model but not revert to a previous instance through the model's history.

Related to:

related to #529 and #632

Description

The SimpleHistoryAdmin reviews the historical model instance permissions instead of just the model instance by overriding the has_xxxx_permission methods in ModelAdmin.

  • enable this feature through settings (SIMPLE_HISTORY_PERMISSIONS_ENABLED);

  • honor superuser privileges;

  • user/group permissions granted for the historical model cannot exceed those of the model. That is, granting view and change permissions for the historical model but only view permission for the model means the user has view permission only for the historical model;

  • text on the changelist and form becomes context sensitive (change permission vs view permission);

  • setting SIMPLE_HISTORY_REVERT_DISABLED=True limits all users (other than superuser) to view access. (this works for all Django versions)

  • the [Revert] button is changed to a [Close] button if the user has:
    -- if Django >= 2.1, feature is disabled, and has view on model;
    -- if Django >= 2.1, feature is enable and has view model permission and view historical model permission;
    -- if settings. SIMPLE_HISTORY_REVERT_DISABLED = True (regardless of Django version).

How Has This Been Tested?

See tests in test_admin_with_permissions.py

A few tests in test_admin.py required very minor modifications.

Screenshots (if appropriate):

For a user with "view" only permission on a historical object, the changelist looks like this:
20_permissions

and the form looks like this:
30_permissions

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 make format command to format my code
  • 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.

@erikvw
Copy link
Contributor Author

erikvw commented Feb 16, 2020

was #529, still in progress. Still to review all comments on #529.

@erikvw erikvw changed the title Admin revert permissions2 Admin revert enabled/disabled, view only permissions Feb 16, 2020
@codecov-io
Copy link

codecov-io commented Feb 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@8bcf4c4). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #631   +/-   ##
=========================================
  Coverage          ?   97.99%           
=========================================
  Files             ?       17           
  Lines             ?      946           
  Branches          ?      140           
=========================================
  Hits              ?      927           
  Misses            ?        7           
  Partials          ?       12
Impacted Files Coverage Δ
simple_history/admin.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bcf4c4...35be075. Read the comment docs.

@dwasyl
Copy link

dwasyl commented Sep 24, 2020

@erikvw this is just an updated version of your last PR right? Any chance that it'd be ready to merge soon? I'd love to see this feature in a current build.

@JohnieBraaf
Copy link

I also am in need of this PR, as it would resolve this issue: #704

@Leo5677
Copy link

Leo5677 commented Aug 7, 2021

I also am in need of this PR, as it would resolve this issue: #704

Também preciso deste PR, pois isso resolveria o problema: # 704

Yes... I also need!!!

@jeking3
Copy link
Contributor

jeking3 commented Sep 17, 2021

This needs to be rebased.

@jeking3
Copy link
Contributor

jeking3 commented Oct 25, 2021

@erikvw This needs to be rebased.

rolandcrosby-check added a commit to rolandcrosby-check/django-simple-history that referenced this pull request Nov 17, 2021
No functional changes, just a mechanical rebase of the changes in
jazzband#631 onto the latest master branch.

Co-authored-by: erikvw <[email protected]>
@rolandcrosby-check
Copy link

@jeking3 I rebased the changes onto latest master and put the resulting commit up as a separate PR here: #915. I haven't run the full test suite or made any functional changes, so I'm not sure if anything has broken in the year and a half since this PR was originally opened.

@erikvw if you like, you can pull the rebased commit into your branch to continue addressing review feedback; I'm just hoping to help move this along however I can, as I'd really like to be able to use this feature eventually.

@jeking3
Copy link
Contributor

jeking3 commented Feb 18, 2022

@rolandcrosby-check re #915 that needs to be rebased on master so CI tests can run.

@rolandcrosby-check
Copy link

@jeking3 I'm not sure if I'll get a chance to do that, I just did the original rebase with the goal of helping @erikvw move this along. If anyone else wants to pick this up and re-rebase #915 I'd welcome that.

@erikvw
Copy link
Contributor Author

erikvw commented Aug 3, 2022

see #1017 , this might be a better option.

This pull request was closed.
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.

7 participants