FEATURE: add delay to native push notifications (#28314)

This change ensures native push notifications respect the site setting for push_notification_time_window_mins. Previously only web push notifications would account for the delay, now we can bring more consistency between Discourse in browser vs Hub, by applying the same delay strategy to both forms of push notifications.

app/jobs/regular/push_notification.rb

@@ -3,6 +3,10 @@
 module Jobs
   class PushNotification < ::Jobs::Base
     def execute(args)
+      user = User.find_by(id: args["user_id"])
+      push_window = SiteSetting.push_notification_time_window_mins
+      return if !user || (push_window > 0 && user.seen_since?(push_window.minutes.ago))
+
       notification = args["payload"]
       notification["url"] = UrlHelper.absolute_without_cdn(
         Discourse.base_path + notification["post_url"],
@@ -24,21 +28,25 @@ module Jobs
 
           next if push_url.blank?
 
-          result =
-            Excon.post(
-              push_url,
-              body: payload.merge(notifications: notifications).to_json,
-              headers: {
-                "Content-Type" => "application/json",
-                "Accept" => "application/json",
-              },
-            )
+          begin
+            result =
+              Excon.post(
+                push_url,
+                body: payload.merge(notifications: notifications).to_json,
+                headers: {
+                  "Content-Type" => "application/json",
+                  "Accept" => "application/json",
+                },
+              )
 
-          if result.status != 200
-            # we failed to push a notification ... log it
-            Rails.logger.warn(
-              "Failed to push a notification to #{push_url} Status: #{result.status}: #{result.status_line}",
-            )
+            if result.status != 200
+              # we failed to push a notification ... log it
+              Rails.logger.warn(
+                "Failed to push a notification to #{push_url} Status: #{result.status}: #{result.status_line}",
+              )
+            end
+          rescue => e
+            Rails.logger.error("An error occurred while pushing a notification: #{e.message}")
           end
         end
     end

app/services/post_alerter.rb

@@ -83,11 +83,13 @@ class PostAlerter
       return
     end
 
+    push_window = SiteSetting.push_notification_time_window_mins
+    if push_window > 0 && user.seen_since?(push_window.minutes.ago)
+      delay = (push_window - (Time.now - user.last_seen_at) / 60)
+    end
+
     if user.push_subscriptions.exists?
-      push_window = SiteSetting.push_notification_time_window_mins
-      if push_window > 0 && user.seen_since?(push_window.minutes.ago)
-        delay =
-          (SiteSetting.push_notification_time_window_mins - (Time.now - user.last_seen_at) / 60)
+      if delay.present?
         Jobs.enqueue_in(delay.minutes, :send_push_notification, user_id: user.id, payload: payload)
       else
         Jobs.enqueue(:send_push_notification, user_id: user.id, payload: payload)
@@ -107,7 +109,17 @@ class PostAlerter
           .order(client_id: :asc)
           .pluck(:client_id, :push_url)
 
-      if clients.length > 0
+      return if clients.length == 0
+
+      if delay.present?
+        Jobs.enqueue_in(
+          delay.minutes,
+          :push_notification,
+          clients: clients,
+          payload: payload,
+          user_id: user.id,
+        )
+      else
         Jobs.enqueue(:push_notification, clients: clients, payload: payload, user_id: user.id)
       end
     end

spec/jobs/push_notification_spec.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require "excon"
+
+RSpec.describe Jobs::PushNotification do
+  fab!(:user)
+  fab!(:post)
+  let(:data) do
+    {
+      "user_id" => user.id,
+      "payload" => {
+        "notification_type" => 1,
+        "post_url" => "/t/#{post.topic_id}/#{post.post_number}",
+        "excerpt" => "Hello you",
+      },
+      "clients" => [[user.id, "http://test.localhost"]],
+    }
+  end
+
+  before { SiteSetting.push_notification_time_window_mins = 5 }
+
+  context "with valid user" do
+    it "does not send push notification when user is online" do
+      user.update!(last_seen_at: 1.minute.ago)
+
+      Excon.expects(:post).never
+
+      Jobs::PushNotification.new.execute(data)
+    end
+
+    it "sends push notification when user is offline" do
+      user.update!(last_seen_at: 10.minutes.ago)
+
+      Excon.expects(:post).once
+
+      Jobs::PushNotification.new.execute(data)
+    end
+  end
+
+  context "with invalid user" do
+    it "does not send push notification" do
+      data["user_id"] = -999
+
+      Excon.expects(:post).never
+
+      Jobs::PushNotification.new.execute(data)
+    end
+  end
+end

spec/services/post_alerter_spec.rb

@@ -1359,6 +1359,15 @@ RSpec.describe PostAlerter do
       )
     end
 
+    it "delays push notification for active online user" do
+      evil_trout.update!(last_seen_at: 5.minutes.ago)
+
+      expect { mention_post }.to change { Jobs::PushNotification.jobs.count }
+      expect(Jobs::PushNotification.jobs[0]["at"]).to be_within(30.second).of(
+        5.minutes.from_now.to_f,
+      )
+    end
+
     context "with push subscriptions" do
       before do
         Fabricate(:push_subscription, user: evil_trout)