Merge branch '29483-spam-check-only-title-and-description' into 'master'

Spam check only when Snippet / Issue "title" or "description" are changed

Closes #29483 and #29645

See merge request !10104

app/controllers/projects/issues_controller.rb

@@ -148,7 +148,14 @@ class Projects::IssuesController < Projects::ApplicationController
       end
 
       format.json do
-        render json: @issue.to_json(include: { milestone: {}, assignee: { only: [:name, :username], methods: [:avatar_url] }, labels: { methods: :text_color } }, methods: [:task_status, :task_status_short])
+        if @issue.valid?
+          render json: @issue.to_json(methods: [:task_status, :task_status_short],
+                                      include: { milestone: {},
+                                                 assignee: { only: [:name, :username], methods: [:avatar_url] },
+                                                 labels: { methods: :text_color } })
+        else
+          render json: { errors: @issue.errors.full_messages }, status: :unprocessable_entity
+        end
       end
     end
 

app/models/concerns/spammable.rb

@@ -41,7 +41,7 @@ module Spammable
   def check_for_spam
     error_msg = if Gitlab::Recaptcha.enabled?
                   "Your #{spammable_entity_type} has been recognized as spam. "\
-                  "You can still submit it by solving Captcha."
+                  "Please, change the content or solve the reCAPTCHA to proceed."
                 else
                   "Your #{spammable_entity_type} has been recognized as spam and has been discarded."
                 end

app/models/issue.rb

@@ -211,9 +211,8 @@ class Issue < ActiveRecord::Base
     due_date.try(:past?) || false
   end
 
-  # Only issues on public projects should be checked for spam
   def check_for_spam?
-    project.public?
+    project.public? && (title_changed? || description_changed?)
   end
 
   def as_json(options = {})

app/models/snippet.rb

@@ -132,7 +132,8 @@ class Snippet < ActiveRecord::Base
   end
 
   def check_for_spam?
-    public?
+    visibility_level_changed?(to: Snippet::PUBLIC) ||
+      (public? && (title_changed? || content_changed?))
   end
 
   def spammable_entity_type

app/services/spam_check_service.rb

@@ -14,6 +14,9 @@ module SpamCheckService
     @spam_log_id        = params.delete(:spam_log_id)
   end
 
+  # In order to be proceed to the spam check process, @spammable has to be
+  # a dirty instance, which means it should be already assigned with the new
+  # attribute values.
   def spam_check(spammable, user)
     spam_service = SpamService.new(spammable, @request)
 

changelogs/unreleased/29483-spam-check-only-title-and-description.yml

+---
+title: Spam check only when spammable attributes have changed
+merge_request:
+author:

spec/controllers/projects/issues_controller_spec.rb

@@ -241,10 +241,27 @@ describe Projects::IssuesController do
               expect(spam_logs.first.recaptcha_verified).to be_falsey
             end
 
-            it 'renders verify template' do
-              update_spam_issue
+            context 'as HTML' do
+              it 'renders verify template' do
+                update_spam_issue
+
+                expect(response).to render_template(:verify)
+              end
+            end
+
+            context 'as JSON' do
+              before do
+                update_issue({ title: 'Spam Title', description: 'Spam lives here' }, format: :json)
+              end
+
+              it 'renders json errors' do
+                expect(json_response)
+                  .to eql("errors" => ["Your issue has been recognized as spam. Please, change the content or solve the reCAPTCHA to proceed."])
+              end
 
-              expect(response).to render_template(:verify)
+              it 'returns 422 status' do
+                expect(response).to have_http_status(422)
+              end
             end
           end
 

spec/models/issue_spec.rb

@@ -670,4 +670,41 @@ describe Issue, models: true do
       expect(attrs_hash).to include('time_estimate')
     end
   end
+
+  describe '#check_for_spam' do
+    let(:project) { create :project, visibility_level: visibility_level }
+    let(:issue) { create :issue, project: project }
+
+    subject do
+      issue.assign_attributes(description: description)
+      issue.check_for_spam?
+    end
+
+    context 'when project is public and spammable attributes changed' do
+      let(:visibility_level) { Gitlab::VisibilityLevel::PUBLIC }
+      let(:description) { 'woo' }
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when project is private' do
+      let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE }
+      let(:description) { issue.description }
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when spammable attributes have not changed' do
+      let(:visibility_level) { Gitlab::VisibilityLevel::PUBLIC }
+      let(:description) { issue.description }
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+  end
 end

