From 0948533da7d76cbd4f994c2f1de3f7fc36a2746f Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Thu, 22 Jan 2026 18:02:25 +0100 Subject: [PATCH] Rename push to process on PushTarget for clearer semantics "Push to target" reads naturally - we push the notification to the target. "Target processes" also makes sense - the target receives and handles the notification in its own way. - Add class method PushTarget.process(notification) that instantiates and calls the instance method - Rename instance method from push to process - Add private push_to helper in Pushable for readable iteration Co-Authored-By: Claude Opus 4.5 --- app/models/notification/push_target.rb | 6 +++++- app/models/notification/pushable.rb | 13 ++++++++----- .../notification/push_target/native_test.rb | 8 ++++---- test/models/notification/push_target/web_test.rb | 16 ++++++++-------- test/models/notification/pushable_test.rb | 7 ++----- 5 files changed, 27 insertions(+), 23 deletions(-) diff --git a/app/models/notification/push_target.rb b/app/models/notification/push_target.rb index 26fd2b263..da6c26fdc 100644 --- a/app/models/notification/push_target.rb +++ b/app/models/notification/push_target.rb @@ -3,11 +3,15 @@ class Notification::PushTarget delegate :card, to: :notification + def self.process(notification) + new(notification).process + end + def initialize(notification) @notification = notification end - def push + def process return unless should_push? perform_push diff --git a/app/models/notification/pushable.rb b/app/models/notification/pushable.rb index c8519951a..0f5ae2ed6 100644 --- a/app/models/notification/pushable.rb +++ b/app/models/notification/pushable.rb @@ -16,9 +16,10 @@ module Notification::Pushable private def resolve_push_target(target) - if target.is_a?(Notification::PushTarget) then target - else + if target.is_a?(Symbol) "Notification::PushTarget::#{target.to_s.classify}".constantize + else + target end end end @@ -28,9 +29,7 @@ module Notification::Pushable end def push - self.class.push_targets.each do |target| - target.new(self).push - end + self.class.push_targets.each { |target| push_to(target) } end def pushable? @@ -42,6 +41,10 @@ module Notification::Pushable end private + def push_to(target) + target.process(self) + end + def payload_type source_type.presence_in(%w[ Event Mention ]) || "Default" end diff --git a/saas/test/models/notification/push_target/native_test.rb b/saas/test/models/notification/push_target/native_test.rb index 2a098ebac..a68e6517b 100644 --- a/saas/test/models/notification/push_target/native_test.rb +++ b/saas/test/models/notification/push_target/native_test.rb @@ -38,7 +38,7 @@ class Notification::PushTarget::NativeTest < ActiveSupport::TestCase @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") assert_native_push_delivery(count: 1) do - Notification::PushTarget::Native.new(@notification).push + Notification::PushTarget::Native.new(@notification).process end end @@ -46,7 +46,7 @@ class Notification::PushTarget::NativeTest < ActiveSupport::TestCase @identity.devices.delete_all assert_no_native_push_delivery do - Notification::PushTarget::Native.new(@notification).push + Notification::PushTarget::Native.new(@notification).process end end @@ -56,7 +56,7 @@ class Notification::PushTarget::NativeTest < ActiveSupport::TestCase @notification.update!(creator: users(:system)) assert_no_native_push_delivery do - Notification::PushTarget::Native.new(@notification).push + Notification::PushTarget::Native.new(@notification).process end end @@ -67,7 +67,7 @@ class Notification::PushTarget::NativeTest < ActiveSupport::TestCase @identity.devices.create!(token: "token2", platform: "google", name: "Pixel") assert_native_push_delivery(count: 2) do - Notification::PushTarget::Native.new(@notification).push + Notification::PushTarget::Native.new(@notification).process end end diff --git a/test/models/notification/push_target/web_test.rb b/test/models/notification/push_target/web_test.rb index 627f15357..7182bd08f 100644 --- a/test/models/notification/push_target/web_test.rb +++ b/test/models/notification/push_target/web_test.rb @@ -27,28 +27,28 @@ class Notification::PushTarget::WebTest < ActiveSupport::TestCase subscriptions.count == 1 end - Notification::PushTarget::Web.new(@notification).push + Notification::PushTarget::Web.new(@notification).process end test "does not push when user has no subscriptions" do @user.push_subscriptions.delete_all @web_push_pool.expects(:queue).never - Notification::PushTarget::Web.new(@notification).push + 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).push + 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).push + Notification::PushTarget::Web.new(@notification).process end test "payload includes card title for card events" do @@ -56,7 +56,7 @@ class Notification::PushTarget::WebTest < ActiveSupport::TestCase payload[:title] == @notification.card.title end - Notification::PushTarget::Web.new(@notification).push + Notification::PushTarget::Web.new(@notification).process end test "payload for comment includes RE prefix" do @@ -67,7 +67,7 @@ class Notification::PushTarget::WebTest < ActiveSupport::TestCase payload[:title].start_with?("RE:") end - Notification::PushTarget::Web.new(notification).push + Notification::PushTarget::Web.new(notification).process end test "payload for assignment includes assigned message" do @@ -78,7 +78,7 @@ class Notification::PushTarget::WebTest < ActiveSupport::TestCase payload[:body].include?("Assigned to you") end - Notification::PushTarget::Web.new(notification).push + Notification::PushTarget::Web.new(notification).process end test "payload for mention includes mentioner name" do @@ -89,7 +89,7 @@ class Notification::PushTarget::WebTest < ActiveSupport::TestCase payload[:title].include?("mentioned you") end - Notification::PushTarget::Web.new(notification).push + Notification::PushTarget::Web.new(notification).process end end diff --git a/test/models/notification/pushable_test.rb b/test/models/notification/pushable_test.rb index bdc3f2034..1b8e191b5 100644 --- a/test/models/notification/pushable_test.rb +++ b/test/models/notification/pushable_test.rb @@ -15,12 +15,9 @@ class Notification::PushableTest < ActiveSupport::TestCase end end - test "push calls push on all registered targets" do + test "push calls process on all registered targets" do target_class = mock("push_target_class") - target_instance = mock("push_target_instance") - - target_class.expects(:new).with(@notification).returns(target_instance) - target_instance.expects(:push) + target_class.expects(:process).with(@notification) original_targets = Notification.push_targets Notification.push_targets = [ target_class ]