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

Sending unseen notifications as email periodically. Includes a new us… #1074

Merged
merged 15 commits into from
Oct 11, 2021

Conversation

nickvergessen
Copy link
Member

…er settings page for notifications to configure this option.

Follow up of #1058

@nickvergessen
Copy link
Member Author

No limit on number of notifications in the email

I would limit it to 25 (same amount the web and the client pull). More is just overwelming and doesn't help you to reduce it.

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.

I'm going to fix that. Otherwise as an employee using Nextcloud for work, I would get way to many emails although the period is set to daily or weekly.

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.

Never should definetly be done. ASAP and hourly I really would remove. This is meant to be a fallback, not a live thing and I already see people complaining that although they picked asap, it took more than 10 mins to send the notification (which doesnt help e.g. with a missed call/chat message)

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.

Fix it by just updating the marker is fine. When you remove the email address pending activity mails are also just killed. you can still get them via web or your desktop client. totally fine.


<settings>
<personal>OCA\Notifications\Settings\Personal</personal>
<personal-section>OCA\Notifications\Settings\PersonalSection</personal-section>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should move the setting into the activities section. Should be possible to reuse the same section as it's done with other apps. Currently it would spam a warning, but a simple patch to server would fix that.

@jbarnoin
Copy link
Contributor

jbarnoin commented Sep 2, 2021

I'm going to fix that. Otherwise as an employee using Nextcloud for work, I would get way to many emails although the period is set to daily or weekly.

No problem, though just to be clear, you will never get email any more often than the selected period.
What this changes is mainly that if you haven't got a notification in a while and suddenly you get one, as it stands you will likely get a reminder email unless you view the notification within 15 min. If however we wait for the notification to reach the period's age, you won't get any email for this if you check your notifications at least once a day. So it makes sense to do that so we don't send that first email to someone who does check their notifications every day or so, but whatever happens you wouldn't get another email before the period is passed even in the current version.

@nickvergessen
Copy link
Member Author

After some discussions and seeing this will mostly be about chat notifications there was the request to have a 2-4 hour setting also picked as default. ASAP and hourly might be too much, but daily means you will not notice it until the day after.

@nickvergessen
Copy link
Member Author

nickvergessen commented Sep 30, 2021

Todo:

  • We need to create the entry for all users on update with last_send_id set to their current last notification if they have any
  • We need to create the entry for all new users on login or something alike
  • Heal in case of a lost entry (done when showing/saving settings, but might need it more generic)

Copy link

@vitormattos vitormattos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On apps is necessary add /js folder to versioning? I believe is for build files.

@nickvergessen
Copy link
Member Author

On apps is necessary add /js folder to versioning? I believe is for build files.

Yes for shipped apps the compiled js/ needs to be in the repository.

jbarnoin and others added 6 commits October 11, 2021 16:03
…er settings page for notifications to configure this option.

Signed-off-by: Julien Barnoin <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants