Further refactor and syntax fixes.

app/controllers/admin/spam_logs_controller.rb

 class Admin::SpamLogsController < Admin::ApplicationController
-
   def index
     @spam_logs = SpamLog.order(id: :desc).page(params[:page])
   end
@@ -19,10 +18,10 @@ class Admin::SpamLogsController < Admin::ApplicationController
   def mark_as_ham
     spam_log = SpamLog.find(params[:id])
 
-    if SpamService.new(spam_log).mark_as_ham!
+    if HamService.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.'
+      redirect_to admin_spam_logs_path, alert: 'Error with Akismet. Please check the logs for more info.'
     end
   end
 end

app/controllers/concerns/spammable_actions.rb

@@ -6,18 +6,17 @@ module SpammableActions
   end
 
   def mark_as_spam
-    if SpamService.new(spammable).mark_as_spam!(current_user)
-      redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.'
+    if SpamService.new(spammable).mark_as_spam!
+      redirect_to spammable, notice: "#{spammable.class.to_s} was submitted to Akismet successfully."
     else
-      flash[:error] = 'Error with Akismet. Please check the logs for more info.'
-      redirect_to spammable
+      redirect_to spammable, alert: 'Error with Akismet. Please check the logs for more info.'
     end
   end
 
   private
 
   def spammable
-    raise NotImplementedError
+    raise NotImplementedError, "#{self.class} does not implement #{__method__}"
   end
 
   def authorize_submit_spammable!

app/models/concerns/spammable.rb

@@ -9,16 +9,19 @@ module Spammable
 
   included do
     has_one :user_agent_detail, as: :subject, dependent: :destroy
+
     attr_accessor :spam
-    after_validation :spam_detected?, on: :create
+
+    after_validation :check_for_spam, on: :create
 
     cattr_accessor :spammable_attrs, instance_accessor: false do
       []
     end
-    delegate :submitted?, to: :user_agent_detail, allow_nil: true
+
+    delegate :ip_address, :user_agent, to: :user_agent_detail, allow_nil: true
   end
 
-  def can_be_submitted?
+  def submittable_as_spam?
     if user_agent_detail
       user_agent_detail.submittable?
     else
@@ -30,46 +33,29 @@ module Spammable
     @spam
   end
 
-  def spam_detected?
+  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
 
-  def owner_id
-    if self.respond_to?(:author_id)
-      self.author_id
-    elsif self.respond_to?(:creator_id)
-      self.creator_id
-    end
-  end
-
-  def owner
-    User.find(owner_id)
-  end
-
   def spam_title
-    attr = self.class.spammable_attrs.select do |_, options|
+    attr = self.class.spammable_attrs.find do |_, options|
       options.fetch(:spam_title, false)
     end
 
-    attr = attr[0].first
-
-    public_send(attr) if respond_to?(attr.to_sym)
+    public_send(attr.first) if attr && respond_to?(attr.first.to_sym)
   end
 
   def spam_description
-    attr = self.class.spammable_attrs.select do |_, options|
+    attr = self.class.spammable_attrs.find do |_, options|
       options.fetch(:spam_description, false)
     end
 
-    attr = attr[0].first
-
-    public_send(attr) if respond_to?(attr.to_sym)
+    public_send(attr.first) if attr && respond_to?(attr.first.to_sym)
   end
 
   def spammable_text
-    result = []
-    self.class.spammable_attrs.map do |attr|
-      result << public_send(attr.first)
+    result = self.class.spammable_attrs.map do |attr|
+      public_send(attr.first)
     end
 
     result.reject(&:blank?).join("\n")
@@ -77,6 +63,6 @@ module Spammable
 
   # Override in Spammable if further checks are necessary
   def check_for_spam?
-    current_application_settings.akismet_enabled
+    true
   end
 end

app/models/issue.rb

@@ -8,7 +8,6 @@ class Issue < ActiveRecord::Base
   include Taskable
   include Spammable
   include FasterCacheKeys
-  include AkismetSubmittable
 
   DueDateStruct = Struct.new(:title, :name).freeze
   NoDueDate     = DueDateStruct.new('No Due Date', '0').freeze
@@ -269,6 +268,6 @@ class Issue < ActiveRecord::Base
 
   # Only issues on public projects should be checked for spam
   def check_for_spam?
-    super && project.public?
+    project.public?
   end
 end

app/models/user_agent_detail.rb

 class UserAgentDetail < ActiveRecord::Base
   belongs_to :subject, polymorphic: true
 
