Merge branch 'refactor-shared-spam-methods' into 'master'

Refactor to extract and rename spam_action_response_fields methods

See merge request gitlab-org/gitlab!54256

app/graphql/mutations/concerns/mutations/can_mutate_spammable.rb

@@ -5,6 +5,7 @@ module Mutations
   # and optionally support the workflow to allow clients to display and solve CAPTCHAs.
   module CanMutateSpammable
     extend ActiveSupport::Concern
+    include Spam::Concerns::HasSpamActionResponseFields
 
     # NOTE: The arguments and fields are intentionally named with 'captcha' instead of 'recaptcha',
     # so that they can be applied to future alternative CAPTCHA implementations other than
@@ -59,25 +60,5 @@ module Mutations
         request: context[:request]
       }
     end
-
-    # with_spam_action_fields(spammable) { {other_fields: true} }    -> hash
-    #
-    # Takes a Spammable and a block as arguments.
-    #
-    # The block passed should be a hash, which the spam action fields will be merged into.
-    def with_spam_action_fields(spammable)
-      spam_action_fields = {
-        spam: spammable.spam?,
-        # NOTE: These fields are intentionally named with 'captcha' instead of 'recaptcha', so
-        # that they can be applied to future alternative CAPTCHA implementations other than
-        # reCAPTCHA (such as FriendlyCaptcha) without having to change the response field name
-        # in the API.
-        needs_captcha_response: spammable.render_recaptcha?,
-        spam_log_id: spammable.spam_log&.id,
-        captcha_site_key: Gitlab::CurrentSettings.recaptcha_site_key
-      }
-
-      yield.merge(spam_action_fields)
-    end
   end
 end

app/graphql/mutations/snippets/create.rb

@@ -56,7 +56,7 @@ module Mutations
         end
 
         snippet = service_response.payload[:snippet]
