Push notification time window mins can be double that time

AI-generated summary

Push Notification Time Window Issue

RJG discovered an issue with the push notification delay on iOS PWA, where the actual delay can be up to twice the set push_notification_time_window_mins. The code checks if a user was online recently, and if so, delays the notification for the set time window. However, this delay is applied in addition to the time it takes to check if the user was online, resulting in a longer delay than expected.

RJG suggests that the delay should be calculated as push_notification_time_window_mins - (now - seen_since), which would fix the issue. Falco has created a pull request to address this issue.

Since we now FINALLY have push notifications on iOS PWA :tada: I was looking into the delay that is applied before a push notification is being sent - because it seemed to take longer than expected.

The code checks if a user was online recently (less than push_notification_time_window_mins ago) and if they were then the notification is delayed for push_notification_time_window_mins

      if user.seen_since?(SiteSetting.push_notification_time_window_mins.minutes.ago)
        Jobs.enqueue_in(
          SiteSetting.push_notification_time_window_mins.minutes,
          ...
        )
      else
        Jobs.enqueue(:send_push_notification, user_id: user.id, payload: payload)
      end

But this means that in practice this can take up to as twice as long as expected since the push_notification_time_window_mins is applied both in the online check as in the delay.

Example:

push_notification_time_window_mins is set to 10

13:00
I leave the site
13:10
Something happens that triggers a notification.
Code checks whether I was online less than 10 minutes ago.
I was, so the notification will be sent at 13:20.

12:59
I leave the site
13:10
Something happens that triggers a notification.
Code checks whether I was online less than 10 minutes ago.
I was not, so the notification will be sent immediately, at 13:10.

So one minute difference in my online time can change the notification delivery with almost push_notification_time_window_mins. Especially if the setting is changed into a higher value this can make a significant difference.

I would expect the delay to be push_notification_time_window_mins - (now - seen_since)

8 Likes

This should fix it

https://github.com/discourse/discourse/pull/20864

9 Likes

This topic was automatically closed after 3 days. New replies are no longer allowed.