From 6693a4fe7c1f8a48e72dcf3048cf787d6a511fb9 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 2 May 2023 14:40:36 +0200 Subject: [PATCH] Change lists to be able to include accounts with pending follow requests (#19727) --- app/models/concerns/account_interactions.rb | 1 + app/models/follow_request.rb | 3 +- app/models/list_account.rb | 16 ++++--- ..._add_follow_request_id_to_list_accounts.rb | 10 +++++ db/schema.rb | 5 ++- .../api/v1/lists/accounts_controller_spec.rb | 45 ++++++++++++++++--- .../concerns/account_interactions_spec.rb | 24 ++++++++++ spec/models/follow_request_spec.rb | 42 ++++++++++++++--- 8 files changed, 127 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20230330155710_add_follow_request_id_to_list_accounts.rb diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb index b2ccddef32..cda221c432 100644 --- a/app/models/concerns/account_interactions.rb +++ b/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 diff --git a/app/models/follow_request.rb b/app/models/follow_request.rb index 78f79c18f0..a5c23e09d4 100644 --- a/app/models/follow_request.rb +++ b/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 diff --git a/app/models/list_account.rb b/app/models/list_account.rb index a5767d3d8b..96128f1363 100644 --- a/app/models/list_account.rb +++ b/app/models/list_account.rb @@ -4,16 +4,18 @@ # # Table name: list_accounts # -# id :bigint(8) not null, primary key -# list_id :bigint(8) not null -# account_id :bigint(8) not null -# follow_id :bigint(8) +# id :bigint(8) not null, primary key +# list_id :bigint(8) not null +# account_id :bigint(8) not null +# 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 diff --git a/db/migrate/20230330155710_add_follow_request_id_to_list_accounts.rb b/db/migrate/20230330155710_add_follow_request_id_to_list_accounts.rb new file mode 100644 index 0000000000..5f4ac374ca --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 7bd6f4d602..8a32e45ce0 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/controllers/api/v1/lists/accounts_controller_spec.rb b/spec/controllers/api/v1/lists/accounts_controller_spec.rb index 337a5645c0..d4550dd769 100644 --- a/spec/controllers/api/v1/lists/accounts_controller_spec.rb +++ b/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 - user.account.follow!(bob) - post :create, params: { list_id: list.id, account_ids: [bob.id] } + context 'when the added account is followed' do + before do + 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 - expect(response).to have_http_status(200) + context 'when the added account has been sent a follow request' do + 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 - expect(list.accounts.include?(bob)).to be true + context 'when the added account is not followed' do + 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 diff --git a/spec/models/concerns/account_interactions_spec.rb b/spec/models/concerns/account_interactions_spec.rb index 7396af6dfa..7984576161 100644 --- a/spec/models/concerns/account_interactions_spec.rb +++ b/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 diff --git a/spec/models/follow_request_spec.rb b/spec/models/follow_request_spec.rb index 569c160aeb..96d818913a 100644 --- a/spec/models/follow_request_spec.rb +++ b/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(:account) { Fabricate(:account) } - let(:target_account) { Fabricate(:account) } + 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 '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(MergeWorker).to receive(:perform_async).with(target_account.id, account.id) + expect(account).to receive(:follow!).with(target_account, reblogs: true, notify: false, uri: follow_request.uri, languages: nil, bypass_limit: true) do + 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