Fix RSpec/LetSetup cop in spec/services (#28459)

.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

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

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

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

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

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

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

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({

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

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
 

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
 

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
 

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
 

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