Merge branch 'cablett-spam-tidy-spamservice-execute' into 'master'

Refactor SpamService

See merge request gitlab-org/gitlab!23236

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,15 @@ 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

@@ -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) }
@@ -31,4 +31,29 @@ describe SpamLog do
       expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
     end
   end
+
+  describe '.verify_recaptcha!' do
+    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: 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: -1, 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
@@ -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_service_spec.rb → spec/services/spam_check_service_spec.rb

@@ -2,24 +2,84 @@
 
 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
 
-    it 'yields block when recaptcha was already verified' do
-      issue = build_stubbed(:issue)
+    context 'when the request is present' do
+      let(:request) { double(:request, env: env) }
 
-      expect(check_spam(issue, nil, true)).to eql('yielded')
+      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_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(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 }
+
+      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
+
+        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 +89,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 +104,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 +116,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 +126,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 +139,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/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

+# 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" 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