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

Management command to refresh permission objects. #1760

Merged
merged 2 commits into from
Oct 30, 2020

Conversation

schwarzkrieger
Copy link
Contributor

Closes #1137

@atodorov atodorov requested review from asankov and RMadjev June 29, 2020 14:56
Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Needs changes, squash all commits together.

@atodorov
Copy link
Member

atodorov commented Aug 3, 2020

Code review adjustments before squashing

Next time squash the commits as we're cleaning them up and working on the review.

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Drop the last commit because it makes things even worse and work to resolve items one by one. Ask if unclear.

@atodorov
Copy link
Member

atodorov commented Aug 4, 2020

FTR the new Django 3.1 comes with remove_stale_contenttypes --include-stale-apps which looks like will resolve the removing of old objects.
https://docs.djangoproject.com/en/3.1/releases/3.1/#django-contrib-contenttypes

@schwarzkrieger schwarzkrieger force-pushed the issue-1137 branch 2 times, most recently from 0fe5722 to 09730e4 Compare August 10, 2020 19:17
Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

There is update_permissions command from django-extensions which we already use to generate the model diagrams here: https://kiwitcms.readthedocs.io/en/latest/db.html

So before you continue spending more time on this issue, we need to make django-extensions into a production requirement and not only something used during development. Make this change in a separate PR.

The entry point will be update_permissions -v 2 and add that to the top of your handle() method via call_command.

django-extensions doesn't remove stale permissions so once the code here is approved it is better to contribute it upstream.

The need to update the Admin & Tester groups will still remain here after all of the changes.

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Rebase onto latest master to resolve the conflicts in docs.

@schwarzkrieger schwarzkrieger force-pushed the issue-1137 branch 2 times, most recently from 1c52660 to 7c311c4 Compare September 6, 2020 12:54
Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Also rebase to latest master which now uses Django 3.1.1

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Looks better than before, minor changes requested.

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Looks better now, a few nitpicks before I go into reviewing the rest.

Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Overall looks good, aside from a few nitpicks the next thing this needs is tests.

The best way to tackle this is to first add tests for assign_default_group_permissions() in a separate PR from master where you don't have the changes; then add tests if necessary for the changes related to the new default params and afterwards work on tests for the management command itself.

@atodorov
Copy link
Member

Rebase to latest master so we can see some test results

Copy link
Member

@atodorov atodorov 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, minor changes requested.

@atodorov atodorov merged commit 24b124a into kiwitcms:master Oct 30, 2020
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.

Management command/function to refresh permission objects
2 participants