Delete notifications when the user loses access to the collection

app/jobs/card/remove_inaccessible_notifications_job.rb

@@ -0,0 +1,5 @@
+class Card::RemoveInaccessibleNotificationsJob < ApplicationJob
+  def perform(card)
+    card.remove_inaccessible_notifications
+  end
+end

app/models/card.rb

@@ -39,6 +39,8 @@ class Card < ApplicationRecord
     end
   end
 
+  delegate :accessible_to?, to: :collection
+
   def cache_key
     [ super, collection.name ].compact.join("/")
   end
@@ -64,6 +66,8 @@ class Card < ApplicationRecord
         track_collection_change_event
         grant_access_to_assignees unless collection.all_access?
       end
+
+      remove_inaccessible_notifications_later
     end
 
     def track_collection_change_event

app/models/card/readable.rb

@@ -7,13 +7,23 @@ module Card::Readable
     end
   end
 
-  def notifications_for(user)
-    scope = user.notifications.unread
-    scope.where(source: event_notification_sources)
-      .or(scope.where(source: mention_notification_sources))
+  def remove_inaccessible_notifications
+    User.find_each do |user|
+      notifications_for(user).destroy_all unless accessible_to?(user)
+    end
   end
 
   private
+    def remove_inaccessible_notifications_later
+      Card::RemoveInaccessibleNotificationsJob.perform_later(self)
+    end
+
+    def notifications_for(user)
+      scope = user.notifications.unread
+      scope.where(source: event_notification_sources)
+        .or(scope.where(source: mention_notification_sources))
+    end
+
     def event_notification_sources
       events.or(comment_creation_events)
     end

test/jobs/card/remove_inaccessible_notifications_job_test.rb

@@ -0,0 +1,11 @@
+require "test_helper"
+
+class Card::RemoveInaccessibleNotificationsJobTest < ActiveJob::TestCase
+  test "calls remove_inaccessible_notifications on the card" do
+    card = cards(:logo)
+
+    Card.any_instance.expects(:remove_inaccessible_notifications)
+
+    Card::RemoveInaccessibleNotificationsJob.perform_now(card)
+  end
+end

test/models/card/readable_test.rb

@@ -27,37 +27,30 @@ class Card::ReadableTest < ActiveSupport::TestCase
     end
   end
 
-  test "notifications for a given user" do
-    # Returns card event notifications
-    kevin_notifications = cards(:logo).notifications_for(users(:kevin))
-    assert_includes kevin_notifications, notifications(:logo_published_kevin)
-    assert_includes kevin_notifications, notifications(:logo_assignment_kevin)
+  test "remove inaccessible notifications" do
+    card = cards(:logo)
+    kevin = users(:kevin)
+    david = users(:david)
 
-    # Returns comment creation event notifications
-    layout_notifications = cards(:layout).notifications_for(users(:kevin))
-    assert_includes layout_notifications, notifications(:layout_commented_kevin)
+    assert card.accessible_to?(kevin)
+    kevin_notifications = [ notifications(:logo_published_kevin), notifications(:logo_assignment_kevin) ]
+    david_notifications = [ notifications(:logo_card_david_mention_by_jz), notifications(:logo_comment_david_mention_by_jz) ]
 
-    # Returns card mention notifications
-    david_notifications = cards(:logo).notifications_for(users(:david))
-    assert_includes david_notifications, notifications(:logo_card_david_mention_by_jz)
+    # Kevin loses access
+    card.collection.accesses.find_by(user: kevin).destroy
+    assert_not card.accessible_to?(kevin)
+    assert card.accessible_to?(david)
 
-    # Returns comment mention notifications
-    assert_includes david_notifications, notifications(:logo_comment_david_mention_by_jz)
+    card.remove_inaccessible_notifications
 
-    # Only returns unread notifications
-    notifications(:logo_published_kevin).read
-    kevin_notifications_after_read = cards(:logo).notifications_for(users(:kevin))
-    assert_not_includes kevin_notifications_after_read, notifications(:logo_published_kevin)
-    assert_includes kevin_notifications_after_read, notifications(:logo_assignment_kevin)
+    # Kevin's notifications removed
+    kevin_notifications.each do |notification|
+      assert_not Notification.exists?(notification.id)
+    end
 
-    # Does not include notifications from other cards
-    other_event = events(:text_published)
-    other_notification = users(:kevin).notifications.create!(source: other_event, creator: users(:david))
-    logo_notifications = cards(:logo).notifications_for(users(:kevin))
-    assert_not_includes logo_notifications, other_notification
-
-    # Does not include notifications for other users
-    assert_not_includes kevin_notifications, notifications(:logo_card_david_mention_by_jz)
-    assert_not_includes kevin_notifications, notifications(:logo_comment_david_mention_by_jz)
+    # David's notifications preserved
+    david_notifications.each do |notification|
+      assert Notification.exists?(notification.id)
+    end
   end
 end