Refactor spam validation to a concern that can be easily reused and improve legibility in `SpamCheckService`akismet-ui-check

9 files changed, 63 insertions, 44 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 6a5b887b16e..a1e233a6539 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
   - Retrieve rendered HTML from cache in one request
   - Nokogiri's various parsing methods are now instrumented
   - Make fork counter always clickable. !5463 (winniehell)
+  - All created issues, API or WebUI, can be submitted to Akismet for spam check !5333
   - Remove `search_id` of labels dropdown filter to fix 'Missleading URI for labels in Merge Requests and Issues view'. !5368 (Scott Le)
   - Load project invited groups and members eagerly in `ProjectTeam#fetch_members`
   - Add GitLab Workhorse version to admin dashboard (Katarzyna Kobierska Ula Budziszewska)
@@ -93,7 +94,6 @@ v 8.10.0
   - Fix viewing notification settings when a project is pending deletion
   - Updated compare dropdown menus to use GL dropdown
   - Redirects back to issue after clicking login link
-  - All created issues, API or WebUI, can be submitted to Akismet for spam check !5333
   - Eager load award emoji on notes
   - Allow to define manual actions/builds on Pipelines and Environments
   - Fix pagination when sorting by columns with lots of ties (like priority)
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index f11e3fac959..d169b408b41 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -79,11 +79,11 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def create
-    @issue = Issues::CreateService.new(project, current_user, issue_params.merge({ request: request })).execute
+    @issue = Issues::CreateService.new(project, current_user, issue_params.merge(request: request)).execute
 
     respond_to do |format|
       format.html do
-        if @issue.errors.empty? && @issue.valid?
+        if @issue.valid?
           redirect_to issue_path(@issue)
         else
           render :new
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb
new file mode 100644
index 00000000000..3b8e6df2da9
--- /dev/null
+++ b/app/models/concerns/spammable.rb
@@ -0,0 +1,16 @@
+module Spammable
+  extend ActiveSupport::Concern
+
+  included do
+    attr_accessor :spam
+    after_validation :check_for_spam, on: :create
+  end
+
+  def spam?
+    @spam
+  end
+
+  def check_for_spam
+    self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam?
+  end
+end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 60af8c15340..d9428ebc9fb 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -6,6 +6,7 @@ class Issue < ActiveRecord::Base
   include Referable
   include Sortable
   include Taskable
+  include Spammable
 
   DueDateStruct = Struct.new(:title, :name).freeze
   NoDueDate     = DueDateStruct.new('No Due Date', '0').freeze
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 1085a1b93b8..5e2de2ccf64 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -2,14 +2,13 @@ module Issues
   class CreateService < Issues::BaseService
     def execute
       filter_params
-      label_params = params[:label_ids]
-      issue = project.issues.new(params.except(:label_ids, :request))
+      label_params = params.delete(:label_ids)
+      request = params.delete(:request)
+      api = params.delete(:api)
+      issue = project.issues.new(params)
       issue.author = params[:author] || current_user
 
-      if SpamCheckService.new(project, current_user, params).spam_detected?
-        issue.errors.add(:base, 'Your issue has been recognized as spam and has been discarded.')
-        return issue
-      end
+      issue.spam = spam_check_service.execute(request, api)
 
       if issue.save
         issue.update_attributes(label_ids: label_params)
@@ -22,5 +21,11 @@ module Issues
 
       issue
     end
+
+    private
+
+    def spam_check_service
+      SpamCheckService.new(project, current_user, params)
+    end
   end
 end
diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb
index 6768047aa63..7c3e692bde9 100644
--- a/app/services/spam_check_service.rb
+++ b/app/services/spam_check_service.rb
@@ -1,28 +1,38 @@
-class SpamCheckService
+class SpamCheckService < BaseService
   include Gitlab::AkismetHelper
 
-  attr_accessor :subject, :current_user, :params
+  attr_accessor :request, :api
 
-  def initialize(subject, user, params = {})
-    @subject, @current_user, @params = subject, user, params.dup
-  end
+  def execute(request, api)
+    @request, @api = request, api
+    return false unless request || check_for_spam?(project)
+    return false unless is_spam?(request.env, current_user, text)
+    
+    create_spam_log
 
-  def spam_detected?
-    request = params[:request]
-    return false unless request || check_for_spam?(subject)
+    true
+  end
 
-    text = [params[:title], params[:description]].reject(&:blank?).join("\n")
+  private
 
-    return false unless is_spam?(request.env, current_user, text)
-
-    attrs = {
+  def text
+    [params[:title], params[:description]].reject(&:blank?).join("\n")
+  end
+  
+  def spam_log_attrs
+    {
       user_id: current_user.id,
-      project_id: subject.id,
+      project_id: project.id,
       title: params[:title],
-      description: params[:description]
+      description: params[:description],
+      source_ip: client_ip(request.env),
+      user_agent: user_agent(request.env),
+      noteable_type: 'Issue',
+      via_api: api
     }
-    create_spam_log(subject, current_user, attrs, request.env, api: false)
+  end
 
-    true
+  def create_spam_log
+    CreateSpamLogService.new(project, current_user, spam_log_attrs).execute
   end
 end
diff --git a/doc/integration/akismet.md b/doc/integration/akismet.md
index 99a28b493c9..c222d21612f 100644
--- a/doc/integration/akismet.md
+++ b/doc/integration/akismet.md
@@ -1,6 +1,6 @@
 # Akismet
 
-> *Note:* Before 8.10 only issues submitted via the API and for non-project
+> *Note:* Before 8.11 only issues submitted via the API and for non-project
 members were submitted to Akismet.
 
 GitLab leverages [Akismet](http://akismet.com) to protect against spam. Currently
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 21b9eb367e7..c4d3134da6c 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -158,15 +158,13 @@ module API
 
         project = user_project
 
-        issue = ::Issues::CreateService.new(project, current_user, attrs.merge({ request: request })).execute
+        issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute
+
+        if issue.spam?
+          render_api_error!({ error: 'Spam detected' }, 400)
+        end
 
         if issue.valid?
-          # Need to check if id is nil here, because if issue is spam, errors
-          # get added, but Rails still thinks it's valid, but it is never saved
-          # so id will be nil
-          if issue.id.nil?
-            render_api_error!({ error: 'Spam detected' }, 400)
-          end
           # Find or create labels and attach to issue. Labels are valid because
           # we already checked its name, so there can't be an error here
           if params[:labels].present?
diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb
index fc1fbc5b600..207736b59db 100644
--- a/lib/gitlab/akismet_helper.rb
+++ b/lib/gitlab/akismet_helper.rb
@@ -43,16 +43,5 @@ module Gitlab
         false
       end
     end
-
-    def create_spam_log(project, current_user, attrs, env, api: true)
-      params = attrs.merge({
-                             source_ip: client_ip(env),
-                             user_agent: user_agent(env),
-                             noteable_type: 'Issue',
-                             via_api: api
-                           })
-
-      ::CreateSpamLogService.new(project, current_user, params).execute
-    end
   end
 end