Refactor notification push system with registry pattern

Replace NotificationPusher with a cleaner architecture:

- Add Notification::Pushable concern with push target registry
- Add Notification::Push base class with template methods
- Add Notification::Push::Web for web push (OSS)
- Add Notification::Push::Native for native push (SaaS)
- Add Notification::WebPushJob and Notification::NativePushJob

Key design:
- Registry pattern: Notification.register_push_target(:web)
- Template method: push calls should_push? then perform_push
- Subclasses override should_push? (with super) and perform_push
- Each target handles its own job enqueueing

Also:
- Add Notification#pushable? for checking push eligibility
- Add Notification#identity delegation to user
- Reorganize tests to match new class structure

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Tidy up saas engine a bit more

app/jobs/notification/web_push_job.rb

@@ -0,0 +1,5 @@
+class Notification::WebPushJob < ApplicationJob
+  def perform(notification)
+    Notification::Push::Web.new(notification).push
+  end
+end

app/jobs/push_notification_job.rb

@@ -1,7 +0,0 @@
-class PushNotificationJob < ApplicationJob
-  discard_on ActiveJob::DeserializationError
-
-  def perform(notification)
-    NotificationPusher.new(notification).push
-  end
-end

app/models/concerns/push_notifiable.rb

@@ -1,12 +0,0 @@
-module PushNotifiable
-  extend ActiveSupport::Concern
-
-  included do
-    after_save_commit :push_notification_later, if: :source_id_previously_changed?
-  end
-
-  private
-    def push_notification_later
-      PushNotificationJob.perform_later(self)
-    end
-end

app/models/notification.rb

@@ -1,5 +1,5 @@
 class Notification < ApplicationRecord
-  include PushNotifiable
+  include Notification::Pushable
 
   belongs_to :account, default: -> { user.account }
   belongs_to :user
@@ -27,6 +27,7 @@ class Notification < ApplicationRecord
   after_destroy_commit -> { broadcast_remove_to user, :notifications }
 
   delegate :notifiable_target, to: :source
+  delegate :identity, to: :user
 
   class << self
     def read_all

app/models/notification/push.rb

@@ -1,9 +1,10 @@
-class NotificationPusher
-  include Rails.application.routes.url_helpers
+class Notification::Push
   include ExcerptHelper
 
   attr_reader :notification
 
+  delegate :card, to: :notification
+
   def initialize(notification)
     @notification = notification
   end
@@ -11,21 +12,16 @@ class NotificationPusher
   def push
     return unless should_push?
 
-    build_payload.tap do |payload|
-      push_to_web(payload)
-    end
+    perform_push
   end
 
   private
     def should_push?
-      push_destination? &&
-        !notification.creator.system? &&
-        notification.user.active? &&
-        notification.account.active?
+      notification.pushable?
     end
 
-    def push_destination?
-      notification.user.push_subscriptions.any?
+    def perform_push
+      raise NotImplementedError
     end
 
     def build_payload
@@ -41,7 +37,6 @@ class NotificationPusher
 
     def build_event_payload
       event = notification.source
