Refactored AkismetHelper into AkismetService and cleaned up `Spammable`

- Refactored SpamCheckService into SpamService

18 files changed, 200 insertions, 229 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
diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb
index f9a02f310da..6677f5e80ba 100644
--- a/db/migrate/20160727163552_create_user_agent_details.rb
+++ b/db/migrate/20160727163552_create_user_agent_details.rb
@@ -1,4 +1,9 @@
 class CreateUserAgentDetails < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
   def change
     create_table :user_agent_details do |t|
       t.string :user_agent, null: false
diff --git a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb
index 5950874d5af..e28ab31d629 100644
--- a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb
+++ b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb
@@ -10,7 +10,7 @@ class RemoveProjectIdFromSpamLogs < ActiveRecord::Migration
   # When a migration requires downtime you **must** uncomment the following
   # constant and define a short and easy to understand explanation as to why the
   # migration requires downtime.
-  DOWNTIME_REASON = 'Removing a table that contains data that is not used anywhere.'
+  DOWNTIME_REASON = 'Removing a column that contains data that is not used anywhere.'
 
   # When using the methods "add_concurrent_index" or "add_column_with_default"
   # you must disable the use of transactions as these methods can not run in an
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index c4d3134da6c..077258faee1 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -3,8 +3,6 @@ module API
   class Issues < Grape::API
     before { authenticate! }
 
-    helpers ::Gitlab::AkismetHelper
-
     helpers do
       def filter_issues_state(issues, state)
         case state
diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb
deleted file mode 100644
index bd71a1aaa51..00000000000
--- a/lib/gitlab/akismet_helper.rb
+++ /dev/null
@@ -1,81 +0,0 @@
-module Gitlab
-  module AkismetHelper
-    def akismet_enabled?
-      current_application_settings.akismet_enabled
-    end
-
-    def akismet_client
-      @akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key,
-        Gitlab.config.gitlab.url)
-    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, user, text)
-      client = akismet_client
-      ip_address = client_ip(environment)
-      user_agent = user_agent(environment)
-
-      params = {
-        type: 'comment',
-        text: text,
-        created_at: DateTime.now,
-        author: user.name,
-        author_email: user.email,
-        referrer: environment['HTTP_REFERER'],
-      }
-
-      begin
-        is_spam, is_blatant = 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!(ip_address, user_agent, text, user)
-      client = akismet_client
-
-      params = {
-        type: 'comment',
-        text: text,
-        author: user.name,
-        author_email: user.email
-      }
-
-      begin
-        client.submit_ham(ip_address, user_agent, params)
-        true
-      rescue => e
-        Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
-        false
-      end
-    end
-
-    def spam!(details, text, user)
-      client = akismet_client
-
-      params = {
-        type: 'comment',
-        text: text,
-        author: user.name,
-        author_email: user.email
-      }
-
-      begin
-        client.submit_spam(details.ip_address, details.user_agent, params)
-        true
-      rescue => e
-        Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
-        false
-      end
-    end
-  end
-end
diff --git a/spec/controllers/admin/spam_logs_controller_spec.rb b/spec/controllers/admin/spam_logs_controller_spec.rb
index f94afd1139d..ac0441d0a4e 100644
--- a/spec/controllers/admin/spam_logs_controller_spec.rb
+++ b/spec/controllers/admin/spam_logs_controller_spec.rb
@@ -37,7 +37,7 @@ describe Admin::SpamLogsController do
 
   describe '#mark_as_ham' do
     before do
-      allow_any_instance_of(Gitlab::AkismetHelper).to receive(:ham!).and_return(true)
+      allow_any_instance_of(AkismetService).to receive(:ham!).and_return(true)
     end
     it 'submits the log as ham' do
       post :mark_as_ham, id: first_spam.id
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 8fcde9a38bc..0e8d4b80b0e 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -275,7 +275,7 @@ describe Projects::IssuesController do
     context 'Akismet is enabled' do
       before do
         allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true)
