commit: effb08edbbaaf61bac4b4fc5c5b9493417b348ec
parent: d1b4ebe07d67d8a0aec3a012ca92f2106ebc9377
Author: Matt Jankowski <mjankowski@thoughtbot.com>
Date: Fri, 28 Apr 2017 09:10:41 -0400
More status specs (#2564)
* Add rough outline of coverage needed for public timeline
* Specs for visibility, replies, boosts
* Specs for silenced account
* Specs for local_only option
* Specs for blocks and mutes
* Add tentative spec around including other silenced account statuses
* Add with_public_visibility scope
* Add simple coverage for tag_timeline
* Tag timeline includes replies
* Replace tag.statuses with a tagged_with scope in tag timeline method
* Use with_public_visibility in tag timeline
* Extract common scope between public and tag timelines to method
* Extract local domain check to local_only scope
* Extract local_only check to starting scope method
* Move list of excluded from timeline account ids to account model
* Simplify excluded accounts list on account model
* Only join accounts when needed
* Rename method for account specific filtering
* Extract method for account exclusions
* Fix bug where silenced accounts were not including statuses from other silenced accounts
* DRY up filter application from account or no account
* timeline_scope can be private
* Add spec showing that account can find its excluded accounts ids
* Add spec which fails if local_only does not have a left outer join
* rubocop
Diffstat:
5 files changed, 191 insertions(+), 21 deletions(-)
diff --git a/app/models/account.rb b/app/models/account.rb
@@ -46,6 +46,8 @@ class Account < ApplicationRecord
# Block relationships
has_many :block_relationships, class_name: 'Block', foreign_key: 'account_id', dependent: :destroy
has_many :blocking, -> { order('blocks.id desc') }, through: :block_relationships, source: :target_account
+ has_many :blocked_by_relationships, class_name: 'Block', foreign_key: :target_account_id, dependent: :destroy
+ has_many :blocked_by, -> { order('blocks.id desc') }, through: :blocked_by_relationships, source: :account
# Mute relationships
has_many :mute_relationships, class_name: 'Mute', foreign_key: 'account_id', dependent: :destroy
@@ -211,6 +213,10 @@ class Account < ApplicationRecord
username
end
+ def excluded_from_timeline_account_ids
+ Rails.cache.fetch("exclude_account_ids_for:#{id}") { blocking.pluck(:target_account_id) + blocked_by.pluck(:account_id) + muting.pluck(:target_account_id) }
+ end
+
class << self
def find_local!(username)
find_remote!(username, nil)
diff --git a/app/models/status.rb b/app/models/status.rb
@@ -37,6 +37,12 @@ class Status < ApplicationRecord
scope :without_replies, -> { where('statuses.reply = FALSE OR statuses.in_reply_to_account_id = statuses.account_id') }
scope :without_reblogs, -> { where('statuses.reblog_of_id IS NULL') }
+ scope :with_public_visibility, -> { where(visibility: :public) }
+ scope :tagged_with, ->(tag) { joins(:statuses_tags).where(statuses_tags: { tag_id: tag }) }
+ scope :local_only, -> { left_outer_joins(:account).where(accounts: { domain: nil }) }
+ scope :excluding_silenced_accounts, -> { left_outer_joins(:account).where(accounts: { silenced: false }) }
+ scope :including_silenced_accounts, -> { left_outer_joins(:account).where(accounts: { silenced: true }) }
+ scope :not_excluded_by_account, ->(account) { where.not(account_id: account.excluded_from_timeline_account_ids) }
cache_associated :account, :application, :media_attachments, :tags, :stream_entry, mentions: :account, reblog: [:account, :application, :stream_entry, :tags, :media_attachments, mentions: :account], thread: :account
@@ -118,25 +124,15 @@ class Status < ApplicationRecord
end
def as_public_timeline(account = nil, local_only = false)
- query = joins('LEFT OUTER JOIN accounts ON statuses.account_id = accounts.id')
- .where(visibility: :public)
- .without_replies
- .without_reblogs
+ query = timeline_scope(local_only).without_replies
- query = query.where('accounts.domain IS NULL') if local_only
-
- account.nil? ? filter_timeline_default(query) : filter_timeline_default(filter_timeline(query, account))
+ apply_timeline_filters(query, account)
end
def as_tag_timeline(tag, account = nil, local_only = false)
- query = tag.statuses
- .joins('LEFT OUTER JOIN accounts ON statuses.account_id = accounts.id')
- .where(visibility: :public)
- .without_reblogs
-
- query = query.where('accounts.domain IS NULL') if local_only
+ query = timeline_scope(local_only).tagged_with(tag)
- account.nil? ? filter_timeline_default(query) : filter_timeline_default(filter_timeline(query, account))
+ apply_timeline_filters(query, account)
end
def as_outbox_timeline(account)
@@ -185,15 +181,36 @@ class Status < ApplicationRecord
private
- def filter_timeline(query, account)
- blocked = Rails.cache.fetch("exclude_account_ids_for:#{account.id}") { Block.where(account: account).pluck(:target_account_id) + Block.where(target_account: account).pluck(:account_id) + Mute.where(account: account).pluck(:target_account_id) }
- query = query.where('statuses.account_id NOT IN (?)', blocked) unless blocked.empty? # Only give us statuses from people we haven't blocked, or muted, or that have blocked us
- query = query.where('accounts.silenced = TRUE') if account.silenced? # and if we're hellbanned, only people who are also hellbanned
- query
+ def timeline_scope(local_only = false)
+ starting_scope = local_only ? Status.local_only : Status
+ starting_scope
+ .with_public_visibility
+ .without_reblogs
+ end
+
+ def apply_timeline_filters(query, account)
+ if account.nil?
+ filter_timeline_default(query)
+ else
+ filter_timeline_for_account(query, account)
+ end
+ end
+
+ def filter_timeline_for_account(query, account)
+ query = query.not_excluded_by_account(account)
+ query.merge(account_silencing_filter(account))
end
def filter_timeline_default(query)
- query.where('accounts.silenced = FALSE')
+ query.excluding_silenced_accounts
+ end
+
+ def account_silencing_filter(account)
+ if account.silenced?
+ including_silenced_accounts
+ else
+ excluding_silenced_accounts
+ end
end
end
diff --git a/spec/fabricators/mute_fabricator.rb b/spec/fabricators/mute_fabricator.rb
@@ -1,3 +1,4 @@
Fabricator(:mute) do
-
+ account
+ target_account { Fabricate(:account) }
end
diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb
@@ -190,6 +190,21 @@ RSpec.describe Account, type: :model do
end
end
+ describe '#excluded_from_timeline_account_ids' do
+ it 'includes account ids of blockings, blocked_bys and mutes' do
+ account = Fabricate(:account)
+ block = Fabricate(:block, account: account)
+ mute = Fabricate(:mute, account: account)
+ block_by = Fabricate(:block, target_account: account)
+
+ results = account.excluded_from_timeline_account_ids
+ expect(results.size).to eq 3
+ expect(results).to include(block.target_account.id)
+ expect(results).to include(mute.target_account.id)
+ expect(results).to include(block_by.account.id)
+ end
+ end
+
describe '.search_for' do
before do
@match = Fabricate(
diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb
@@ -127,6 +127,19 @@ RSpec.describe Status, type: :model do
pending
end
+ describe '.local_only' do
+ it 'returns only statuses from local accounts' do
+ local_account = Fabricate(:account, domain: nil)
+ remote_account = Fabricate(:account, domain: 'test.com')
+ local_status = Fabricate(:status, account: local_account)
+ remote_status = Fabricate(:status, account: remote_account)
+
+ results = described_class.local_only
+ expect(results).to include(local_status)
+ expect(results).not_to include(remote_status)
+ end
+ end
+
describe '.as_home_timeline' do
before do
account = Fabricate(:account)
@@ -153,4 +166,122 @@ RSpec.describe Status, type: :model do
expect(@results).not_to include(@not_followed_status)
end
end
+
+ describe '.as_public_timeline' do
+ it 'only includes statuses with public visibility' do
+ public_status = Fabricate(:status, visibility: :public)
+ private_status = Fabricate(:status, visibility: :private)
+
+ results = Status.as_public_timeline
+ expect(results).to include(public_status)
+ expect(results).not_to include(private_status)
+ end
+
+ it 'does not include replies' do
+ status = Fabricate(:status)
+ reply = Fabricate(:status, in_reply_to_id: status.id)
+
+ results = Status.as_public_timeline
+ expect(results).to include(status)
+ expect(results).not_to include(reply)
+ end
+
+ it 'does not include boosts' do
+ status = Fabricate(:status)
+ boost = Fabricate(:status, reblog_of_id: status.id)
+
+ results = Status.as_public_timeline
+ expect(results).to include(status)
+ expect(results).not_to include(boost)
+ end
+
+ it 'filters out silenced accounts' do
+ account = Fabricate(:account)
+ silenced_account = Fabricate(:account, silenced: true)
+ status = Fabricate(:status, account: account)
+ silenced_status = Fabricate(:status, account: silenced_account)
+
+ results = Status.as_public_timeline
+ expect(results).to include(status)
+ expect(results).not_to include(silenced_status)
+ end
+
+ context 'with a local_only option set' do
+ it 'does not include remote instances statuses' do
+ local_account = Fabricate(:account, domain: nil)
+ remote_account = Fabricate(:account, domain: 'test.com')
+ local_status = Fabricate(:status, account: local_account)
+ remote_status = Fabricate(:status, account: remote_account)
+
+ results = Status.as_public_timeline(nil, true)
+ expect(results).to include(local_status)
+ expect(results).not_to include(remote_status)
+ end
+ end
+
+ describe 'with an account passed in' do
+ before do
+ @account = Fabricate(:account)
+ end
+
+ it 'excludes statuses from accounts blocked by the account' do
+ blocked = Fabricate(:account)
+ Fabricate(:block, account: @account, target_account: blocked)
+ blocked_status = Fabricate(:status, account: blocked)
+
+ results = Status.as_public_timeline(@account)
+ expect(results).not_to include(blocked_status)
+ end
+
+ it 'excludes statuses from accounts who have blocked the account' do
+ blocked = Fabricate(:account)
+ Fabricate(:block, account: blocked, target_account: @account)
+ blocked_status = Fabricate(:status, account: blocked)
+
+ results = Status.as_public_timeline(@account)
+ expect(results).not_to include(blocked_status)
+ end
+
+ it 'excludes statuses from accounts muted by the account' do
+ muted = Fabricate(:account)
+ Fabricate(:mute, account: @account, target_account: muted)
+ muted_status = Fabricate(:status, account: muted)
+
+ results = Status.as_public_timeline(@account)
+ expect(results).not_to include(muted_status)
+ end
+
+ context 'where that account is silenced' do
+ it 'includes statuses from other accounts that are silenced' do
+ @account.update(silenced: true)
+ other_silenced_account = Fabricate(:account, silenced: true)
+ other_status = Fabricate(:status, account: other_silenced_account)
+
+ results = Status.as_public_timeline(@account)
+ expect(results).to include(other_status)
+ end
+ end
+ end
+ end
+
+ describe '.as_tag_timeline' do
+ it 'includes statuses with a tag' do
+ tag = Fabricate(:tag)
+ status = Fabricate(:status, tags: [tag])
+ other = Fabricate(:status)
+
+ results = Status.as_tag_timeline(tag)
+ expect(results).to include(status)
+ expect(results).not_to include(other)
+ end
+
+ it 'allows replies to be included' do
+ original = Fabricate(:status)
+ tag = Fabricate(:tag)
+ status = Fabricate(:status, tags: [tag], in_reply_to_id: original.id)
+
+ results = Status.as_tag_timeline(tag)
+ expect(results).to include(status)
+ end
+ end
end
