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