Merge pull request #2511 from basecamp/native-push-notifications

Support native push notifications

test/integration/notifications_test.rb

@@ -0,0 +1,172 @@
+require "test_helper"
+
+class NotificationDeliveryTest < ActiveSupport::TestCase
+  setup do
+    @assigner = users(:david)
+    @assignee = users(:kevin)
+    @card = cards(:logo)
+
+    @card.assignments.destroy_all
+    @assignee.notifications.destroy_all
+
+    stub_web_push_pool
+
+    @original_targets = Notification.push_targets.dup
+    Notification.push_targets = []
+    Notification.register_push_target(:web)
+    Notification.register_push_target(push_target_with_tracking)
+
+    # Give assignee a web push subscription
+    @assignee.push_subscriptions.create!(
+      endpoint: "https://fcm.googleapis.com/fcm/send/test123",
+      p256dh_key: "test_key",
+      auth_key: "test_auth"
+    )
+
+    Current.user = @assigner
+  end
+
+  teardown do
+    Notification.push_targets = @original_targets
+    @assignee.push_subscriptions.delete_all
+  end
+
+  test "card assignment creates notification and triggers push" do
+    assert_difference -> { Notification.count }, 1 do
+      perform_enqueued_jobs only: [ NotifyRecipientsJob, Notification::PushJob ] do
+        @card.toggle_assignment(@assignee)
+      end
+    end
+
+    notification = Notification.last
+    assert_equal @assignee, notification.user
+    assert_equal @assigner, notification.creator
+    assert_equal "card_assigned", notification.source.action
+
+    assert_push_delivered_for notification
+    assert_web_push_delivered
+  end
+
+  test "card assignment notification is bundled for email delivery when bundling enabled" do
+    @assignee.settings.update!(bundle_email_frequency: :every_few_hours)
+
+    assert_difference -> { Notification.count }, 1 do
+      perform_enqueued_jobs only: NotifyRecipientsJob do
+        @card.toggle_assignment(@assignee)
+      end
+    end
+
+    notification = @assignee.notifications.reload.last
+    assert_not_nil notification, "Notification should be created for assignee"
+
+    bundle = @assignee.notification_bundles.pending.last
+    assert_not_nil bundle, "Bundle should be created when bundling is enabled"
+    assert_includes bundle.notifications, notification
+  end
+
+  test "comment creates notification for card watchers and triggers push" do
+    @card.watch_by(@assignee)
+
+    assert_difference -> { Notification.count }, 1 do
+      perform_enqueued_jobs only: [ NotifyRecipientsJob, Notification::PushJob ] do
+        @card.comments.create!(body: "Great work on this!", creator: @assigner)
+      end
+    end
+
+    notification = Notification.last
+    assert_equal @assignee, notification.user
+    assert_equal "comment_created", notification.source.action
+
+    assert_push_delivered
+    assert_web_push_delivered
+  end
+
+  test "mention creates notification and triggers push" do
+    mention_html = ActionText::Attachment.from_attachable(@assignee).to_html
+
+    perform_enqueued_jobs only: [ Mention::CreateJob, NotifyRecipientsJob, Notification::PushJob ] do
+      @card.comments.create!(
+        body: "#{mention_html} check this out",
+        creator: @assigner
+      )
+    end
+
+    mention_notification = @assignee.notifications.find_by(source_type: "Mention")
+    assert_not_nil mention_notification
+
+    assert_push_delivered_for mention_notification
+    assert_web_push_delivered
+  end
+
+  test "system user actions do not create notifications" do
+    Current.user = users(:system)
+
+    assert_no_difference -> { Notification.count } do
+      perform_enqueued_jobs only: [ NotifyRecipientsJob, Notification::PushJob ] do
+        @card.toggle_assignment(@assignee)
+      end
+    end
+
+    assert_no_push_delivered
+    assert_no_web_push_delivered
+  end
+
+  test "notifications for inactive users are created but do not trigger push" do
+    @assignee.deactivate
+
+    assert_difference -> { Notification.count }, 1 do
+      perform_enqueued_jobs only: [ NotifyRecipientsJob, Notification::PushJob ] do
+        @card.toggle_assignment(@assignee)
+      end
+    end
+
+    assert_no_push_delivered
+    assert_no_web_push_delivered
+  end
+
+  private
+    def stub_web_push_pool
+      @web_push_calls = []
+      web_push_pool = stub("web_push_pool")
+      web_push_pool.stubs(:queue).with do |payload, subs|
+        @web_push_calls << { payload: payload, subscriptions: subs }
+      end
+
+      Rails.configuration.x.stubs(:web_push_pool).returns(web_push_pool)
+    end
+
+    def push_target_with_tracking
+      @push_target_calls = []
+      fake_push_target = Class.new(Notification::PushTarget) do
+        class << self
+          attr_accessor :calls
+        end
+
+        def self.process(notification)
+          calls << notification
+        end
+      end
+
+      fake_push_target.tap { it.calls = @push_target_calls }
+    end
+
+    def assert_push_delivered
+      assert_not_empty @push_target_calls, "Expected push to be delivered"
+    end
+
+    def assert_push_delivered_for(notification)
+      assert_includes @push_target_calls, notification, "Expected push to be delivered for notification"
+    end
+
+    def assert_no_push_delivered
+      assert_empty @push_target_calls, "Expected no push to be delivered"
+    end
+
+    def assert_web_push_delivered
+      assert_not_empty @web_push_calls, "Expected web push to be delivered"
+    end
+
+    def assert_no_web_push_delivered
+      assert_empty @web_push_calls, "Expected no web push to be delivered"
+    end
+end

