From 6e312c6028f8d67af5a83df22bd8215e1cf53e6a Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Wed, 7 Oct 2026 11:25:16 -0700
Subject: [PATCH] Stop sending push notifications to banned users (#338)

Banning a user deletes their sessions and closes their connections but
keeps their push subscriptions, and Room::MessagePusher chose recipients
by membership alone. A banned user's browser or phone therefore went on
receiving the room name, sender and text of new direct messages,
mentions, and messages in rooms they had set to everything.

Choose subscriptions from active users only. The subscriptions are kept,
so unbanning brings notifications back without the user having to
subscribe again (the client doesn't resubscribe while the browser still
holds a subscription).

Co-authored-by: Marcello Costagliola <176920116+namespaceMarcello@users.noreply.github.com>
---
 app/models/room/message_pusher.rb |  3 +++
 test/models/room/push_test.rb     | 37 +++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/app/models/room/message_pusher.rb b/app/models/room/message_pusher.rb
index 77f6778..e5e3c64 100644
--- a/app/models/room/message_pusher.rb
+++ b/app/models/room/message_pusher.rb
@@ -53,9 +53,12 @@ class Room::MessagePusher
       relevant_subscriptions.merge(Membership.involved_in_mentions).where(user_id: mentionees.ids)
     end
 
+    # Banning keeps the user's subscriptions, so that unbanning brings their notifications back,
+    # but nobody who can't sign in should be sent what's said in their rooms meanwhile.
     def relevant_subscriptions
       Push::Subscription
         .joins(user: :memberships)
+        .merge(User.active)
         .merge(Membership.visible.disconnected.where(room: room).where.not(user: message.creator))
     end
 
diff --git a/test/models/room/push_test.rb b/test/models/room/push_test.rb
index 8ecfdc6..9d83c73 100644
--- a/test/models/room/push_test.rb
+++ b/test/models/room/push_test.rb
@@ -63,7 +63,44 @@ class Room::PushTest < ActiveSupport::TestCase
     end
   end
 
+  test "does not notify banned users" do
+    users(:kevin).ban
+
+    assert_not_includes pushed_users { post_to_designers_mentioning_kevin }, users(:kevin)
+  end
+
+  test "notifies users again once they're unbanned" do
+    users(:kevin).ban
+    users(:kevin).unban
+
+    assert_includes pushed_users { post_to_designers_mentioning_kevin }, users(:kevin)
+  end
+
+  test "does not notify banned users of direct messages" do
+    users(:kevin).ban
+
+    pushed = pushed_users do
+      rooms(:david_and_kevin).messages.create! body: "Just between us", client_message_id: "earth", creator: users(:david)
+    end
+    assert_not_includes pushed, users(:kevin)
+  end
+
+  test "notifies active users mentioned" do
+    assert_includes pushed_users { post_to_designers_mentioning_kevin }, users(:kevin)
+  end
+
   private
+    def post_to_designers_mentioning_kevin
+      rooms(:designers).messages.create! body: "Hey #{mention_attachment_for(:kevin)}", client_message_id: "earth", creator: users(:david)
+    end
+
+    def pushed_users(&block)
+      queued = []
+      Rails.configuration.x.web_push_pool.stubs(:queue).with { |_payload, subscriptions| queued.concat subscriptions.to_a }
+      perform_enqueued_jobs(only: Room::PushMessageJob, &block)
+      queued.map(&:user).uniq
+    end
+
     def wait_for_web_push_delivery_pool_tasks(count)
       wait_for_pool_tasks(Rails.configuration.x.web_push_pool.delivery_pool, count)
     end