[Feature] System notifications with notification feature announcement #10570
Conversation
petertgiles
added
blocked: copy
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10570 +/- ##
============================================
- Coverage 38.59% 37.98% -0.62%
- Complexity 1455 1467 +12
============================================
Files 1013 1015 +2
Lines 30899 30988 +89
Branches 6632 6621 -11
============================================
- Hits 11926 11770 -156
- Misses 18809 19183 +374
+ Partials 164 35 -129
Flags with carried forward coverage won't be shown. Click here to find out more. β View full report in Codecov by Sentry. |
| if (! is_null($singleEmailAddress)) { | ||
| // single email address provided | ||
| $recipientUsers = [ | ||
| User::where('email', $singleEmailAddress)->sole(), |
There was a problem hiding this comment.
is this for testing purpose?
There was a problem hiding this comment.
Yeah, exactly. You can try sending yourself the email first, before sending it to every single user.
|
|
||
| return Command::SUCCESS; | ||
| } | ||
| $recipientUsers = User::all()->all(); |
There was a problem hiding this comment.
Does it include soft deleted users as well? Can we exclude them?
Also can we select only the necessary columns in the user object?
There was a problem hiding this comment.
To query deleted users you'd need to use the withTrashed scope.
https://laravel.com/docs/10.x/eloquent#querying-soft-deleted-models
There was a problem hiding this comment.
Regarding selecting necessary columns, this command is supposed to be generic and used without modification for any templates we want to add. So, we don't really know what the necessary columns are. I'm not too worried about perf here since this is being run from the console. If it takes a few minutes to complete it shouldn't bother any users.
| { | ||
| $locale = $this->locale ?? $notifiable->preferredLocale(); | ||
|
|
||
| if ($locale == Language::EN->value) { |
There was a problem hiding this comment.
Bilingual notification could have been easier in this case. No need to check every user's preferred locale.
There was a problem hiding this comment.
Yeah, I remember talking about bilingual notifications for the talent search request notification. We did a bilingual one there because the user wasn't logged in but there was a strong preference for localized notifications when possible.
|
OK, I think the problem in Azure was fixed with create cache directory. I think that solved the problem with the rate limiter too. I intentionally broke it with intentionally break notifications and wasn't able to replicate the spamming. Here it is, waiting 300 seconds before trying again: |
π€ Resolves #10220
π Introduction
This branch sets up a framework for creating one-time system notifications and also adds the first one - a notification feature announcement.
π΅οΈ Details
This is set up with a mostly-empty GCNotify template. A group of Laravel views with blade templates take the user object and fill in the required data to send to the GCNotify template. (Yo dawg, I heard you like templates...)
The idea is that for future system messages we can just commit some new views and run the existing command pointing at those.
π§ͺ Testing
./artisan send-notifications:system notification_announcement (myemailaddress)./artisan send-notifications:system notification_announcementπΈ Screenshot
π Deployment
New environment variables:
GCNOTIFY_TEMPLATE_SYSTEM_NOTIFICATION_EN=a4f234a4-34bd-4ccd-9ade-8c6e265e134e
GCNOTIFY_TEMPLATE_SYSTEM_NOTIFICATION_FR=f579056c-a183-4517-bfd5-32544131a545