Initial Notifications for Subscribed Users#38
Initial Notifications for Subscribed Users#38bmckay959 wants to merge 6 commits intoinxilpro:mainfrom
Conversation
skylerkatz
left a comment
skylerkatz
left a comment
There was a problem hiding this comment.
I am not sure this will get us what we want, which is that if someone sends 10 updates in a short period of time, we don't want to send 10 emails, we only want to send 1 email.
I might be mistaken by what @DanielCoulbourne originally meant in Discord though.
|
@skylerkatz Since the notification job uses |
I might be totally reading the docs incorrectly, but I think it will just put the job back on the queue to try again. It will only allow one job of the same lock to process at a time, but it won't drop the job. |
2 different issues being solved here:
|
|
You can now run |
| foreach($check_ins as $check_in) { | ||
| $check_in->update(['notifications_sent_at' => now()]); | ||
| Cache::lock('send-notifications:'.$this->phone_number->id, 30)->get(function () { | ||
| $check_ins = $this->phone_number->not_notified_check_ins()->get(); |
There was a problem hiding this comment.
I'm not sure we want to send all of the check-ins over email since there's no upper limit. We may just want to let the subscriber know their person has checked in and provide a link to the phone number's show page.
There was a problem hiding this comment.
I agree, seems sensible to maybe show the latest checkin and link to where the rest are shown.
There was a problem hiding this comment.
I agree, I think we should just add a link directly to their phone number.
There was a problem hiding this comment.
Ok. I was thinking that since it can only send from the last 2 minutes, we would be at 120 records if someone sent 1 per second(Which seems unrealistic).
If no one is opposed, I'll put the last 10 that have not been notified, then also put a link to the phone number page too.
There was a problem hiding this comment.
Just made a new commit here. I'm still uncertain about the "no upper limit" and think this should not be solved here, but on a rate limit for the incoming messages in general. I'm allowing up to 1 per second here per phone number in theory. But, the issue is that we can get someone to spam the system and get 100 per second. I'll take a look at this in a different PR.
@skylerkatz might be the |
I just reread the docs, and since it is delayed two min, it does look like no other jobs will be put on the queue (for the phone number) until after the job has run. |
| $check_ins = $this->phone_number->not_notified_check_ins()->get(); | ||
|
|
||
| if ($check_ins->count()) { | ||
| $this->phone_number->subscriptions() |
There was a problem hiding this comment.
Should we fire a SubscriptionWasNotified event and do the rest of the code from this job in the event?
The Check In models themselves are created from events, so if we replay events in the future the notification_sent_at date is going to be null
|
I haven't had a chance to look this through fully yet, but I think the existence of a |
|
@inxilpro I'm good with whatever here, but my thought process was that we could have multiple messages inside of a single notification. So this would allow us to send a notification for all "check_ins" where If we only use a 2 minutes delay and we say "get everything in the last 2 minutes", we are not guaranteed that it runs exactly at the 2 minute mark, so grabbing all that are null guarantees that we send each check_in. Also....I've never used Verbs, but wanted to help so may have a fundamental misunderstanding of how it actually works(Though I did see the wizardry at laracon). Let me know if you want me to do anything specific , but also happy to have you nuke it if needed. |
6 participants
Initial implementation for sending emails to subscribed users.