Sending unseen notifications as email periodically.

[jbarnoin]

This change adds support for sending users an email about their unseen notifications. This should close #314 .
It's my first contribution, I did my best to respect guidelines but I'll adjust whatever I might have missed of course.

A new Notifications personal settings page allows enabling the feature and selecting the maximum mailing frequency.
A new background job checks for new emails due to be sent.
It also does an instant check at the moment a notification is sent, and sends the email right away if the user has the "as soon as possible" frequency set (but not more than once per background job cycle to avoid excessive spamming). Longer frequencies are sent at the next background job period, but not more often than the desired period.

The email format can probably be improved, but here's how it looks like at the moment:

• I'm absuing the ability to include buttons in the email format to represent the possible response actions to the notification. It works, but they're clearly not intended for this so the spacing looks terrible. Any thoughts on how to improve?
• The emails include the full text of notification messages received, which includes the contents of chat messages etc. I think it's very useful, but maybe that's a privacy issue to have included? Could be a setting users can turn on?
• There's no limit right now to the maximum number of notifications included in a message. Maybe there should be one with some text that says "and x more", but I haven't done that yet.

The notification settings page is very simple for now. I don't think there's categories we can filter like for Activities?

The naming of things when comparing the user Activity settings versus Notifications settings pages is confusing, since the Activity settings page has a heading that says "Notifications" and a setting labeled "Send notification emails". I'm not sure how to clarify this, but I imagine it would involve changes to the Activity settings, so I can't do anything about it here.

There's currently no admin settings page. There probably should be one. The Activities app allows admins to allow the email notifications feature or not, I'm guessing this would be good to have here too. I am reading the notifications app setting 'enable_email' with a default to 'yes', in the same way the Activities app does it, but there's currently no interface to disable it.

[nickvergessen]

This change adds support for sending users an email about their unseen notifications. This should close #314 .
It's my first contribution, I did my best to respect guidelines but I'll adjust whatever I might have missed of course.

Sound like a good idea, but you picked a tough topic :D

I'm absuing the ability to include buttons in the email format to represent the possible response actions to the notification. It works, but they're clearly not intended for this so the spacing looks terrible. Any thoughts on how to improve?

As mentioned inline, it will only work for actions with the request type "WEB". For other actions a button could be shown which just opens the web so the user could interact with the action buttons there.

The emails include the full text of notification messages received, which includes the contents of chat messages etc. I think it's very useful, but maybe that's a privacy issue to have included? Could be a setting users can turn on?

No setting, either we do it or we don't

There's no limit right now to the maximum number of notifications included in a message. Maybe there should be one with some text that says "and x more", but I haven't done that yet.

yeah not sure either. We can not sent unlimited length, so I guess a limit of 50-100 makes sense and then say X-more

The notification settings page is very simple for now. I don't think there's categories we can filter like for Activities?

good enough

The naming of things when comparing the user Activity settings versus Notifications settings pages is confusing, since the Activity settings page has a heading that says "Notifications" and a setting labeled "Send notification emails". I'm not sure how to clarify this, but I imagine it would involve changes to the Activity settings, so I can't do anything about it here.

yeah, I also see the confusion happening. The question is if we even want to go as far. I could also imagine having only a single check box for this which is "Remind me with an email about notifications which are older than 7 days" and then only send the mail in that case.

In the future a second setting is planned (whether or not to play a sound), and for that I would have liked the setting to be inside the popover somehow.

There's currently no admin settings page. There probably should be one. The Activities app allows admins to allow the email notifications feature or not, I'm guessing this would be good to have here too. I am reading the notifications app setting 'enable_email' with a default to 'yes', in the same way the Activities app does it, but there's currently no interface to disable it.

I don't think we need a admin default setting option for this one.

[jbarnoin]

Sound like a good idea, but you picked a tough topic :D

I realize that, haha. I picked what was most needed to me at the moment. I appreciate the detailed notes and will try to address them soon.

yeah, I also see the confusion happening. The question is if we even want to go as far. I could also imagine having only a single check box for this which is "Remind me with an email about notifications which are older than 7 days" and then only send the mail in that case.

Do you mean that you don't think it's necessary to adjust frequency? It seems quite useful to me to be able to adjust how many email you like to get vs. how timely your notifications are.
If I have my mail client open, but not the NextCloud page open in my browser and someone sends me a chat message, I expect to get an email immediately so that I can reply to them, so I always have "as soon as possible" set. Other people might not like getting the email so often, but I don't mind. Delaying them every 15 min when the background job is running next would be fine, but delaying 7 days is too long to respond to someone wanting to chat with me.
In the end I don't even get that many email either, because I'll get the first email, click on that, and then I have the chat page open for a while so new messages won't trigger email every time.

