Refactored AkismetHelper into AkismetService and cleaned up `Spammable`

- Refactored SpamCheckService into SpamService

9 files changed, 180 insertions, 98 deletions

diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb
index d15f00bf84c..7876d2ee767 100644
--- a/app/controllers/admin/spam_logs_controller.rb
+++ b/app/controllers/admin/spam_logs_controller.rb
@@ -1,5 +1,4 @@
 class Admin::SpamLogsController < Admin::ApplicationController
-  include Gitlab::AkismetHelper
 
   def index
     @spam_logs = SpamLog.order(id: :desc).page(params[:page])
@@ -20,8 +19,7 @@ class Admin::SpamLogsController < Admin::ApplicationController
   def mark_as_ham
     spam_log = SpamLog.find(params[:id])
 
-    if ham!(spam_log.source_ip, spam_log.user_agent, spam_log.text, spam_log.user)
-      spam_log.update_attribute(:submitted_as_ham, true)
+    if SpamService.new(spam_log).mark_as_ham!
       redirect_to admin_spam_logs_path, notice: 'Spam log successfully submitted as ham.'
     else
       redirect_to admin_spam_logs_path, notice: 'Error with Akismet. Please check the logs for more info.'
diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb
index 85be25d84cc..296811267e5 100644
--- a/app/controllers/concerns/spammable_actions.rb
+++ b/app/controllers/concerns/spammable_actions.rb
@@ -6,13 +6,7 @@ module SpammableActions
   end
 
   def mark_as_spam
-    if spammable.submit_spam
-      spammable.user_agent_detail.update_attribute(:submitted, true)
-
-      if spammable.is_a?(Issuable)
-        SystemNoteService.submit_spam(spammable, spammable.project, current_user)
-      end
-
+    if SpamService.new(spammable).mark_as_spam!(current_user)
       redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.'
     else
       flash[:error] = 'Error with Akismet. Please check the logs for more info.'
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb
index f272e7c5a55..694e2efcade 100644
--- a/app/models/concerns/spammable.rb
+++ b/app/models/concerns/spammable.rb
@@ -1,55 +1,36 @@
 module Spammable
   extend ActiveSupport::Concern
-  include Gitlab::AkismetHelper
 
   module ClassMethods
-    def attr_spammable(*attrs)
-      attrs.each do |attr|
-        spammable_attrs << attr.to_s
-      end
+    def attr_spammable(attr, options = {})
+      spammable_attrs << [attr.to_s, options]
     end
   end
 
   included do
     has_one :user_agent_detail, as: :subject, dependent: :destroy
     attr_accessor :spam
-    after_validation :check_for_spam, on: :create
+    after_validation :spam_detected?, on: :create
 
     cattr_accessor :spammable_attrs, instance_accessor: false do
       []
     end
+    delegate :submitted?, to: :user_agent_detail, allow_nil: true
   end
 
   def can_be_submitted?
     if user_agent_detail
-      user_agent_detail.submittable? && akismet_enabled?
+      user_agent_detail.submittable?
     else
       false
     end
   end
 
-  def submit_spam
-    return unless akismet_enabled? && can_be_submitted?
-    spam!(user_agent_detail, spammable_text, owner)
-  end
-
-  def spam_detected?(env)
-    @spam = is_spam?(env, owner, spammable_text)
-  end
-
   def spam?
     @spam
   end
 
-  def submitted?
-    if user_agent_detail
-      user_agent_detail.submitted
-    else
-      false
-    end
-  end
-
-  def check_for_spam
+  def spam_detected?
     self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam?
   end
 
@@ -61,34 +42,41 @@ module Spammable
     end
   end
 
-  def to_ability_name
-    self.class.to_s.underscore
-  end
-
-  # Override this method if an additional check is needed before calling Akismet
-  def check_for_spam?
-    akismet_enabled?
+  def owner
+    User.find(owner_id)
   end
 
   def spam_title
