From 360b6d3a442c1a95e073043e0d62e46ead7728a2 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 2 Dec 2024 10:26:04 +0100 Subject: [PATCH] Fix exclusive lists interfering with notifications (#28162) --- app/lib/feed_manager.rb | 61 +++++++++++++------------ app/workers/feed_insert_worker.rb | 20 ++++---- spec/lib/feed_manager_spec.rb | 2 + spec/workers/feed_insert_worker_spec.rb | 8 ++-- 4 files changed, 51 insertions(+), 40 deletions(-) diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index 55baee021a..8a0043268b 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -32,24 +32,31 @@ class FeedManager "feed:#{type}:#{id}:#{subtype}" end + # The filter result of the status to a particular feed + # @param [Symbol] timeline_type + # @param [Status] status + # @param [Account|List] receiver + # @return [void|Symbol] nil, :filter, or :skip_home + def filter(timeline_type, status, receiver) + case timeline_type + when :home + filter_from_home(status, receiver.id, build_crutches(receiver.id, [status]), :home) + when :list + (filter_from_list?(status, receiver) ? :filter : nil) || filter_from_home(status, receiver.account_id, build_crutches(receiver.account_id, [status]), :list) + when :mentions + filter_from_mentions?(status, receiver.id) ? :filter : nil + when :tags + filter_from_tags?(status, receiver.id, build_crutches(receiver.id, [status])) ? :filter : nil + end + end + # Check if the status should not be added to a feed # @param [Symbol] timeline_type # @param [Status] status # @param [Account|List] receiver # @return [Boolean] def filter?(timeline_type, status, receiver) - case timeline_type - when :home - filter_from_home?(status, receiver.id, build_crutches(receiver.id, [status]), :home) - when :list - filter_from_list?(status, receiver) || filter_from_home?(status, receiver.account_id, build_crutches(receiver.account_id, [status]), :list) - when :mentions - filter_from_mentions?(status, receiver.id) - when :tags - filter_from_tags?(status, receiver.id, build_crutches(receiver.id, [status])) - else - false - end + !!filter(timeline_type, status, receiver) end # Add a status to a home feed and send a streaming API update @@ -125,7 +132,7 @@ class FeedManager crutches = build_crutches(into_account.id, statuses) statuses.each do |status| - next if filter_from_home?(status, into_account.id, crutches) + next if filter_from_home(status, into_account.id, crutches) add_to_feed(:home, into_account.id, status, aggregate_reblogs: aggregate) end @@ -153,7 +160,7 @@ class FeedManager crutches = build_crutches(list.account_id, statuses) statuses.each do |status| - next if filter_from_home?(status, list.account_id, crutches) || filter_from_list?(status, list) + next if filter_from_home(status, list.account_id, crutches) || filter_from_list?(status, list) add_to_feed(:list, list.id, status, aggregate_reblogs: aggregate) end @@ -285,7 +292,7 @@ class FeedManager crutches = build_crutches(account.id, statuses) statuses.each do |status| - next if filter_from_home?(status, account.id, crutches) + next if filter_from_home(status, account.id, crutches) add_to_feed(:home, account.id, status, aggregate_reblogs: aggregate) end @@ -378,12 +385,12 @@ class FeedManager # @param [Status] status # @param [Integer] receiver_id # @param [Hash] crutches - # @return [Boolean] - def filter_from_home?(status, receiver_id, crutches, timeline_type = :home) - return false if receiver_id == status.account_id - return true if status.reply? && (status.in_reply_to_id.nil? || status.in_reply_to_account_id.nil?) - return true if timeline_type != :list && crutches[:exclusive_list_users][status.account_id].present? - return true if crutches[:languages][status.account_id].present? && status.language.present? && !crutches[:languages][status.account_id].include?(status.language) + # @return [void|Symbol] nil, :skip_home, or :filter + def filter_from_home(status, receiver_id, crutches, timeline_type = :home) + return if receiver_id == status.account_id + return :filter if status.reply? && (status.in_reply_to_id.nil? || status.in_reply_to_account_id.nil?) + return :skip_home if timeline_type != :list && crutches[:exclusive_list_users][status.account_id].present? + return :filter if crutches[:languages][status.account_id].present? && status.language.present? && !crutches[:languages][status.account_id].include?(status.language) check_for_blocks = crutches[:active_mentions][status.id] || [] check_for_blocks.push(status.account_id) @@ -393,24 +400,22 @@ class FeedManager check_for_blocks.concat(crutches[:active_mentions][status.reblog_of_id] || []) end - return true if check_for_blocks.any? { |target_account_id| crutches[:blocking][target_account_id] || crutches[:muting][target_account_id] } - return true if crutches[:blocked_by][status.account_id] + return :filter if check_for_blocks.any? { |target_account_id| crutches[:blocking][target_account_id] || crutches[:muting][target_account_id] } + return :filter if crutches[:blocked_by][status.account_id] if status.reply? && !status.in_reply_to_account_id.nil? # Filter out if it's a reply should_filter = !crutches[:following][status.in_reply_to_account_id] # and I'm not following the person it's a reply to should_filter &&= receiver_id != status.in_reply_to_account_id # and it's not a reply to me should_filter &&= status.account_id != status.in_reply_to_account_id # and it's not a self-reply - - return !!should_filter elsif status.reblog? # Filter out a reblog should_filter = crutches[:hiding_reblogs][status.account_id] # if the reblogger's reblogs are suppressed should_filter ||= crutches[:blocked_by][status.reblog.account_id] # or if the author of the reblogged status is blocking me should_filter ||= crutches[:domain_blocking][status.reblog.account.domain] # or the author's domain is blocked - - return !!should_filter + else + should_filter = false end - false + should_filter ? :filter : nil end # Check if status should not be added to the mentions feed diff --git a/app/workers/feed_insert_worker.rb b/app/workers/feed_insert_worker.rb index fd7dbd30da..e883daf3ea 100644 --- a/app/workers/feed_insert_worker.rb +++ b/app/workers/feed_insert_worker.rb @@ -29,27 +29,31 @@ class FeedInsertWorker private def check_and_insert - if feed_filtered? + filter_result = feed_filter + + if filter_result perform_unpush if update? else perform_push - perform_notify if notify? end + + perform_notify if notify?(filter_result) end - def feed_filtered? + def feed_filter case @type when :home - FeedManager.instance.filter?(:home, @status, @follower) + FeedManager.instance.filter(:home, @status, @follower) when :tags - FeedManager.instance.filter?(:tags, @status, @follower) + FeedManager.instance.filter(:tags, @status, @follower) when :list - FeedManager.instance.filter?(:list, @status, @list) + FeedManager.instance.filter(:list, @status, @list) end end - def notify? - return false if @type != :home || @status.reblog? || (@status.reply? && @status.in_reply_to_account_id != @status.account_id) + def notify?(filter_result) + return false if @type != :home || @status.reblog? || (@status.reply? && @status.in_reply_to_account_id != @status.account_id) || + filter_result == :filter Follow.find_by(account: @follower, target_account: @status.account)&.notify? end diff --git a/spec/lib/feed_manager_spec.rb b/spec/lib/feed_manager_spec.rb index afb007af42..1d3123d343 100644 --- a/spec/lib/feed_manager_spec.rb +++ b/spec/lib/feed_manager_spec.rb @@ -164,6 +164,7 @@ RSpec.describe FeedManager do allow(List).to receive(:where).and_return(list) status = Fabricate(:status, text: 'I post a lot', account: bob) expect(subject.filter?(:home, status, alice)).to be true + expect(subject.filter(:home, status, alice)).to be :skip_home end it 'returns true for reblog from followee on exclusive list' do @@ -174,6 +175,7 @@ RSpec.describe FeedManager do status = Fabricate(:status, text: 'I post a lot', account: bob) reblog = Fabricate(:status, reblog: status, account: jeff) expect(subject.filter?(:home, reblog, alice)).to be true + expect(subject.filter(:home, reblog, alice)).to be :skip_home end it 'returns false for post from followee on non-exclusive list' do diff --git a/spec/workers/feed_insert_worker_spec.rb b/spec/workers/feed_insert_worker_spec.rb index 92ae304d0e..9d1279bb89 100644 --- a/spec/workers/feed_insert_worker_spec.rb +++ b/spec/workers/feed_insert_worker_spec.rb @@ -32,7 +32,7 @@ RSpec.describe FeedInsertWorker do context 'when there are real records' do it 'skips the push when there is a filter' do - instance = instance_double(FeedManager, push_to_home: nil, filter?: true) + instance = instance_double(FeedManager, push_to_home: nil, filter?: true, filter: :filter) allow(FeedManager).to receive(:instance).and_return(instance) result = subject.perform(status.id, follower.id) @@ -41,7 +41,7 @@ RSpec.describe FeedInsertWorker do end it 'pushes the status onto the home timeline without filter' do - instance = instance_double(FeedManager, push_to_home: nil, filter?: false) + instance = instance_double(FeedManager, push_to_home: nil, filter?: false, filter: nil) allow(FeedManager).to receive(:instance).and_return(instance) result = subject.perform(status.id, follower.id, :home) @@ -50,7 +50,7 @@ RSpec.describe FeedInsertWorker do end it 'pushes the status onto the tags timeline without filter' do - instance = instance_double(FeedManager, push_to_home: nil, filter?: false) + instance = instance_double(FeedManager, push_to_home: nil, filter?: false, filter: nil) allow(FeedManager).to receive(:instance).and_return(instance) result = subject.perform(status.id, follower.id, :tags) @@ -59,7 +59,7 @@ RSpec.describe FeedInsertWorker do end it 'pushes the status onto the list timeline without filter' do - instance = instance_double(FeedManager, push_to_list: nil, filter?: false) + instance = instance_double(FeedManager, push_to_list: nil, filter?: false, filter: nil) allow(FeedManager).to receive(:instance).and_return(instance) result = subject.perform(status.id, list.id, :list)