diff --git a/app/jobs/notification/web_push_job.rb b/app/jobs/notification/web_push_job.rb new file mode 100644 index 000000000..cd30e3581 --- /dev/null +++ b/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 diff --git a/app/jobs/push_notification_job.rb b/app/jobs/push_notification_job.rb deleted file mode 100644 index c912e141d..000000000 --- a/app/jobs/push_notification_job.rb +++ /dev/null @@ -1,7 +0,0 @@ -class PushNotificationJob < ApplicationJob - discard_on ActiveJob::DeserializationError - - def perform(notification) - NotificationPusher.new(notification).push - end -end diff --git a/app/models/concerns/push_notifiable.rb b/app/models/concerns/push_notifiable.rb deleted file mode 100644 index 4bf9b575d..000000000 --- a/app/models/concerns/push_notifiable.rb +++ /dev/null @@ -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 diff --git a/app/models/notification.rb b/app/models/notification.rb index 417a2f22d..a5eab3ccd 100644 --- a/app/models/notification.rb +++ b/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 diff --git a/app/models/notification_pusher.rb b/app/models/notification/push.rb similarity index 78% rename from app/models/notification_pusher.rb rename to app/models/notification/push.rb index 12b4965bd..c94f121f1 100644 --- a/app/models/notification_pusher.rb +++ b/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, diff --git a/app/models/notification/push/web.rb b/app/models/notification/push/web.rb new file mode 100644 index 000000000..33d34e3e7 --- /dev/null +++ b/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 diff --git a/app/models/notification/pushable.rb b/app/models/notification/pushable.rb new file mode 100644 index 000000000..a4f1a56c6 --- /dev/null +++ b/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 diff --git a/config/initializers/push_notifications.rb b/config/initializers/push_notifications.rb new file mode 100644 index 000000000..fa82a46a7 --- /dev/null +++ b/config/initializers/push_notifications.rb @@ -0,0 +1,3 @@ +Rails.application.config.to_prepare do + Notification.register_push_target(:web) +end diff --git a/saas/app/jobs/notification/native_push_job.rb b/saas/app/jobs/notification/native_push_job.rb new file mode 100644 index 000000000..c6f08f840 --- /dev/null +++ b/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 diff --git a/saas/app/models/notification_pusher/native.rb b/saas/app/models/notification/push/native.rb similarity index 75% rename from saas/app/models/notification_pusher/native.rb rename to saas/app/models/notification/push/native.rb index 2fc38ed72..0030cf4b5 100644 --- a/saas/app/models/notification_pusher/native.rb +++ b/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 diff --git a/saas/lib/fizzy/saas/engine.rb b/saas/lib/fizzy/saas/engine.rb index 31fadee33..a582718b8 100644 --- a/saas/lib/fizzy/saas/engine.rb +++ b/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 diff --git a/saas/test/models/notification_pusher_test.rb b/saas/test/models/notification/push/native_test.rb similarity index 52% rename from saas/test/models/notification_pusher_test.rb rename to saas/test/models/notification/push/native_test.rb index ef7fcd0d5..8ba0521a5 100644 --- a/saas/test/models/notification_pusher_test.rb +++ b/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 diff --git a/test/models/notification/push/web_test.rb b/test/models/notification/push/web_test.rb new file mode 100644 index 000000000..4c81d6719 --- /dev/null +++ b/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 diff --git a/test/models/notification/pushable_test.rb b/test/models/notification/pushable_test.rb new file mode 100644 index 000000000..2f99ac2c5 --- /dev/null +++ b/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 diff --git a/test/models/notification_pusher_test.rb b/test/models/notification_pusher_test.rb deleted file mode 100644 index 53380effe..000000000 --- a/test/models/notification_pusher_test.rb +++ /dev/null @@ -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