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

[groups] Added default "operator" and "user manager" groups. #40

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

R9295
Copy link
Member

@R9295 R9295 commented Oct 22, 2018

Implements and closes #31

@R9295 R9295 force-pushed the master branch 3 times, most recently from b227ae3 to 53f822b Compare October 23, 2018 17:28
@coveralls
Copy link

coveralls commented Oct 23, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 2e0893c on R9295:master into 767d673 on openwisp:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Tested and works! A minor correction is needed for older django versions, see my comments below.

I created the issues in the other modules to add the permissions to the operator group, we could have some of those issues put as GCI tasks! :-)

operator = group.objects.filter(name='operator')
if operator.count() == 0:
operator = group.objects.create(name='operator')
operator.permissions.set([])
Copy link
Member

Choose a reason for hiding this comment

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

this line is not needed now, right?

user_manager.permissions.set([
Permission.objects.get(codename='add_user').pk,
Permission.objects.get(codename='change_user').pk,
Permission.objects.get(codename='view_user').pk,
Copy link
Member

Choose a reason for hiding this comment

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

this permission is available only from django 2.1 onwards (finally!). In order to support older versions of django the simples thing you can do in this case is to build the list of permissions (except the view one) before the set call, then append the view_user permission within a try, except DoesNotExist: block.

@nemesifier
Copy link
Member

PS: also rebase on the current master when you can

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@R9295 while testing @atb00ker's work on #38 I understood I forgot an important permission for user-managers: they should also be able to manage/view/delete/change OrganizationUser objects. This will work well once #38 is merged. Can you add those permissions as well please?

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great work @R9295! 👍

@nemesifier nemesifier merged commit 797cae3 into openwisp:master Oct 31, 2018
pandafy pushed a commit that referenced this pull request Jan 12, 2021
Removed the logics which were duplicated
in the others modules and redefined it
here to enable reusability and ease debugging.

Fixes #40
pandafy pushed a commit that referenced this pull request Jan 12, 2021
[admin] Removed logic duplicates #40
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.

[groups] Add default "operator" and "admin" group
3 participants