-  validates :user_agent,
-            presence: true
-  validates :ip_address,
-            presence: true
-  validates :subject_id,
-            presence: true
-  validates :subject_type,
-            presence: true
+  validates :user_agent, :ip_address, :subject_id, :subject_type, presence: true
 
   def submittable?
-    user_agent.present? && ip_address.present?
+    !submitted?
   end
 end

app/services/akismet_service.rb

 class AkismetService
-  attr_accessor :spammable
+  attr_accessor :owner, :text, :options
 
-  def initialize(spammable)
-    @spammable = spammable
+  def initialize(owner, text, options = {})
+    @owner = owner
+    @text = text
+    @options = options
   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)
+  def is_spam?
+    return false unless akismet_enabled?
 
     params = {
       type: 'comment',
-      text: spammable.spammable_text,
+      text: text,
       created_at: DateTime.now,
-      author: spammable.owner.name,
-      author_email: spammable.owner.email,
-      referrer: environment['HTTP_REFERER'],
+      author: owner.name,
+      author_email: owner.email,
+      referrer: options[:referrer],
     }
 
     begin
-      is_spam, is_blatant = akismet_client.check(ip_address, user_agent, params)
+      is_spam, is_blatant = akismet_client.check(options[:ip_address], options[:user_agent], params)
       is_spam || is_blatant
     rescue => e
       Rails.logger.error("Unable to connect to Akismet: #{e}, skipping check")
@@ -35,16 +28,18 @@ class AkismetService
     end
   end
 
-  def ham!
+  def submit_ham
+    return false unless akismet_enabled?
+
     params = {
       type: 'comment',
-      text: spammable.text,
-      author: spammable.user.name,
-      author_email: spammable.user.email
+      text: text,
+      author: owner.name,
+      author_email: owner.email
     }
 
     begin
-      akismet_client.submit_ham(spammable.source_ip, spammable.user_agent, params)
+      akismet_client.submit_ham(options[:ip_address], options[:user_agent], params)
       true
     rescue => e
       Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
@@ -52,16 +47,18 @@ class AkismetService
     end
   end
 
-  def spam!
+  def submit_spam
+    return false unless akismet_enabled?
+
     params = {
       type: 'comment',
-      text: spammable.spammable_text,
-      author: spammable.owner.name,
-      author_email: spammable.owner.email
+      text: text,
+      author: owner.name,
+      author_email: owner.email
     }
 
     begin
-      akismet_client.submit_spam(spammable.user_agent_detail.ip_address, spammable.user_agent_detail.user_agent, params)
+      akismet_client.submit_spam(options[:ip_address], options[:user_agent], params)
       true
     rescue => e
       Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
@@ -75,4 +72,8 @@ class AkismetService
     @akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key,
                                               Gitlab.config.gitlab.url)
   end
+
+  def akismet_enabled?
+    current_application_settings.akismet_enabled
+  end
 end

app/services/ham_service.rb

+class HamService
+  attr_accessor :spam_log
+
+  def initialize(spam_log)
+    @spam_log = spam_log
+  end
+
+  def mark_as_ham!
+    if akismet.submit_ham
+      spam_log.update_attribute(:submitted_as_ham, true)
+    else
+      false
+    end
+  end
+
+  private
+
+  def akismet
+    @akismet ||= AkismetService.new(
+      spam_log.user,
+      spam_log.text,
+      ip_address: spam_log.source_ip,
+      user_agent: spam_log.user_agent
+    )
+  end
+end

app/services/issues/create_service.rb

@@ -8,7 +8,7 @@ module Issues
       @issue = project.issues.new(params)
       @issue.author = params[:author] || current_user
 
-      @issue.spam = spam_service.check(@api, @request)
+      @issue.spam = spam_service.check(@api)
 
       if @issue.save
         @issue.update_attributes(label_ids: label_params)
@@ -26,7 +26,7 @@ module Issues
     private
 
     def spam_service
-      SpamService.new(@issue)
+      SpamService.new(@issue, @request)
     end
 
     def user_agent_detail_service

app/services/spam_service.rb

 class SpamService
-  attr_accessor :spammable
+  attr_accessor :spammable, :request, :options
 
-  def initialize(spammable)
+  def initialize(spammable, request = nil)
     @spammable = spammable
