From 7ba115769a9e33dbed4136650d8606900357ba69 Mon Sep 17 00:00:00 2001 From: Selase Krakani <849886+s3lase@users.noreply.github.com> Date: Mon, 19 Dec 2022 20:17:40 +0000 Subject: [PATCH] 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 Co-authored-by: Bianca Nenciu --- app/jobs/regular/send_push_notification.rb | 4 ++- app/services/post_alerter.rb | 11 ++++++- config/locales/server.en.yml | 1 + config/site_settings.yml | 3 ++ .../push_subscription_fabricator.rb | 6 ++++ spec/jobs/send_push_notification_spec.rb | 32 +++++++++++++++++++ spec/services/post_alerter_spec.rb | 21 ++++++++++++ 7 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 spec/fabricators/push_subscription_fabricator.rb create mode 100644 spec/jobs/send_push_notification_spec.rb diff --git a/app/jobs/regular/send_push_notification.rb b/app/jobs/regular/send_push_notification.rb index 4fbd7c761b0..057e32dafb0 100644 --- a/app/jobs/regular/send_push_notification.rb +++ b/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 diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 9b9545ea0c3..c20ed382f0d 100644 --- a/app/services/post_alerter.rb +++ b/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? diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 69eaaf5d5eb..c4eceddaf30 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/config/site_settings.yml b/config/site_settings.yml index ba500824f75..be88c877d9c 100644 --- a/config/site_settings.yml +++ b/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 diff --git a/spec/fabricators/push_subscription_fabricator.rb b/spec/fabricators/push_subscription_fabricator.rb new file mode 100644 index 00000000000..89c5a4e99e1 --- /dev/null +++ b/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 diff --git a/spec/jobs/send_push_notification_spec.rb b/spec/jobs/send_push_notification_spec.rb new file mode 100644 index 00000000000..3ec6de32140 --- /dev/null +++ b/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 diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index ca484b6efca..0369f0a3dae 100644 --- a/spec/services/post_alerter_spec.rb +++ b/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