Refactor and move SpamService

- Rename to SpamCheckService so it's obvious what it does
- Use `execute` method for consistency with other services
- Update unit tests
- move ActiveRecord logic into the model
- Add unit tests for new SpamLog method

app/models/spam_log.rb

@@ -12,4 +12,8 @@ class SpamLog < ApplicationRecord
   def text
     [title, description].join("\n")
   end
+
+  def self.verify_recaptcha!(id:, user_id:)
+    find_by(id: id, user_id: user_id)&.update!(recaptcha_verified: true)
+  end
 end

app/services/concerns/spam_check_methods.rb

@@ -22,14 +22,14 @@ module SpamCheckMethods
   # a dirty instance, which means it should be already assigned with the new
   # attribute values.
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
-  # rubocop: disable CodeReuse/ActiveRecord
   def spam_check(spammable, user)
-    spam_service = SpamService.new(spammable: spammable, request: @request)
-
-    spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do
-      user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true)
-    end
+    SpamCheckService.new(
+      spammable: spammable,
+      request: @request
+    ).execute(api: @api,
+      recaptcha_verified: @recaptcha_verified,
+      spam_log_id: @spam_log_id,
+      user_id: user.id)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 end

app/services/spam_service.rb → app/services/spam_check_service.rb

 # frozen_string_literal: true
 
-class SpamService
+class SpamCheckService
   include AkismetMethods
 
   attr_accessor :spammable, :request, :options
@@ -21,14 +21,14 @@ class SpamService
     end
   end
 
-  def when_recaptcha_verified(recaptcha_verified, api = false)
-    # In case it's a request which is already verified through recaptcha, yield
-    # block.
+  def execute(api: false, recaptcha_verified:, spam_log_id:, user_id:)
     if recaptcha_verified
-      yield
+      # If it's a request which is already verified through recaptcha,
+      # update the spam log accordingly.
+      SpamLog.verify_recaptcha!(user_id: user_id, id: spam_log_id)
     else
-      # Otherwise, it goes to Akismet and check if it's a spam. If that's the
-      # case, it assigns spammable record as "spam" and create a SpamLog record.
+      # Otherwise, it goes to Akismet for spam check.
+      # If so, it assigns spammable object as "spam" and creates a SpamLog record.
       possible_spam = check(api)
       spammable.spam = possible_spam unless spammable.allow_possible_spam?
       spammable.spam_log = spam_log
@@ -38,9 +38,9 @@ class SpamService
   private
 
   def check(api)
-    return false unless request && check_for_spam?
-
-    return false unless akismet.spam?
+    return unless request
+    return unless check_for_spam?
+    return unless akismet.spam?
 
     create_spam_log(api)
     true

spec/models/spam_log_spec.rb

@@ -31,4 +31,29 @@ describe SpamLog do
       expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
     end
   end
+
+  describe '.verify_recaptcha!' do
+    let(:spam_log) { create(:spam_log, id: 123, 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)
+
+        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
+      end
+    end
+
+    context 'the record exists' do
+      it 'updates recaptcha_verified' do
+        expect { described_class.verify_recaptcha!(id: spam_log.id, user_id: admin.id) }
+          .to change { spam_log.reload.recaptcha_verified }.from(false).to(true)
+      end
+    end
+  end
 end

spec/requests/api/issues/post_projects_issues_spec.rb

@@ -389,7 +389,7 @@ describe API::Issues do
     end
 
     before do
-      expect_next_instance_of(SpamService) do |spam_service|
+      expect_next_instance_of(SpamCheckService) do |spam_service|
         expect(spam_service).to receive_messages(check_for_spam?: true)
       end
       expect_next_instance_of(AkismetService) do |akismet_service|

spec/requests/api/issues/put_projects_issues_spec.rb

@@ -194,7 +194,7 @@ describe API::Issues do
     end
 
     before do
-      expect_next_instance_of(SpamService) do |spam_service|
+      expect_next_instance_of(SpamCheckService) do |spam_service|
         expect(spam_service).to receive_messages(check_for_spam?: true)
       end
       expect_next_instance_of(AkismetService) do |akismet_service|

spec/services/issues/create_service_spec.rb

@@ -385,7 +385,7 @@ describe Issues::CreateService do
 
       context 'when recaptcha was not verified' do
         before do
-          expect_next_instance_of(SpamService) do |spam_service|
+          expect_next_instance_of(SpamCheckService) do |spam_service|
             expect(spam_service).to receive_messages(check_for_spam?: true)
           end
         end

spec/services/spam_service_spec.rb → spec/services/spam_check_service_spec.rb

@@ -2,24 +2,87 @@
 
 require 'spec_helper'
 
-describe SpamService do
-  describe '#when_recaptcha_verified' do
-    def check_spam(issue, request, recaptcha_verified)
-      described_class.new(spammable: issue, request: request).when_recaptcha_verified(recaptcha_verified) do
-        'yielded'
+describe SpamCheckService do
+  let(:fake_ip) { '1.2.3.4' }
+  let(:fake_user_agent) { 'fake-user-agent' }
+  let(:fake_referrer) { 'fake-http-referrer' }
+  let(:env) do
+    { 'action_dispatch.remote_ip' => fake_ip,
+      'HTTP_USER_AGENT' => fake_user_agent,
+      'HTTP_REFERRER' => fake_referrer }
   end
+  let(:request) { double(:request, env: env) }
+
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:issue) { create(:issue, project: project, author: user) }
+
+  before do
+    issue.spam = false
+  end
+
+  describe '#initialize' do
+    subject { described_class.new(spammable: issue, request: request) }
+
+    context 'when the request is nil' do
+      let(:request) { nil }
+
+      it 'assembles the options with information from the spammable' do
+        aggregate_failures do
+          expect(subject.options[:ip_address]).to eq(issue.ip_address)
+          expect(subject.options[:user_agent]).to eq(issue.user_agent)
+          expect(subject.options.key?(:referrer)).to be_falsey
+        end
+      end
+    end
+
+    context 'when the request is present' do
+      let(:request) { double(:request, env: env) }
+
+      it 'assembles the options with information from the spammable' do
+        aggregate_failures do
+          expect(subject.options[:ip_address]).to eq(fake_ip)
+          expect(subject.options[:user_agent]).to eq(fake_user_agent)
+          expect(subject.options[:referrer]).to eq(fake_referrer)
+        end
+      end
+    end
+  end
+
+  describe '#execute' do
+    let(:request) { double(:request, env: env) }
+    let(:params) do
+      { user_id: user.id, api: nil, spam_log_id: 123 }
     end
 