-    raise NotImplementedError
+    attr = self.class.spammable_attrs.select do |_, options|
+      options.fetch(:spam_title, false)
+    end
+
+    attr = attr[0].first
+
+    public_send(attr) if respond_to?(attr.to_sym)
   end
 
   def spam_description
-    raise NotImplementedError
-  end
+    attr = self.class.spammable_attrs.select do |_, options|
+      options.fetch(:spam_description, false)
+    end
 
-  private
+    attr = attr[0].first
+
+    public_send(attr) if respond_to?(attr.to_sym)
+  end
 
   def spammable_text
     result = []
-    self.class.spammable_attrs.each do |entry|
-      result << self.send(entry)
+    self.class.spammable_attrs.map do |attr|
+      result << public_send(attr.first)
     end
+
     result.reject(&:blank?).join("\n")
   end
 
-  def owner
-    User.find(owner_id)
+  # Override in Spammable if further checks are necessary
+  def check_for_spam?
+    current_application_settings.akismet_enabled
   end
 end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 40028e56489..ab98d0cf9df 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -37,7 +37,8 @@ class Issue < ActiveRecord::Base
   scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') }
   scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') }
 
-  attr_spammable :title, :description
+  attr_spammable :title, spam_title: true
+  attr_spammable :description, spam_description: true
 
   state_machine :state, initial: :opened do
     event :close do
@@ -266,16 +267,8 @@ class Issue < ActiveRecord::Base
     due_date.try(:past?) || false
   end
 
-  # To allow polymorphism with Spammable
+  # Only issues on public projects should be checked for spam
   def check_for_spam?
     super && project.public?
   end
-
-  def spam_title
-    title
-  end
-
-  def spam_description
-    description
-  end
 end
diff --git a/app/services/akismet_service.rb b/app/services/akismet_service.rb
new file mode 100644
index 00000000000..c09663bce85
--- /dev/null
+++ b/app/services/akismet_service.rb
@@ -0,0 +1,78 @@
+class AkismetService
+  attr_accessor :spammable
+
+  def initialize(spammable)
+    @spammable = spammable
+  end
+
+  def client_ip(env)
+    env['action_dispatch.remote_ip'].to_s
+  end
+
+  def user_agent(env)
+    env['HTTP_USER_AGENT']
+  end
+
+  def is_spam?(environment)
+    ip_address = client_ip(environment)
+    user_agent = user_agent(environment)
+
+    params = {
+      type: 'comment',
+      text: spammable.spammable_text,
+      created_at: DateTime.now,
+      author: spammable.owner.name,
+      author_email: spammable.owner.email,
+      referrer: environment['HTTP_REFERER'],
+    }
+
+    begin
+      is_spam, is_blatant = akismet_client.check(ip_address, user_agent, params)
+      is_spam || is_blatant
+    rescue => e
+      Rails.logger.error("Unable to connect to Akismet: #{e}, skipping check")
+      false
+    end
+  end
+
+  def ham!
+    params = {
+      type: 'comment',
+      text: spammable.text,
+      author: spammable.user.name,
+      author_email: spammable.user.email
+    }
+
+    begin
+      akismet_client.submit_ham(spammable.source_ip, spammable.user_agent, params)
+      true
+    rescue => e
+      Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
+      false
+    end
+  end
+
+  def spam!
+    params = {
+      type: 'comment',
+      text: spammable.spammable_text,
+      author: spammable.owner.name,
+      author_email: spammable.owner.email
+    }
+
+    begin
+      akismet_client.submit_spam(spammable.user_agent_detail.ip_address, spammable.user_agent_detail.user_agent, params)
+      true
+    rescue => e
+      Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
+      false
+    end
+  end
+
+  private
+
+  def akismet_client
+    @akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key,
+                                              Gitlab.config.gitlab.url)
+  end
+end
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 9f8a642a75b..67125d5c0e4 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -8,7 +8,7 @@ module Issues
       @issue = project.issues.new(params)
       @issue.author = params[:author] || current_user
 
