Merge pull request #1129 from basecamp/flavorjones/perf-collection-clean-inaccessible-data

Improved performance of cleaning inaccessible data

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

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

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