feat: Waiting list automated removals

[BenEsen97]

This PR adds functionality to have waiting list members automatically removed as directed in the ATC training handbook, when their network activity falls below the requirements.

The work adds a scheduled task which runs and performs the following functions:

• Identifies members of a waiting list who do not meet the activity requirements and are not already marked for removal, marking them for removal
• Checks waiting list members who are marked for removal and actions the removal process (sending a 5 day reminder, and finally removing them)
• Cancels pending removal action when members meet the activity requirement
• Emails are sent out throughout this process, and the relevant notifications and email views have been created.

Finally, the UI has been updated for viewing waiting lists to provide members with information relating to this process.

This is my first contribution to core, and I am happy to receive feedback and make any required amendments.

This PR relates to an upcoming training policy change, and should not be merged without prior co-ordination with the ATC training director.

[AxonC]

General Comments:

Given the scale of the number of members on lists, I'd suggest that some of the jobs, like marking for approval, detect if those marked for approval are now back into eligibility, and sending the notifications are done via an event-driven architecture. In Laravel terms, this is where an event is emitted and a queued listener responds to the relevant action. Blocking the command until the completion of this process could lead to performance issues.

This would make it easier for other things in the future to start the process of marking them for removal e.g. if it was that we wanted to start looking for activity on specific positions, or whether a theory exam had been completed within X time of joining the list, for example. You'd be able to trigger from multiple sources, rather than coupling to the implementation of the command.

You may wish to have a separate command that runs more frequently to check the removals so you are avoiding the edge conditions you have with the daily checks. Separating the command out would be my advice in general here.

I'll do a second review if you have any questions. Feel free to reach out on DIscord with any other questions.

[BenEsen97]

General Comments:

Given the scale of the number of members on lists, I'd suggest that some of the jobs, like marking for approval, detect if those marked for approval are now back into eligibility, and sending the notifications are done via an event-driven architecture. In Laravel terms, this is where an event is emitted and a queued listener responds to the relevant action. Blocking the command until the completion of this process could lead to performance issues.

This would make it easier for other things in the future to start the process of marking them for removal e.g. if it was that we wanted to start looking for activity on specific positions, or whether a theory exam had been completed within X time of joining the list, for example. You'd be able to trigger from multiple sources, rather than coupling to the implementation of the command.

You may wish to have a separate command that runs more frequently to check the removals so you are avoiding the edge conditions you have with the daily checks. Separating the command out would be my advice in general here.

I'll do a second review if you have any questions. Feel free to reach out on DIscord with any other questions.

Hi @AxonC

I have refactored this PR, including splitting the system into two isolated commands that be be ran on separate schedules.

The first command is responsible for checking waiting list accounts meet the activity requirement, both those not marked for removal and those which are.

The second command is responsible for actioning pending removals.

I've also created unit tests to cover both of the new commands.

[AxonC]

I'd suggest since you're using this filter for ATC lists in more than one place, you create a local scope: https://laravel.com/docs/9.x/eloquent#local-scopes

[BenEsen97]

Hi @AxonC

I have created a local scope for this exact purpose in #2804 as their was a similar requirement in that PR, and a local scope was suggested by @atoff.

Makes sense to utilize that here, however this change is blocked until 2804 is merged.

[AxonC]

Another example of where this logic should be abstracted to a model method.

[BenEsen97]

Hi @AxonC

I am struggling with this one a little - can you provide an example of the logic?

I don't work with Laravel in my day-to-day so lack an in-depth knowledge of it at this level.

The main issue I am facing is that as far as I can see, to add a model method such as activeAccountsPendingRemoval() to replace the following two filters:

->filter(function ($account) {
    return $account->pivot->current_status->name == 'Active';
})->filter(function ($account) {
    return $account->pivot->pending_removal?->status == 'Pending';
})

I would need to add it to the Account model, not the WaitingListAccount model. Adding it to the WaitingListAccount seems like the correct place, but is then not accessible in the query builder as far as I can see due to it being on a pivot and not the parent model.

I believe I could do something like:

->filter(function ($account) {
    return $account->pivot->activeAccountsPendingRemoval();
})

But using a filter appears to defeat the point of the change.

[AxonC]

The magic number 6 has no relationship to the comment - please explain why in a comment subDays(6) is used since it isn't self-explanatory.

[AxonC]

I'd suggest rather than relying on this, a consistent processing time should be used, or there needs to be more edge case testing of this.

[AxonC]

Given the requirements, I am happy to do this on should the amount of feedback be overwhelming.