From 8f04cf0eadbcde7fc5d1c970741e30ca8b97967d Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Wed, 20 Jul 2016 20:15:17 -0500
Subject: [PATCH] Refactor `SpamCheckService` to make it cleaner and clearer.

---
 CHANGELOG                                     |  1 -
 app/controllers/projects/issues_controller.rb |  8 +-----
 app/services/issues/create_service.rb         |  5 ++--
 app/services/issues/spam_check_service.rb     | 25 -----------------
 app/services/spam_check_service.rb            | 28 +++++++++++++++++++
 lib/api/issues.rb                             | 10 ++++---
 6 files changed, 38 insertions(+), 39 deletions(-)
 delete mode 100644 app/services/issues/spam_check_service.rb
 create mode 100644 app/services/spam_check_service.rb

diff --git a/CHANGELOG b/CHANGELOG
index 27e68431488..6a5b887b16e 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -93,7 +93,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
-  - Submit issues created via the WebUI by non project members to Akismet !5333
   - 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
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index b527dd0f4f2..f11e3fac959 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -81,15 +81,9 @@ class Projects::IssuesController < Projects::ApplicationController
   def create
     @issue = Issues::CreateService.new(project, current_user, issue_params.merge({ request: request })).execute
 
-    if @issue.nil?
-      @issue = @project.issues.new
-      flash[:notice] = 'Your issue has been recognized as spam and has been discarded.'
-      render :new and return
-    end
-
     respond_to do |format|
       format.html do
-        if @issue.valid?
+        if @issue.errors.empty? && @issue.valid?
           redirect_to issue_path(@issue)
         else
           render :new
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 496ea5a86a2..1085a1b93b8 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -6,8 +6,9 @@ module Issues
       issue = project.issues.new(params.except(:label_ids, :request))
       issue.author = params[:author] || current_user
 
-      if Issues::SpamCheckService.new(project, current_user, params).spam_detected?
-        return nil
+      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
 
       if issue.save
diff --git a/app/services/issues/spam_check_service.rb b/app/services/issues/spam_check_service.rb
deleted file mode 100644
index b8d4e37faf5..00000000000
--- a/app/services/issues/spam_check_service.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-module Issues
-  class SpamCheckService < BaseService
-    include Gitlab::AkismetHelper
-
-    def spam_detected?
-      text = [params[:title], params[:description]].reject(&:blank?).join("\n")
-      request = params[:request]
-
-      if request
-        if check_for_spam?(project) && is_spam?(request.env, current_user, text)
-          attrs = {
-            user_id: current_user.id,
-            project_id: project.id,
-            title: params[:title],
-            description: params[:description]
-          }
-          create_spam_log(project, current_user, attrs, request.env, api: false)
-          return true
-        end
-      end
-
-      false
-    end
-  end
-end
diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb
new file mode 100644
index 00000000000..6768047aa63
--- /dev/null
+++ b/app/services/spam_check_service.rb
@@ -0,0 +1,28 @@
+class SpamCheckService
+  include Gitlab::AkismetHelper
+
+  attr_accessor :subject, :current_user, :params
+
+  def initialize(subject, user, params = {})
+    @subject, @current_user, @params = subject, user, params.dup
+  end
+
+  def spam_detected?
+    request = params[:request]
+    return false unless request || check_for_spam?(subject)
+
+    text = [params[:title], params[:description]].reject(&:blank?).join("\n")
+
+    return false unless is_spam?(request.env, current_user, text)
+
+    attrs = {
+      user_id: current_user.id,
+      project_id: subject.id,
+      title: params[:title],
+      description: params[:description]
+    }
+    create_spam_log(subject, current_user, attrs, request.env, api: false)
+
+    true
+  end
+end
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 787d416b960..21b9eb367e7 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -160,11 +160,13 @@ module API
 
         issue = ::Issues::CreateService.new(project, current_user, attrs.merge({ request: request })).execute
 
-        if issue.nil?
-          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?