From 0954ae70a690494eb0102c0e3ba821b975fd38dd Mon Sep 17 00:00:00 2001 From: David Battersby Date: Tue, 13 Aug 2024 12:12:05 +0400 Subject: [PATCH] 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 | 36 ++++++++++++-------- app/services/post_alerter.rb | 22 +++++++++--- spec/jobs/push_notification_spec.rb | 49 +++++++++++++++++++++++++++ spec/services/post_alerter_spec.rb | 9 +++++ 4 files changed, 97 insertions(+), 19 deletions(-) create mode 100644 spec/jobs/push_notification_spec.rb diff --git a/app/jobs/regular/push_notification.rb b/app/jobs/regular/push_notification.rb index c3d514e9df7..8db0c47f345 100644 --- a/app/jobs/regular/push_notification.rb +++ b/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 diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 18b401a0e5a..d74ff37005e 100644 --- a/app/services/post_alerter.rb +++ b/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 diff --git a/spec/jobs/push_notification_spec.rb b/spec/jobs/push_notification_spec.rb new file mode 100644 index 00000000000..e081fd677c5 --- /dev/null +++ b/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 diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index d858813d9f7..8651b27adc7 100644 --- a/spec/services/post_alerter_spec.rb +++ b/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)