Change lists to be able to include accounts with pending follow requests (#19727)

app/models/concerns/account_interactions.rb

@@ -272,6 +272,7 @@ module AccountInteractions
 
   def lists_for_local_distribution
     lists.joins(account: :user)
+         .where.not(list_accounts: { follow_id: nil })
          .where('users.current_sign_in_at > ?', User::ACTIVE_DURATION.ago)
   end
 

app/models/follow_request.rb

@@ -32,7 +32,8 @@ class FollowRequest < ApplicationRecord
   validates :languages, language: true
 
   def authorize!
-    account.follow!(target_account, reblogs: show_reblogs, notify: notify, languages: languages, uri: uri, bypass_limit: true)
+    follow = account.follow!(target_account, reblogs: show_reblogs, notify: notify, languages: languages, uri: uri, bypass_limit: true)
+    ListAccount.where(follow_request: self).update_all(follow_request_id: nil, follow_id: follow.id) # rubocop:disable Rails/SkipsModelValidations
     MergeWorker.perform_async(target_account.id, account.id) if account.local?
     destroy!
   end

app/models/list_account.rb

@@ -4,16 +4,18 @@
 #
 # Table name: list_accounts
 #
-#  id         :bigint(8)        not null, primary key
+#  id                :bigint(8)        not null, primary key
-#  list_id    :bigint(8)        not null
+#  list_id           :bigint(8)        not null
-#  account_id :bigint(8)        not null
+#  account_id        :bigint(8)        not null
-#  follow_id  :bigint(8)
+#  follow_id         :bigint(8)
+#  follow_request_id :bigint(8)
 #
 
 class ListAccount < ApplicationRecord
   belongs_to :list
   belongs_to :account
   belongs_to :follow, optional: true
+  belongs_to :follow_request, optional: true
 
   validates :account_id, uniqueness: { scope: :list_id }
 
@@ -22,6 +24,10 @@ class ListAccount < ApplicationRecord
   private
 
   def set_follow
-    self.follow = Follow.find_by!(account_id: list.account_id, target_account_id: account.id) unless list.account_id == account.id
+    return if list.account_id == account.id
+
+    self.follow = Follow.find_by!(account_id: list.account_id, target_account_id: account.id)
+  rescue ActiveRecord::RecordNotFound
+    self.follow_request = FollowRequest.find_by!(account_id: list.account_id, target_account_id: account.id)
   end
 end

db/migrate/20230330155710_add_follow_request_id_to_list_accounts.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class AddFollowRequestIdToListAccounts < ActiveRecord::Migration[6.1]
+  disable_ddl_transaction!
+
+  def change
+    safety_assured { add_reference :list_accounts, :follow_request, foreign_key: { on_delete: :cascade }, index: false }
+    add_index :list_accounts, :follow_request_id, algorithm: :concurrently, where: 'follow_request_id IS NOT NULL'
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2023_03_30_140036) do
+ActiveRecord::Schema.define(version: 2023_03_30_155710) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -554,8 +554,10 @@ ActiveRecord::Schema.define(version: 2023_03_30_140036) do
     t.bigint "list_id", null: false
     t.bigint "account_id", null: false
     t.bigint "follow_id"
+    t.bigint "follow_request_id"
     t.index ["account_id", "list_id"], name: "index_list_accounts_on_account_id_and_list_id", unique: true
     t.index ["follow_id"], name: "index_list_accounts_on_follow_id", where: "(follow_id IS NOT NULL)"
+    t.index ["follow_request_id"], name: "index_list_accounts_on_follow_request_id", where: "(follow_request_id IS NOT NULL)"
     t.index ["list_id", "account_id"], name: "index_list_accounts_on_list_id_and_account_id"
   end
 
@@ -1198,6 +1200,7 @@ ActiveRecord::Schema.define(version: 2023_03_30_140036) do
   add_foreign_key "imports", "accounts", name: "fk_6db1b6e408", on_delete: :cascade
   add_foreign_key "invites", "users", on_delete: :cascade
   add_foreign_key "list_accounts", "accounts", on_delete: :cascade
+  add_foreign_key "list_accounts", "follow_requests", on_delete: :cascade
   add_foreign_key "list_accounts", "follows", on_delete: :cascade
   add_foreign_key "list_accounts", "lists", on_delete: :cascade
   add_foreign_key "lists", "accounts", on_delete: :cascade

spec/controllers/api/v1/lists/accounts_controller_spec.rb

@@ -29,17 +29,48 @@ describe Api::V1::Lists::AccountsController do
     let(:scopes) { 'write:lists' }
     let(:bob) { Fabricate(:account, username: 'bob') }
 
-    before do
+    context 'when the added account is followed' do
-      user.account.follow!(bob)
+      before do
-      post :create, params: { list_id: list.id, account_ids: [bob.id] }
+        user.account.follow!(bob)
+        post :create, params: { list_id: list.id, account_ids: [bob.id] }
+      end
+
+      it 'returns http success' do
+        expect(response).to have_http_status(200)
+      end
+
+      it 'adds account to the list' do
+        expect(list.accounts.include?(bob)).to be true
+      end
     end
 