[nickvergessen]

Maybe this is a bit because I see the other use side. I receive tons of notifications and with instant emails we can't even group them anymore even when it's within 5 seconds. Which is why I would only do a reminder on old notifications, not live emails.

[jbarnoin]

Well, I've expressed my personal preference and I like having precise control over such settings, but I won't go against your judgement if you feel reminder email once a week are enough. It's not exactly what I'd like but is a lot better than no email ever.
One thing to note is that with this implementation, even with the 1 week setting, you will get the first notification email within 15 minutes of a notification coming in (grouped if you got more than 1 within the 15 min), but then it won't send another until a week later which will contain all notifications from that week grouped.

This is good in the sense that if you forgot the instance was there for a long time and someone suddenly wants to talk, even with the 1 week setting, you'll get a timely reminder. Hopefully after that message you have a good chance to keep checking the instance for the next week.

On the other hand it might be a bit weird that if you get many notifications every week, the timing of the email you're getting every week will be based on the arbitrary time and day you got the first notification, not based on something predictable like for the "Send daily activity summary in the morning" setting. It might not be that bad though.

In the end since you wanted to have the notification sound setting elsewhere, are you saying you'd rather not have a settings page for Notifications at all? From a user's perspective, I would expect these to be somewhere in settings, but I would expect it to be on the same page as the current Activity settings if it could be renamed to something more general. However they're 2 different apps so I don't know if they can share a settings page with separate sections and still work if one or the other is disabled.

[jbarnoin]

Another approach would be "send me an email only when I have a notification that I have not seen after a full week".
This would minimize the amount of email sent, but also means the notification would be quite outdated by the time you get it, which for my purpose (ensuring users of my instance can participate in the chat when it happens) is not good enough and I'd have to keep a local patch for my instance to address it.

[szaimen]

Personally, I'd vote for a reminder mail once a day. cc @jancborchardt and @nimisha-vijay on this

[nickvergessen]

