Fix various edge cases with local moves (#24812)

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

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

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)

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
+  shared_examples 'common tests' do
-    describe 'perform' 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 'common tests'
-      include_examples 'block and mute handling'
-      include_examples 'followers count handling'
-      include_examples 'lists handling'
-    end
     end
 
-  context 'target account is local' do
+    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 'common tests'
-      include_examples 'block and mute handling'
-      include_examples 'followers count handling'
-      include_examples 'lists handling'
-    end
     end
 
-  context 'both target and source accounts are local' do
+    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 'common tests'
-      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
 
       it 'does not allow the moved account to follow themselves' do
         source_account.follow!(target_account)