Notifications into the queue

[luceos]

Forces notifications into a dedicated SendNotificationsJob and passed to the queue
One static method re-used in the job ::getAttributes, is that okay or use a trait?
Do we want to use this solution and refactor into a better Hub after stable, postpone this implementation or use it in b11?

Fixes #1869

Changes proposed in this pull request:

Forces all notifications into the queue, not just those from subscriptions. Guaranteeing a more streamlined user experience.

Reviewers should focus on:

• Static method call from NotificationSyncer is reused on the SendNotificationsJob, the ::getAttribute. Is this okay or do we want to use a trait?
• Do we want to use this solution and plan out a better notification hub for after stable?
• This implementation assumes all notification channels are only async, this implementation works well with that assumption.

I decided against trying to introduce Laravel notifications channels, because that would bloat our app and increase the weight of this implementation too much.

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

Required changes:

• Related documentation PR: (Remove if irrelevant)
• Related core extension PRs: (Remove if irrelevant)

[dsevillamartin]

Do we want to use this solution and plan out a better notification hub for after stable?

Probably, yeah.

This implementation assumes all notification channels are only async

What about the sync queue? We can't assume notification channels are async if there's only ne other queue implementation available for Flarum right now - and it's not even an official one

What problems does this implementation have with the sync queue? As far as I can tell while skimming the code, it should work fine with it.

[luceos]

What I considered is that if any of the broadcast channel require or expect immediate user interaction, they will fail. I agree there should be no issue running these notifications with sync up until a certain number of recipients is reached and a specific amount of channels is configured. The longer processing the notifications takes the more likely the XHR will time out and the user receives no feedback on whatever they were actually doing..

[franzliedke]

Cool stuff! Great approach for moving all our emails into the sync queue at once, much better than my modest proposal in #1869. 😉

A few comments - let's discuss this, I am curious to hear what you think.

[franzliedke]

Just noticed this will also fix #978. 💪

[luceos]

@franzliedke I decided in favor of an abstract, can give an opinion on this?

[franzliedke]

Rebased, diff is looking much better now.

[franzliedke]

Good job, what a huge step forward! 👏

However, if I am not misunderstanding things, a small inconsistency got introduced - probably in the latest refactoring step.

[franzliedke]

Okay, we cannot quite consider #978 to be fixed with this yet - this "only" tackles notification emails. There are a few other emails, so those need to be queued as well. See #1920 for more discussion on that topic.

[franzliedke]

Had a call with @luceos in the afternoon and made a few tweaks. I am happy with this now.

We will need the changes from flarum/subscriptions#21 to also move the potentially costly call to the NotificationSyncer for post subscription notifications to the queue.

[franzliedke]

I removed by weird BlueprintBC backwards compatibility layer again, see 1cda9dc - having getAttributes() in one place is actually quite nice.

I moved all of it to the Notification model, though - the getAttributes() stuff was only ever called in conjunction with fetching or creating notification models, so this felt like it belongs there.