framework icon indicating copy to clipboard operation
framework copied to clipboard

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

Open davwheat opened this issue 2 years ago • 3 comments

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

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

Confirmed

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

davwheat avatar Jul 06 '22 20:07 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 avatar Jul 11 '22 05:07 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.

davwheat avatar Jul 11 '22 05:07 davwheat

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..

luceos avatar Jul 25 '22 14:07 luceos

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.

davwheat avatar Aug 22 '22 21:08 davwheat

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 avatar Aug 22 '22 21:08 SychO9

lets try that again...

davwheat avatar Aug 30 '22 17:08 davwheat