Refactored spam related code even further

- Removed unnecessary column from `SpamLog`
- Moved creation of SpamLogs out of its own service and into SpamCheckService
- Simplified code in SpamCheckService.
- Moved move spam related code into Spammable concern

15 files changed, 137 insertions, 93 deletions

diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb
index 3a2f0185315..bc6fce0ec4e 100644
--- a/app/controllers/admin/spam_logs_controller.rb
+++ b/app/controllers/admin/spam_logs_controller.rb
@@ -14,4 +14,10 @@ class Admin::SpamLogsController < Admin::ApplicationController
       head :ok
     end
   end
+
+  def ham
+    spam_log = SpamLog.find(params[:id])
+
+    Gitlab::AkismetHelper.ham!(spam_log.source_ip, spam_log.user_agent, spam_log.description, spam_log.user)
+  end
 end
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb
index bbf6a3e0be3..5c75275b6e2 100644
--- a/app/models/concerns/spammable.rb
+++ b/app/models/concerns/spammable.rb
@@ -28,26 +28,42 @@ module Spammable
     end
   end
 
-  def submit_ham
-    return unless akismet_enabled? && can_be_submitted?
-    ham!(user_agent_detail, spammable_text, creator)
-  end
-
   def submit_spam
     return unless akismet_enabled? && can_be_submitted?
-    spam!(user_agent_detail, spammable_text, creator)
+    spam!(user_agent_detail, spammable_text, owner)
   end
 
-  def spam?(env, user)
-    is_spam?(env, user, spammable_text)
+  def spam_detected?(env)
+    @spam = is_spam?(env, owner, spammable_text)
   end
 
-  def spam_detected?
+  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_detected?
+    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
+
+  # Override this method if an additional check is needed before calling Akismet
+  def check_for_spam?
+    akismet_enabled?
+  end
+
+  def spam_title
+    raise 'Implement in included model!'
+  end
+
+  def spam_description
+    raise 'Implement in included model!'
   end
 
   private
@@ -60,11 +76,7 @@ module Spammable
     result.reject(&:blank?).join("\n")
   end
 
-  def creator
-    if self.author_id
-      User.find(self.author_id)
-    elsif self.creator_id
-      User.find(self.creator_id)
-    end
+  def owner
+    User.find(owner_id)
   end
 end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 5408e24b21c..40028e56489 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -265,4 +265,17 @@ class Issue < ActiveRecord::Base
   def overdue?
     due_date.try(:past?) || false
   end
+
+  # To allow polymorphism with Spammable
+  def check_for_spam?
+    super && project.public?
+  end
+
+  def spam_title
+    title
+  end
+
+  def spam_description
+    description
+  end
 end
diff --git a/app/services/create_spam_log_service.rb b/app/services/create_spam_log_service.rb
deleted file mode 100644
index 59a66fde47a..00000000000
--- a/app/services/create_spam_log_service.rb
+++ /dev/null
@@ -1,13 +0,0 @@
-class CreateSpamLogService < BaseService
-  def initialize(project, user, params)
-    super(project, user, params)
-  end
-
-  def execute
-    spam_params = params.merge({ user_id: @current_user.id,
-                                 project_id: @project.id } )
-    spam_log = SpamLog.new(spam_params)
-    spam_log.save
-    spam_log
-  end
-end
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index d580834be83..9f8a642a75b 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -3,34 +3,34 @@ module Issues
     def execute
       filter_params
       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
+      @request = params.delete(:request)
+      @api = params.delete(:api)
+      @issue = project.issues.new(params)
+      @issue.author = params[:author] || current_user
 
-      issue.spam = spam_check_service.execute(request, api, issue)
+      spam_check_service.execute
 
-      if issue.save
-        issue.update_attributes(label_ids: label_params)
-        notification_service.new_issue(issue, current_user)
-        todo_service.new_issue(issue, current_user)
-        event_service.open_issue(issue, current_user)
-        user_agent_detail_service(issue, request).create
-        issue.create_cross_references!(current_user)
-        execute_hooks(issue, 'open')
+      if @issue.save
+        @issue.update_attributes(label_ids: label_params)
+        notification_service.new_issue(@issue, current_user)
+        todo_service.new_issue(@issue, current_user)
+        event_service.open_issue(@issue, current_user)
+        user_agent_detail_service.create
+        @issue.create_cross_references!(current_user)
+        execute_hooks(@issue, 'open')
       end
 
-      issue
+      @issue
     end
 
     private
 
     def spam_check_service
-      SpamCheckService.new(project, current_user, params)
+      SpamCheckService.new(@request, @api, @issue)
     end
 
-    def user_agent_detail_service(issue, request)
-      UserAgentDetailService.new(issue, request)
+    def user_agent_detail_service
+      UserAgentDetailService.new(@issue, @request)
     end
   end
 end
diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb
index 7d6754546a8..71b9436a22e 100644
--- a/app/services/spam_check_service.rb
+++ b/app/services/spam_check_service.rb
@@ -1,32 +1,33 @@
-class SpamCheckService < BaseService
-  attr_accessor :request, :api, :subject
+class SpamCheckService
+  attr_accessor :request, :api, :spammable
 
-  def execute(request, api, subject)
-    @request, @api, @subject = request, api, subject
-    return false unless request || subject.check_for_spam?(project)
-    return false unless subject.spam?(request.env, current_user)
-
-    create_spam_log
+  def initialize(request, api, spammable)
+    @request, @api, @spammable = request, api, spammable
+  end
 
