From ee8d0b94473df357677cd1f82581251ce0423c01 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 4 Mar 2024 07:35:20 +0100 Subject: [PATCH] Fix follow suggestions potentially including silenced or blocked accounts (#29306) --- .../friends_of_friends_source.rb | 34 ++------ .../similar_profiles_source.rb | 3 +- app/models/account_suggestions/source.rb | 2 + .../friends_of_friends_source_spec.rb | 82 +++++++++++++++++++ .../models/account_suggestions/source_spec.rb | 20 +++-- 5 files changed, 107 insertions(+), 34 deletions(-) create mode 100644 spec/models/account_suggestions/friends_of_friends_source_spec.rb diff --git a/app/models/account_suggestions/friends_of_friends_source.rb b/app/models/account_suggestions/friends_of_friends_source.rb index 93fb10f3b0..0c95d21a3e 100644 --- a/app/models/account_suggestions/friends_of_friends_source.rb +++ b/app/models/account_suggestions/friends_of_friends_source.rb @@ -2,31 +2,15 @@ class AccountSuggestions::FriendsOfFriendsSource < AccountSuggestions::Source def get(account, limit: DEFAULT_LIMIT) - Account.find_by_sql([<<~SQL.squish, { id: account.id, limit: limit }]).map { |row| [row.id, key] } - WITH first_degree AS ( - SELECT target_account_id - FROM follows - JOIN accounts AS target_accounts ON follows.target_account_id = target_accounts.id - WHERE account_id = :id - AND NOT target_accounts.hide_collections - ) - SELECT accounts.id, COUNT(*) AS frequency - FROM accounts - JOIN follows ON follows.target_account_id = accounts.id - JOIN account_stats ON account_stats.account_id = accounts.id - LEFT OUTER JOIN follow_recommendation_mutes ON follow_recommendation_mutes.target_account_id = accounts.id AND follow_recommendation_mutes.account_id = :id - WHERE follows.account_id IN (SELECT * FROM first_degree) - AND NOT EXISTS (SELECT 1 FROM follows f WHERE f.target_account_id = follows.target_account_id AND f.account_id = :id) - AND follows.target_account_id <> :id - AND accounts.discoverable - AND accounts.suspended_at IS NULL - AND accounts.silenced_at IS NULL - AND accounts.moved_to_account_id IS NULL - AND follow_recommendation_mutes.target_account_id IS NULL - GROUP BY accounts.id, account_stats.id - ORDER BY frequency DESC, account_stats.followers_count ASC - LIMIT :limit - SQL + first_degree = account.following.where.not(hide_collections: true).select(:id).reorder(nil) + base_account_scope(account) + .joins(:account_stat) + .where(id: Follow.where(account_id: first_degree).select(:target_account_id)) + .group('accounts.id, account_stats.id') + .reorder('frequency DESC, followers_count DESC') + .limit(limit) + .pluck(Arel.sql('accounts.id, COUNT(*) AS frequency')) + .map { |id, _frequency| [id, key] } end private diff --git a/app/models/account_suggestions/similar_profiles_source.rb b/app/models/account_suggestions/similar_profiles_source.rb index 3ece20aa51..7ecdd607e5 100644 --- a/app/models/account_suggestions/similar_profiles_source.rb +++ b/app/models/account_suggestions/similar_profiles_source.rb @@ -51,7 +51,8 @@ class AccountSuggestions::SimilarProfilesSource < AccountSuggestions::Source recently_followed_account_ids = account.active_relationships.recent.limit(5).pluck(:target_account_id) if Chewy.enabled? && !recently_followed_account_ids.empty? - QueryBuilder.new(recently_followed_account_ids, account).build.limit(limit).hits.pluck('_id').map(&:to_i).zip([key].cycle) + ids_from_es = QueryBuilder.new(recently_followed_account_ids, account).build.limit(limit).hits.pluck('_id').map(&:to_i) + base_account_scope(account).where(id: ids_from_es).pluck(:id).zip([key].cycle) else [] end diff --git a/app/models/account_suggestions/source.rb b/app/models/account_suggestions/source.rb index b2c3c7a3a2..7afc4c80ed 100644 --- a/app/models/account_suggestions/source.rb +++ b/app/models/account_suggestions/source.rb @@ -12,6 +12,8 @@ class AccountSuggestions::Source def base_account_scope(account) Account .searchable + .where(discoverable: true) + .without_silenced .where.not(follows_sql, id: account.id) .where.not(follow_requests_sql, id: account.id) .not_excluded_by_account(account) diff --git a/spec/models/account_suggestions/friends_of_friends_source_spec.rb b/spec/models/account_suggestions/friends_of_friends_source_spec.rb new file mode 100644 index 0000000000..56a974add5 --- /dev/null +++ b/spec/models/account_suggestions/friends_of_friends_source_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe AccountSuggestions::FriendsOfFriendsSource do + describe '#get' do + subject { described_class.new } + + let!(:bob) { Fabricate(:account, discoverable: true, hide_collections: false) } + let!(:alice) { Fabricate(:account, discoverable: true, hide_collections: true) } + let!(:eve) { Fabricate(:account, discoverable: true, hide_collections: false) } + let!(:mallory) { Fabricate(:account, discoverable: false, hide_collections: false) } + let!(:eugen) { Fabricate(:account, discoverable: true, hide_collections: false) } + let!(:john) { Fabricate(:account, discoverable: true, hide_collections: false) } + let!(:jerk) { Fabricate(:account, discoverable: true, hide_collections: false) } + let!(:neil) { Fabricate(:account, discoverable: true, hide_collections: false) } + let!(:larry) { Fabricate(:account, discoverable: true, hide_collections: false) } + + context 'with follows and blocks' do + before do + bob.block!(jerk) + FollowRecommendationMute.create!(account: bob, target_account: neil) + + # bob follows eugen, alice and larry + [eugen, alice, larry].each { |account| bob.follow!(account) } + + # alice follows eve and mallory + [john, mallory].each { |account| alice.follow!(account) } + + # eugen follows eve, john, jerk, larry and neil + [eve, mallory, jerk, larry, neil].each { |account| eugen.follow!(account) } + end + + it 'returns eligible accounts', :aggregate_failures do + results = subject.get(bob) + + # eve is returned through eugen + expect(results).to include([eve.id, :friends_of_friends]) + + # john is not reachable because alice hides who she follows + expect(results).to_not include([john.id, :friends_of_friends]) + + # mallory is not discoverable + expect(results).to_not include([mallory.id, :friends_of_friends]) + + # larry is not included because he's followed already + expect(results).to_not include([larry.id, :friends_of_friends]) + + # jerk is blocked + expect(results).to_not include([jerk.id, :friends_of_friends]) + + # the suggestion for neil has already been rejected + expect(results).to_not include([neil.id, :friends_of_friends]) + end + end + + context 'with deterministic order' do + before do + # bob follows eve and mallory + [eve, mallory].each { |account| bob.follow!(account) } + + # eve follows eugen, john, and jerk + [jerk, eugen, john].each { |account| eve.follow!(account) } + + # mallory follows eugen, john, and neil + [neil, eugen, john].each { |account| mallory.follow!(account) } + + john.follow!(eugen) + john.follow!(neil) + end + + it 'returns eligible accounts in the expected order' do + expect(subject.get(bob)).to eq [ + [eugen.id, :friends_of_friends], # followed by 2 friends, 3 followers total + [john.id, :friends_of_friends], # followed by 2 friends, 2 followers total + [neil.id, :friends_of_friends], # followed by 1 friend, 2 followers total + [jerk.id, :friends_of_friends], # followed by 1 friend, 1 follower total + ] + end + end + end +end diff --git a/spec/models/account_suggestions/source_spec.rb b/spec/models/account_suggestions/source_spec.rb index d8227e01bc..1666094082 100644 --- a/spec/models/account_suggestions/source_spec.rb +++ b/spec/models/account_suggestions/source_spec.rb @@ -11,14 +11,16 @@ RSpec.describe AccountSuggestions::Source do end context 'with follows and follow requests' do - let!(:account_domain_blocked_account) { Fabricate(:account, domain: 'blocked.host') } - let!(:account) { Fabricate(:account) } - let!(:blocked_account) { Fabricate(:account) } - let!(:eligible_account) { Fabricate(:account) } - let!(:follow_recommendation_muted_account) { Fabricate(:account) } - let!(:follow_requested_account) { Fabricate(:account) } - let!(:following_account) { Fabricate(:account) } - let!(:moved_account) { Fabricate(:account, moved_to_account: Fabricate(:account)) } + let!(:account_domain_blocked_account) { Fabricate(:account, domain: 'blocked.host', discoverable: true) } + let!(:account) { Fabricate(:account, discoverable: true) } + let!(:blocked_account) { Fabricate(:account, discoverable: true) } + let!(:eligible_account) { Fabricate(:account, discoverable: true) } + let!(:follow_recommendation_muted_account) { Fabricate(:account, discoverable: true) } + let!(:follow_requested_account) { Fabricate(:account, discoverable: true) } + let!(:following_account) { Fabricate(:account, discoverable: true) } + let!(:moved_account) { Fabricate(:account, moved_to_account: Fabricate(:account), discoverable: true) } + let!(:silenced_account) { Fabricate(:account, silenced: true, discoverable: true) } + let!(:undiscoverable_account) { Fabricate(:account, discoverable: false) } before do Fabricate :account_domain_block, account: account, domain: account_domain_blocked_account.domain @@ -40,6 +42,8 @@ RSpec.describe AccountSuggestions::Source do .and not_include(follow_requested_account) .and not_include(following_account) .and not_include(moved_account) + .and not_include(silenced_account) + .and not_include(undiscoverable_account) end end end