-        allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true)
+        allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
       end
 
       def post_spam_issue
@@ -325,7 +325,9 @@ describe Projects::IssuesController do
   describe 'POST #mark_as_spam' do
     context 'properly submits to Akismet' do
       before do
-        allow_any_instance_of(Spammable).to receive_messages(can_be_submitted?: true, submit_spam: true)
+        allow_any_instance_of(AkismetService).to receive_messages(spam!: true)
+        allow_any_instance_of(ApplicationSetting).to receive_messages(akismet_enabled: true)
+        allow_any_instance_of(SpamService).to receive_messages(can_be_submitted?: true)
       end
 
       def post_spam
diff --git a/spec/lib/gitlab/akismet_helper_spec.rb b/spec/lib/gitlab/akismet_helper_spec.rb
deleted file mode 100644
index 80b4f912d41..00000000000
--- a/spec/lib/gitlab/akismet_helper_spec.rb
+++ /dev/null
@@ -1,24 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::AkismetHelper, type: :helper do
-  let(:project) { create(:project, :public) }
-  let(:user) { create(:user) }
-
-  before do
-    allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
-    allow_any_instance_of(ApplicationSetting).to receive(:akismet_enabled).and_return(true)
-    allow_any_instance_of(ApplicationSetting).to receive(:akismet_api_key).and_return('12345')
-  end
-
-  describe '#is_spam?' do
-    it 'returns true for spam' do
-      environment = {
-        'action_dispatch.remote_ip' => '127.0.0.1',
-        'HTTP_USER_AGENT' => 'Test User Agent'
-      }
-
-      allow_any_instance_of(::Akismet::Client).to receive(:check).and_return([true, true])
-      expect(helper.is_spam?(environment, user, 'Is this spam?')).to eq(true)
-    end
-  end
-end
diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb
index 4e52d05918f..7944305e7b3 100644
--- a/spec/models/concerns/spammable_spec.rb
+++ b/spec/models/concerns/spammable_spec.rb
@@ -14,25 +14,24 @@ describe Issue, 'Spammable' do
   end
 
   describe 'InstanceMethods' do
-    before do
-      allow_any_instance_of(Gitlab::AkismetHelper).to receive(:akismet_enabled?).and_return(true)
-    end
-
     it 'should return the correct creator' do
-      expect(issue.send(:owner).id).to eq(issue.author_id)
+      expect(issue.owner_id).to eq(issue.author_id)
     end
 
     it 'should be invalid if spam' do
-      issue.spam = true
-      expect(issue.valid?).to be_truthy
+      issue = build(:issue, spam: true)
+      expect(issue.valid?).to be_falsey
     end
 
-    it 'should be submittable' do
+    it 'should not be submitted' do
       create(:user_agent_detail, subject: issue)
-      expect(issue.can_be_submitted?).to be_truthy
+      expect(issue.submitted?).to be_falsey
     end
 
     describe '#check_for_spam?' do
+      before do
+        allow_any_instance_of(ApplicationSetting).to receive(:akismet_enabled).and_return(true)
+      end
       it 'returns true for public project' do
         issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
         expect(issue.check_for_spam?).to eq(true)
@@ -43,14 +42,4 @@ describe Issue, 'Spammable' do
       end
     end
   end
-
-  describe 'AkismetMethods' do
-    before do
-      allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: true, akismet_enabled?: true)
-      allow_any_instance_of(Spammable).to receive(:can_be_submitted?).and_return(true)
-    end
-
-    it { expect(issue.spam_detected?(:mock_env)).to be_truthy }
-    it { expect(issue.submit_spam).to be_truthy }
-  end
 end
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 353b01d4a09..30b939c797c 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -532,7 +532,7 @@ describe API::API, api: true  do
   describe 'POST /projects/:id/issues with spam filtering' do
     before do
       allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true)
-      allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true)
+      allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true)
     end
 
     let(:params) do