Spam check only when spammable attributes have changed

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/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)) }
-        it 'doesnt check as spam when request is missing' do
-          check_spam(issue, nil, false)
-
-          expect(issue.spam).to be_falsey
+      context 'when spammable attributes have not changed' do
+        before do
+          issue.closed_at = Time.zone.now
+          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
-        it 'doesnt yield block' do
-          expect(check_spam(issue, request, false))
-            .to eql(SpamLog.last)
+      end
+      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)) }
-        it 'returns false' do
-          expect(check_spam(issue, request, false)).to be_falsey
+        context 'when indicated as spam by akismet' do
+          before do
+            allow(AkismetService).to receive(:new).and_return(double(is_spam?: true))
+          end
+          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