-        with_spam_action_fields(snippet) do
+        with_spam_action_response_fields(snippet) do
           {
             snippet: service_response.success? ? snippet : nil,
             errors: errors_on_object(snippet)

app/graphql/mutations/snippets/update.rb

@@ -45,7 +45,7 @@ module Mutations
         end
 
         snippet = service_response.payload[:snippet]
-        with_spam_action_fields(snippet) do
+        with_spam_action_response_fields(snippet) do
           {
             snippet: service_response.success? ? snippet : snippet.reset,
             errors: errors_on_object(snippet)

app/services/spam/spam_action_service.rb

@@ -9,7 +9,9 @@ module Spam
     # after the spammable is created/updated based on the remaining parameters.
     #
     # Takes a hash of parameters from an incoming request to modify a model (via a controller,
-    # service, or GraphQL mutation).
+    # service, or GraphQL mutation). The parameters will either be camelCase (if they are
+    # received directly via controller params) or underscore_case (if they have come from
+    # a GraphQL mutation which has converted them to underscore)
     #
     # Deletes the parameters which are related to spam and captcha processing, and returns
     # them in a SpamParams parameters object. See:
@@ -18,12 +20,12 @@ module Spam
       # NOTE: The 'captcha_response' field can be expanded to multiple fields when we move to future
       # alternative captcha implementations such as FriendlyCaptcha. See
       # https://gitlab.com/gitlab-org/gitlab/-/issues/273480
-      captcha_response = params.delete(:captcha_response)
+      captcha_response = params.delete(:captcha_response) || params.delete(:captchaResponse)
 
       SpamParams.new(
         api: params.delete(:api),
         captcha_response: captcha_response,
-        spam_log_id: params.delete(:spam_log_id)
+        spam_log_id: params.delete(:spam_log_id) || params.delete(:spamLogId)
       )
     end
 

lib/quality/test_level.rb

@@ -43,6 +43,7 @@ module Quality
         serializers
         services
         sidekiq
+        spam
         support_specs
         tasks
         uploaders

lib/spam/concerns/has_spam_action_response_fields.rb

+# frozen_string_literal: true
+
+module Spam
+  module Concerns
+    # This concern is shared by the controller and GraphQL layer to handle
+    # addition of spam/CAPTCHA related fields in the response.
+    module HasSpamActionResponseFields
+      extend ActiveSupport::Concern
+
+      # spam_action_response_fields(spammable)    -> hash
+      #
+      # Takes a Spammable as an argument and returns response fields necessary to display a CAPTCHA on
+      # the client.
+      def spam_action_response_fields(spammable)
+        {
+          spam: spammable.spam?,
+          # NOTE: These fields are intentionally named with 'captcha' instead of 'recaptcha', so
+          # that they can be applied to future alternative CAPTCHA implementations other than
+          # reCAPTCHA (such as FriendlyCaptcha) without having to change the response field name
+          # in the API.
+          needs_captcha_response: spammable.render_recaptcha?,
+          spam_log_id: spammable.spam_log&.id,
+          captcha_site_key: Gitlab::CurrentSettings.recaptcha_site_key
+        }
+      end
+
+      # with_spam_action_response_fields(spammable) { {other_fields: true} }    -> hash
+      #
+      # Takes a Spammable and a block as arguments.
+      #
+      # The block passed should be a hash, which the spam_action_fields will be merged into.
+      def with_spam_action_response_fields(spammable)
+        yield.merge(spam_action_response_fields(spammable))
+      end
+    end
+  end
+end

spec/graphql/mutations/concerns/mutations/can_mutate_spammable_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe Mutations::CanMutateSpammable do
     end
 
     it 'merges in spam action fields from spammable' do
-      result = subject.send(:with_spam_action_fields, spammable) do
+      result = subject.send(:with_spam_action_response_fields, spammable) do
         { other_field: true }
       end
       expect(result)

spec/lib/quality/test_level_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe Quality::TestLevel do
     context 'when level is unit' do
       it 'returns a pattern' do
         expect(subject.pattern(:unit))
-          .to eq("spec/{bin,channels,config,db,dependencies,elastic,elastic_integration,experiments,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,models,policies,presenters,rack_servers,replicators,routing,rubocop,serializers,services,sidekiq,support_specs,tasks,uploaders,validators,views,workers,tooling}{,/**/}*_spec.rb")
+          .to eq("spec/{bin,channels,config,db,dependencies,elastic,elastic_integration,experiments,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,models,policies,presenters,rack_servers,replicators,routing,rubocop,serializers,services,sidekiq,spam,support_specs,tasks,uploaders,validators,views,workers,tooling}{,/**/}*_spec.rb")
       end
     end
 
@@ -103,7 +103,7 @@ RSpec.describe Quality::TestLevel do
     context 'when level is unit' do
       it 'returns a regexp' do
         expect(subject.regexp(:unit))
-          .to eq(%r{spec/(bin|channels|config|db|dependencies|elastic|elastic_integration|experiments|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|models|policies|presenters|rack_servers|replicators|routing|rubocop|serializers|services|sidekiq|support_specs|tasks|uploaders|validators|views|workers|tooling)})
+          .to eq(%r{spec/(bin|channels|config|db|dependencies|elastic|elastic_integration|experiments|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|models|policies|presenters|rack_servers|replicators|routing|rubocop|serializers|services|sidekiq|spam|support_specs|tasks|uploaders|validators|views|workers|tooling)})
       end
     end
 

spec/spam/concerns/has_spam_action_response_fields_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Spam::Concerns::HasSpamActionResponseFields do
+  subject do
+    klazz = Class.new
+    klazz.include described_class
+    klazz.new
+  end
+
+  describe '#with_spam_action_response_fields' do
+    let(:spam_log) { double(:spam_log, id: 1) }
+    let(:spammable) { double(:spammable, spam?: true, render_recaptcha?: true, spam_log: spam_log) }
+    let(:recaptcha_site_key) { 'abc123' }
+
+    before do
+      allow(Gitlab::CurrentSettings).to receive(:recaptcha_site_key) { recaptcha_site_key }
+    end
+
+    it 'merges in spam action fields from spammable' do
+      result = subject.send(:with_spam_action_response_fields, spammable) do
+        { other_field: true }
+      end
+      expect(result)
+        .to eq({
+                 spam: true,
+                 needs_captcha_response: true,
+                 spam_log_id: 1,
+                 captcha_site_key: recaptcha_site_key,
+                 other_field: true
+               })
+    end
+  end
+end

spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb

@@ -21,14 +21,14 @@ RSpec.shared_examples 'a mutation which can mutate a spammable' do
     end
   end
 
-  describe "#with_spam_action_fields" do
+  describe "#with_spam_action_response_fields" do
     it 'resolves with spam action fields' do
       subject
 
       # NOTE: We do not need to assert on the specific values of spam action fields here, we only need
-      # to verify that #with_spam_action_fields was invoked and that the fields are present in the
-      # response. The specific behavior of #with_spam_action_fields is covered in the
-      # CanMutateSpammable unit tests.
+      # to verify that #with_spam_action_response_fields was invoked and that the fields are present in the
+      # response. The specific behavior of #with_spam_action_response_fields is covered in the
+      # HasSpamActionResponseFields unit tests.
       expect(mutation_response.keys)
         .to include('spam', 'spamLogId', 'needsCaptchaResponse', 'captchaSiteKey')
     end