Fix exclusive lists interfering with notifications (#28162)

app/lib/feed_manager.rb

@@ -32,26 +32,33 @@ class FeedManager
     "feed:#{type}:#{id}:#{subtype}"
   end
 
-  # Check if the status should not be added to a feed
+  # The filter result of the status to a particular feed
   # @param [Symbol] timeline_type
   # @param [Status] status
   # @param [Account|List] receiver
-  # @return [Boolean]
-  def filter?(timeline_type, status, 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)
+      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)
+      (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_from_mentions?(status, receiver.id) ? :filter : nil
     when :tags
-      filter_from_tags?(status, receiver.id, build_crutches(receiver.id, [status]))
-    else
-      false
+      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)
+    !!filter(timeline_type, status, receiver)
+  end
+
   # Add a status to a home feed and send a streaming API update
   # @param [Account] account
   # @param [Status] status
@@ -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

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

spec/lib/feed_manager_spec.rb

@@ -162,6 +162,7 @@ RSpec.describe FeedManager do
         allow(List).to receive(:where).and_return(list)
         status = Fabricate(:status, text: 'I post a lot', account: bob)
         expect(described_class.instance.filter?(:home, status, alice)).to be true
+        expect(described_class.instance.filter(:home, status, alice)).to be :skip_home
       end
 
       it 'returns true for reblog from followee on exclusive list' do
@@ -172,6 +173,7 @@ RSpec.describe FeedManager do
         status = Fabricate(:status, text: 'I post a lot', account: bob)
         reblog = Fabricate(:status, reblog: status, account: jeff)
         expect(described_class.instance.filter?(:home, reblog, alice)).to be true
+        expect(described_class.instance.filter(:home, reblog, alice)).to be :skip_home
       end
 
       it 'returns false for post from followee on non-exclusive list' do

spec/workers/feed_insert_worker_spec.rb

@@ -8,6 +8,7 @@ describe FeedInsertWorker do
   describe 'perform' do
     let(:follower) { Fabricate(:account) }
     let(:status) { Fabricate(:status) }
+    let(:list) { Fabricate(:list) }
 
     context 'when there are no records' do
       it 'skips push with missing status' do
@@ -31,7 +32,7 @@ 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)
 
@@ -40,13 +41,31 @@ 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)
 
         expect(result).to be_nil
         expect(instance).to have_received(:push_to_home).with(follower, status, update: nil)
       end
+
+      it 'pushes the status onto the tags timeline without filter' do
+        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)
+
+        expect(result).to be_nil
+        expect(instance).to have_received(:push_to_home).with(follower, status, update: nil)
+      end
+
+      it 'pushes the status onto the list timeline without filter' do
+        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)
+
+        expect(result).to be_nil
+        expect(instance).to have_received(:push_to_list).with(list, status, update: nil)
+      end
     end
   end
 end