+    @request = request
+    @options = {}
+
+    if @request
+      @options[:ip_address] = @request.env['action_dispatch.remote_ip'].to_s
+      @options[:user_agent] = @request.env['HTTP_USER_AGENT']
+      @options[:referrer] = @request.env['HTTP_REFERRER']
+    else
+      @options[:ip_address] = @spammable.ip_address
+      @options[:user_agent] = @spammable.user_agent
+    end
   end
 
-  def check(api, request)
-    return false unless request && spammable.check_for_spam?
-    return false unless akismet.is_spam?(request.env)
+  def check(api = false)
+    return false unless request && check_for_spam?
 
-    create_spam_log(api, request)
+    return false unless akismet.is_spam?
+
+    create_spam_log(api)
     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)
+  def mark_as_spam!
+    return false unless spammable.submittable_as_spam?
 
-      if spammable.is_a?(Issuable)
-        SystemNoteService.submit_spam(spammable, spammable.project, current_user)
-      end
-      true
+    if akismet.submit_spam
+      spammable.user_agent_detail.update_attribute(:submitted, true)
     else
       false
     end
   end
 
-  def mark_as_ham!
-    return false unless spammable.is_a?(SpamLog)
+  private
 
-    if akismet.ham!
-      spammable.update_attribute(:submitted_as_ham, true)
-      true
-    else
-      false
-    end
+  def akismet
+    @akismet ||= AkismetService.new(
+      spammable_owner,
+      spammable.spammable_text,
+      options
+    )
   end
 
-  private
+  def spammable_owner
+    @user ||= User.find(spammable_owner_id)
+  end
 
-  def akismet
-    @akismet ||= AkismetService.new(spammable)
+  def spammable_owner_id
+    @owner_id ||=
+      if spammable.respond_to?(:author_id)
+        spammable.author_id
+      elsif spammable.respond_to?(:creator_id)
+        spammable.creator_id
+      end
   end
 
-  def akismet_enabled?
-    current_application_settings.akismet_enabled
+  def check_for_spam?
+    spammable.check_for_spam?
   end
 
-  def create_spam_log(api, request)
+  def create_spam_log(api)
     SpamLog.create(
       {
-        user_id: spammable.owner_id,
+        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),
+        source_ip: options[:ip_address],
+        user_agent: options[:user_agent],
         noteable_type: spammable.class.to_s,
         via_api: api
       }

app/services/system_note_service.rb

@@ -395,23 +395,6 @@ module SystemNoteService
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
 
-  # Called when Issuable is submitted as spam
-  #
-  # noteable - Noteable object
-  # project  - Project owning noteable
-  # author   - User performing the change
-  #
-  # Example Note text:
-  #
-  #   "Issue submitted as spam."
-  #
-  # Returns the created Note object
-  def submit_spam(noteable, project, author)
-    body = "Submitted this #{noteable.class.to_s.downcase} as spam"
-
-    create_note(noteable: noteable, project: project, author: author, note: body)
-  end
-
   private
 
   def notes_for_mentioner(mentioner, noteable, notes)

app/services/user_agent_detail_service.rb

@@ -7,6 +7,7 @@ class UserAgentDetailService
 
   def create
     return unless request
+
     spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s)
   end
 end

app/views/projects/issues/show.html.haml

@@ -37,10 +37,9 @@
                 = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue'
               %li
                 = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue)
-            - if @issue.can_be_submitted? && current_user.admin?
-              - unless @issue.submitted?
-                %li
-                  = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'btn-spam', title: 'Submit as spam'
+            - if @issue.submittable_as_spam? && current_user.admin?
+              %li
+                = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'btn-spam', title: 'Submit as spam'
 
         - if can?(current_user, :create_issue, @project)
           = link_to new_namespace_project_issue_path(@project.namespace, @project), class: 'hidden-xs hidden-sm btn btn-grouped new-issue-link btn-new btn-inverted', title: 'New issue', id: 'new_issue_link' do
@@ -48,10 +47,9 @@
         - if can?(current_user, :update_issue, @issue)
           = link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue'
           = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue'
-          = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit'
-        - if @issue.can_be_submitted? && current_user.admin?
-          - unless @issue.submitted?
+          - if @issue.submittable_as_spam? && current_user.admin?
             = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'hidden-xs hidden-sm btn btn-grouped btn-spam', title: 'Submit as spam'
+          = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit'
 
 
 .issue-details.issuable-details

db/migrate/20160727163552_create_user_agent_details.rb

@@ -10,7 +10,7 @@ class CreateUserAgentDetails < ActiveRecord::Migration
       t.string :ip_address, null: false
       t.integer :subject_id, null: false
       t.string :subject_type, null: false