-      spam_check_service.execute
+      @issue.spam = spam_service.check(@api, @request)
 
       if @issue.save
         @issue.update_attributes(label_ids: label_params)
@@ -25,8 +25,8 @@ module Issues
 
     private
 
-    def spam_check_service
-      SpamCheckService.new(@request, @api, @issue)
+    def spam_service
+      SpamService.new(@issue)
     end
 
     def user_agent_detail_service
diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb
deleted file mode 100644
index 71b9436a22e..00000000000
--- a/app/services/spam_check_service.rb
+++ /dev/null
@@ -1,33 +0,0 @@
-class SpamCheckService
-  attr_accessor :request, :api, :spammable
-
-  def initialize(request, api, spammable)
-    @request, @api, @spammable = request, api, spammable
-  end
-
-  def execute
-    if request && spammable.check_for_spam?
-      if spammable.spam_detected?(request.env)
-        create_spam_log
-      end
-    end
-  end
-
-  private
-  
-  def spam_log_attrs
-    {
-      user_id: spammable.owner_id,
-      title: spammable.spam_title,
-      description: spammable.spam_description,
-      source_ip: spammable.client_ip(request.env),
-      user_agent: spammable.user_agent(request.env),
-      noteable_type: spammable.class.to_s,
-      via_api: api
-    }
-  end
-
-  def create_spam_log
-    SpamLog.create(spam_log_attrs)
-  end
-end
diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb
new file mode 100644
index 00000000000..ad60de368aa
--- /dev/null
+++ b/app/services/spam_service.rb
@@ -0,0 +1,64 @@
+class SpamService
+  attr_accessor :spammable
+
+  def initialize(spammable)
+    @spammable = spammable
+  end
+
+  def check(api, request)
+    return false unless request && spammable.check_for_spam?
+    return false unless akismet.is_spam?(request.env)
+
+    create_spam_log(api, request)
+    true
+  end
+
+  def mark_as_spam!(current_user)
+    return false unless akismet_enabled? && spammable.can_be_submitted?
+    if akismet.spam!
+      spammable.user_agent_detail.update_attribute(:submitted, true)
+
+      if spammable.is_a?(Issuable)
+        SystemNoteService.submit_spam(spammable, spammable.project, current_user)
+      end
+      true
+    else
+      false
+    end
+  end
+
+  def mark_as_ham!
+    return false unless spammable.is_a?(SpamLog)
+
+    if akismet.ham!
+      spammable.update_attribute(:submitted_as_ham, true)
+      true
+    else
+      false
+    end
+  end
+
+  private
+
+  def akismet
+    @akismet ||= AkismetService.new(spammable)
+  end
+
+  def akismet_enabled?
+    current_application_settings.akismet_enabled
+  end
+
+  def create_spam_log(api, request)
+    SpamLog.create(
+      {
+        user_id: spammable.owner_id,
+        title: spammable.spam_title,
+        description: spammable.spam_description,
+        source_ip: akismet.client_ip(request.env),
+        user_agent: akismet.user_agent(request.env),
+        noteable_type: spammable.class.to_s,
+        via_api: api
+      }
+    )
+  end
+end
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 56d3329f5bd..35c9ce909e6 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -395,7 +395,7 @@ module SystemNoteService
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
 
-  # Called when the status of a Issuable is submitted as spam
+  # Called when Issuable is submitted as spam
   #
   # noteable - Noteable object
   # project  - Project owning noteable
@@ -407,7 +407,7 @@ module SystemNoteService
   #
   # Returns the created Note object
   def submit_spam(noteable, project, author)
-    body = "Submitted #{noteable.class.to_s.downcase} as spam"
+    body = "Submitted this #{noteable.class.to_s.downcase} as spam"
 
     create_note(noteable: noteable, project: project, author: author, note: body)
   end