feat(subscriptions): add option to send notifications when not caught up

[davwheat]

Changes proposed in this pull request:

This PR adds a new configurable option to Flarum Subscriptions to allow the sending of notifications even when a user is not caught up to a discussion yet. This could be seen as a easier migration step from one forum software to another, where users are more used to receiving emails for all new posts, rather than just the first new one.

Due to the notification subject for subscriptions being the discussion itself rather than the post, notifications in the frontend are still grouped into one single notification, so this change really only affects emails. Changing the subject would be a breaking change, though, as would introducing a new notification which replaces the old one.

Reviewers should focus on:

This should retain existing behaviour by default, using the memory-based settings defaults.

Necessity

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

[SychO9]

Looks good, two thoughts though:

• I think when it comes to notification behavior it might be better to introduce this as a user preference instead of a global admin setting.
• dont_notify_unless_caught_up (and the locale value) is hard to understand unless you read further or look at the code. Could we change it to notify_first_new_unread_post_only (and the locale text) ?

[davwheat]

This code is rather... interesting.

I'm not sure how to go about doing validation for user preferences -- if I could be pointed in the right direction, I'd appreciate that.

I also think my DB queries might not be very performant for large communities. Will seek feedback on that from some others. If that's the case, it might be wise adding a new column on the users table to store this preference, thus allowing all the new logic in the PHP to be moved to part of the DB query instead.

The code does all work, though, it's just not that great. 😅

[davwheat]

I think the best option is to either move the criteria option to a column on the user table, or a new table with a FK. A new col on the users table would not be a very performant migration for very large communities, though.

[luceos]

I've looked at this PR again to see whether we should "just do it", but when I'm looking at the logic to match preference to see whom to send notifications to I can only cringe. This logic seems overly convoluted and confusing. I know this isn't really a great PR review comment, sorry about that. We are all doing our best to find solutions to things that are (much) requested, but I'm unsure that this is the right approach.

This request almost warrants a completely different implementation for handling subscriptions and notifications..

[SychO9]

This logic seems overly convoluted and confusing.

Our preferences system really needs a bit of a refactor. Storing them in a column is not flexible.

I think the complication lies in the: preference vs admin default vs admin enforcement. I don't think admin enforcement here is really necessary. I think we should also drop the admin default for a better admin default user preferences system in the future which would integrate seamlessly with reading user preferences.

That would probably cut down on the complexity.

[SychO9]

Looks good, just a couple of comments, and I took the liberty to push integration tests.

[davwheat]

what are you referring to I'm not following 🤔

At the bottom of the email translation, it says that the user won't be notified for new.replies until the thread has been read. That needs to be removed based on user prefs.

[SychO9]

what are you referring to I'm not following thinking

At the bottom of the email translation, it says that the user won't be notified for new.replies until the thread has been read. That needs to be removed based on user prefs.

oh, yea I see it now, I don't think we can do a per preference phrase, the blueprint specifies the same email body to all users, it isn't aware of individual ones, so we can only try to tweak the wording of the email maybe so that it hints to preferences?

[SychO9]

One last thing, the UI needs tobe consistent, the option should go under Automatically follow discussions that I reply to, we can do that in addSubscriptionSettings() instead without adding a new section in user settings.

[davwheat]

lets try that again...