Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to set their desired frequency of notifications (immediately, delayed, never) #344

Open
timdiggins opened this issue Jul 20, 2016 · 14 comments
Assignees

Comments

@timdiggins
Copy link
Collaborator

timdiggins commented Jul 20, 2016

@jayroh @glebm As mentioned in #340 (comment) would like to be able to have a setting for (Email) Notification frequency of new unread posts.

Currently the frequency is "immediately"
But obvious alternative would be: "never".

Additionally would be nice to have some intermediate frequencies.
In the ideal the frequency options are configurable by site owner but for MVP might be easier to just have precanned options: after an hour, after 24 hours.

When one of the intermediate frequencies is chosen, you then get an email whenever you've got a new post in one of your followed topics which you haven't been notified about which is xxx hours old. But you only get one notification for a topic. (so it's a " new stuff" email, not a "posts you still haven't read" email). But in any case if you keep reading your stuff online you won't get a notification (which is good).

Quick check:

  • I propose that this is a global user setting not a per-messageboard user setting (harder to make it work with varying settings per messageboard) -- ok with that
  • I propose this just covers followed public topics and not private messages (that could be a different but subsequent PR) -- ok with that?
  • Ideally we build this out so we can allow drop-in alternative notification channesl -- but only email would be part of the thredded core
@timdiggins timdiggins changed the title Adjust Frequency of notifications Allow users to set their desired frequency of notifications (immediately, delayed, never) Jul 20, 2016
@timdiggins
Copy link
Collaborator Author

PS - to make it clear:

the system would wait until there is an unread followed post that is [desired-frequency] old, but then it would notifiy you about all the unread posts at that time. So you would only get max one email per [desired-frequency]

@glebm
Copy link
Collaborator

glebm commented Jul 20, 2016

Sounds good overall!

I propose that this is a global user setting not a per-messageboard user setting (harder to make it work with varying settings per messageboard) -- ok with that

All the other settings have a global and per-messageboard version (that defaults to global unless explicitly set), so it would be nice to keep it consistent.

the system would wait until there is an unread followed post that is [desired-frequency] old, but then it would notifiy you about all the unread posts at that time. So you would only get max one email per [desired-frequency]

Would we schedule a job to happen [desired-frequency] from now? Care needs to be taken when a frequency is changed in the presence of pending jobs.

@jayroh
Copy link
Member

jayroh commented Jul 20, 2016

Would we schedule a job to happen [desired-frequency] from now? Care needs to be taken when a frequency is changed in the presence of pending jobs.

Totally agree. On this front I wouldn't recommend adjusting, or canceling, or re-scheduling, existing jobs. I've been down this road before and it was less than fun.

Messaging around this sort of change could be along the lines of "Future scheduled notifications will now adhere to your new frequency" ... or something like that.

@timdiggins
Copy link
Collaborator Author

All the other settings have a global and per-messageboard version (that defaults to global unless explicitly set), so it would be nice to keep it consistent.

Would we schedule a job to happen [desired-frequency] from now? Care needs to be taken when a frequency is changed in the presence of pending jobs.

I agree with both of your reservations around rescheduling future scheduled jobs, but I hadn't yet begun to think about the implementation mechanism. Would like to think about other alternative implementations too.

In any case, if future scheduled jobs were created, then they would have to check back to the db on their execution (are there any followed topics with still unread posts to notify about), at which point they may find that they are no longer valid and have to make a new scheduling.

My gut feel is that it might be easier to do the scheduling the other way (ie. have a repeating scheduled task that looks for unread unnotified posts and checks if they should be notified).

All the other settings have a global and per-messageboard version (that defaults to global unless explicitly set), so it would be nice to keep it consistent.

Agreed with consistency in general, but it also needs to be coherent, and relatively straightforward to implement, and (if possible) extensible. I'd rather see a (real-world if possible) reason for the per-messageboard preference on this version (my usage of thredded doesn't have this)

@timdiggins
Copy link
Collaborator Author

timdiggins commented Jul 21, 2016

