Introduce StatusThreadingConcern (#3490)

* Add a StatusFilter class to identify visibility of statuses by accounts

* Extract StatusThreadingConcern from Status

* Clarify purpose of checking for nil account

app/lib/status_filter.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+class StatusFilter
+  attr_reader :status, :account
+
+  def initialize(status, account)
+    @status = status
+    @account = account
+  end
+
+  def filtered?
+    account_present? && filtered_status?
+  end
+
+  private
+
+  def account_present?
+    !account.nil?
+  end
+
+  def filtered_status?
+    blocking_account? || blocking_domain? || muting_account? || silenced_account? || blocked_by_policy?
+  end
+
+  def blocking_account?
+    account.blocking? status.account_id
+  end
+
+  def blocking_domain?
+    account.domain_blocking? status.account_domain
+  end
+
+  def muting_account?
+    account.muting? status.account_id
+  end
+
+  def silenced_account?
+    status_account_silenced? && !account_following_status_account?
+  end
+
+  def status_account_silenced?
+    status.account.silenced?
+  end
+
+  def account_following_status_account?
+    account.following? status.account_id
+  end
+
+  def blocked_by_policy?
+    !policy_allows_show?
+  end
+
+  def policy_allows_show?
+    StatusPolicy.new(account, status).show?
+  end
+end

app/models/concerns/status_threading_concern.rb

@@ -0,0 +1,89 @@
+# frozen_string_literal: true
+
+module StatusThreadingConcern
+  extend ActiveSupport::Concern
+
+  def ancestors(account = nil)
+    find_statuses_from_tree_path(ancestor_ids, account)
+  end
+
+  def descendants(account = nil)
+    find_statuses_from_tree_path(descendant_ids, account)
+  end
+
+  private
+
+  def ancestor_ids
+    Rails.cache.fetch("ancestors:#{id}") do
+      ancestors_without_self.pluck(:id)
+    end
+  end
+
+  def ancestors_without_self
+    ancestor_statuses - [self]
+  end
+
+  def ancestor_statuses
+    Status.find_by_sql([<<-SQL.squish, id: id])
+      WITH RECURSIVE search_tree(id, in_reply_to_id, path)
+      AS (
+        SELECT id, in_reply_to_id, ARRAY[id]
+        FROM statuses
+        WHERE id = :id
+        UNION ALL
+        SELECT statuses.id, statuses.in_reply_to_id, path || statuses.id
+        FROM search_tree
+        JOIN statuses ON statuses.id = search_tree.in_reply_to_id
+        WHERE NOT statuses.id = ANY(path)
+      )
+      SELECT id
+      FROM search_tree
+      ORDER BY path DESC
+    SQL
+  end
+
+  def descendant_ids
+    descendants_without_self.pluck(:id)
+  end
+
+  def descendants_without_self
+    descendant_statuses - [self]
+  end
+
+  def descendant_statuses
+    Status.find_by_sql([<<-SQL.squish, id: id])
+      WITH RECURSIVE search_tree(id, path)
+      AS (
+        SELECT id, ARRAY[id]
+        FROM statuses
+        WHERE id = :id
+        UNION ALL
+        SELECT statuses.id, path || statuses.id
+        FROM search_tree
+        JOIN statuses ON statuses.in_reply_to_id = search_tree.id
+        WHERE NOT statuses.id = ANY(path)
+      )
+      SELECT id
+      FROM search_tree
+      ORDER BY path
+    SQL
+  end
+
+  def find_statuses_from_tree_path(ids, account)
+    statuses = statuses_with_accounts(ids).to_a
+
+    # FIXME: n+1 bonanza
+    statuses.reject! { |status| filter_from_context?(status, account) }
+
+    # Order ancestors/descendants by tree path
+    statuses.sort_by! { |status| ids.index(status.id) }
+  end
+
+  def statuses_with_accounts(ids)
+    Status.where(id: ids).includes(:account)
+  end
+
+  def filter_from_context?(status, account)
+    StatusFilter.new(status, account).filtered?
+  end
+end

app/models/status.rb

@@ -28,6 +28,7 @@ class Status < ApplicationRecord
   include Paginable
   include Streamable
   include Cacheable
+  include StatusThreadingConcern
 
   enum visibility: [:public, :unlisted, :private, :direct], _suffix: :visibility
 
@@ -115,16 +116,6 @@ class Status < ApplicationRecord
     private_visibility? || direct_visibility?
   end
 
-  def ancestors(account = nil)
-    ids = Rails.cache.fetch("ancestors:#{id}") { (Status.find_by_sql(['WITH RECURSIVE search_tree(id, in_reply_to_id, path) AS (SELECT id, in_reply_to_id, ARRAY[id] FROM statuses WHERE id = ? UNION ALL SELECT statuses.id, statuses.in_reply_to_id, path || statuses.id FROM search_tree JOIN statuses ON statuses.id = search_tree.in_reply_to_id WHERE NOT statuses.id = ANY(path)) SELECT id FROM search_tree ORDER BY path DESC', id]) - [self]).pluck(:id) }
-    find_statuses_from_tree_path(ids, account)
-  end
-
-  def descendants(account = nil)
-    ids = (Status.find_by_sql(['WITH RECURSIVE search_tree(id, path) AS (SELECT id, ARRAY[id] FROM statuses WHERE id = ? UNION ALL SELECT statuses.id, path || statuses.id FROM search_tree JOIN statuses ON statuses.in_reply_to_id = search_tree.id WHERE NOT statuses.id = ANY(path)) SELECT id FROM search_tree ORDER BY path', id]) - [self]).pluck(:id)
-    find_statuses_from_tree_path(ids, account)
-  end
-
   def non_sensitive_with_media?
     !sensitive? && media_attachments.any?
   end
@@ -277,23 +268,4 @@ class Status < ApplicationRecord
       thread.account_id
     end
   end
-
-  def find_statuses_from_tree_path(ids, account)
-    statuses = Status.where(id: ids).includes(:account).to_a
-
-    # FIXME: n+1 bonanza
-    statuses.reject! { |status| filter_from_context?(status, account) }
-
-    # Order ancestors/descendants by tree path
-    statuses.sort_by! { |status| ids.index(status.id) }
-  end
-
-  def filter_from_context?(status, account)
-    should_filter   = account&.blocking?(status.account_id)
-    should_filter ||= account&.domain_blocking?(status.account_domain)
-    should_filter ||= account&.muting?(status.account_id)
-    should_filter ||= (status.account.silenced? && !account&.following?(status.account_id))
-    should_filter ||= !StatusPolicy.new(account, status).show?
-    should_filter
-  end
 end

spec/lib/status_filter_spec.rb

@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe StatusFilter do
+  describe '#filtered?' do
+    let(:status) { Fabricate(:status) }
+
+    context 'without an account' do
+      subject { described_class.new(status, nil) }
+
+      it { is_expected.not_to be_filtered }
+    end
+
+    context 'with real account' do
+      let(:account) { Fabricate(:account) }
+      subject { described_class.new(status, account) }
+
+      context 'when there are no connections' do
+        it { is_expected.not_to be_filtered }
+      end
+
+      context 'when status account is blocked' do
+        before do
+          Fabricate(:block, account: account, target_account: status.account)
+        end
+
+        it { is_expected.to be_filtered }
+      end
+
+      context 'when status account domain is blocked' do
+        before do
+          status.account.update(domain: 'example.com')
+          Fabricate(:account_domain_block, account: account, domain: status.account_domain)
+        end
+
+        it { is_expected.to be_filtered }
+      end
+
+      context 'when status account is muted' do
+        before do
+          Fabricate(:mute, account: account, target_account: status.account)
+        end
+
+        it { is_expected.to be_filtered }
+      end
+
+      context 'when status account is silenced' do
+        before do
+          status.account.update(silenced: true)
+        end
+
+        it { is_expected.to be_filtered }
+      end
+
+      context 'when status policy does not allow show' do
+        before do
+          expect_any_instance_of(StatusPolicy).to receive(:show?).and_return(false)
+        end
+
+        it { is_expected.to be_filtered }
+      end
+    end
+  end
+end

spec/models/concerns/status_threading_concern_spec.rb

@@ -0,0 +1,100 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe StatusThreadingConcern do
+  describe '#ancestors' do
+    let!(:alice)  { Fabricate(:account, username: 'alice') }
+    let!(:bob)    { Fabricate(:account, username: 'bob', domain: 'example.com') }
+    let!(:jeff)   { Fabricate(:account, username: 'jeff') }
+    let!(:status) { Fabricate(:status, account: alice) }
+    let!(:reply1) { Fabricate(:status, thread: status, account: jeff) }
+    let!(:reply2) { Fabricate(:status, thread: reply1, account: bob) }
+    let!(:reply3) { Fabricate(:status, thread: reply2, account: alice) }
+    let!(:viewer) { Fabricate(:account, username: 'viewer') }
+
+    it 'returns conversation history' do
+      expect(reply3.ancestors).to include(status, reply1, reply2)
+    end
+
+    it 'does not return conversation history user is not allowed to see' do
+      reply1.update(visibility: :private)
+      status.update(visibility: :direct)
+
+      expect(reply3.ancestors(viewer)).to_not include(reply1, status)
+    end
+
+    it 'does not return conversation history from blocked users' do
+      viewer.block!(jeff)
+      expect(reply3.ancestors(viewer)).to_not include(reply1)
+    end
+
+    it 'does not return conversation history from muted users' do
+      viewer.mute!(jeff)
+      expect(reply3.ancestors(viewer)).to_not include(reply1)
+    end
+
+    it 'does not return conversation history from silenced and not followed users' do
+      jeff.update(silenced: true)
+      expect(reply3.ancestors(viewer)).to_not include(reply1)
+    end
+
+    it 'does not return conversation history from blocked domains' do
+      viewer.block_domain!('example.com')
+      expect(reply3.ancestors(viewer)).to_not include(reply2)
+    end
+
+    it 'ignores deleted records' do
+      first_status  = Fabricate(:status, account: bob)
+      second_status = Fabricate(:status, thread: first_status, account: alice)
+
+      # Create cache and delete cached record
+      second_status.ancestors
+      first_status.destroy
+
+      expect(second_status.ancestors).to eq([])
+    end
+  end
+
+  describe '#descendants' do
+    let!(:alice)  { Fabricate(:account, username: 'alice') }
+    let!(:bob)    { Fabricate(:account, username: 'bob', domain: 'example.com') }
+    let!(:jeff)   { Fabricate(:account, username: 'jeff') }
+    let!(:status) { Fabricate(:status, account: alice) }
+    let!(:reply1) { Fabricate(:status, thread: status, account: alice) }
+    let!(:reply2) { Fabricate(:status, thread: status, account: bob) }
+    let!(:reply3) { Fabricate(:status, thread: reply1, account: jeff) }
+    let!(:viewer) { Fabricate(:account, username: 'viewer') }
+
+    it 'returns replies' do
+      expect(status.descendants).to include(reply1, reply2, reply3)
+    end
+
+    it 'does not return replies user is not allowed to see' do
+      reply1.update(visibility: :private)
+      reply3.update(visibility: :direct)
+
+      expect(status.descendants(viewer)).to_not include(reply1, reply3)
+    end
+
+    it 'does not return replies from blocked users' do
+      viewer.block!(jeff)
+      expect(status.descendants(viewer)).to_not include(reply3)
+    end
+
+    it 'does not return replies from muted users' do
+      viewer.mute!(jeff)
+      expect(status.descendants(viewer)).to_not include(reply3)
+    end
+
+    it 'does not return replies from silenced and not followed users' do
+      jeff.update(silenced: true)
+      expect(status.descendants(viewer)).to_not include(reply3)
+    end
+
+    it 'does not return replies from blocked domains' do
+      viewer.block_domain!('example.com')
+      expect(status.descendants(viewer)).to_not include(reply2)
+    end
+  end
+end

spec/models/status_spec.rb

@@ -119,101 +119,6 @@ RSpec.describe Status, type: :model do
     end
   end
 
-  describe '#ancestors' do
-    let!(:alice)  { Fabricate(:account, username: 'alice') }
-    let!(:bob)    { Fabricate(:account, username: 'bob', domain: 'example.com') }
-    let!(:jeff)   { Fabricate(:account, username: 'jeff') }
-    let!(:status) { Fabricate(:status, account: alice) }
-    let!(:reply1) { Fabricate(:status, thread: status, account: jeff) }
-    let!(:reply2) { Fabricate(:status, thread: reply1, account: bob) }
-    let!(:reply3) { Fabricate(:status, thread: reply2, account: alice) }
-    let!(:viewer) { Fabricate(:account, username: 'viewer') }
-
-    it 'returns conversation history' do
-      expect(reply3.ancestors).to include(status, reply1, reply2)
-    end
-
-    it 'does not return conversation history user is not allowed to see' do
-      reply1.update(visibility: :private)
-      status.update(visibility: :direct)
-
-      expect(reply3.ancestors(viewer)).to_not include(reply1, status)
-    end
-
-    it 'does not return conversation history from blocked users' do
-      viewer.block!(jeff)
-      expect(reply3.ancestors(viewer)).to_not include(reply1)
-    end
-
-    it 'does not return conversation history from muted users' do
-      viewer.mute!(jeff)
-      expect(reply3.ancestors(viewer)).to_not include(reply1)
-    end
-
-    it 'does not return conversation history from silenced and not followed users' do
-      jeff.update(silenced: true)
-      expect(reply3.ancestors(viewer)).to_not include(reply1)
-    end
-
-    it 'does not return conversation history from blocked domains' do
-      viewer.block_domain!('example.com')
-      expect(reply3.ancestors(viewer)).to_not include(reply2)
-    end
-
-    it 'ignores deleted records' do
-      first_status  = Fabricate(:status, account: bob)
-      second_status = Fabricate(:status, thread: first_status, account: alice)
-
-      # Create cache and delete cached record
-      second_status.ancestors
-      first_status.destroy
-
-      expect(second_status.ancestors).to eq([])
-    end
-  end
-
-  describe '#descendants' do
-    let!(:alice)  { Fabricate(:account, username: 'alice') }
-    let!(:bob)    { Fabricate(:account, username: 'bob', domain: 'example.com') }
-    let!(:jeff)   { Fabricate(:account, username: 'jeff') }
-    let!(:status) { Fabricate(:status, account: alice) }
-    let!(:reply1) { Fabricate(:status, thread: status, account: alice) }
-    let!(:reply2) { Fabricate(:status, thread: status, account: bob) }
-    let!(:reply3) { Fabricate(:status, thread: reply1, account: jeff) }
-    let!(:viewer) { Fabricate(:account, username: 'viewer') }
-
-    it 'returns replies' do
-      expect(status.descendants).to include(reply1, reply2, reply3)
-    end
-
-    it 'does not return replies user is not allowed to see' do
-      reply1.update(visibility: :private)
-      reply3.update(visibility: :direct)
-
-      expect(status.descendants(viewer)).to_not include(reply1, reply3)
-    end
-
-    it 'does not return replies from blocked users' do
-      viewer.block!(jeff)
-      expect(status.descendants(viewer)).to_not include(reply3)
-    end
-
-    it 'does not return replies from muted users' do
-      viewer.mute!(jeff)
-      expect(status.descendants(viewer)).to_not include(reply3)
-    end
-
-    it 'does not return replies from silenced and not followed users' do
-      jeff.update(silenced: true)
-      expect(status.descendants(viewer)).to_not include(reply3)
-    end
-
-    it 'does not return replies from blocked domains' do
-      viewer.block_domain!('example.com')
-      expect(status.descendants(viewer)).to_not include(reply2)
-    end
-  end
-
   describe '.mutes_map' do
     let(:status)  { Fabricate(:status) }
     let(:account) { Fabricate(:account) }