-    true
+  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: current_user.id,
-      project_id: project.id,
-      title: params[:title],
-      description: params[:description],
-      source_ip: subject.client_ip(request.env),
-      user_agent: subject.user_agent(request.env),
-      noteable_type: subject.class.to_s,
+      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
-    CreateSpamLogService.new(project, current_user, spam_log_attrs).execute
+    SpamLog.create(spam_log_attrs)
   end
 end
diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb
index dd995955be3..c07e2ca12a6 100644
--- a/app/services/user_agent_detail_service.rb
+++ b/app/services/user_agent_detail_service.rb
@@ -1,12 +1,12 @@
 class UserAgentDetailService
-  attr_accessor :subject, :request
+  attr_accessor :spammable, :request
 
-  def initialize(subject, request)
-    @subject, @request = subject, request
+  def initialize(spammable, request)
+    @spammable, @request = spammable, request
   end
 
   def create
     return unless request
-    subject.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s)
+    spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s)
   end
 end
diff --git a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb
new file mode 100644
index 00000000000..5950874d5af
--- /dev/null
+++ b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb
@@ -0,0 +1,29 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveProjectIdFromSpamLogs < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = true
+
+  # 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.'
+
+  # 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
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    remove_column :spam_logs, :project_id, :integer
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 2e5ffac5935..cc881e54763 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -926,7 +926,6 @@ ActiveRecord::Schema.define(version: 20160810142633) do
     t.string   "source_ip"
     t.string   "user_agent"
     t.boolean  "via_api"
-    t.integer  "project_id"
     t.string   "noteable_type"
     t.string   "title"
     t.text     "description"
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 7bbfc137c2c..c4d3134da6c 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -160,7 +160,7 @@ module API
 
         issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute
 
-        if issue.spam_detected?
+        if issue.spam?
           render_api_error!({ error: 'Spam detected' }, 400)
         end
 
diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb
index 19e73820321..b74d8176cc7 100644
--- a/lib/gitlab/akismet_helper.rb
+++ b/lib/gitlab/akismet_helper.rb
@@ -17,10 +17,6 @@ module Gitlab
       env['HTTP_USER_AGENT']
     end
 
-    def check_for_spam?(project)
-      akismet_enabled? && project.public?
-    end
-
     def is_spam?(environment, user, text)
       client = akismet_client
       ip_address = client_ip(environment)
@@ -44,7 +40,7 @@ module Gitlab
       end
     end
 
-    def ham!(details, text, user)
+    def ham!(ip_address, user_agent, text, user)
       client = akismet_client
 
       params = {
@@ -55,7 +51,7 @@ module Gitlab
       }
 
       begin
-        client.submit_ham(details.ip_address, details.user_agent, params)
+        client.submit_ham(ip_address, user_agent, params)
       rescue => e
         Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
       end
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 4e39826d694..efca838613f 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/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(Gitlab::AkismetHelper).to receive(:check_for_spam?).and_return(true)
+        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)
       end
 
diff --git a/spec/lib/gitlab/akismet_helper_spec.rb b/spec/lib/gitlab/akismet_helper_spec.rb
index b08396da4d2..80b4f912d41 100644
--- a/spec/lib/gitlab/akismet_helper_spec.rb
+++ b/spec/lib/gitlab/akismet_helper_spec.rb
@@ -10,17 +10,6 @@ describe Gitlab::AkismetHelper, type: :helper do
     allow_any_instance_of(ApplicationSetting).to receive(:akismet_api_key).and_return('12345')
   end
 
-  describe '#check_for_spam?' do
-    it 'returns true for public project' do
-      expect(helper.check_for_spam?(project)).to eq(true)
-    end
-
-    it 'returns false for private project' do
-      project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE)
-      expect(helper.check_for_spam?(project)).to eq(false)
-    end
-  end
-
   describe '#is_spam?' do
     it 'returns true for spam' do
       environment = {
diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb
index e61a6dcb69d..3f7d2721d22 100644
--- a/spec/models/concerns/spammable_spec.rb
+++ b/spec/models/concerns/spammable_spec.rb
@@ -15,7 +15,7 @@ describe Issue, 'Spammable' do
 
   describe 'InstanceMethods' do
     it 'should return the correct creator' do
-      expect(issue.send(:creator).id).to eq(issue.author_id)
+      expect(issue.send(:owner).id).to eq(issue.author_id)
     end
 
     it 'should be invalid if spam' do
@@ -27,15 +27,28 @@ describe Issue, 'Spammable' do
       create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s)
       expect(issue.can_be_submitted?).to be_truthy
     end
+
+    describe '#check_for_spam?' do
+      before do
+        allow_any_instance_of(Gitlab::AkismetHelper).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)
+      end
+
+      it 'returns false for other visibility levels' do
+        expect(issue.check_for_spam?).to eq(false)
+      end
+    end
   end
 
   describe 'AkismetMethods' do
     before do
-      allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(check_for_spam?: true, is_spam?: true, ham!: nil, spam!: nil)
+      allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: nil)
     end
 
-    it { expect(issue.spam?(:mock_env, :mock_user)).to be_truthy }
+    it { expect(issue.spam_detected?(:mock_env)).to be_truthy }
     it { expect(issue.submit_spam).to be_nil }
-    it { expect(issue.submit_ham).to be_nil }
   end
 end
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 3cd4e981fb2..353b01d4a09 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/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(Gitlab::AkismetHelper).to receive(:check_for_spam?).and_return(true)
+      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)
     end
 
@@ -554,7 +554,6 @@ describe API::API, api: true  do
       expect(spam_logs[0].description).to eq('content here')
       expect(spam_logs[0].user).to eq(user)
       expect(spam_logs[0].noteable_type).to eq('Issue')
-      expect(spam_logs[0].project_id).to eq(project.id)
     end
   end