Implement Volunteer Group structure #57
Conversation
… under feature flags
There was a problem hiding this comment.
Thanks for working on this. The logic looks pretty reasonable here, but I do have some feedback about readability and documentation. If we really needed to merge this I wouldn't block it, but IMO most of this feedback is better addressed before we start adding more functionality.
Key requests:
- Don't update an old migration. Make a new one.
- [Follow-up] Use a static select in the slack modal.
- Add some high-level comments to
ChannelInterfaceexplaining how Channel implementations relate to theVolunteerGroupChannelandVolunteerGroupmodel classes, as well as behavior & expectations from the methods. - Seriously consider alternative names for VolunteerGroup:
Naming feedback
VolunteerGroup sounds oddly specific for how general of a system this is. It's really a reactor-based implementation of a role-based access control/user group system. It is going to be used for things that don't have much to do with "volunteering", eg "table saw user".
Consider a more generic name such as MemberGroup or PrivilegeGroup. TBH, the "group" part is really already handled by way of UserMemberships.
In reality, VolunteerGroup is a class that (barely) extends a subset of the MembershipPlan table (which we don't currently sync to webhooks. I'm not really convinced that it's necessary at all, but if it is it's really something like RoleAccessGroup.
"Channel" is kinda doing double duty here. There's ChannelInterface implementations for privilege providers such as github, slack, and google. Then, there's the instances of privileges, which are stored as VolunteerGroupChannels.
Naming is hard and will never be perfect, but I think I'd probably set it up and document it something like this:
MemberGroup stores information about a group to which members can belong, which by association grants members access to certain asset(s). A MemberGroupPrivilege stores configurations for each permission associated with a MemberGroup. eg: 3D printer trainers get access to the #maintainers-3d-fdm slack channel and the "octoprint users" Azure LDAP group, while laser cutter users get access to the "laser computers" Azure group. For each system to which access can be granted - eg slack, google, github, azure, etc - a class is created that implements the PrivilegeManager interface, which defines the logic for adding and removing members to a channel/group/asset within that service.
| ->blockId(self::PLAN_ID) | ||
| ->label('Plan ID that grants access') | ||
| ->newSelectMenu(self::PLAN_ID) | ||
| ->forExternalOptions() |
There was a problem hiding this comment.
Do we really need this to be an external select?
External selects load options on keypress. There's noticeable latency and unbeknownst to slack or the user, our response will (almost) never change since we're not actually doing any filtering in getOptions. It's silly to be doing these extra network roundtrips for most of our select fields.
There was a problem hiding this comment.
You're probably right. The max number of options for a static select is 100, so we would need to handle switching to an external select dynamically. Not the end of the world, I just didn't want to deal with that. We do also filter all options automatically in the SlackOptionsController so we're not returning the same thing each time, we're just handling the filtering in one place. If you return the same JSON blob to Slack, they don't do any filtering for you. So they would notice if we were returning the same thing over and over.
I can try to implement that behavior with graceful fallback to an external select in another PR, but I don't want to handle it here.
|
|
||
| use App\Models\Customer; | ||
|
|
||
| interface ChannelInterface |
There was a problem hiding this comment.
Interfaces are a great place to add documentation about the purpose of a system and how to use it.
It's not obvious to me what a "Channel" is. It might refer to a slack channel. It might be an abstract concept. I suspect the later, and hopefully I'll know by the time I finish the review, however new readers shouldn't have to guess.
|
|
||
| function remove(Customer $customer, string $channelValue): void; | ||
|
|
||
| static function getTypeKey(): string; |
| use App\Models\Customer; | ||
| use App\Models\VolunteerGroupChannel; | ||
|
|
||
| class GoogleGroup implements ChannelInterface |
There was a problem hiding this comment.
Google groups are less familiar and thereby less intuitive to me compared to something like a slack channel. What kind of things do we do with google groups that we might want to manage via the volunteer groups system?
There was a problem hiding this comment.
Well, we can manage access to the membership list, for example, by making it depend on plan id 6410, for example. And then we wouldn't have to hardcode that. Things like getting access to [email protected] if you're on the board, [email protected] if you're on ops, etc.
|
|
||
| static function removeOnMembershipLost(): bool | ||
| { | ||
| // There's no overall structure to Google Groups that someone can be removed from. We have to handle each group |
There was a problem hiding this comment.
I'm sure this comment is meaningful, but it's lost on me what structure a group might be expected to have and what you might expect to do besides handling each group individually.
Based on your comment on the slack channel implementation, I'm guessing you're saying something like:
"Google doesn't give us the ability to pause/revoke access to all groups, so we have to remove them from each group individually."
|
|
||
| if (in_array($status, ['paused', 'expired', 'cancelled'])) { | ||
| $volunteerGroup->channels()->each(fn($ch) => $ch->remove($customer)); | ||
| } elseif (in_array($status, ['active', 'free_trial', 'pending'])) { |
There was a problem hiding this comment.
Hmm, probably ok for now but it seems a bit sus that we need to globally set whether statuses such as "pending" and "paused" constitute a need to add or remove access.
Specifically, "pending" sounds like a person who is not yet a member of a group, and here we're giving them access. Maybe it makes sense for some use cases, but seems like it may eventually become problematic.
Also, I'm still building out a concept of an "inactive" trainer. I'm not sure whether it would make sense to set that membership to "paused" or to store that somewhere else. My gut was that we wouldn't remove inactive trainers from the slack channels by default, because they might want to start picking up trainings or be available to fill in as a substitute.
My point is merely that it may be hard to build a one-size-fits-all model for these status mappings, and we may have to configure them in the VolunteerGroupChannel.
There was a problem hiding this comment.
Specifically, "pending" sounds like a person who is not yet a member of a group
Nope, it's someone who is pending cancellation. You can see the statuses here, but unfortunately not the shortened versions of them. Still, we will probably want to match the concept to what the WooCommerce membership plugin uses.
My point is merely that it may be hard to build a one-size-fits-all model for these status mappings, and we may have to configure them in the VolunteerGroupChannel.
We might. I'd like to avoid doing that for now. We can get better clarity about the changes we would need as we try to explore that feature. I agree that pausing a membership would be a way to handle that.
|
|
||
| static function getTypeKey(): string; | ||
|
|
||
| static function removeOnMembershipLost(): bool; |
There was a problem hiding this comment.
removeOn sounds like a verb, but this method returns a boolean and has no side effects.
Can we call it shouldRemoveOn?
|
Realizing my comments from like |
The idea behind volunteer groups is making it easier to handle logic in the form of "membership in user group X gets you access to Y resource". For example, board members get added to the board channel and the board user group and the board email. Ops has similar rules. General membership gets added to the mailing list. Getting trained on some equipment gets you added to channels.
This wouldn't be things such as "you're invited to our Slack instance" or "You're invited to our GitHub organization". Instead it's "You are invited to this specific channel" or "You have been invited to this specific team".
For the most part, this should make that type of logic easier to audit, understand, and less one off. We can end up with things such as "This group adds you to this Active Directory group" or "This group access to the badge reader upstairs".
I'm trying to break this work down into a series of PRs. Right now, I'm not focusing too much on the UI (though there were some UI changes in here, I realized that wasn't as important right now) and instead I'm focusing on functionality. There's a feature flag for slack channels and equipment authorizations. Each equipment authorization has a slack channel id and an email associated with it (one each for user / trainer). We can migrate these to the volunteer groups and I figured I'd start by handling those rows manually and using a feature flag. If it works well, we can make sure the volunteer group channel's are updated automatically in a future PR. Then we can remove those 4 columns on the trainable_equipment table.