So from my POV those are 2 different things @jbarnoin
One topic is getting notifications as emails live (which is what I think we shouldn't do) and the other thing is to have a reminder when you didn't check back on notifications which is where I think a daily/weekly reminder like "you still have X notifications that require attention" is good enough. Combining both will make either of them not as useful as they would be individually.

But let's see what the designers say about it.

[nimishavijay]

Agreed with @nickvergessen that immediate notifications by email might be a bit much, since this is regarding just a reminder or nudge about unread notifications, not live email notifications itself. So maybe something like this in the settings can work

Remind me about unread notifications: Daily / Weekly / Never

• If it's set to weekly, then in the email could there be some separation based on time? (1 hour ago, 3 days ago, etc). I don't think it's necessary for daily as there is already a timestamp which covers it
• We can show notifications that are up to maybe 1 month old, older than that would be unnecessary

What do you think?
cc @jancborchardt also

[jbarnoin]

Agreed with @nickvergessen that immediate notifications by email might be a bit much, since this is regarding just a reminder or nudge about unread notifications, not live email notifications itself. So maybe something like this in the settings can work

Remind me about unread notifications: Daily / Weekly / Never

* If it's set to weekly, then in the email could there be some separation based on time? (1 hour ago, 3 days ago, etc). I don't think it's necessary for daily as there is already a timestamp which covers it

Yeah, I understand the point about not mixing live notifications with wanting reminders for people who forget about the instance. I liked the idea of getting the benefit of both but the important thing is reminders for me as well.

If there's options shorter than one week, such as daily, I'd be fine with the reminder being setup so that it's only sent once you have a notification that has reached the age of the period, I just feel always waiting a week is a bit much.

However, I would add that if we're already offering options for Daily / Weekly / Never, I don't see the harm in adding Hourly / As Soon As Possible as well since that's clearly the user's choice at this point, it would not be a default for anyone, and it's clear to the user what they'll get if they select it (and the email includes a link to the page to change the frequency if they get tired of it). It's just a number in the system at that point so there's no extra implementation cost either - I was going to remove the immediate path in any case as requested, so it's all the same code path triggered when the background job runs, every 15 min at the moment.

The only potential drawback I can see is in terms of performance concerns if too many people on a server enable "As soon as possible" notifications, but I would argue that if so many people volontarily enable this that it becomes a performance issue, you'll have a clear signal that there's enough demand to spend time optimizing it.

* We can show notifications that are up to maybe 1 month old, older than that would be unnecessary

To be clear though, at the moment I don't ever re-send an email for an unseen notificaiton if it's been sent already. If we want that it can be done easily, but I assumed getting one email per notification would be enough. For update notifications, if you don't act on it for a while you'll probably get another one for a new update at some point anyway.

[jbarnoin]

One more thought: Even if we default to Never for everyone else, could it make sense to default to at least weekly notifications for admins? Setting up a server, forgetting about it after a while and leaving it live never updating it could be a security risk that this could mitigate at least a bit.

[nickvergessen]

One more thought: Even if we default to Never for everyone else, could it make sense to default to at least weekly notifications for admins? Setting up a server, forgetting about it after a while and leaving it live never updating it could be a security risk that this could mitigate at least a bit.

This is nothing that should be done via the notification app from my POV. It's a valid case, but can totally come with an out of line solution: there is another ticket about sending regular emails about pending updates (nextcloud/server#3596) and even about disabling outdated servers (nextcloud/server#14230).

[jbarnoin]

This is nothing that should be done via the notification app from my POV. It's a valid case, but can totally come with an out of line solution: there is another ticket about sending regular emails about pending updates (nextcloud/server#3596) and even about disabling outdated servers (nextcloud/server#14230).

Got it. At least until that system is up there'll be a way for admins to get these notifications if they enable it, which will cover at least some of them, so I consider that's still progress in the right direction.

By the way, it sounds like there's going to be a number of separate background jobs for sending email doing similar checks and actions, at some point this might call for unifying this under a common background email service that apps can plug into.
I'm not offering to do this myself right now, but it's just a thought as I think we're repeating code and settings with slight variations (activities, calendars, updates, deck maybe, etc...), and at the moment it's quite hard to setup such a service correctly for your app, at least from the perspective of a newcomer like me.

[jbarnoin]

I pushed a revised commit with the following main changes:

• Moved settings page to Vue instead of jQuery, as well as using initialStateService
• Made the SettingsController an OCSController
• Removed live notifications path and all related code
• Addressed other small issues mentioned in the review (php strict declares, return types, etc.)

This should be ready for another round of review.

[jbarnoin]

The eslint / php / unit test validation errors should be fixed.
I don't know if I've correctly fixed the node workflow validation which was showing diffs in notifications-userSettings.js* though. I removed my local node_modules and ran the build again, got a different result so I committed that. I was assuming package-lock.json would make sure node_modules was always identical but I guess I was wrong.

However I also fetched latest from master branch and now I'm getting differences in js/notifications-main.js* which I haven't touched, so I haven't committed these changes as it sounds fishy.
@nickvergessen any hint on the right way to validate this? Am I supposed to use some standardized environment rather than my local machine's npm & node versions to get things correct or something?

There was also one about appinfo/info.xml not liking the "<commands>" section, but I didn't add that, so not sure why this was flagged either if it was accepted previously.
Edit: seems like this last one was due to the order not matching what was expected, should be fixed now if I got this right.

[nickvergessen]

However I also fetched latest from master branch and now I'm getting differences in js/notifications-main.js* which I haven't touched, so I haven't committed these changes as it sounds fishy.

Ignore that for now, we can fix it in the end.

[nickvergessen]

Should fix that instead, but we can take care of it

[jbarnoin]

Yeah, I wasn't sure if I should deviate from the naming conventions / capitalization that I saw from the code in another app I used as an example so I copied that as well, I agree it's not great.

[jbarnoin]

So apart from the node giving me a different output, this is now passing validation.
For completeness though I'd like to point out some remaining limitations that we may have mentioned but aren't addressed yet, let me know if you think they should be addressed in a subsequent PR or I should amend this

• No limit on number of notifications in the email
• The first email for a user is still sent the next time a background job runs (so within 15 min), and then again after each selected period as long as there's more notifications. There was discussion of delaying the first email until the first notification was as old as the email period, or maybe some smaller age but not immediate, but I wasn't sure if there was a final conclusion so I didn't implement yet.
• There's still both a checkbox to enable and a list of periods in the settings, this could potentially be clearer if the checkbox was replaced by adding a choice of "Never" to the list of periods. Also I kept the As Soon As Possible and Hourly options as I wasn't sure if there was consensus on restricting these periods.
• I also think there might be a wasteful edge case if a user removes their email address after enabling notifications or if the email delivery otherwise fails. In that case the notifications would get prepared again for this user at every job cycle but the notifications email never marked as delivered. Not sure how bad this is but I thought I'd mention it.

[nickvergessen]

Hi @jbarnoin for easier collaboration I added you to the Nextcloud organisation. Once you accepted the invite, you can push to the PR #1074