-    it 'yields block when recaptcha was already verified' do
-      issue = build_stubbed(:issue)
+    subject do
+      described_service = described_class.new(spammable: issue, request: request)
+      described_service.execute(params.merge(recaptcha_verified: recaptcha_verified, spam_log_id: 123))
+    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!)
+          expect(an_instance_of(described_class)).not_to receive(:check)
+        end
 
-      expect(check_spam(issue, nil, true)).to eql('yielded')
+        subject
+      end
+
+      it 'updates spam log' do
+        subject
+
+        expect(existing_spam_log.reload.recaptcha_verified).to be_truthy
+      end
     end
 
     context 'when recaptcha was not verified' do
-      let(:project) { create(:project, :public) }
-      let(:issue)   { create(:issue, project: project) }
-      let(:request) { double(:request, env: {}) }
+      let(:recaptcha_verified) { false }
 
       context 'when spammable attributes have not changed' do
         before do
@@ -29,11 +92,11 @@ describe SpamService do
         end
 
         it 'returns false' do
-          expect(check_spam(issue, request, false)).to be_falsey
+          expect(subject).to be_falsey
         end
 
         it 'does not create a spam log' do
-          expect { check_spam(issue, request, false) }
+          expect { subject }
             .not_to change { SpamLog.count }
         end
       end
@@ -44,24 +107,6 @@ describe SpamService do
         end
 
         context 'when indicated as spam by akismet' do
-          shared_examples 'akismet spam' do
-            it "doesn't check as spam when request is missing" do
-              check_spam(issue, nil, false)
-
-              expect(issue).not_to be_spam
-            end
-
-            it 'creates a spam log' do
-              expect { check_spam(issue, request, false) }
-                .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue')
-            end
-
-            it 'does not yield to the block' do
-              expect(check_spam(issue, request, false))
-                .to eql(SpamLog.last)
-            end
-          end
-
           before do
             allow(AkismetService).to receive(:new).and_return(double(spam?: true))
           end
@@ -74,9 +119,9 @@ describe SpamService do
             it_behaves_like 'akismet spam'
 
             it 'checks as spam' do
-              check_spam(issue, request, false)
+              subject
 
-              expect(issue.spam).to be_truthy
+              expect(issue.reload.spam).to be_truthy
             end
           end
 
@@ -84,9 +129,9 @@ describe SpamService do
             it_behaves_like 'akismet spam'
 
             it 'does not check as spam' do
-              check_spam(issue, request, false)
+              subject
 
-              expect(issue.spam).to be_nil
+              expect(issue.spam).to be_falsey
             end
           end
         end
@@ -97,11 +142,11 @@ describe SpamService do
           end
 
           it 'returns false' do
-            expect(check_spam(issue, request, false)).to be_falsey
+            expect(subject).to be_falsey
           end
 
           it 'does not create a spam log' do
-            expect { check_spam(issue, request, false) }
+            expect { subject }
               .not_to change { SpamLog.count }
           end
         end

spec/support/shared_examples/spam_check_shared_examples.rb

+# frozen_string_literal: true
+
+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
+      subject
+
+      expect(issue).not_to be_spam
+    end
+  end
+
+  context 'when request exists' do
+    it 'creates a spam log' do
+      expect { subject }
+          .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue')
+    end
+  end
+end