From a2a22bad236868c4f32d30c00eb899d8bb7be64d Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 3 May 2023 19:19:25 +0200 Subject: [PATCH] Fix various edge cases with local moves (#24812) --- app/models/list_account.rb | 9 +++ app/services/follow_migration_service.rb | 12 +++ app/workers/move_worker.rb | 26 +++++++ spec/workers/move_worker_spec.rb | 93 ++++++++++++++++-------- 4 files changed, 108 insertions(+), 32 deletions(-) diff --git a/app/models/list_account.rb b/app/models/list_account.rb index 96128f13637..e7016f2714a 100644 --- a/app/models/list_account.rb +++ b/app/models/list_account.rb @@ -18,6 +18,7 @@ class ListAccount < ApplicationRecord belongs_to :follow_request, optional: true validates :account_id, uniqueness: { scope: :list_id } + validate :validate_relationship before_validation :set_follow @@ -30,4 +31,12 @@ class ListAccount < ApplicationRecord rescue ActiveRecord::RecordNotFound self.follow_request = FollowRequest.find_by!(account_id: list.account_id, target_account_id: account.id) end + + def validate_relationship + return if list.account_id == account_id + + errors.add(:account_id, 'follow relationship missing') if follow_id.nil? && follow_request_id.nil? + errors.add(:follow, 'mismatched accounts') if follow_id.present? && follow.target_account_id != account_id + errors.add(:follow_request, 'mismatched accounts') if follow_request_id.present? && follow_request.target_account_id != account_id + end end diff --git a/app/services/follow_migration_service.rb b/app/services/follow_migration_service.rb index a2ee801b5b2..12ed96f9840 100644 --- a/app/services/follow_migration_service.rb +++ b/app/services/follow_migration_service.rb @@ -33,6 +33,16 @@ class FollowMigrationService < FollowService follow_request end + def change_follow_options! + migrate_list_accounts! + super + end + + def change_follow_request_options! + migrate_list_accounts! + super + end + def direct_follow! follow = super @@ -45,6 +55,8 @@ class FollowMigrationService < FollowService def migrate_list_accounts! ListAccount.where(follow_id: @original_follow.id).includes(:list).find_each do |list_account| list_account.list.accounts << @target_account + rescue ActiveRecord::RecordInvalid + nil end end end diff --git a/app/workers/move_worker.rb b/app/workers/move_worker.rb index 1b1d0f4a70e..cb091671dfa 100644 --- a/app/workers/move_worker.rb +++ b/app/workers/move_worker.rb @@ -31,6 +31,32 @@ class MoveWorker def rewrite_follows! num_moved = 0 + # First, approve pending follow requests for the new account, + # this allows correctly processing list memberships with pending + # follow requests + FollowRequest.where(account: @source_account.followers, target_account_id: @target_account.id).find_each do |follow_request| + ListAccount.where(follow_id: follow_request.id).includes(:list).find_each do |list_account| + list_account.list.accounts << @target_account + rescue ActiveRecord::RecordInvalid + nil + end + + follow_request.authorize! + end + + # Then handle accounts that follow both the old and new account + @source_account.passive_relationships + .where(account: Account.local) + .where(account: @target_account.followers.local) + .in_batches do |follows| + ListAccount.where(follow: follows).includes(:list).find_each do |list_account| + list_account.list.accounts << @target_account + rescue ActiveRecord::RecordInvalid + nil + end + end + + # Finally, handle the common case of accounts not following the new account @source_account.passive_relationships .where(account: Account.local) .where.not(account: @target_account.followers.local) diff --git a/spec/workers/move_worker_spec.rb b/spec/workers/move_worker_spec.rb index a0adcbec2b9..81ff7f1dc5c 100644 --- a/spec/workers/move_worker_spec.rb +++ b/spec/workers/move_worker_spec.rb @@ -93,14 +93,64 @@ describe MoveWorker do end shared_examples 'lists handling' do - it 'updates lists' do + it 'puts the new account on the list' do subject.perform(source_account.id, target_account.id) expect(list.accounts.include?(target_account)).to be true end + + it 'does not create invalid list memberships' do + subject.perform(source_account.id, target_account.id) + expect(ListAccount.all).to all be_valid + end end - context 'both accounts are distant' do - describe 'perform' do + shared_examples 'common tests' do + include_examples 'user note handling' + include_examples 'block and mute handling' + include_examples 'followers count handling' + include_examples 'lists handling' + + context 'when a local user already follows both source and target' do + before do + local_follower.request_follow!(target_account) + end + + include_examples 'user note handling' + include_examples 'block and mute handling' + include_examples 'followers count handling' + include_examples 'lists handling' + + context 'and the local user already has the target in a list' do + before do + list.accounts << target_account + end + + include_examples 'lists handling' + end + end + + context 'when a local follower already has a pending request to the target' do + before do + local_follower.follow!(target_account) + end + + include_examples 'user note handling' + include_examples 'block and mute handling' + include_examples 'followers count handling' + include_examples 'lists handling' + + context 'and the local user already has the target in a list' do + before do + list.accounts << target_account + end + + include_examples 'lists handling' + end + end + end + + describe '#perform' do + context 'when both accounts are distant' do it 'calls UnfollowFollowWorker' do Sidekiq::Testing.fake! do subject.perform(source_account.id, target_account.id) @@ -109,17 +159,12 @@ describe MoveWorker do end end - include_examples 'user note handling' - include_examples 'block and mute handling' - include_examples 'followers count handling' - include_examples 'lists handling' + include_examples 'common tests' end - end - context 'target account is local' do - let(:target_account) { Fabricate(:account) } + context 'when target account is local' do + let(:target_account) { Fabricate(:account) } - describe 'perform' do it 'calls UnfollowFollowWorker' do Sidekiq::Testing.fake! do subject.perform(source_account.id, target_account.id) @@ -128,35 +173,19 @@ describe MoveWorker do end end - include_examples 'user note handling' - include_examples 'block and mute handling' - include_examples 'followers count handling' - include_examples 'lists handling' + include_examples 'common tests' end - end - context 'both target and source accounts are local' do - let(:target_account) { Fabricate(:account) } - let(:source_account) { Fabricate(:account) } + context 'when both target and source accounts are local' do + let(:target_account) { Fabricate(:account) } + let(:source_account) { Fabricate(:account) } - describe 'perform' do it 'calls makes local followers follow the target account' do subject.perform(source_account.id, target_account.id) expect(local_follower.following?(target_account)).to be true end - include_examples 'user note handling' - include_examples 'block and mute handling' - include_examples 'followers count handling' - include_examples 'lists handling' - - it 'does not fail when a local user is already following both accounts' do - double_follower = Fabricate(:account) - double_follower.follow!(source_account) - double_follower.follow!(target_account) - subject.perform(source_account.id, target_account.id) - expect(local_follower.following?(target_account)).to be true - end + include_examples 'common tests' it 'does not allow the moved account to follow themselves' do source_account.follow!(target_account)