-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Update group invitation check to always allow owner #2772
Conversation
… admin role to invite to groups that they are a superset of
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.
Can add to any new group. Looks good.
} | ||
|
||
// Users with `owner` and/or `admin` roles can invite to any group | ||
// Also a user with `admin` can invite to only groups they have permissions that are a superset of |
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.
These two comment lines seem to contradict each other.
Can a user with admin
permissions invite to any group, or only groups they have superset of permissions for?
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.
@impactmass want to make sure you see this comment as I think these comments should be clarified, but going to merge anyway.
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.
I'll note to update the comment in next PR I open, but I meant the latter: a user with admin can invite to only groups they have superset of permissions for.
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.
That's what I figured. Next PR is good
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.
1 confusing comment for me, but looks good and is a good fix.
At some point we should figure out how to split some of the client Reaction
methods out of main.js
because that file is getting a little long.
Update after #2662 Scoped invitation.
What Happened Earlier:
[a,b,c,d]
, she can only invite a user to groups with those permissions or a subset of them. We set upReaction.canInviteToGroup
method to do this check.addRolesToGroups
. This will leave the owner inadvertently cut out bycanInviteToGroup
Changes in this PR:
owner
rolecanInviteToGroup
to client Reaction, so it's possible to do Reaction.canInviteToGroup client side. This is useful when deciding dropdown options.How To test
** Adding a user to an existing group
** Adding a user to a new group created via the Accounts UI
** Adding a user to a new group created by Groups.insert(groupData) like in the case of a custom package
Confirm that an owner can invite, in all those cases.
Confirm that previous functionality stays intact by:
Invite Form
that the user doesn't have dropdown for higher groups e.g "Shop Manager"You can also test all these with a new shop other than the primary shop