Internal/batch notifications #168
Conversation
~ splitting feature and unit tests
+ test views so Send could be more streamline ~ some more data for migrations + new state "canceled" so we dont bomb users with needless info
+ comments on tests and the new job
|
Split it into a job and we're set to go! Tell me if there's any formatting, commenting, etc. that you'd like me to improve on! |
|
Two quick questions! What features does this PR add?
How ready is this? I have an event starting shift signup in 2 weeks, and notifications would be nice :) |
|
Hey @jcam! Currently, the PR only notifies people if they've been signed up for a shift or multiple shifts. We're turning the rest of the batchable emails into notifications so they can be crunched into a singular daily email (if they have notifications). These all look like amazing features and look to pertain to #40, which I mistakenly wrote in the PR. My bad! |
|
Ah! Okay, I'll get less excited then :D |
There was a problem hiding this comment.
there are a few errors that need checked there's code in the slot controller that allows banned users to sign up, simple update to the conditional should work, and I ran the notifications:send command manually, and i got a wierd lil error telling me my shift started from a date to a time.
| @@ -53,6 +53,10 @@ private function userAllowed(Slot $slot) | |||
| $allowed = true; | |||
| } | |||
| } | |||
| if($slotRoles->isEmpty()) | |||
| { | |||
| $allowed = true; | |||
There was a problem hiding this comment.
this allows for banned users to take shifts.
| */ | ||
| public function handle($event) | ||
| { | ||
| // |
There was a problem hiding this comment.
why is this listener created? i don't see anything being handled just yet.
| Schema::create('notifications', function (Blueprint $table) | ||
| { | ||
| $table->increments('id'); | ||
| $table->enum('type', ['info', 'warning', 'email']); |
There was a problem hiding this comment.
while enums are more efficient, strings are better cause we could have more info types in the future, and changing migrations is not easy to deal with.
|
|
||
| @foreach($notification_metadata as $metadata) | ||
| @include('emails.'.$metadata['layout'], $metadata) | ||
| <br> |
There was a problem hiding this comment.
I managed to manually run this and I got a wierd result, it looks like its putting the date of when it starts as the start time
`
Daily Reminder about your shifts!
Thank you for being a part of Your Volunteer Database. Here's your daily dump of some weird stuff.
<h1>You're signed up for the Vice President Of Human Resources shift!</h1>
This is a confirmation email for the Vice President Of Human Resources shift you recently picked up for the Vice President Of Human Resources event.
This shift takes place on 2019-09-05 between the times of 2019-09-05 and 09:00:00.
If you did NOT sign-up for this shift or would like to CANCEL this shift, click here.
Otherwise, we look forward to seeing you there!
If any of these look strange, please log into your account and review your shifts.
`~ ScheduleFactory and UserFactory to come with "volunteer" role ~ view now displays start_time ~ Notifications migration no longer user enums, just strings
Fixes #165
A way to compress notifications to single email so we don't annoy the hell out of the volunteers.
Still needs a few comments and probably should move the logic from the command to a job that is queueable.