-
-
Notifications
You must be signed in to change notification settings - Fork 299
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
Conversation
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.
Needs changes, squash all commits together.
Next time squash the commits as we're cleaning them up and working on the review. |
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.
Drop the last commit because it makes things even worse and work to resolve items one by one. Ask if unclear.
FTR the new Django 3.1 comes with |
0fe5722
to
09730e4
Compare
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.
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.
09730e4
to
b788a46
Compare
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.
Rebase onto latest master to resolve the conflicts in docs.
1c52660
to
7c311c4
Compare
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.
Also rebase to latest master which now uses Django 3.1.1
7c311c4
to
aaa3040
Compare
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 better than before, minor changes requested.
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 better now, a few nitpicks before I go into reviewing the rest.
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.
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.
9a8131b
to
2d683d5
Compare
Rebase to latest master so we can see some test results |
2d683d5
to
ca39f40
Compare
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, minor changes requested.
Closes #1137