test/lib/web_push/persistent_request_test.rb

@@ -12,7 +12,7 @@ class WebPush::PersistentRequestTest < ActiveSupport::TestCase
     notification = WebPush::Notification.new(
       title: "Test",
       body: "Test notification",
-      path: "/test",
+      url: "/test",
       badge: 0,
       endpoint: ENDPOINT,
       endpoint_ip: PUBLIC_TEST_IP,

test/models/concerns/push_notifiable_test.rb

@@ -1,28 +0,0 @@
-require "test_helper"
-
-class PushNotifiableTest < ActiveSupport::TestCase
-  test "enqueues push notification job when notification is created" do
-    assert_enqueued_with(job: PushNotificationJob) do
-      users(:david).notifications.create!(
-        source: events(:layout_published),
-        creator: users(:jason)
-      )
-    end
-  end
-
-  test "enqueues push notification job when notification source changes" do
-    notification = notifications(:logo_mentioned_david)
-
-    assert_enqueued_with(job: PushNotificationJob) do
-      notification.update!(source: events(:logo_published))
-    end
-  end
-
-  test "does not enqueue push notification job for other updates" do
-    notification = notifications(:logo_mentioned_david)
-
-    assert_no_enqueued_jobs only: PushNotificationJob do
-      notification.update!(unread_count: 5)
-    end
-  end
-end

test/models/notification/push_target/web_test.rb

@@ -0,0 +1,75 @@
+require "test_helper"
+
+class Notification::PushTarget::WebTest < ActiveSupport::TestCase
+  setup do
+    @user = users(:david)
+    @notification = notifications(:logo_mentioned_david)
+
+    @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::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).process
+  end
+
+  test "payload includes card title for card events" do
+    @notification.update!(source: events(:logo_published))
+
+    @web_push_pool.expects(:queue).once.with do |payload, _|
+      payload[:title] == @notification.card.title
+    end
+
+    Notification::PushTarget::Web.new(@notification).process
+  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::PushTarget::Web.new(notification).process
+  end
+
+  test "payload for assignment includes assigned message" do
+    @notification.update!(source: events(:logo_assignment_david))
+
+    @web_push_pool.expects(:queue).once.with do |payload, _|
+      payload[:body].include?("Assigned to you")
+    end
+
+    Notification::PushTarget::Web.new(@notification).process
+  end
+
+  test "payload for mention includes mentioner name" do
+    @web_push_pool.expects(:queue).once.with do |payload, _|
+      payload[:title].include?("mentioned you")
+    end
+
+    Notification::PushTarget::Web.new(@notification).process
+  end
+end

test/models/notification/pushable_test.rb

@@ -0,0 +1,97 @@
+require "test_helper"
+
+class Notification::PushableTest < ActiveSupport::TestCase
+  setup do
+    @user = users(:david)
+    @notification = notifications(:logo_mentioned_david)
+  end
+
+  test "push_later enqueues Notification::PushJob" do
+    assert_enqueued_with(job: Notification::PushJob, args: [ @notification ]) do
+      @notification.push_later
+    end
+  end
+
+  test "push calls process on all registered targets" 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 "push_later is called after notification is created" do
+    assert_enqueued_with(job: Notification::PushJob) do
+      @user.notifications.create!(
+        source: events(:layout_published),
+        creator: users(:jason)
+      )
+    end
+  end
+
+  test "push_later is called when notification source changes" do
+    assert_enqueued_with(job: Notification::PushJob) do
+      @notification.update!(source: events(:logo_published))
+    end
+  end
+
+  test "push_later is not called for other updates" do
+    assert_no_enqueued_jobs only: Notification::PushJob do
+      @notification.update!(unread_count: 5)
+    end
+  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::PushTarget::Web
+  ensure
+    Notification.push_targets = original_targets
+  end
+
+  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 "push skips targets when creator is system user" do
+    @notification.update!(creator: users(:system))
+
+    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 "push skips targets for cancelled accounts" do
+    @user.account.cancel(initiated_by: @user)
+
+    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

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