DEV: Skip push notifications for active online users (#19502)

* DEV: Skip push notifications for active online users

Currently, users with active push subscriptions get push notifications
regardless of their "presence" on the site.

This change introduces a `push_notification_time_window_mins`
site setting which is used in conjunction with a user's `last_seen_at` to
determine if push notifications should be sent. A user is considered to
be actively online if their `last_seen_at` is within `push_notification_time_window_mins`
minutes. `push_notification_time_window_mins` is set to 10 by default.

* DEV: Remove client param for push_notification_time_window_mins site setting

Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

app/jobs/regular/send_push_notification.rb

@@ -4,7 +4,9 @@ module Jobs
   class SendPushNotification < ::Jobs::Base
     def execute(args)
       user = User.find_by(id: args[:user_id])
-      PushNotificationPusher.push(user, args[:payload]) if user
+      return if !user || user.seen_since?(SiteSetting.push_notification_time_window_mins.minutes.ago)
+
+      PushNotificationPusher.push(user, args[:payload])
     end
   end
 end

app/services/post_alerter.rb

@@ -46,7 +46,16 @@ class PostAlerter
     end
 
     if user.push_subscriptions.exists?
-      Jobs.enqueue(:send_push_notification, user_id: user.id, payload: payload)
+      if user.seen_since?(SiteSetting.push_notification_time_window_mins.minutes.ago)
+        Jobs.enqueue_in(
+          SiteSetting.push_notification_time_window_mins.minutes,
+          :send_push_notification,
+          user_id: user.id,
+          payload: payload
+        )
+      else
+        Jobs.enqueue(:send_push_notification, user_id: user.id, payload: payload)
+      end
     end
 
     if SiteSetting.allow_user_api_key_scopes.split("|").include?("push") && SiteSetting.allowed_user_api_push_urls.present?

config/locales/server.en.yml

@@ -2357,6 +2357,7 @@ en:
     push_notifications_prompt: "Display user consent prompt."
     push_notifications_icon: "The badge icon that appears in the notification corner. A 96×96 monochromatic PNG with transparency is recommended."
     enable_desktop_push_notifications: "Enable desktop push notifications"
+    push_notification_time_window_mins: "Wait (n) minutes before sending push notification. Helps prevent push notifications from being sent to an active online user."
 
     base_font: "Base font to use for most text on the site. Themes can override via the `--font-family` CSS custom property."
     heading_font: "Font to use for headings on the site. Themes can override via the `--heading-font-family` CSS custom property."

config/site_settings.yml

@@ -348,6 +348,9 @@ basic:
   enable_desktop_push_notifications:
     default: true
     client: true
+  push_notification_time_window_mins:
+    default: 10
+    min: 0
   short_title:
     default: ""
     max: 12

spec/fabricators/push_subscription_fabricator.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+Fabricator(:push_subscription) do
+  user
+  data '{"endpoint": "https://example.com/send","keys": {"p256dh": "BJpN7S_sh_RX5atymPB7J1","auth": "5M-xiXhbcFhkkw3YE7uIK"}}'
+end

spec/jobs/send_push_notification_spec.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::SendPushNotification do
+  fab!(:user) { Fabricate(:user) }
+  fab!(:subscription) { Fabricate(:push_subscription) }
+  let(:payload) { { notification_type: 1, excerpt: "Hello you" } }
+
+  before do
+    freeze_time
+    SiteSetting.push_notification_time_window_mins = 10
+  end
+
+  context "with active online user" do
+    it "does not send push notification" do
+      user.update!(last_seen_at: 5.minutes.ago)
+
+      PushNotificationPusher.expects(:push).with(user, payload).never
+
+      Jobs::SendPushNotification.new.execute(user_id: user, payload: payload)
+    end
+  end
+
+  context "with inactive offline user" do
+    it "sends push notification" do
+      user.update!(last_seen_at: 40.minutes.ago)
+
+      PushNotificationPusher.expects(:push).with(user, payload)
+
+      Jobs::SendPushNotification.new.execute(user_id: user, payload: payload)
+    end
+  end
+end

spec/services/post_alerter_spec.rb

@@ -1102,6 +1102,27 @@ RSpec.describe PostAlerter do
       expect(JSON.parse(body)).to eq(payload)
 
     end
+
+    context "with push subscriptions" do
+      before do
+        Fabricate(:push_subscription, user: evil_trout)
+        SiteSetting.push_notification_time_window_mins = 10
+      end
+
+      it "delays sending push notification for active online user" do
+        evil_trout.update!(last_seen_at: 5.minutes.ago)
+
+        expect { mention_post }.to change { Jobs::SendPushNotification.jobs.count }
+        expect(Jobs::SendPushNotification.jobs[0]["at"]).not_to be_nil
+      end
+
+      it "does not delay push notification for inactive offline user" do
+        evil_trout.update!(last_seen_at: 40.minutes.ago)
+
+        expect { mention_post }.to change { Jobs::SendPushNotification.jobs.count }
+        expect(Jobs::SendPushNotification.jobs[0]["at"]).to be_nil
+      end
+    end
   end
 
   describe ".create_notification_alert" do