From 7f7ecf0346dc6f24669bc2e0cc13db1626ee6cea Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 10 Sep 2025 10:24:20 +0200 Subject: [PATCH] Delete notifications when the user loses access to the collection --- .../remove_inaccessible_notifications_job.rb | 5 ++ app/models/card.rb | 4 ++ app/models/card/readable.rb | 18 +++++-- ...ove_inaccessible_notifications_job_test.rb | 11 +++++ test/models/card/readable_test.rb | 47 ++++++++----------- 5 files changed, 54 insertions(+), 31 deletions(-) create mode 100644 app/jobs/card/remove_inaccessible_notifications_job.rb create mode 100644 test/jobs/card/remove_inaccessible_notifications_job_test.rb diff --git a/app/jobs/card/remove_inaccessible_notifications_job.rb b/app/jobs/card/remove_inaccessible_notifications_job.rb new file mode 100644 index 000000000..db7dec4e3 --- /dev/null +++ b/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 diff --git a/app/models/card.rb b/app/models/card.rb index c9dea378c..00214fa5d 100644 --- a/app/models/card.rb +++ b/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 diff --git a/app/models/card/readable.rb b/app/models/card/readable.rb index 26b25b720..06c811715 100644 --- a/app/models/card/readable.rb +++ b/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 diff --git a/test/jobs/card/remove_inaccessible_notifications_job_test.rb b/test/jobs/card/remove_inaccessible_notifications_job_test.rb new file mode 100644 index 000000000..38b76314e --- /dev/null +++ b/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 diff --git a/test/models/card/readable_test.rb b/test/models/card/readable_test.rb index ffbfdcc44..ff4a44662 100644 --- a/test/models/card/readable_test.rb +++ b/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