-      card = event.card
 
       base_payload = {
         title: card_notification_title(card),
@@ -80,7 +75,6 @@ class NotificationPusher
 
     def build_mention_payload
       mention = notification.source
-      card = mention.card
 
       {
         title: "#{mention.mentioner.first_name} mentioned you",
@@ -93,19 +87,10 @@ class NotificationPusher
       {
         title: "New notification",
         body: "You have a new notification",
-        url: notifications_url(**url_options)
+        url: notifications_url
       }
     end
 
-    def push_to_web(payload)
-      subscriptions = notification.user.push_subscriptions
-      enqueue_payload_for_delivery(payload, subscriptions)
-    end
-
-    def enqueue_payload_for_delivery(payload, subscriptions)
-      Rails.configuration.x.web_push_pool.queue(payload, subscriptions)
-    end
-
     def card_notification_title(card)
       card.title.presence || "Card #{card.number}"
     end
@@ -118,6 +103,10 @@ class NotificationPusher
       Rails.application.routes.url_helpers.card_url(card, **url_options)
     end
 
+    def notifications_url
+      Rails.application.routes.url_helpers.notifications_url(**url_options)
+    end
+
     def card_url_with_comment_anchor(comment)
       Rails.application.routes.url_helpers.card_url(
         comment.card,

app/models/notification/push/web.rb

@@ -0,0 +1,18 @@
+class Notification::Push::Web < Notification::Push
+  def self.push_later(notification)
+    Notification::WebPushJob.perform_later(notification)
+  end
+
+  private
+    def should_push?
+      super && subscriptions.any?
+    end
+
+    def perform_push
+      Rails.configuration.x.web_push_pool.queue(build_payload, subscriptions)
+    end
+
+    def subscriptions
+      @subscriptions ||= notification.user.push_subscriptions
+    end
+end

app/models/notification/pushable.rb

@@ -0,0 +1,36 @@
+module Notification::Pushable
+  extend ActiveSupport::Concern
+
+  included do
+    class_attribute :push_targets, default: []
+
+    after_create_commit :push_later
+    after_update_commit :push_later, if: :source_id_previously_changed?
+  end
+
+  class_methods do
+    def register_push_target(target)
+      target = resolve_push_target(target)
+      push_targets << target unless push_targets.include?(target)
+    end
+
+    private
+      def resolve_push_target(target)
+        if target.is_a?(Symbol)
+          "Notification::Push::#{target.to_s.classify}".constantize
+        else
+          target
+        end
+      end
+  end
+
+  def push_later
+    self.class.push_targets.each do |target|
+      target.push_later(self)
+    end
+  end
+
+  def pushable?
+    !creator.system? && user.active? && account.active?
+  end
+end

config/initializers/push_notifications.rb

@@ -0,0 +1,3 @@
+Rails.application.config.to_prepare do
+  Notification.register_push_target(:web)
+end

saas/app/jobs/notification/native_push_job.rb

@@ -0,0 +1,5 @@
+class Notification::NativePushJob < ApplicationJob
+  def perform(notification)
+    Notification::Push::Native.new(notification).push
+  end
+end

saas/app/models/notification/push/native.rb

@@ -1,25 +1,19 @@
-module NotificationPusher::Native
-  extend ActiveSupport::Concern
-
-  def push
-    return unless should_push?
-
-    build_payload.tap do |payload|
-      push_to_web(payload) if notification.user.push_subscriptions.any?
-      push_to_native(payload)
-    end
+class Notification::Push::Native < Notification::Push
+  def self.push_later(notification)
+    Notification::NativePushJob.perform_later(notification)
   end
 
   private
-    def push_destination?
-      notification.user.push_subscriptions.any? || notification.user.identity.devices.any?
+    def should_push?
+      super && devices.any?
     end
 
-    def push_to_native(payload)
-      devices = notification.user.identity.devices
-      return if devices.empty?
+    def perform_push
+      native_notification(build_payload).deliver_later_to(devices)
+    end
 
-      native_notification(payload).deliver_later_to(devices)
+    def devices
+      @devices ||= notification.identity.devices
     end
 
     def native_notification(payload)
@@ -82,8 +76,4 @@ module NotificationPusher::Native
       return unless notification.creator.respond_to?(:avatar) && notification.creator.avatar.attached?
       Rails.application.routes.url_helpers.url_for(notification.creator.avatar)
     end
-
-    def card
-      @card ||= notification.card
-    end
 end

saas/lib/fizzy/saas/engine.rb

@@ -154,12 +154,14 @@ module Fizzy
       config.to_prepare do
         ::Account.include Account::Billing, Account::Limited
         ::User.include User::NotifiesAccountOfEmailChange
-        ::Identity.include Identity::Devices
-        ::NotificationPusher.prepend NotificationPusher::Native
-        ::Signup.prepend Fizzy::Saas::Signup
+        ::Identity.include Authorization::Identity, Identity::Devices
+        ::Signup.prepend Signup
+        ApplicationController.include Authorization::Controller
         CardsController.include(Card::LimitedCreation)
         Cards::PublishesController.include(Card::LimitedPublishing)
 
+        Notification.register_push_target(:native)
+
         Queenbee::Subscription.short_names = Subscription::SHORT_NAMES
 
         # Default to local dev QB token if not set
@@ -170,9 +172,6 @@ module Fizzy
           ::Object.send(:remove_const, const_name) if ::Object.const_defined?(const_name)
           ::Object.const_set const_name, Subscription.const_get(short_name, false)
         end
-
-        ::ApplicationController.include Fizzy::Saas::Authorization::Controller
-        ::Identity.include Fizzy::Saas::Authorization::Identity
       end
     end
   end

saas/test/models/notification/push/native_test.rb

@@ -1,11 +1,10 @@
 require "test_helper"
 
-class NotificationPusherNativeTest < ActiveSupport::TestCase
+class Notification::Push::NativeTest < ActiveSupport::TestCase
   setup do
     @user = users(:kevin)
     @identity = @user.identity
     @notification = notifications(:logo_published_kevin)
-    @pusher = NotificationPusher.new(@notification)
 
     # Ensure user has no web push subscriptions (we want to test native push independently)
     @user.push_subscriptions.delete_all
@@ -13,137 +12,100 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase
 
   test "notification_category returns assignment for card_assigned" do
     notification = notifications(:logo_assignment_kevin)
-    pusher = NotificationPusher.new(notification)
+    @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
 
-    assert_equal "assignment", pusher.send(:notification_category)
+    push = Notification::Push::Native.new(notification)
+
+    assert_equal "assignment", push.send(:notification_category)
   end
 
   test "notification_category returns comment for comment_created" do
     notification = notifications(:layout_commented_kevin)
-    pusher = NotificationPusher.new(notification)
+    @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
 
-    assert_equal "comment", pusher.send(:notification_category)
+    push = Notification::Push::Native.new(notification)
+
+    assert_equal "comment", push.send(:notification_category)
   end
 
   test "notification_category returns mention for mentions" do
     notification = notifications(:logo_card_david_mention_by_jz)
-    pusher = NotificationPusher.new(notification)
+    notification.user.identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
 
-    assert_equal "mention", pusher.send(:notification_category)
+    push = Notification::Push::Native.new(notification)
+
+    assert_equal "mention", push.send(:notification_category)
   end
 
   test "notification_category returns card for other card events" do
-    notification = notifications(:logo_published_kevin)
-    pusher = NotificationPusher.new(notification)
+    @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
 
-    assert_equal "card", pusher.send(:notification_category)
+    push = Notification::Push::Native.new(@notification)
+
+    assert_equal "card", push.send(:notification_category)
   end
 
   test "interruption_level is time-sensitive for assignments" do
     notification = notifications(:logo_assignment_kevin)
-    pusher = NotificationPusher.new(notification)
+    @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
 
-    assert_equal "time-sensitive", pusher.send(:interruption_level)
+    push = Notification::Push::Native.new(notification)
+
+    assert_equal "time-sensitive", push.send(:interruption_level)
   end
 
   test "interruption_level is active for non-assignments" do
-    notification = notifications(:logo_published_kevin)
-    pusher = NotificationPusher.new(notification)
-
-    assert_equal "active", pusher.send(:interruption_level)
-  end
-
-  test "push_destination returns true when identity has native devices" do
     @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
 
-    assert @pusher.send(:push_destination?)
+    push = Notification::Push::Native.new(@notification)
+
+    assert_equal "active", push.send(:interruption_level)
   end
 
-  test "push_destination returns true when user has web subscriptions" do
-    @user.push_subscriptions.create!(
-      endpoint: "https://fcm.googleapis.com/fcm/send/test",
-      p256dh_key: "test_p256dh",
-      auth_key: "test_auth"
-    )
-
-    assert @pusher.send(:push_destination?)
-  end
-
-  test "push_destination returns false when user has neither" do
-    @identity.devices.delete_all
-    @user.push_subscriptions.delete_all
-
-    assert_not @pusher.send(:push_destination?)
-  end
-
-  test "push delivers to native devices when user has devices" do
+  test "pushes to native devices when user has devices" do
     stub_push_services
     @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
 
     assert_native_push_delivery(count: 1) do
-      @pusher.push
+      Notification::Push::Native.new(@notification).push
     end
   end
 
-  test "push does not deliver to native when user has no devices" do
+  test "does not push when user has no devices" do
     @identity.devices.delete_all
 
     assert_no_native_push_delivery do
-      @pusher.push
+      Notification::Push::Native.new(@notification).push
     end
   end
 
-  test "push does not deliver when creator is system user" do
+  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))
 
-    result = @pusher.push
-
-    assert_nil result
+    assert_no_native_push_delivery do
+      Notification::Push::Native.new(@notification).push
+    end
   end
 
-  test "push delivers to multiple devices" do
+  test "pushes to multiple devices" do
     stub_push_services
     @identity.devices.delete_all
     @identity.devices.create!(token: "token1", platform: "apple", name: "iPhone")
     @identity.devices.create!(token: "token2", platform: "google", name: "Pixel")
 
     assert_native_push_delivery(count: 2) do
-      @pusher.push
-    end
-  end
-
-  test "push delivers to both web and native when user has both" do
-    stub_push_services
-
-    # Set up web push subscription
-    @user.push_subscriptions.create!(
-      endpoint: "https://fcm.googleapis.com/fcm/send/test",
-      p256dh_key: "test_p256dh_key",
-      auth_key: "test_auth_key"
-    )
-
-    # Set up native device
-    @identity.devices.create!(token: "native_token", platform: "apple", name: "iPhone")
-
-    # Mock web push pool to verify it receives the payload
-    web_push_pool = mock("web_push_pool")
-    web_push_pool.expects(:queue).once.with do |payload, subscriptions|
-      payload.is_a?(Hash) && subscriptions.count == 1
-    end
-    Rails.configuration.x.stubs(:web_push_pool).returns(web_push_pool)
-
-    # Verify native push is also delivered
-    assert_native_push_delivery(count: 1) do
-      @pusher.push
+      Notification::Push::Native.new(@notification).push
     end
   end
 
   test "native notification includes required fields" do
     @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
-    payload = @pusher.send(:build_payload)
-    native = @pusher.send(:native_notification, payload)
+
+    push = Notification::Push::Native.new(@notification)
+    payload = push.send(:build_payload)
+    native = push.send(:native_notification, payload)
 
     assert_not_nil native.title
     assert_not_nil native.body
@@ -152,8 +114,10 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase
 
   test "native notification sets thread_id from card" do
     @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
-    payload = @pusher.send(:build_payload)
-    native = @pusher.send(:native_notification, payload)
+
+    push = Notification::Push::Native.new(@notification)
+    payload = push.send(:build_payload)
+    native = push.send(:native_notification, payload)
 
     assert_equal @notification.card.id, native.thread_id
   end
@@ -161,26 +125,30 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase
   test "native notification sets high_priority for assignments" do
     notification = notifications(:logo_assignment_kevin)
     notification.user.identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
-    pusher = NotificationPusher.new(notification)
 
-    payload = pusher.send(:build_payload)
-    native = pusher.send(:native_notification, payload)
+    push = Notification::Push::Native.new(notification)
+    payload = push.send(:build_payload)
+    native = push.send(:native_notification, payload)
 
     assert native.high_priority
   end
 
   test "native notification sets normal priority for non-assignments" do
     @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
-    payload = @pusher.send(:build_payload)
-    native = @pusher.send(:native_notification, payload)
+
+    push = Notification::Push::Native.new(@notification)
+    payload = push.send(:build_payload)
+    native = push.send(:native_notification, payload)
 
     assert_not native.high_priority
   end
 
   test "native notification includes apple-specific fields" do
     @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
-    payload = @pusher.send(:build_payload)
-    native = @pusher.send(:native_notification, payload)
+
+    push = Notification::Push::Native.new(@notification)
+    payload = push.send(:build_payload)
+    native = push.send(:native_notification, payload)
 
     assert_equal 1, native.apple_data.dig(:aps, :"mutable-content")
     assert_includes %w[active time-sensitive], native.apple_data.dig(:aps, :"interruption-level")
@@ -189,19 +157,29 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase
 
   test "native notification sets android notification to nil for data-only" do
     @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
-    payload = @pusher.send(:build_payload)
-    native = @pusher.send(:native_notification, payload)
+
+    push = Notification::Push::Native.new(@notification)
+    payload = push.send(:build_payload)
+    native = push.send(:native_notification, payload)
 
     assert_nil native.google_data.dig(:android, :notification)
   end
 
   test "native notification includes data payload" do
     @identity.devices.create!(token: "test123", platform: "apple", name: "Test iPhone")
-    payload = @pusher.send(:build_payload)
-    native = @pusher.send(:native_notification, payload)
+
+    push = Notification::Push::Native.new(@notification)
+    payload = push.send(:build_payload)
+    native = push.send(:native_notification, payload)
 
     assert_not_nil native.data[:url]
     assert_equal @notification.account.external_account_id, native.data[:account_id]
     assert_equal @notification.creator.name, native.data[:creator_name]
   end
+
+  test "push_later enqueues Notification::NativePushJob" do
+    assert_enqueued_with(job: Notification::NativePushJob, args: [ @notification ]) do
+      Notification::Push::Native.push_later(@notification)
+    end
+  end
 end

test/models/notification/push/web_test.rb

@@ -0,0 +1,100 @@
+require "test_helper"
+
+class Notification::Push::WebTest < ActiveSupport::TestCase
+  setup do
+    @user = users(:david)
+    @notification = @user.notifications.create!(
+      source: events(:logo_published),
+      creator: users(:jason)
+    )
+
+    @user.push_subscriptions.create!(
+      endpoint: "https://fcm.googleapis.com/fcm/send/test123",
+      p256dh_key: "test_key",
+      auth_key: "test_auth"
+    )
+
+    @web_push_pool = mock("web_push_pool")
+    Rails.configuration.x.stubs(:web_push_pool).returns(@web_push_pool)
+  end
+
+  test "pushes to web when user has subscriptions" do
+    @web_push_pool.expects(:queue).once.with do |payload, subscriptions|
+      payload.is_a?(Hash) &&
+        payload[:title].present? &&
+        payload[:body].present? &&
+        payload[:url].present? &&
+        subscriptions.count == 1
+    end
+
+    Notification::Push::Web.new(@notification).push
+  end
+
+  test "does not push when user has no subscriptions" do
+    @user.push_subscriptions.delete_all
+    @web_push_pool.expects(:queue).never
+
+    Notification::Push::Web.new(@notification).push
+  end
+
+  test "does not push for cancelled accounts" do
+    @user.account.cancel(initiated_by: @user)
+    @web_push_pool.expects(:queue).never
+
+    Notification::Push::Web.new(@notification).push
+  end
+
+  test "does not push when creator is system user" do
+    @notification.update!(creator: users(:system))
+    @web_push_pool.expects(:queue).never
+
+    Notification::Push::Web.new(@notification).push
+  end
+
+  test "payload includes card title for card events" do
+    @web_push_pool.expects(:queue).once.with do |payload, _|
+      payload[:title] == @notification.card.title
+    end
+
+    Notification::Push::Web.new(@notification).push
+  end
+
+  test "payload for comment includes RE prefix" do
+    event = events(:layout_commented)
+    notification = @user.notifications.create!(source: event, creator: event.creator)
+
+    @web_push_pool.expects(:queue).once.with do |payload, _|
+      payload[:title].start_with?("RE:")
+    end
+
+    Notification::Push::Web.new(notification).push
+  end
+
+  test "payload for assignment includes assigned message" do
+    event = events(:logo_assignment_david)
+    notification = @user.notifications.create!(source: event, creator: event.creator)
+
+    @web_push_pool.expects(:queue).once.with do |payload, _|
+      payload[:body].include?("Assigned to you")
+    end
+
+    Notification::Push::Web.new(notification).push
+  end
+
+  test "payload for mention includes mentioner name" do
+    mention = mentions(:logo_card_david_mention_by_jz)
+    notification = @user.notifications.create!(source: mention, creator: users(:jz))
+
+    @web_push_pool.expects(:queue).once.with do |payload, _|
+      payload[:title].include?("mentioned you")
+    end
+
+    Notification::Push::Web.new(notification).push
+  end
+
+  test "push_later enqueues Notification::WebPushJob" do
+    assert_enqueued_with(job: Notification::WebPushJob, args: [ @notification ]) do
+      Notification::Push::Web.push_later(@notification)
+    end
+  end
+end

test/models/notification/pushable_test.rb

@@ -0,0 +1,58 @@
+require "test_helper"
+
+class Notification::PushableTest < ActiveSupport::TestCase
+  setup do
+    @user = users(:david)
+    @notification = @user.notifications.create!(
+      source: events(:logo_published),
+      creator: users(:jason)
+    )
+  end
+
+  test "push_later calls push_later on all registered targets" do
+    target = mock("push_target")
+    target.expects(:push_later).with(@notification)
+
+    original_targets = Notification.push_targets
+    Notification.push_targets = [ target ]
+
+    @notification.push_later
+  ensure
+    Notification.push_targets = original_targets
+  end
+
+  test "push_later is called after notification is created" do
+    Notification.any_instance.expects(:push_later)
+
+    @user.notifications.create!(
+      source: events(:logo_published),
+      creator: users(:jason)
+    )
+  end
+
+  test "register_push_target accepts symbols" do
+    original_targets = Notification.push_targets.dup
+
+    Notification.register_push_target(:web)
+
+    assert_includes Notification.push_targets, Notification::Push::Web
+  ensure
+    Notification.push_targets = original_targets
+  end
+
+  test "pushable? returns true for normal notifications" do
+    assert @notification.pushable?
+  end
+
+  test "pushable? returns false when creator is system user" do
+    @notification.update!(creator: users(:system))
+
+    assert_not @notification.pushable?
+  end
+
+  test "pushable? returns false for cancelled accounts" do
+    @user.account.cancel(initiated_by: @user)
+
+    assert_not @notification.pushable?
+  end
+end

test/models/notification_pusher_test.rb

@@ -1,29 +0,0 @@
-require "test_helper"
-
-class NotificationPusherTest < ActiveSupport::TestCase
-  setup do
-    @user = users(:david)
-    @notification = notifications(:logo_mentioned_david)
-    @pusher = NotificationPusher.new(@notification)
-
-    @user.push_subscriptions.create!(
-      endpoint: "https://fcm.googleapis.com/fcm/send/test123",
-      p256dh_key: "test_key",
-      auth_key: "test_auth"
-    )
-  end
-
-  test "push does not send notifications for cancelled accounts" do
-    @user.account.cancel(initiated_by: @user)
-
-    result = @pusher.push
-
-    assert_nil result, "Should not push notifications for cancelled accounts"
-  end
-
-  test "push sends notifications for active accounts with subscriptions" do
-    result = @pusher.push
-
-    assert_not_nil result, "Should push notifications for active accounts with subscriptions"
-  end
-end