-    it 'returns http success' do
+    context 'when the added account has been sent a follow request' do
-      expect(response).to have_http_status(200)
+      before do
+        user.account.follow_requests.create!(target_account: bob)
+        post :create, params: { list_id: list.id, account_ids: [bob.id] }
+      end
+
+      it 'returns http success' do
+        expect(response).to have_http_status(200)
+      end
+
+      it 'adds account to the list' do
+        expect(list.accounts.include?(bob)).to be true
+      end
     end
 
-    it 'adds account to the list' do
+    context 'when the added account is not followed' do
-      expect(list.accounts.include?(bob)).to be true
+      before do
+        post :create, params: { list_id: list.id, account_ids: [bob.id] }
+      end
+
+      it 'returns http not found' do
+        expect(response).to have_http_status(404)
+      end
+
+      it 'does not add the account to the list' do
+        expect(list.accounts.include?(bob)).to be false
+      end
     end
   end
 

spec/models/concerns/account_interactions_spec.rb

@@ -683,4 +683,28 @@ describe AccountInteractions do
       end
     end
   end
+
+  describe '#lists_for_local_distribution' do
+    let!(:inactive_follower_user) { Fabricate(:user, current_sign_in_at: 5.years.ago) }
+    let!(:follower_user)          { Fabricate(:user, current_sign_in_at: Time.now.utc) }
+    let!(:follow_request_user)    { Fabricate(:user, current_sign_in_at: Time.now.utc) }
+
+    let!(:inactive_follower_list) { Fabricate(:list, account: inactive_follower_user.account) }
+    let!(:follower_list)          { Fabricate(:list, account: follower_user.account) }
+    let!(:follow_request_list)    { Fabricate(:list, account: follow_request_user.account) }
+
+    before do
+      inactive_follower_user.account.follow!(account)
+      follower_user.account.follow!(account)
+      follow_request_user.account.follow_requests.create!(target_account: account)
+
+      inactive_follower_list.accounts << account
+      follower_list.accounts << account
+      follow_request_list.accounts << account
+    end
+
+    it 'includes only the list from the active follower' do
+      expect(account.lists_for_local_distribution.to_a).to eq [follower_list]
+    end
+  end
 end

spec/models/follow_request_spec.rb

@@ -4,13 +4,27 @@ require 'rails_helper'
 
 RSpec.describe FollowRequest, type: :model do
   describe '#authorize!' do
-    let(:follow_request) { Fabricate(:follow_request, account: account, target_account: target_account) }
+    let!(:follow_request) { Fabricate(:follow_request, account: account, target_account: target_account) }
-    let(:account)        { Fabricate(:account) }
+    let(:account)         { Fabricate(:account) }
-    let(:target_account) { Fabricate(:account) }
+    let(:target_account)  { Fabricate(:account) }
+
+    context 'when the to-be-followed person has been added to a list' do
+      let!(:list) { Fabricate(:list, account: account) }
+
+      before do
+        list.accounts << target_account
+      end
+
+      it 'updates the ListAccount' do
+        expect { follow_request.authorize! }.to change { [list.list_accounts.first.follow_request_id, list.list_accounts.first.follow_id] }.from([follow_request.id, nil]).to([nil, anything])
+      end
+    end
 
     it 'calls Account#follow!, MergeWorker.perform_async, and #destroy!' do
-      expect(account).to        receive(:follow!).with(target_account, reblogs: true, notify: false, uri: follow_request.uri, languages: nil, bypass_limit: true)
+      expect(account).to receive(:follow!).with(target_account, reblogs: true, notify: false, uri: follow_request.uri, languages: nil, bypass_limit: true) do
-      expect(MergeWorker).to    receive(:perform_async).with(target_account.id, account.id)
+        account.active_relationships.create!(target_account: target_account)
+      end
+      expect(MergeWorker).to receive(:perform_async).with(target_account.id, account.id)
       expect(follow_request).to receive(:destroy!)
       follow_request.authorize!
     end
@@ -29,4 +43,22 @@ RSpec.describe FollowRequest, type: :model do
       expect(follow_request.account.muting_reblogs?(target)).to be true
     end
   end
+
+  describe '#reject!' do
+    let!(:follow_request) { Fabricate(:follow_request, account: account, target_account: target_account) }
+    let(:account)         { Fabricate(:account) }
+    let(:target_account)  { Fabricate(:account) }
+
+    context 'when the to-be-followed person has been added to a list' do
+      let!(:list) { Fabricate(:list, account: account) }
+
+      before do
+        list.accounts << target_account
+      end
+
+      it 'deletes the ListAccount record' do
+        expect { follow_request.reject! }.to change { list.accounts.count }.from(1).to(0)
+      end
+    end
+  end
 end