From 53ae8a33e37f69ca22c016350f8dacdde0b33146 Mon Sep 17 00:00:00 2001
From: Mike Dalessio <mike@37signals.com>
Date: Wed, 17 Sep 2025 15:56:18 -0400
Subject: [PATCH] Improved performance of cleaning inaccessible data

ref: https://fizzy.37signals.com/5986089/cards/1865
---
 app/models/collection/accessible.rb           | 38 +++++++++++++++----
 test/models/access_test.rb                    |  8 ++--
 .../accessible_test.rb}                       |  2 +-
 3 files changed, 36 insertions(+), 12 deletions(-)
 rename test/models/{collection_test.rb => collection/accessible_test.rb} (94%)

diff --git a/app/models/collection/accessible.rb b/app/models/collection/accessible.rb
index 022185069..4e8cd40b2 100644
--- a/app/models/collection/accessible.rb
+++ b/app/models/collection/accessible.rb
@@ -43,8 +43,8 @@ module Collection::Accessible
   def clean_inaccessible_data_for(user)
     return if accessible_to?(user)
 
-    clean_inaccessible_records user.notifications
-    clean_inaccessible_records user.mentions
+    mentions_for_user(user).destroy_all
+    notifications_for_user(user).destroy_all
   end
 
   private
@@ -56,11 +56,33 @@ module Collection::Accessible
       accesses.grant_to(User.all) if all_access_previously_changed?(to: true)
     end
 
-    def clean_inaccessible_records(records)
-      records.find_each do |record|
-        if record.card&.collection == self
-          record.destroy
-        end
-      end
+    def mentions_for_user(user)
+      # Query handles 2 paths:
+      #
+      # 1. Mention->Card
+      # 2. Mention->Comment->Card
+      user.mentions
+        .joins("LEFT JOIN cards ON mentions.source_id = cards.id AND mentions.source_type = 'Card'")
+        .joins("LEFT JOIN comments ON mentions.source_id = comments.id AND mentions.source_type = 'Comment'")
+        .joins("LEFT JOIN cards AS comment_cards ON comments.card_id = comment_cards.id")
+        .where("(mentions.source_type = 'Card' AND cards.collection_id = ?) OR (mentions.source_type = 'Comment' AND comment_cards.collection_id = ?)", id, id)
+    end
+
+    def notifications_for_user(user)
+      # Query handles 2 paths:
+      #
+      # 1. Notification->Event->Card
+      # 2. Notification->Event->Comment->Card
+      #
+      # Notification->Event->Mention->Card and Notification->Event->Mention->Comment->Card are
+      # handled by destroying mentions_for_user.
+      user.notifications
+        .joins("LEFT JOIN events ON notifications.source_id = events.id AND notifications.source_type = 'Event'")
+        .joins("LEFT JOIN cards AS event_cards ON events.eventable_id = event_cards.id AND events.eventable_type = 'Card'")
+        .joins("LEFT JOIN comments AS event_comments ON events.eventable_id = event_comments.id AND events.eventable_type = 'Comment'")
+        .joins("LEFT JOIN cards AS event_comment_cards ON event_comments.card_id = event_comment_cards.id")
+        .where("(notifications.source_type = 'Event' AND events.eventable_type = 'Card' AND event_cards.collection_id = ?) OR
+              (notifications.source_type = 'Event' AND events.eventable_type = 'Comment' AND event_comment_cards.collection_id = ?)",
+               id, id)
     end
 end
diff --git a/test/models/access_test.rb b/test/models/access_test.rb
index 1bd1af0ae..9a7581674 100644
--- a/test/models/access_test.rb
+++ b/test/models/access_test.rb
@@ -15,11 +15,12 @@ class AccessTest < ActiveSupport::TestCase
     end
   end
 
-  test "notifications are destroyed when access is lost" do
+  test "event notifications are destroyed when access is lost" do
     kevin = users(:kevin)
     collection = collections(:writebook)
 
-    assert kevin.notifications.count > 0
+    # make sure we have test coverage for both cards and comments
+    assert kevin.notifications.map(&:source).map(&:eventable_type).uniq.sort == [ "Card", "Comment" ]
 
     notifications_to_be_destroyed = kevin.notifications.select do |notification|
       notification.card&.collection == collection
@@ -43,7 +44,8 @@ class AccessTest < ActiveSupport::TestCase
     david = users(:david)
     collection = collections(:writebook)
 
-    assert david.mentions.count > 0
+    # make sure we have test coverage for both cards and comments
+    assert david.mentions.map(&:source_type).uniq.sort == [ "Card", "Comment" ]
 
     mentions_to_be_destroyed = david.mentions.select do |mention|
       mention.card&.collection == collection
diff --git a/test/models/collection_test.rb b/test/models/collection/accessible_test.rb
similarity index 94%
rename from test/models/collection_test.rb
rename to test/models/collection/accessible_test.rb
index 3f61def6d..b28a5c284 100644
--- a/test/models/collection_test.rb
+++ b/test/models/collection/accessible_test.rb
@@ -1,6 +1,6 @@
 require "test_helper"
 
-class CollectionTest < ActiveSupport::TestCase
+class Collection::AccessibleTest < ActiveSupport::TestCase
   test "revising access" do
     collections(:writebook).update! all_access: false