diff --git a/app/models/notification/push_target.rb b/app/models/notification/push_target.rb index da6c26fdc..1fc250259 100644 --- a/app/models/notification/push_target.rb +++ b/app/models/notification/push_target.rb @@ -12,17 +12,6 @@ class Notification::PushTarget end def process - return unless should_push? - - perform_push + raise NotImplementedError end - - private - def should_push? - notification.pushable? - end - - def perform_push - raise NotImplementedError - end end diff --git a/app/models/notification/push_target/web.rb b/app/models/notification/push_target/web.rb index 68c971d8f..9bf839906 100644 --- a/app/models/notification/push_target/web.rb +++ b/app/models/notification/push_target/web.rb @@ -1,13 +1,11 @@ class Notification::PushTarget::Web < Notification::PushTarget - private - def should_push? - super && subscriptions.any? - end - - def perform_push + def process + if subscriptions.any? Rails.configuration.x.web_push_pool.queue(notification.payload.to_h, subscriptions) end + end + private def subscriptions @subscriptions ||= notification.user.push_subscriptions end diff --git a/app/models/notification/pushable.rb b/app/models/notification/pushable.rb index 0f5ae2ed6..4d5d14d0d 100644 --- a/app/models/notification/pushable.rb +++ b/app/models/notification/pushable.rb @@ -29,11 +29,9 @@ module Notification::Pushable end def push - self.class.push_targets.each { |target| push_to(target) } - end + return unless pushable? - def pushable? - !creator.system? && user.active? && account.active? + self.class.push_targets.each { |target| push_to(target) } end def payload @@ -41,6 +39,10 @@ module Notification::Pushable end private + def pushable? + !creator.system? && user.active? && account.active? + end + def push_to(target) target.process(self) end diff --git a/saas/app/models/notification/push_target/native.rb b/saas/app/models/notification/push_target/native.rb index 28727ff0c..d4dce6168 100644 --- a/saas/app/models/notification/push_target/native.rb +++ b/saas/app/models/notification/push_target/native.rb @@ -1,13 +1,11 @@ class Notification::PushTarget::Native < Notification::PushTarget - private - def should_push? - super && devices.any? - end - - def perform_push + def process + if devices.any? native_notification.deliver_later_to(devices) end + end + private def devices @devices ||= notification.identity.devices end diff --git a/saas/test/models/notification/push_target/native_test.rb b/saas/test/models/notification/push_target/native_test.rb index a68e6517b..5e038fa87 100644 --- a/saas/test/models/notification/push_target/native_test.rb +++ b/saas/test/models/notification/push_target/native_test.rb @@ -50,16 +50,6 @@ class Notification::PushTarget::NativeTest < ActiveSupport::TestCase end end - test "does not push when creator is system user" do - stub_push_services - @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") - @notification.update!(creator: users(:system)) - - assert_no_native_push_delivery do - Notification::PushTarget::Native.new(@notification).process - end - end - test "pushes to multiple devices" do stub_push_services @identity.devices.delete_all diff --git a/test/models/notification/push_target/web_test.rb b/test/models/notification/push_target/web_test.rb index 7182bd08f..b492b48dd 100644 --- a/test/models/notification/push_target/web_test.rb +++ b/test/models/notification/push_target/web_test.rb @@ -37,20 +37,6 @@ class Notification::PushTarget::WebTest < ActiveSupport::TestCase Notification::PushTarget::Web.new(@notification).process end - test "does not push for cancelled accounts" do - @user.account.cancel(initiated_by: @user) - @web_push_pool.expects(:queue).never - - Notification::PushTarget::Web.new(@notification).process - end - - test "does not push when creator is system user" do - @notification.update!(creator: users(:system)) - @web_push_pool.expects(:queue).never - - Notification::PushTarget::Web.new(@notification).process - end - test "payload includes card title for card events" do @web_push_pool.expects(:queue).once.with do |payload, _| payload[:title] == @notification.card.title diff --git a/test/models/notification/pushable_test.rb b/test/models/notification/pushable_test.rb index 1b8e191b5..7c54b0036 100644 --- a/test/models/notification/pushable_test.rb +++ b/test/models/notification/pushable_test.rb @@ -46,19 +46,43 @@ class Notification::PushableTest < ActiveSupport::TestCase Notification.push_targets = original_targets end - test "pushable? returns true for normal notifications" do - assert @notification.pushable? + test "push processes targets for normal notifications" do + target_class = mock("push_target_class") + target_class.expects(:process).with(@notification) + + original_targets = Notification.push_targets + Notification.push_targets = [ target_class ] + + @notification.push + ensure + Notification.push_targets = original_targets end - test "pushable? returns false when creator is system user" do + test "push skips targets when creator is system user" do @notification.update!(creator: users(:system)) - assert_not @notification.pushable? + target_class = mock("push_target_class") + target_class.expects(:process).never + + original_targets = Notification.push_targets + Notification.push_targets = [ target_class ] + + @notification.push + ensure + Notification.push_targets = original_targets end - test "pushable? returns false for cancelled accounts" do + test "push skips targets for cancelled accounts" do @user.account.cancel(initiated_by: @user) - assert_not @notification.pushable? + target_class = mock("push_target_class") + target_class.expects(:process).never + + original_targets = Notification.push_targets + Notification.push_targets = [ target_class ] + + @notification.push + ensure + Notification.push_targets = original_targets end end