spec/models/snippet_spec.rb

@@ -198,4 +198,47 @@ describe Snippet, models: true do
       expect(snippet.participants).to include(note1.author, note2.author)
     end
   end
+
+  describe '#check_for_spam' do
+    let(:snippet) { create :snippet, visibility_level: visibility_level }
+
+    subject do
+      snippet.assign_attributes(title: title)
+      snippet.check_for_spam?
+    end
+
+    context 'when public and spammable attributes changed' do
+      let(:visibility_level) { Snippet::PUBLIC }
+      let(:title) { 'woo' }
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when private' do
+      let(:visibility_level) { Snippet::PRIVATE }
+      let(:title) { snippet.title }
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+
+      it 'returns true when switching to public' do
+        snippet.save!
+        snippet.visibility_level = Snippet::PUBLIC
+
+        expect(snippet.check_for_spam?).to be_truthy
+      end
+    end
+
+    context 'when spammable attributes have not changed' do
+      let(:visibility_level) { Snippet::PUBLIC }
+      let(:title) { snippet.title }
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+  end
 end

spec/services/spam_service_spec.rb

@@ -19,42 +19,67 @@ describe SpamService, services: true do
       let(:issue)   { create(:issue, project: project) }
       let(:request) { double(:request, env: {}) }
 
-      context 'when indicated as spam by akismet' do
-        before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) }
+      context 'when spammable attributes have not changed' do
+        before do
+          issue.closed_at = Time.zone.now
 
-        it 'doesnt check as spam when request is missing' do
-          check_spam(issue, nil, false)
-
-          expect(issue.spam).to be_falsey
+          allow(AkismetService).to receive(:new).and_return(double(is_spam?: true))
         end
 
-        it 'checks as spam' do
-          check_spam(issue, request, false)
-
-          expect(issue.spam).to be_truthy
+        it 'returns false' do
+          expect(check_spam(issue, request, false)).to be_falsey
         end
 
-        it 'creates a spam log' do
+        it 'does not create a spam log' do
           expect { check_spam(issue, request, false) }
-            .to change { SpamLog.count }.from(0).to(1)
+            .not_to change { SpamLog.count }
         end
+      end
 
-        it 'doesnt yield block' do
-          expect(check_spam(issue, request, false))
-            .to eql(SpamLog.last)
+      context 'when spammable attributes have changed' do
+        before do
+          issue.description = 'SPAM!'
         end
-      end
 
-      context 'when not indicated as spam by akismet' do
-        before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) }
+        context 'when indicated as spam by akismet' do
+          before do
+            allow(AkismetService).to receive(:new).and_return(double(is_spam?: true))
+          end
 
-        it 'returns false' do
-          expect(check_spam(issue, request, false)).to be_falsey
+          it 'doesnt check as spam when request is missing' do
+            check_spam(issue, nil, false)
+
+            expect(issue.spam).to be_falsey
+          end
+
+          it 'checks as spam' do
+            check_spam(issue, request, false)
+
+            expect(issue.spam).to be_truthy
+          end
+
+          it 'creates a spam log' do
+            expect { check_spam(issue, request, false) }
+              .to change { SpamLog.count }.from(0).to(1)
+          end
+
+          it 'doesnt yield block' do
+            expect(check_spam(issue, request, false))
+              .to eql(SpamLog.last)
+          end
         end
 
-        it 'does not create a spam log' do
-          expect { check_spam(issue, request, false) }
-            .not_to change { SpamLog.count }
+        context 'when not indicated as spam by akismet' do
+          before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) }
+
+          it 'returns false' do
+            expect(check_spam(issue, request, false)).to be_falsey
+          end
+
+          it 'does not create a spam log' do
+            expect { check_spam(issue, request, false) }
+              .not_to change { SpamLog.count }
+          end
         end
       end
     end