-      t.boolean :submitted, default: false
+      t.boolean :submitted, default: false, null: false
 
       t.timestamps null: false
     end

db/schema.rb

@@ -1004,7 +1004,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do
     t.string   "ip_address",   null: false
     t.integer  "subject_id",   null: false
     t.string   "subject_type", null: false
-    t.boolean  "submitted",    default: false
+    t.boolean  "submitted",    default: false, null: false
     t.datetime "created_at",   null: false
     t.datetime "updated_at",   null: false
   end

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(AkismetService).to receive(:ham!).and_return(true)
+      allow_any_instance_of(AkismetService).to receive(:submit_ham).and_return(true)
     end
     it 'submits the log as ham' do
       post :mark_as_ham, id: first_spam.id

spec/controllers/projects/issues_controller_spec.rb

@@ -274,7 +274,7 @@ describe Projects::IssuesController do
   describe 'POST #create' 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(SpamService).to receive(:check_for_spam?).and_return(true)
         allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
       end
 
@@ -317,7 +317,7 @@ describe Projects::IssuesController do
       end
 
       it 'creates a user agent detail' do
-        expect{ post_new_issue }.to change(UserAgentDetail, :count)
+        expect{ post_new_issue }.to change(UserAgentDetail, :count).by(1)
       end
     end
   end
@@ -325,9 +325,8 @@ describe Projects::IssuesController do
   describe 'POST #mark_as_spam' do
     context 'properly submits to Akismet' do
       before do
-        allow_any_instance_of(AkismetService).to receive_messages(spam!: true)
+        allow_any_instance_of(AkismetService).to receive_messages(submit_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
@@ -342,13 +341,9 @@ describe Projects::IssuesController do
         }
       end
 
-      it 'creates a system note' do
-        expect{ post_spam }.to change(Note, :count)
-      end
-
       it 'updates issue' do
         post_spam
-        expect(issue.submitted?).to be_truthy
+        expect(issue.submittable_as_spam?).to be_falsey
       end
     end
   end

spec/factories/user_agent_details.rb

@@ -2,11 +2,6 @@ FactoryGirl.define do
   factory :user_agent_detail do
     ip_address '127.0.0.1'
     user_agent 'AppleWebKit/537.36'
-    subject_id 1
-    subject_type 'Issue'
-
-    trait :on_issue do
-      association :subject, factory: :issue
-    end
+    association :subject, factory: :issue
   end
 end

spec/models/concerns/spammable_spec.rb

@@ -9,29 +9,17 @@ describe Issue, 'Spammable' do
 
   describe 'ClassMethods' do
     it 'should return correct attr_spammable' do
-      expect(issue.send(:spammable_text)).to eq("#{issue.title}\n#{issue.description}")
+      expect(issue.spammable_text).to eq("#{issue.title}\n#{issue.description}")
     end
   end
 
   describe 'InstanceMethods' do
-    it 'should return the correct creator' do
-      expect(issue.owner_id).to eq(issue.author_id)
-    end
-
     it 'should be invalid if spam' do
       issue = build(:issue, spam: true)
       expect(issue.valid?).to be_falsey
     end
 
-    it 'should not be submitted' do
-      create(:user_agent_detail, subject: issue)
-      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)

spec/models/user_agent_detail_spec.rb

@@ -2,16 +2,30 @@ require 'rails_helper'
 
 describe UserAgentDetail, type: :model do
   describe '.submittable?' do
-    it 'should be submittable' do
-      detail = create(:user_agent_detail, :on_issue)
+    it 'is submittable when not already submitted' do
+      detail = build(:user_agent_detail)
+
       expect(detail.submittable?).to be_truthy
     end
+
+    it 'is not submittable when already submitted' do
+      detail = build(:user_agent_detail, submitted: true)
+
+      expect(detail.submittable?).to be_falsey
+    end
   end
 
   describe '.valid?' do
-    it 'should be valid with a subject' do
-      detail = create(:user_agent_detail, :on_issue)
+    it 'is valid with a subject' do
+      detail = build(:user_agent_detail)
+
       expect(detail).to be_valid
     end
+
+    it 'is invalid without a subject' do
+      detail = build(:user_agent_detail, subject: nil)
+
+      expect(detail).not_to be_valid
+    end
   end
 end

spec/requests/api/issues_spec.rb

@@ -531,7 +531,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(SpamService).to receive(:check_for_spam?).and_return(true)
       allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true)
     end