Merge branch 'cablett-spam-tidy-mark-spam-service' into 'master'

Abstract out MarkAsSpamService

See merge request gitlab-org/gitlab!22948

app/controllers/concerns/spammable_actions.rb

@@ -11,7 +11,7 @@ module SpammableActions
   end
 
   def mark_as_spam
-    if SpamService.new(spammable: spammable).mark_as_spam!
+    if Spam::MarkAsSpamService.new(spammable: spammable).execute
       redirect_to spammable_path, notice: _("%{spammable_titlecase} was submitted to Akismet successfully.") % { spammable_titlecase: spammable.spammable_entity_type.titlecase }
     else
       redirect_to spammable_path, alert: _('Error with Akismet. Please check the logs for more info.')

app/graphql/mutations/snippets/mark_as_spam.rb

@@ -24,7 +24,7 @@ module Mutations
       private
 
       def mark_as_spam(snippet)
-        SpamService.new(spammable: snippet).mark_as_spam!
+        Spam::MarkAsSpamService.new(spammable: snippet).execute
       end
 
       def authorized_resource?(snippet)

app/services/concerns/akismet_methods.rb

+# frozen_string_literal: true
+
+module AkismetMethods
+  def spammable_owner
+    @user ||= User.find(spammable_owner_id)
+  end
+
+  def spammable_owner_id
+    @owner_id ||=
+      if spammable.respond_to?(:author_id)
+        spammable.author_id
+      elsif spammable.respond_to?(:creator_id)
+        spammable.creator_id
+      end
+  end
+
+  def akismet
+    @akismet ||= AkismetService.new(
+      spammable_owner.name,
+      spammable_owner.email,
+      spammable.spammable_text,
+      options
+    )
+  end
+end

app/services/spam/mark_as_spam_service.rb

+# frozen_string_literal: true
+
+module Spam
+  class MarkAsSpamService
+    include ::AkismetMethods
+
+    attr_accessor :spammable, :options
+
+    def initialize(spammable:)
+      @spammable = spammable
+      @options = {}
+
+      @options[:ip_address] = @spammable.ip_address
+      @options[:user_agent] = @spammable.user_agent
+    end
+
+    def execute
+      return unless spammable.submittable_as_spam?
+      return unless akismet.submit_spam
+
+      spammable.user_agent_detail.update_attribute(:submitted, true)
+    end
+  end
+end

app/services/spam_service.rb

 # frozen_string_literal: true
 
 class SpamService
+  include AkismetMethods
+
   attr_accessor :spammable, :request, :options
   attr_reader :spam_log
 
-  def initialize(spammable:, request: nil)
+  def initialize(spammable:, request:)
     @spammable = spammable
     @request = request
     @options = {}
@@ -19,16 +21,6 @@ class SpamService
     end
   end
 
-  def mark_as_spam!
-    return false unless spammable.submittable_as_spam?
-
-    if akismet.submit_spam
-      spammable.user_agent_detail.update_attribute(:submitted, true)
-    else
-      false
-    end
-  end
-
   def when_recaptcha_verified(recaptcha_verified, api = false)
     # In case it's a request which is already verified through recaptcha, yield
     # block.
@@ -54,28 +46,6 @@ class SpamService
     true
   end
 
-  def akismet
-    @akismet ||= AkismetService.new(
-      spammable_owner.name,
-      spammable_owner.email,
-      spammable.spammable_text,
-      options
-    )
-  end
-
-  def spammable_owner
-    @user ||= User.find(spammable_owner_id)
-  end
-
-  def spammable_owner_id
-    @owner_id ||=
-      if spammable.respond_to?(:author_id)
-        spammable.author_id
-      elsif spammable.respond_to?(:creator_id)
-        spammable.creator_id
-      end
-  end
-
   def check_for_spam?
     spammable.check_for_spam?
   end

spec/requests/api/graphql/mutations/snippets/mark_as_spam_spec.rb

@@ -52,8 +52,8 @@ describe 'Mark snippet as spam' do
       end
 
       it 'marks snippet as spam' do
-        expect_next_instance_of(SpamService) do |instance|
-          expect(instance).to receive(:mark_as_spam!)
+        expect_next_instance_of(Spam::MarkAsSpamService) do |instance|
+          expect(instance).to receive(:execute)
         end
 
         post_graphql_mutation(mutation, current_user: current_user)

spec/services/spam/mark_as_spam_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Spam::MarkAsSpamService do
+  let(:user_agent_detail) { build(:user_agent_detail) }
+  let(:spammable) { build(:issue, user_agent_detail: user_agent_detail) }
+  let(:fake_akismet_service) { double(:akismet_service, submit_spam: true) }
+
+  subject { described_class.new(spammable: spammable) }
+
+  describe '#execute' do
+    before do
+      allow(subject).to receive(:akismet).and_return(fake_akismet_service)
+    end
+
+    context 'when the spammable object is not submittable' do
+      before do
+        allow(spammable).to receive(:submittable_as_spam?).and_return false
+      end
+
+      it 'does not submit as spam' do
+        expect(subject.execute).to be_falsey
+      end
+    end
+
+    context 'spam is submitted successfully' do
+      before do
+        allow(spammable).to receive(:submittable_as_spam?).and_return true
+        allow(fake_akismet_service).to receive(:submit_spam).and_return true
+      end
+
+      it 'submits as spam' do
+        expect(subject.execute).to be_truthy
+      end
+
+      it "updates the spammable object's user agent detail as being submitted as spam" do
+        expect(user_agent_detail).to receive(:update_attribute)
+
+        subject.execute
+      end
+
+      context 'when Akismet does not consider it spam' do
+        it 'does not update the spammable object as spam' do
+          allow(fake_akismet_service).to receive(:submit_spam).and_return false
+
+          expect(subject.execute).to be_falsey
+        end
+      end
+    end
+  end
+end