Apply reviewer feedback

- Consistent method calls
- Tidy contexts and test objects
- Modify `log_spam` matcher to check for single created
spam log during block call, not total

app/services/concerns/spam_check_methods.rb

@@ -26,7 +26,8 @@ module SpamCheckMethods
     SpamCheckService.new(
       spammable: spammable,
       request: @request
-    ).execute(api: @api,
+    ).execute(
+      api: @api,
       recaptcha_verified: @recaptcha_verified,
       spam_log_id: @spam_log_id,
       user_id: user.id)

spec/models/spam_log_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe SpamLog do
-  let(:admin) { create(:admin) }
+  let_it_be(:admin) { create(:admin) }
 
   describe 'associations' do
     it { is_expected.to belong_to(:user) }
@@ -33,19 +33,19 @@ describe SpamLog do
   end
 
   describe '.verify_recaptcha!' do
-    let(:spam_log) { create(:spam_log, id: 123, user: admin, recaptcha_verified: false) }
+    let_it_be(:spam_log) { create(:spam_log, user: admin, recaptcha_verified: false) }
 
     context 'the record cannot be found' do
       it 'updates nothing' do
         expect(instance_of(described_class)).not_to receive(:update!)
 
-        described_class.verify_recaptcha!(id: 9999, user_id: admin.id)
+        described_class.verify_recaptcha!(id: spam_log.id, user_id: admin.id)
 
         expect(spam_log.recaptcha_verified).to be_falsey
       end
 
       it 'does not error despite not finding a record' do
-        expect { described_class.verify_recaptcha!(id: 9999, user_id: admin.id) }.not_to raise_error
+        expect { described_class.verify_recaptcha!(id: -1, user_id: admin.id) }.not_to raise_error
       end
     end
 

spec/services/issues/create_service_spec.rb

@@ -408,7 +408,7 @@ describe Issues::CreateService do
 
             it 'creates a new spam_log' do
               expect { issue }
-                .to log_spam(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue')
+                  .to have_spam_log(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue')
             end
 
             it 'assigns a spam_log to an issue' do
@@ -431,7 +431,7 @@ describe Issues::CreateService do
 
             it 'creates a new spam_log' do
               expect { issue }
-                .to log_spam(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue')
+                  .to have_spam_log(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue')
             end
 
             it 'assigns a spam_log to an issue' do

spec/services/snippets/create_service_spec.rb

@@ -86,7 +86,7 @@ describe Snippets::CreateService do
 
         it 'creates a new spam_log' do
           expect { snippet }
-            .to log_spam(title: snippet.title, noteable_type: snippet.class.name)
+            .to have_spam_log(title: snippet.title, noteable_type: snippet.class.name)
         end
 
         it 'assigns a spam_log to an issue' do

spec/services/spam_check_service_spec.rb

@@ -51,20 +51,17 @@ describe SpamCheckService do
 
   describe '#execute' do
     let(:request) { double(:request, env: env) }
-    let(:params) do
-      { user_id: user.id, api: nil, spam_log_id: 123 }
-    end
+
+    let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) }
 
     subject do
       described_service = described_class.new(spammable: issue, request: request)
-      described_service.execute(params.merge(recaptcha_verified: recaptcha_verified, spam_log_id: 123))
+      described_service.execute(user_id: user.id, api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id)
     end
 
     context 'when recaptcha was already verified' do
       let(:recaptcha_verified) { true }
 
-      let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false, id: 123) }
-
       it "updates spam log and doesn't check Akismet" do
         aggregate_failures do
           expect(SpamLog).not_to receive(:create!)

spec/support/matchers/log_spam.rb

 # frozen_string_literal: true
 
 # This matcher checks if one spam log with provided attributes was created
+# during the block evocation.
 #
 # Example:
 #
-# expect { create_issue }.to log_spam
-RSpec::Matchers.define :log_spam do |expected|
-  def spam_logs
-    SpamLog.all
-  end
+# expect { create_issue }.to log_spam(key1: value1, key2: value2)
 
+RSpec::Matchers.define :log_spam do |expected|
   match do |block|
+    @existing_logs_count = SpamLog.count
+
     block.call
 
-    expect(spam_logs).to contain_exactly(
-      have_attributes(expected)
-    )
+    @new_logs_count = SpamLog.count
+    @last_spam_log = SpamLog.last
+
+    expect(@new_logs_count - @existing_logs_count).to eq 1
+    expect(@last_spam_log).to have_attributes(expected)
   end
 
   description do
-    count = spam_logs.count
+    count = @new_logs_count - @existing_logs_count
 
     if count == 1
       keys = expected.keys.map(&:to_s)
-      actual = spam_logs.first.attributes.slice(*keys)
+      actual = @last_spam_log.attributes.slice(*keys)
       "create a spam log with #{expected} attributes. #{actual} created instead."
     else
       "create exactly 1 spam log with #{expected} attributes. #{count} spam logs created instead."
@@ -32,3 +34,34 @@ RSpec::Matchers.define :log_spam do |expected|
 
   supports_block_expectations
 end
+
+# This matcher checks that the last spam log
+# has the attributes provided.
+# The spam log does not have to be created during the block evocation.
+# The number of total spam logs just has to be more than one.
+#
+# Example:
+#
+# expect { create_issue }.to have_spam_log(key1: value1, key2: value2)
+
+RSpec::Matchers.define :have_spam_log do |expected|
+  match do |block|
+    block.call
+
+    @total_logs_count = SpamLog.count
+    @latest_spam_log = SpamLog.last
+    expect(SpamLog.last).to have_attributes(expected)
+  end
+
+  description do
+    if @total_logs_count > 0
+      keys = expected.keys.map(&:to_s)
+      actual = @latest_spam_log.attributes.slice(*keys)
+      "the last spam log to have #{expected} attributes. Last spam log has #{actual} attributes instead."
+    else
+      "there to be a spam log, but there are no spam logs."
+    end
+  end
+
+  supports_block_expectations
+end

spec/support/shared_examples/spam_check_shared_examples.rb

@@ -4,7 +4,7 @@ shared_examples 'akismet spam' do
   context 'when request is missing' do
     subject { described_class.new(spammable: issue, request: nil) }
 
-    it "doesn't check as spam when request is missing" do
+    it "doesn't check as spam" do
       subject
 
       expect(issue).not_to be_spam