From c98b012583fd7368008b083407b4fc7c92905aae Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 3 May 2023 14:03:38 +0200 Subject: [PATCH] Change Move handler to also move list memberships (#24808) --- app/services/follow_migration_service.rb | 18 +++++++++--- app/workers/move_worker.rb | 16 +++++++---- spec/workers/move_worker_spec.rb | 36 +++++++++++++++++++----- 3 files changed, 54 insertions(+), 16 deletions(-) diff --git a/app/services/follow_migration_service.rb b/app/services/follow_migration_service.rb index cfe9093cb..a2ee801b5 100644 --- a/app/services/follow_migration_service.rb +++ b/app/services/follow_migration_service.rb @@ -9,10 +9,10 @@ class FollowMigrationService < FollowService def call(source_account, target_account, old_target_account, bypass_locked: false) @old_target_account = old_target_account - follow = source_account.active_relationships.find_by(target_account: old_target_account) - reblogs = follow&.show_reblogs? - notify = follow&.notify? - languages = follow&.languages + @original_follow = source_account.active_relationships.find_by(target_account: old_target_account) + reblogs = @original_follow&.show_reblogs? + notify = @original_follow&.notify? + languages = @original_follow&.languages super(source_account, target_account, reblogs: reblogs, notify: notify, languages: languages, bypass_locked: bypass_locked, bypass_limit: true) end @@ -21,6 +21,7 @@ class FollowMigrationService < FollowService def request_follow! follow_request = @source_account.request_follow!(@target_account, **follow_options.merge(rate_limit: @options[:with_rate_limit], bypass_limit: @options[:bypass_limit])) + migrate_list_accounts! if @target_account.local? LocalNotificationWorker.perform_async(@target_account.id, follow_request.id, follow_request.class.name, 'follow_request') @@ -34,7 +35,16 @@ class FollowMigrationService < FollowService def direct_follow! follow = super + + migrate_list_accounts! UnfollowService.new.call(@source_account, @old_target_account, skip_unmerge: true) + follow end + + def migrate_list_accounts! + ListAccount.where(follow_id: @original_follow.id).includes(:list).find_each do |list_account| + list_account.list.accounts << @target_account + end + end end diff --git a/app/workers/move_worker.rb b/app/workers/move_worker.rb index 3b429928e..1b1d0f4a7 100644 --- a/app/workers/move_worker.rb +++ b/app/workers/move_worker.rb @@ -8,9 +8,9 @@ class MoveWorker @target_account = Account.find(target_account_id) if @target_account.local? && @source_account.local? - nb_moved = rewrite_follows! - @source_account.update_count!(:followers_count, -nb_moved) - @target_account.update_count!(:followers_count, nb_moved) + num_moved = rewrite_follows! + @source_account.update_count!(:followers_count, -num_moved) + @target_account.update_count!(:followers_count, num_moved) else queue_follow_unfollows! end @@ -29,12 +29,18 @@ class MoveWorker private def rewrite_follows! + num_moved = 0 + @source_account.passive_relationships .where(account: Account.local) .where.not(account: @target_account.followers.local) .where.not(account_id: @target_account.id) - .in_batches - .update_all(target_account_id: @target_account.id) + .in_batches do |follows| + ListAccount.where(follow: follows).in_batches.update_all(account_id: @target_account.id) + num_moved += follows.update_all(target_account_id: @target_account.id) + end + + num_moved end def queue_follow_unfollows! diff --git a/spec/workers/move_worker_spec.rb b/spec/workers/move_worker_spec.rb index e93060adb..a0adcbec2 100644 --- a/spec/workers/move_worker_spec.rb +++ b/spec/workers/move_worker_spec.rb @@ -5,22 +5,28 @@ require 'rails_helper' describe MoveWorker do subject { described_class.new } - let(:local_follower) { Fabricate(:account) } + let(:local_follower) { Fabricate(:account, domain: nil) } let(:blocking_account) { Fabricate(:account) } let(:muting_account) { Fabricate(:account) } - let(:source_account) { Fabricate(:account, protocol: :activitypub, domain: 'example.com') } - let(:target_account) { Fabricate(:account, protocol: :activitypub, domain: 'example.com') } + let(:source_account) { Fabricate(:account, protocol: :activitypub, domain: 'example.com', uri: 'https://example.org/a', inbox_url: 'https://example.org/a/inbox') } + let(:target_account) { Fabricate(:account, protocol: :activitypub, domain: 'example.com', uri: 'https://example.org/b', inbox_url: 'https://example.org/b/inbox') } let(:local_user) { Fabricate(:user) } let(:comment) { 'old note prior to move' } let!(:account_note) { Fabricate(:account_note, account: local_user.account, target_account: source_account, comment: comment) } + let(:list) { Fabricate(:list, account: local_follower) } let(:block_service) { double } before do + stub_request(:post, 'https://example.org/a/inbox').to_return(status: 200) + stub_request(:post, 'https://example.org/b/inbox').to_return(status: 200) + local_follower.follow!(source_account) blocking_account.block!(source_account) muting_account.mute!(source_account) + list.accounts << source_account + allow(BlockService).to receive(:new).and_return(block_service) allow(block_service).to receive(:call) end @@ -86,16 +92,27 @@ describe MoveWorker do end end + shared_examples 'lists handling' do + it 'updates lists' do + subject.perform(source_account.id, target_account.id) + expect(list.accounts.include?(target_account)).to be true + end + end + context 'both accounts are distant' do describe 'perform' do it 'calls UnfollowFollowWorker' do - expect_push_bulk_to_match(UnfollowFollowWorker, [[local_follower.id, source_account.id, target_account.id, false]]) - subject.perform(source_account.id, target_account.id) + Sidekiq::Testing.fake! do + subject.perform(source_account.id, target_account.id) + expect(UnfollowFollowWorker).to have_enqueued_sidekiq_job(local_follower.id, source_account.id, target_account.id, false) + Sidekiq::Worker.drain_all + end end include_examples 'user note handling' include_examples 'block and mute handling' include_examples 'followers count handling' + include_examples 'lists handling' end end @@ -104,13 +121,17 @@ describe MoveWorker do describe 'perform' do it 'calls UnfollowFollowWorker' do - expect_push_bulk_to_match(UnfollowFollowWorker, [[local_follower.id, source_account.id, target_account.id, true]]) - subject.perform(source_account.id, target_account.id) + Sidekiq::Testing.fake! do + subject.perform(source_account.id, target_account.id) + expect(UnfollowFollowWorker).to have_enqueued_sidekiq_job(local_follower.id, source_account.id, target_account.id, true) + Sidekiq::Worker.clear_all + end end include_examples 'user note handling' include_examples 'block and mute handling' include_examples 'followers count handling' + include_examples 'lists handling' end end @@ -127,6 +148,7 @@ describe MoveWorker do 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)