@glebm @jayroh Thinking a bit more about this implementation (the current option in my head, though the other alternative is setting future-timestamped scheduled jobs) , We could extend UserTopicReadState to have a notified_at datetime. The sql call to find candidate users to notify is a bit grizzly, but it can be done (even if we have per-messageboard settings, though that's a bit more complicated). Will keep thinking. Other implementation options welcome! [EDIT: too many "candidates" -- I confused myself]

@timdiggins
Copy link
Collaborator Author

timdiggins commented Sep 13, 2016

[EDITED to fill out and make clearer]

Here's a bit more detail on planned implementation of the delayed (max once per hour) email.

When I have "max once per hour" (delayed notifications) switched on, I receive "New message emails" as follows:

An hour after a topic I follow has a new post:

  • I should be sent an email with a link to that topic
  • If another topic has an new post in the meantime, the email I'm sent has a link to that post too.
  • I should not be sent an email, if I have already read it in the meantime
  • I should not be sent an email, if I have already been sent a following email in the meantime
  • I should not be sent an email if I have already been notified of this post by email.

Ditto for "a new topic which mentions me"

NB: the delayed feature would cross messageboards, so I'm proposing not (at least initially) to do it with per-messageboard support:

The idea here is that you receive a "Thredded updates" email at most once per hour ("things that have happened since you've been away") and that this includes everything that you haven't read online or been notified about already. The system would try to send this as late as possible, but aiming to get one per hour.

Two different implementation plans in my head:

  1. For every post for a topic with followers (including first topic), just schedule a job for one hour in the future called a SendDelayedUpdates (as well as an job for now to send immediate notifications, which sends to users who are set for immediate notification).
  • The SendDelayedUpdates checks for each follower whether this topic is notifiable and whether the user is notifiable, and then sends the topic if so, and then send the notification email for that user (see below)
  1. An alternative version would be top down rather than bottom up and run as often as you like (main_app user would have to schedule a regular job, say every 10 minutes) -- and it would check for users who haven't been notified_at for an hour AND who have something to be notified about which is at least 50 minutes old (say), and then compose the Notification email

Commonalities of both implementation options

  • Notification email: The notification email to a user would compose an email with a short excerpt of unread unnotified posts for every followed topic and then mark those topics and that user as notified_at now.
  • Topic notifiable for a user: Whether a topic is notifiable for a user is if the updated_at for the topic is greater than the read_at and the notified_at for the UserTopicReadState.
  • User notifiable: Whether a user is notifiable is if they haven't been notified_at in the last hour.

Collisions: In both of these implementation options, collisions (two tasks sending the same email) are possible.

(I'm tempted by a slight future-proofing but I think I'm going to ignore because YAGNI, but noting here to remind myself is to say that users can be notified by different channels separately (e.g. push notification / Trello and email) so rather than User#notified_at should be looking at ChannelUserNotification#notified_at)

In general I'm leaning towards the first option, as it seems clearer to me, and the parts are simpler, and also doesn't require the main app user to set up a cron (just an activejob handler, as currently).

@timdiggins
Copy link
Collaborator Author

Having worked on this for a bit I think we're splitting this into two parts:

Customizable Notifiers

  • as an app owner, be able to add and customize extra notifiers for immediate notification (e.g. Slack, Pushover, your own app)
  • as a user, be able to opt out of given notifiers
  • as a developer, be able to write a gem for new notification channel

Daily (or whatever) email updates of unread followed topics / unread private messages

  • this is probably an main_app-specific thing

I have split the first one off into a separate issue #441
and would propose we close this one, unless there's a need to continue with it.

@glebm
Copy link
Collaborator

glebm commented Oct 31, 2016

I think notification frequency and grouping would be a useful feature in general (most mailing list software have this), but if no-one is planning to implement it in the foreseeable future let's close.

@glebm glebm closed this as completed Oct 31, 2016
@timdiggins timdiggins mentioned this issue Nov 4, 2016
9 tasks
@zapnap
Copy link
Contributor

zapnap commented Nov 21, 2016

The 'daily' updates (items not covered in the other issue) would presumably be something like a daily or weekly digest? I could see that being extremely valuable... Just adding my 0.02 :)

@jayroh
Copy link
Member

jayroh commented Nov 21, 2016

@zapnap yeah that's been on the docket for a while :). see issue #238

@timdiggins
Copy link
Collaborator Author

@jayroh @glebm this (equivalent to #238) is on my hitlist -- will reopen it when I have a PR to offer.

I'm thinking of a option which is:

notify me by (notifier): never / immediately / at most once per hour / at most once per day (open to more but not to make it configurable (yet))

Do we have anything for/against "cronjobs" (by which I mean clock-based externally initiated tasks, not literally cron as a mechanism) -- we don't have any at present, so it does add one more task to the provisioner. But they will be cleaner to implement I think than future-scheduled jobs.

The notifiers might need some configuration of what they notify with (depending on what they can notify with -- a push-based notifier can only do a message.).

  • Message:
    "You have 12 unread followed topics total. 3 have had new posts since an hour ago (link to global unread page)
  • List:
    "You have 12 unread followed topics. Since an hour ago, you have had posts in the following 3 topics: (list of topics with links)"
  • Digest:
    "You have 12 unread followed topics total. Here are the the new posts since an hour ago (list of topics with only new posts)"

However I might just do message to begin with, as that works for everyone.

(These texts need tweaking to make localisation friendly).

thoughts welcome!

@timdiggins timdiggins self-assigned this Oct 16, 2018
@glebm
Copy link
Collaborator

glebm commented Oct 16, 2018

Do we have anything for/against "cronjobs"

A periodic job does seem like a better fit here.

We'd need to provide a method like Thredded.schedule that returns a hash in the resque-scheduler format (same as sidekiq-scheduler), and the user would have to configure it themselves (and perhaps we could auto-configure for common schedulers).

@glebm glebm reopened this Oct 16, 2018
@timdiggins
Copy link
Collaborator Author

@glebm can you use that rescue-scheduler file on heroku?
I was thinking of something as basic as rake periodic:hourly and rake periodic:daily and then leave it up to the provisioner to set up (with guides for different provisioning systems - heroku, and then the various docker orchestration tools -- kubernetes and co). But maybe we can work stepwise from a periodic.rake + docs to fully auto-configuration (where available -- heroku's scheduler doesn't have auto-configuration AFAICT).

@glebm
Copy link
Collaborator

glebm commented Oct 16, 2018

can you use that rescue-scheduler file on heroku?

Yes, it's easy on heroku, just add bin/rake resque:scheduler to Procfile (similar for sidekiq).
(aside: Resque Scheduler on Rails 5 also requires this compatibility gem btw https://github.com/JustinAiken/active_scheduler).

For the rake job, we could take the list of jobs from Thredded.schedule and call perform_now on them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants