denhac / denhac-webhooks

This repo holds some of our membership automation stuff. It listens to webhooks from the main wordpress site

denhac-webhooks

This repo holds some of our membership automation. It listens to webhooks from the main site and updates slack and google groups membership as well as updating access cards. It also listens to slack commands like /membership which allows us to customize that list based on the slack user that issued it, whether they're a board member, have an active subscription, etc.

Architecture

Quite a bit of the code runs using event sourcing. The MembershipAggregate is the main entry point for a lot of the functionality as it decides based on subscription updates if someone is a member or not. Various projectors update models in the database and various reactors do things like send emails when needed.

Helpful Things

Aggregate Version Reset

Usually, you don't delete events from the database, but it can be useful to do so in the event of deprecation of events or fixing badly stored events. Sometimes, when that happens, the version numbers for the aggregate get out of wack. Here is a helper command to fix that:

php artisan event-sourcing:fix-aggregate-version

denhac-webhooks's Issues

[Flaky Test] customer_updated_with_different_last_name_is_not_assigned_to_customer

customer_updated_with_different_last_name_is_not_assigned_to_customer failed during automated testing on a completely unrelated PR. On retry, it succeeded.

Failed asserting that 'Kozey' is not equal to 'Kozey'.

Initial run

Retry succeeded

[Slack][CreateTrainableEquipment] Form does not prevent duplicate submissions

Discovered in relation to #69, there is nothing preventing the user from creating redundant pieces of trainable equipment if they submit the form twice or just submit equipment that already exists.

There should be a uniqueness check prior to form submission, and equipment with the same name (case insensitive) should be rejected with an error message that explains that equipment with this name already exists.

Bonus: Ideally we'd do a fuzzy search and warn/prompt for confirmation if similar equipment already exists. EG: There have been two forges ("forge safety" and "forge") and three wood lathes ("wood lathe", "jet lathe", and "jet wood lathe") that have been created independently by different people. Since non-user/trainers cannot browse the full list of equipment, there is nothing but tribal knowledge or database access that tells a well-wishing contributor that a piece of equipment already exists.

[Actions] Create AuthorizeEquipment action

We should create an action to encapsulate the work of associating UserPlans to Users.

Since actions are replayable, we will be more resilient to failures that can arise from needing to post one request per user per plan.

This allows us to decouple the intent of a modal submission from the execution of potentially queue-able work. We can respond quickly to avoid timing out on the user's request, and we can retry as necessary if any failures occur while processing the work.

I know I wrote this part of the code, but one issue that's come up is that if this line fails, the Slack UI fails and no other authorizations happen. Because of this, I've been migrating things that use an API but that aren't mission critical to be done right that second in the dialog over to an Action that can be queued and retried.

php artisan make:action <name> to make one, and I'd put it in a new folder called "WordPress". I'm also starting to move things with "WooCommerce" in the name over to WordPress but that's a separate thing.

Originally posted by @Jnesselr in #38 (comment)

Sync & store slack handles on Customer

Many denhac members do not use their name as their slack handle. It is common for members to be known to other members exclusively/primarily by their slack handle. We store several name fields on Customers, but none of them are slack handle. This causes confusion/inconvenience when trying to look up member accounts for tasks including equipment authorization and incident investigation.

Notably, slack handles can be changed by the user. We need to watch for update events from slack and keep this field up to date.

It is my opinion that slack handle and Customer->username should be the same. username is infrequently used/visible, and often different from the slack handle. Some longtime members have laughed when identified by their customer username, saying "oh that's very old I had no idea I was still using that anywhere". Counterpoint: I believe username can be used to log in to wordpress. It would be confusing to some users if their login username suddenly changed. Personally, I'd rather disable that if possible and make people log in with their email addresses.

[Slack][Membership Command] Use static selects for MembershipOptions, TrainableEquipment

We should use static selects in our Slack modals rather than external selects whenever possible. Obvious candidates are the initial MembershipOptionsModal and the equipment list in EquipmentAuthorization.

External selects require slack to send a network request to webhooks to fetch the list of options, and are useful for lists that may change or are too large to prepopulate. However, this introduces noticeable latency to the modal as the user has to wait for select menus to populate. The static select element allows us to send the options as part of the view update, eliminating this round trip and improving UX.

Simplify USER_CHECK in EquipmentAuthorization::handle

I took a shortcut when bypassing some deprecated functionality, and forgot to clean it up.

The USER_CHECK constant is no longer required. I can't comment on the line where it's declared, for some reason.

Originally posted by @Jnesselr in #38 (comment)

[Slack][Membership] Skip MembershipOptionsModal for users who have only one capability

Many equipment trainers are regular members with no special privileges besides equipment authorization. When they use the /membership command, they are greeted with a modal that asks "What do you want to do?" that has an empty select field. When they click on it, they see that they have one capability from which to choose.

This is suboptimal UX and unnecessary friction.

When we handle MembershipOptionsModal, we should check the options length before we post the view. It may be as simple as delegating the return value to the handle method of the modal associated with the only option. There may be some complexity there; I haven't explored that yet.

[Slack][EquipmentAuthorization] Auto-select only available equipment

If I'm a trainer who is only authorized on one piece of equipment, it would be nice if the equipment selection menu was prefilled for me when I open the EquipmentAuthorization modal via the /membership command.

This is likely only viable to implement after #42.

Persist trainer id in EquipmentAuthorization UserMemberships

It is potentially valuable to know who authorized (trained) a member to use a piece of equipment.

We should extend the appropriate endpoint and model at denhac.org, and pass this information through when adding UserMemberships for equipment authorization.

If you want $actorId to carry over into the metadata, we can do that too as a separate PR.

Originally posted by @Jnesselr in #38 (comment)

Requests to slack conversations.list are exceeding a rate limit

This issue was first noticed following (and appears connected to) my recent update enabling batch submissions of equipment authorizations (#38).

We're seeing rate limit errors (429 Too Many Requests) from slack on the https://denhac.slack.com/api/conversations.list endpoint initiated by the AddToChannel job. These errors come in rapid succession shortly following a submission of equipment authorizations.

It appears that when denhac.org receives the batch of userMemberships, it sends us a batch of userMembership.created messages, each of which triggers a call to SlackReactor::onUserMembershipCreated, which in turn calls AddToChannel::queue()->execute($customerId, $slackId). execute calls SlackActionTrait::channelIdFromChannel, which calls ConversationsApi::toSlackIds, which calls ConversationsApi::list, which throws the error.

Both occurrences of this problem have been for woodshop training. I'm not sure what slack channel it's trying to get (#help-woodworking?), but there's four tools being authorized (Table saw, jointer, planer, router table). One or more of them must have user_slack_id or trainer_slack_id. No trainers have been authorized in either of these batches, so it should be a user_slack_id.

We could likely eliminate the error by caching the results of channelIdFromChannel, as it's read many times and unlikely to change - especially in short succession.

However, it's a bit surprising that we need to call channelIdFromChannel at all, given that the input to the AddToChannel job is named _slack_id. Perhaps it's an inconsistent naming convention, or perhaps we're doing unnecessary and/or nonsensical work in this job.

[Slack][CreateTrainableEquipment] Form completes successfully but throws slack error

The slack modal for creating new trainable equipment throws a generic "something went wrong" error in slack on form submission, even though the task completes successfully. There do not appear to be any error logs written.