[Slack][Equipment Authorization] Refactor batch authorization into queuable action(s)

[chungl]

Overview

Moves equipment authorization logic out of the slack modal handler and into Action classes (#45)

The primary motivation that drove this is that modal request times out after three seconds and sometimes the wordpress server takes longer than that to process a batch of updates.

This change creates two actions: a synchronous BatchAuthorizeEquipmentAction that validates that the trainer has the appropriate trainer permissions. It also checks for any permissions that the members already has, and skips enqueuing those. Then, it enqueues an asynchronous AuthorizeEquipmentAction for each user-equipment plan. The expectation is that these individual queued actions can be run on a separate thread and can be retried in the event of failures - without having to rerun the whole batch.

Also cleans up dead code in EquipmentAuthorization(#41).

Testing

This was tested in dev, but in dev the queued actions still run synchronously - presumably because an async queue service such as horizon has not yet been set up. Functionality still works (users are marked as trained), but the request can still time out because the queued actions still run synchronously during the modal submission request. The authorization check and error was tested in dev and catches lack of trainer permission:

Followup

We had to manually register the actions to the app provider in order for them to be available. It seems like we probably need to set up an actions provider that is more automatic, but I need to do further research on best practices for dependency injection in Laravel.

[chungl]

I left several comments, but one thing I absolutely missed on the previous PR is a quirk in how collections work. empty() will return false on an empty collection:

> $c = collect()
= Illuminate\Support\Collection {#6789
    all: [],
  }

> empty($c)
= false

You have to use ->isEmpty() or ->isNotEmpty() on the collection. This is lines 186-194 of the EuquipmentAuthorization modal class. In testing, you should see that even with nothing selected, you don't get the "no equipment" and "no person" showing up.

That's pretty strange. In testing, I do in fact see the "no equipment" and "no person" showing up - in dev, and in prod. I did run your snippet in tinker and empty($c) does return false so I don't know what to say.

I'll update the empty test, but it makes me wonder if it was worth refactoring the loops I originally had to use these collections that come with this really unintuitive behavior.

[Jnesselr]

If you haven't selected any equipment or people, you're still seeing what is enabled in the constructor. Btw, if we're getting rid of noEquipment and noPerson methods, you can refactor setUpModalCommon into the constructor and remove that call in the onBlockAction method. You should see them both disappear the second you select a person or equipment even though only one of them should have.

I'll update the empty test, but it makes me wonder if it was worth refactoring the loops I originally had to use these collections that come with this really unintuitive behavior.

Well, so this is the thing. A lot of laravel classes actually do the smart thing and protect you from shooting yourself in the foot. PHP's empty is one of those gripes I still have about it. The empty method is basically !isset($var) || $var == false. When you pass in a collection, it's an object which evaluates to true always and there's no way to override it. In other words. It's actually one of the reasons I use is_null when I know the variable will be set and I'm using null coalescing. Anyway, point is it's not Laravel's fault here, it's bad naming for this PHP function and there's nothing the Laravel team can do about it.

[chungl]

I left several comments, but one thing I absolutely missed on the previous PR is a quirk in how collections work. empty() will return false on an empty collection:

> $c = collect()
= Illuminate\Support\Collection {#6789
    all: [],
  }

> empty($c)
= false

You have to use ->isEmpty() or ->isNotEmpty() on the collection. This is lines 186-194 of the EuquipmentAuthorization modal class. In testing, you should see that even with nothing selected, you don't get the "no equipment" and "no person" showing up.

This is unrelated to the current changes and is fixed in #53.

[chungl]

This pull request really became a much bigger discussion than I was expecting when I made what I thought was a small change to reduce the impact the user from an unrelated, pre-existing issue that became more obvious due to my recent upgrade to the equipment authorization modal.

I've fixed the majority of the issues raised here, some in separate pull requests. The major objection left surrounds the creation of the BatchEquipmentAuthorization action, which Justin considers premature optimization.

I believe the code is intelligible and functional as-is, and people are waiting for this issue to be fixed. I request that it be considered as-is. Any further changes can be reasonably fixed in future pull requests.