From 9251779d755c62dca9326df82687c4b62e2abb8a Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 21 Dec 2023 09:23:53 -0500 Subject: [PATCH] Fix `RSpec/LetSetup` cop in spec/services (#28459) --- .rubocop_todo.yml | 13 ---- .../batched_remove_status_service_spec.rb | 2 +- spec/services/block_domain_service_spec.rb | 16 ++-- spec/services/bulk_import_service_spec.rb | 10 ++- spec/services/delete_account_service_spec.rb | 73 +++++++++---------- spec/services/import_service_spec.rb | 2 +- spec/services/notify_service_spec.rb | 9 ++- spec/services/remove_status_service_spec.rb | 14 ++-- spec/services/report_service_spec.rb | 3 +- spec/services/resolve_account_service_spec.rb | 3 +- spec/services/suspend_account_service_spec.rb | 2 +- spec/services/unallow_domain_service_spec.rb | 1 + .../unsuspend_account_service_spec.rb | 2 +- .../scheduler/user_cleanup_scheduler_spec.rb | 11 ++- 14 files changed, 74 insertions(+), 87 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 6b6a4a89e9..afd5305304 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -53,19 +53,6 @@ RSpec/LetSetup: - 'spec/models/account_statuses_cleanup_policy_spec.rb' - 'spec/models/status_spec.rb' - 'spec/services/activitypub/fetch_featured_collection_service_spec.rb' - - 'spec/services/batched_remove_status_service_spec.rb' - - 'spec/services/block_domain_service_spec.rb' - - 'spec/services/bulk_import_service_spec.rb' - - 'spec/services/delete_account_service_spec.rb' - - 'spec/services/import_service_spec.rb' - - 'spec/services/notify_service_spec.rb' - - 'spec/services/remove_status_service_spec.rb' - - 'spec/services/report_service_spec.rb' - - 'spec/services/resolve_account_service_spec.rb' - - 'spec/services/suspend_account_service_spec.rb' - - 'spec/services/unallow_domain_service_spec.rb' - - 'spec/services/unsuspend_account_service_spec.rb' - - 'spec/workers/scheduler/user_cleanup_scheduler_spec.rb' RSpec/MultipleExpectations: Max: 8 diff --git a/spec/services/batched_remove_status_service_spec.rb b/spec/services/batched_remove_status_service_spec.rb index 8201c9d51f..991a852f0e 100644 --- a/spec/services/batched_remove_status_service_spec.rb +++ b/spec/services/batched_remove_status_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe BatchedRemoveStatusService, type: :service do let!(:jeff) { Fabricate(:account) } let!(:hank) { Fabricate(:account, username: 'hank', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } - let(:status_alice_hello) { PostStatusService.new.call(alice, text: 'Hello @bob@example.com') } + let(:status_alice_hello) { PostStatusService.new.call(alice, text: "Hello @#{bob.pretty_acct}") } let(:status_alice_other) { PostStatusService.new.call(alice, text: 'Another status') } before do diff --git a/spec/services/block_domain_service_spec.rb b/spec/services/block_domain_service_spec.rb index 36dce9d196..fc3a1397d9 100644 --- a/spec/services/block_domain_service_spec.rb +++ b/spec/services/block_domain_service_spec.rb @@ -21,19 +21,19 @@ RSpec.describe BlockDomainService, type: :service do end it 'removes remote accounts from that domain' do - expect(Account.find_remote('badguy666', 'evil.org').suspended?).to be true + expect(bad_account.reload.suspended?).to be true end it 'records suspension date appropriately' do - expect(Account.find_remote('badguy666', 'evil.org').suspended_at).to eq DomainBlock.find_by(domain: 'evil.org').created_at + expect(bad_account.reload.suspended_at).to eq DomainBlock.find_by(domain: 'evil.org').created_at end it 'keeps already-banned accounts banned' do - expect(Account.find_remote('badguy', 'evil.org').suspended?).to be true + expect(already_banned_account.reload.suspended?).to be true end it 'does not overwrite suspension date of already-banned accounts' do - expect(Account.find_remote('badguy', 'evil.org').suspended_at).to_not eq DomainBlock.find_by(domain: 'evil.org').created_at + expect(already_banned_account.reload.suspended_at).to_not eq DomainBlock.find_by(domain: 'evil.org').created_at end it 'removes the remote accounts\'s statuses and media attachments' do @@ -53,19 +53,19 @@ RSpec.describe BlockDomainService, type: :service do end it 'silences remote accounts from that domain' do - expect(Account.find_remote('badguy666', 'evil.org').silenced?).to be true + expect(bad_account.reload.silenced?).to be true end it 'records suspension date appropriately' do - expect(Account.find_remote('badguy666', 'evil.org').silenced_at).to eq DomainBlock.find_by(domain: 'evil.org').created_at + expect(bad_account.reload.silenced_at).to eq DomainBlock.find_by(domain: 'evil.org').created_at end it 'keeps already-banned accounts banned' do - expect(Account.find_remote('badguy', 'evil.org').silenced?).to be true + expect(already_banned_account.reload.silenced?).to be true end it 'does not overwrite suspension date of already-banned accounts' do - expect(Account.find_remote('badguy', 'evil.org').silenced_at).to_not eq DomainBlock.find_by(domain: 'evil.org').created_at + expect(already_banned_account.reload.silenced_at).to_not eq DomainBlock.find_by(domain: 'evil.org').created_at end it 'leaves the domains status and attachments, but clears media' do diff --git a/spec/services/bulk_import_service_spec.rb b/spec/services/bulk_import_service_spec.rb index 3a3f95ccc6..f703650c37 100644 --- a/spec/services/bulk_import_service_spec.rb +++ b/spec/services/bulk_import_service_spec.rb @@ -271,14 +271,15 @@ RSpec.describe BulkImportService do let(:import_type) { 'domain_blocking' } let(:overwrite) { false } - let!(:rows) do + let(:rows) do [ { 'domain' => 'blocked.com' }, { 'domain' => 'to_block.com' }, - ].map { |data| import.rows.create!(data: data) } + ] end before do + rows.each { |data| import.rows.create!(data: data) } account.block_domain!('alreadyblocked.com') account.block_domain!('blocked.com') end @@ -298,14 +299,15 @@ RSpec.describe BulkImportService do let(:import_type) { 'domain_blocking' } let(:overwrite) { true } - let!(:rows) do + let(:rows) do [ { 'domain' => 'blocked.com' }, { 'domain' => 'to_block.com' }, - ].map { |data| import.rows.create!(data: data) } + ] end before do + rows.each { |data| import.rows.create!(data: data) } account.block_domain!('alreadyblocked.com') account.block_domain!('blocked.com') end diff --git a/spec/services/delete_account_service_spec.rb b/spec/services/delete_account_service_spec.rb index a2c57f1c1e..f04ba9cf14 100644 --- a/spec/services/delete_account_service_spec.rb +++ b/spec/services/delete_account_service_spec.rb @@ -28,74 +28,69 @@ RSpec.describe DeleteAccountService, type: :service do let!(:account_note) { Fabricate(:account_note, account: account) } it 'deletes associated owned and target records and target notifications' do - expect { subject } - .to delete_associated_owned_records - .and delete_associated_target_records - .and delete_associated_target_notifications + subject + + expect_deletion_of_associated_owned_records + expect_deletion_of_associated_target_records + expect_deletion_of_associated_target_notifications end - def delete_associated_owned_records - change do - [ - account.statuses, - account.media_attachments, - account.notifications, - account.favourites, - account.active_relationships, - account.passive_relationships, - account.polls, - account.account_notes, - ].map(&:count) - end.from([2, 1, 1, 1, 1, 1, 1, 1]).to([0, 0, 0, 0, 0, 0, 0, 0]) + def expect_deletion_of_associated_owned_records + expect { status.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { status_with_mention.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { mention.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { media_attachment.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { favourite.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { active_relationship.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { passive_relationship.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { poll.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { poll_vote.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { account_note.reload }.to raise_error(ActiveRecord::RecordNotFound) end - def delete_associated_target_records - change(account_pins_for_account, :count).from(1).to(0) + def expect_deletion_of_associated_target_records + expect { endorsement.reload }.to raise_error(ActiveRecord::RecordNotFound) end - def account_pins_for_account - AccountPin.where(target_account: account) - end - - def delete_associated_target_notifications - change do - %w( - poll favourite status mention follow - ).map { |type| Notification.where(type: type).count } - end.from([1, 1, 1, 1, 1]).to([0, 0, 0, 0, 0]) + def expect_deletion_of_associated_target_notifications + expect { favourite_notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { follow_notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { mention_notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { poll_notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { status_notification.reload }.to raise_error(ActiveRecord::RecordNotFound) end end describe '#call on local account' do before do - stub_request(:post, 'https://alice.com/inbox').to_return(status: 201) - stub_request(:post, 'https://bob.com/inbox').to_return(status: 201) + stub_request(:post, remote_alice.inbox_url).to_return(status: 201) + stub_request(:post, remote_bob.inbox_url).to_return(status: 201) end let!(:remote_alice) { Fabricate(:account, inbox_url: 'https://alice.com/inbox', domain: 'alice.com', protocol: :activitypub) } let!(:remote_bob) { Fabricate(:account, inbox_url: 'https://bob.com/inbox', domain: 'bob.com', protocol: :activitypub) } include_examples 'common behavior' do - let!(:account) { Fabricate(:account) } - let!(:local_follower) { Fabricate(:account) } + let(:account) { Fabricate(:account) } + let(:local_follower) { Fabricate(:account) } it 'sends a delete actor activity to all known inboxes' do subject - expect(a_request(:post, 'https://alice.com/inbox')).to have_been_made.once - expect(a_request(:post, 'https://bob.com/inbox')).to have_been_made.once + expect(a_request(:post, remote_alice.inbox_url)).to have_been_made.once + expect(a_request(:post, remote_bob.inbox_url)).to have_been_made.once end end end describe '#call on remote account' do before do - stub_request(:post, 'https://alice.com/inbox').to_return(status: 201) - stub_request(:post, 'https://bob.com/inbox').to_return(status: 201) + stub_request(:post, account.inbox_url).to_return(status: 201) end include_examples 'common behavior' do - let!(:account) { Fabricate(:account, inbox_url: 'https://bob.com/inbox', protocol: :activitypub, domain: 'bob.com') } - let!(:local_follower) { Fabricate(:account) } + let(:account) { Fabricate(:account, inbox_url: 'https://bob.com/inbox', protocol: :activitypub, domain: 'bob.com') } + let(:local_follower) { Fabricate(:account) } it 'sends expected activities to followed and follower inboxes' do subject diff --git a/spec/services/import_service_spec.rb b/spec/services/import_service_spec.rb index 3936b2363f..d67da66caf 100644 --- a/spec/services/import_service_spec.rb +++ b/spec/services/import_service_spec.rb @@ -190,7 +190,7 @@ RSpec.describe ImportService, type: :service do # Make sure to not actually go to the remote server before do - stub_request(:post, 'https://թութ.հայ/inbox').to_return(status: 200) + stub_request(:post, nare.inbox_url).to_return(status: 200) end it 'follows the listed account' do diff --git a/spec/services/notify_service_spec.rb b/spec/services/notify_service_spec.rb index 8fcb586580..50055dafc7 100644 --- a/spec/services/notify_service_spec.rb +++ b/spec/services/notify_service_spec.rb @@ -67,9 +67,10 @@ RSpec.describe NotifyService, type: :service do context 'when the message chain is initiated by recipient, but is not direct message' do let(:reply_to) { Fabricate(:status, account: recipient) } - let!(:mention) { Fabricate(:mention, account: sender, status: reply_to) } let(:activity) { Fabricate(:mention, account: recipient, status: Fabricate(:status, account: sender, visibility: :direct, thread: reply_to)) } + before { Fabricate(:mention, account: sender, status: reply_to) } + it 'does not notify' do expect { subject }.to_not change(Notification, :count) end @@ -77,10 +78,11 @@ RSpec.describe NotifyService, type: :service do context 'when the message chain is initiated by recipient, but without a mention to the sender, even if the sender sends multiple messages in a row' do let(:reply_to) { Fabricate(:status, account: recipient) } - let!(:mention) { Fabricate(:mention, account: sender, status: reply_to) } let(:dummy_reply) { Fabricate(:status, account: sender, visibility: :direct, thread: reply_to) } let(:activity) { Fabricate(:mention, account: recipient, status: Fabricate(:status, account: sender, visibility: :direct, thread: dummy_reply)) } + before { Fabricate(:mention, account: sender, status: reply_to) } + it 'does not notify' do expect { subject }.to_not change(Notification, :count) end @@ -88,9 +90,10 @@ RSpec.describe NotifyService, type: :service do context 'when the message chain is initiated by the recipient with a mention to the sender' do let(:reply_to) { Fabricate(:status, account: recipient, visibility: :direct) } - let!(:mention) { Fabricate(:mention, account: sender, status: reply_to) } let(:activity) { Fabricate(:mention, account: recipient, status: Fabricate(:status, account: sender, visibility: :direct, thread: reply_to)) } + before { Fabricate(:mention, account: sender, status: reply_to) } + it 'does notify' do expect { subject }.to change(Notification, :count) end diff --git a/spec/services/remove_status_service_spec.rb b/spec/services/remove_status_service_spec.rb index 7754ae8004..08ce0b0456 100644 --- a/spec/services/remove_status_service_spec.rb +++ b/spec/services/remove_status_service_spec.rb @@ -12,15 +12,15 @@ RSpec.describe RemoveStatusService, type: :service do let!(:bill) { Fabricate(:account, username: 'bill', protocol: :activitypub, domain: 'example2.com', inbox_url: 'http://example2.com/inbox') } before do - stub_request(:post, 'http://example.com/inbox').to_return(status: 200) - stub_request(:post, 'http://example2.com/inbox').to_return(status: 200) + stub_request(:post, hank.inbox_url).to_return(status: 200) + stub_request(:post, bill.inbox_url).to_return(status: 200) jeff.follow!(alice) hank.follow!(alice) end context 'when removed status is not a reblog' do - let!(:status) { PostStatusService.new.call(alice, text: 'Hello @bob@example.com ThisIsASecret') } + let!(:status) { PostStatusService.new.call(alice, text: "Hello @#{bob.pretty_acct} ThisIsASecret") } before do FavouriteService.new.call(jeff, status) @@ -39,7 +39,7 @@ RSpec.describe RemoveStatusService, type: :service do it 'sends Delete activity to followers' do subject.call(status) - expect(a_request(:post, 'http://example.com/inbox').with( + expect(a_request(:post, hank.inbox_url).with( body: hash_including({ 'type' => 'Delete', 'object' => { @@ -53,7 +53,7 @@ RSpec.describe RemoveStatusService, type: :service do it 'sends Delete activity to rebloggers' do subject.call(status) - expect(a_request(:post, 'http://example2.com/inbox').with( + expect(a_request(:post, bill.inbox_url).with( body: hash_including({ 'type' => 'Delete', 'object' => { @@ -78,7 +78,7 @@ RSpec.describe RemoveStatusService, type: :service do it 'sends Undo activity to followers' do subject.call(status) - expect(a_request(:post, 'http://example.com/inbox').with( + expect(a_request(:post, hank.inbox_url).with( body: hash_including({ 'type' => 'Undo', 'object' => hash_including({ @@ -96,7 +96,7 @@ RSpec.describe RemoveStatusService, type: :service do it 'sends Undo activity to followers' do subject.call(status) - expect(a_request(:post, 'http://example.com/inbox').with( + expect(a_request(:post, hank.inbox_url).with( body: hash_including({ 'type' => 'Undo', 'object' => hash_including({ diff --git a/spec/services/report_service_spec.rb b/spec/services/report_service_spec.rb index d3bcd5d31c..2a919f6405 100644 --- a/spec/services/report_service_spec.rb +++ b/spec/services/report_service_spec.rb @@ -156,9 +156,8 @@ RSpec.describe ReportService, type: :service do -> { described_class.new.call(source_account, target_account) } end - let!(:other_report) { Fabricate(:report, target_account: target_account) } - before do + Fabricate(:report, target_account: target_account) ActionMailer::Base.deliveries.clear source_account.user.settings['notification_emails.report'] = true source_account.user.save diff --git a/spec/services/resolve_account_service_spec.rb b/spec/services/resolve_account_service_spec.rb index 9f5fdca297..3c4b182e29 100644 --- a/spec/services/resolve_account_service_spec.rb +++ b/spec/services/resolve_account_service_spec.rb @@ -20,7 +20,7 @@ RSpec.describe ResolveAccountService, type: :service do let!(:remote_account) { Fabricate(:account, username: 'foo', domain: 'ap.example.com', protocol: 'activitypub') } context 'when domain is banned' do - let!(:domain_block) { Fabricate(:domain_block, domain: 'ap.example.com', severity: :suspend) } + before { Fabricate(:domain_block, domain: 'ap.example.com', severity: :suspend) } it 'does not return an account' do expect(subject.call('foo@ap.example.com', skip_webfinger: true)).to be_nil @@ -214,6 +214,7 @@ RSpec.describe ResolveAccountService, type: :service do expect(account.domain).to eq 'ap.example.com' expect(account.inbox_url).to eq 'https://ap.example.com/users/foo/inbox' expect(account.uri).to eq 'https://ap.example.com/users/foo' + expect(status.reload.account).to eq(account) end end diff --git a/spec/services/suspend_account_service_spec.rb b/spec/services/suspend_account_service_spec.rb index c258995b7e..b309ce511e 100644 --- a/spec/services/suspend_account_service_spec.rb +++ b/spec/services/suspend_account_service_spec.rb @@ -46,9 +46,9 @@ RSpec.describe SuspendAccountService, type: :service do let!(:account) { Fabricate(:account) } let!(:remote_follower) { Fabricate(:account, uri: 'https://alice.com', inbox_url: 'https://alice.com/inbox', protocol: :activitypub, domain: 'alice.com') } let!(:remote_reporter) { Fabricate(:account, uri: 'https://bob.com', inbox_url: 'https://bob.com/inbox', protocol: :activitypub, domain: 'bob.com') } - let!(:report) { Fabricate(:report, account: remote_reporter, target_account: account) } before do + Fabricate(:report, account: remote_reporter, target_account: account) remote_follower.follow!(account) end diff --git a/spec/services/unallow_domain_service_spec.rb b/spec/services/unallow_domain_service_spec.rb index 19d40e7e86..d96e354187 100644 --- a/spec/services/unallow_domain_service_spec.rb +++ b/spec/services/unallow_domain_service_spec.rb @@ -27,6 +27,7 @@ RSpec.describe UnallowDomainService, type: :service do end it 'removes remote accounts from that domain' do + expect { already_banned_account.reload }.to raise_error(ActiveRecord::RecordNotFound) expect(Account.where(domain: 'evil.org').exists?).to be false end diff --git a/spec/services/unsuspend_account_service_spec.rb b/spec/services/unsuspend_account_service_spec.rb index 2f737c6215..211c39e6c7 100644 --- a/spec/services/unsuspend_account_service_spec.rb +++ b/spec/services/unsuspend_account_service_spec.rb @@ -39,9 +39,9 @@ RSpec.describe UnsuspendAccountService, type: :service do let!(:account) { Fabricate(:account) } let!(:remote_follower) { Fabricate(:account, uri: 'https://alice.com', inbox_url: 'https://alice.com/inbox', protocol: :activitypub, domain: 'alice.com') } let!(:remote_reporter) { Fabricate(:account, uri: 'https://bob.com', inbox_url: 'https://bob.com/inbox', protocol: :activitypub, domain: 'bob.com') } - let!(:report) { Fabricate(:report, account: remote_reporter, target_account: account) } before do + Fabricate(:report, account: remote_reporter, target_account: account) remote_follower.follow!(account) end diff --git a/spec/workers/scheduler/user_cleanup_scheduler_spec.rb b/spec/workers/scheduler/user_cleanup_scheduler_spec.rb index 9909795008..8fda246ba8 100644 --- a/spec/workers/scheduler/user_cleanup_scheduler_spec.rb +++ b/spec/workers/scheduler/user_cleanup_scheduler_spec.rb @@ -18,12 +18,11 @@ describe Scheduler::UserCleanupScheduler do confirmed_user.update!(confirmed_at: 1.day.ago) end - it 'deletes the old unconfirmed user' do - expect { subject.perform }.to change { User.exists?(old_unconfirmed_user.id) }.from(true).to(false) - end - - it "deletes the old unconfirmed user's account" do - expect { subject.perform }.to change { Account.exists?(old_unconfirmed_user.account_id) }.from(true).to(false) + it 'deletes the old unconfirmed user, their account, and the moderation note' do + expect { subject.perform } + .to change { User.exists?(old_unconfirmed_user.id) }.from(true).to(false) + .and change { Account.exists?(old_unconfirmed_user.account_id) }.from(true).to(false) + expect { moderation_note.reload }.to raise_error(ActiveRecord::RecordNotFound) end it 'does not delete the new unconfirmed user or their account' do