Skip to content

[NEW] Better Push and Email Notification logic#17357

Merged
sampaiodiego merged 46 commits intodevelopfrom
notification-logic
Apr 21, 2020
Merged

[NEW] Better Push and Email Notification logic#17357
sampaiodiego merged 46 commits intodevelopfrom
notification-logic

Conversation

@rodrigok
Copy link
Copy Markdown
Member

@rodrigok rodrigok commented Apr 18, 2020 •
edited by milton-rucks
Loading

Changelog Description

We are still using the same logic to define which notifications every new message will generate, it takes some servers' settings, users's preferences and subscriptions' settings in consideration to determine who will receive each notification type (desktop, audio, email and mobile push), but now it doesn't check the user's status (online, away, offline) for email and mobile push notifications but send those notifications to a new queue with the following rules:

  • When the user is online the notification is scheduled to be sent in 120 seconds
  • When the user is away the notification is scheduled to be sent in 120 seconds minus the amount of time he is away
  • When the user is offline the notification is scheduled to be sent right away
  • When the user reads a channel all the notifications for that user are removed (clear queue)
  • When a notification is processed to be sent to a user and there are other scheduled notifications:
    • All the scheduled notifications for that user are rescheduled to now
    • The current notification goes back to the queue to be processed ordered by creation date

Technical info

maxScheduleDelaySeconds = 120

  • Create a description for this PR
  • Merge base PR and change the target merge branch here to develop
  • Make cyclePause, maxBatchSize, and maxScheduleDelaySeconds configurable
  • if connection online always schedule for maxScheduleDelaySeconds time
  • if connection away schedule for min(maxScheduleDelaySeconds - (now - lastUpdatedAt), 0)
  • if connection offline schedule for 0
  • on a notification remove all schedules of other user's notification to flush the queue
  • If user read a room, clear all the scheduled notifications for the user
  • Create indexes
  • Remove the push queue
  • TTL to remove old records (2h maybe?)
  • Migration to remove Notifications_Always_Notify_Mobile and Push_Debug
  • Migrate and drop the _raix_push_notifications collection
    • Migrate query.userId to userId

Future

  • We may improve to create a tick to set user's activity later (from the client) and use this field to calculate the schedule and to clear the queue
  • Set push tokens inside the session token

@rodrigok rodrigok added this to the 3.2.0 milestone Apr 18, 2020
@rodrigok rodrigok requested a review from sampaiodiego April 18, 2020 19:50
@rodrigok rodrigok changed the base branch from develop to push April 18, 2020 19:51
@sampaiodiego sampaiodiego merged commit 8084de8 into develop Apr 21, 2020
@sampaiodiego sampaiodiego deleted the notification-logic branch April 21, 2020 06:32
gabriellsh added a commit that referenced this pull request Apr 22, 2020
@gabriellsh
Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into adm/…
750b050 
…users_and_rooms

* 'develop' of github.com:RocketChat/Rocket.Chat: (29 commits)
  [FIX] 2FA not showing codes for Spanish translation (#17378)
  [NEW] [ENTERPRISE] Restrict the permissions configuration for guest users  (#17333)
  [NEW] Federation event for when users left rooms (#17091)
  [FIX] CSV Importer fails when there are no users to import (#16790)
  Import slack's mpims as direct rooms instead of private groups (#17206)
  [FIX] SAML Idp Initiated Logout Error (#17324)
  [NEW] Better Push and Email Notification logic (#17357)
  [NEW] Error page when browser is not supported (#17372)
  [NEW] [ENTERPRISE] Omnichannel queue priorities (#17141)
  [IMPROVE] Change the SAML metadata order to conform to XSD specification (#15488)
  [IMPROVE] Filter markdown in notifications (#9995)
  [IMPROVE] User gets UI feedback when message is pinned or unpinned (#16056)
  Remove set as alias setting (#16343)
  [IMPROVE] Add `file-title` and `file-desc` as new filter tag options on message search (#16858)
  [NEW]  Add ability to set tags in the Omnichannel room closing dialog (#17254)
  [FIX] Show active admin and user account menu item (#17047)
  [NEW] [ENTERPRISE] Allows to set a group of departments accepted for forwarding chats (#17335)
  [FIX] Prevent user from getting stuck on login, if there is some bad fname (#17331)
  [FIX] Remove properties from users.info response (#17238)
  Bump version to 3.1.1
  ...
@ankar84
Copy link
Copy Markdown

ankar84 commented Apr 22, 2020

I think that PR could be a great fix for
#16867
#16765
@rodrigok @sampaiodiego thank you guys!
That push notification algorithm is much straightforward and I believe convenient to rocket chat users.
Thanks a lot again!

@sampaiodiego
Copy link
Copy Markdown
Member

sampaiodiego commented Apr 22, 2020

thanks @ankar84 .. I believe both issues can be closed now, what do you think? even the preference Always notify mobile was removed.

This was referenced Apr 22, 2020
Closed
Closed
This was referenced Apr 23, 2020
Closed
Closed
@ankar84
Copy link
Copy Markdown

ankar84 commented Apr 23, 2020

I believe both issues can be closed now, what do you think?

Sure! They both closed now.
Thanks again!

@sampaiodiego sampaiodiego mentioned this pull request Apr 27, 2020
Merged
@hogthrob hogthrob mentioned this pull request May 11, 2020
Closed
@jazztickets jazztickets mentioned this pull request May 17, 2020
Merged
12 tasks
@sampaiodiego sampaiodiego mentioned this pull request Jun 2, 2020
Closed
@sampaiodiego sampaiodiego mentioned this pull request Jun 13, 2020
Merged
12 tasks
@sampaiodiego sampaiodiego mentioned this pull request Jun 21, 2020
Closed
@ceefour
Copy link
Copy Markdown
Contributor

ceefour commented Sep 29, 2020 •
edited
Loading

@rodrigok @sampaiodiego I'd like to confirm behavior or following scenario:

  1. App is online/foreground
  2. Server has a posted message, and since user is online according to server, meaning push is scheduled in 120 seconds
  3. Before message is received by app, and before 120 seconds timed out, app is now offline and server knows the status is now offline (perhaps away also applies here)

Since the status is now (known to be) offline, the best user experience means that push needs to be sent immediately, without waiting for the scheduled 120 seconds.

Is that already the current behavior?

My observation seems to imply that, even now the user is known to be offline, all scheduled messages will still be waiting until 120 seconds. If this is the current (undesirable) behavior, then I'll create a new ticket for this.

This was referenced Oct 1, 2020
Open
Open
Merged
This was referenced Dec 18, 2020
Closed
Closed
@ankar84 ankar84 mentioned this pull request Mar 3, 2021
Open
@dosmanak dosmanak mentioned this pull request May 3, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sampaiodiego sampaiodiego sampaiodiego requested changes

Assignees

No one assigned

Labels

Invalid PR Title

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

6 participants

@rodrigok @ankar84 @sampaiodiego @ceefour @ggazzo @milton-rucks