Denhac / denhac-webhooks

This repo holds some of our membership automation stuff. It listens to webhooks from the main wordpress site
7 stars 2 forks source link

[Slack] Refactor EquipmentAuthorizationModal to support authorizing multiple people and equipment #38

Closed chungl closed 5 months ago

chungl commented 6 months ago

Overview

This PR modifies the EquipmentAuthorization modal within the /membership command. It makes it possible to submit authorizations for multiple people and multiple pieces of equipment with a single form submission. This greatly reduces the amount of time it takes trainers to enter authorizations, which is currently a pain point for trainers running large classes - especially for the woodshop which typically trains on four pieces of equipment at once.

Before/After:

Equipment Authorization BeforeAfter

Changes

This feature requires two sets of changes:

Using Multiselect

The primary change is to switch the member and equipment selects to be multi-select inputs. This required an update to ModalTrait::getStateValues to add support for type=multi_external_select. In EquipmentAuthorization::handle, we now iterate over people and equipment to call addMembership as many times as necessary.

Decoupling Checkboxes from User State

Previously, when a user and piece of equipment was selected, EquipmentAuthorization::onBlockAction would update the form to show checkboxes for user and trainer permissions, or messages that stated that the user already had these permissions - depending on the user's state. In order to maintain full granularity of these permissions, we would have needed a table with two checkboxes per user. This seemed unnecessarily complicated to implement and too cluttered for how people tend to use this form.

I made the following decisions:

Testing

I have tested this change locally by merging it with my docker dev branch. While still in-progress, it's functional enough to be able to to run this slack command. It currently requires manually seeding the database with at least two users, connecting one of them to your slack id, and either manually manually giving yourself the meta trainer permission or bypassing those permission checks in the MembershipOptionsModal I haven't yet explored unit testing this feature.

Below are the log outputs for the dev stack that were generated from the submission in the "After" photo: Screenshot 2024-01-07 at 4 37 42 PM

TrainableEquipment: Screenshot 2024-01-07 at 6 14 32 PM

The trainer (Fizz Buzz, User 1) is already a trainer on all three pieces of equipment, and a user on two (unrealistic, but useful for testing). They already have five of six possible memberships. User 2, Buzz Lightyear, is not a user or trainer on any of the equipment, and so needs all six possible memberships. We correctly see the following relations posted to "denhac.org":

Screenshot 2024-01-07 at 5 02 02 PM

Potential Further Improvements

maps for code reuse and readability

There are four loops that that iterate over the multiselect values, eg foreach($personValues as $personValue) {. I'd have rather made private static function personFromValue($personValue) and used it as $allPeople = array_map(self::$personFromValue, $personValues), however I'm new to php and couldn't get the syntax right.

Submit modal should have the option to authorize more

Given that the trainer checkbox applies to all users, it would be nice if the success modal gave you the option to go back to the form and submit another batch. This could also be useful if some people didn't receive training for all equipment. It's just faster than rerunning the whole slash command. In fact, if such a button existed it may enough to alleviate much of the need for this PR.

Preload some options

The membership command has a few select fields that don't need to be loaded asynchronously. When we launch the membership modal, we already know who you are and what permissions you have. We should use a static select there. We can also do this for the equipment list, as it's a small list and is very unlikely to change while you have the modal open.

Autoselect single option

For many of our users, the only command available in the membership command is the training item. Similarly, they may only be a trainer for one piece of equipment. In these cases, it would be ideal to skip the "what would you like to do?" page and preselect the only available piece of equipment.

Jnesselr commented 6 months ago

This looks really good and I very much appreciate your time on it.

Re further improvements: I'd definitely refactor parsing the person/equipment into its own methods in this diff, since that code is used twice and has a tiny amount of logic. I'd look into Laravel Collections for some of this. You could do a crossJoin for example across your people and equipment instead of doing the nested for loop. There's also some map operations that should help instead of using PHP's built in array_map utility.

For authorizing more, the only issue I can see is how you clear the fields. Since the block/action id's are all the same, Slack will want to be helpful by filling in the data that was entered in already. We technically use a view update response on submission to try and avoid being limited to the "3 cards on a stack" aspect Slack has for modals when you add a new modal. However all of the fields are probably different so I don't expect data to cross over from the previous modal into the new one. You'll be running smack into that issue, unfortunately. Definitely wouldn't want that in this PR. The Slack modal docs are here.

I fully agree with preloading options and if someone only has one entry, auto populating it. If you do get to it, I look forward to seeing those PRs.

Jnesselr commented 5 months ago

Just for the record, the protection preventing someone from merging in didn't work as expected, but Casey and I had discussed merging this and while I didn't actually hit the "I approve" button, I do approve.