diff --git a/app/models/issue.rb b/app/models/issue.rb
index 602eed86d9ed78035fc10933dc6a93bdb3af764b..10a5d9d2a248914a867d42ee35e495578aeb7ad3 100644
--- a/app/models/issue.rb
+++ b/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 = {})
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index dbd564e5e7d7e772934b6754e7a7514dc16fd242..30aca62499c26e3f86bcd87d1a752a2eb6619775 100644
--- a/app/models/snippet.rb
+++ b/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
diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb
index 023e0824e85e4189a8bb77f928f737ac0df6bd8e..11030bee8f1b196775a6059a4f8510d27a532ebb 100644
--- a/app/services/spam_check_service.rb
+++ b/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)
diff --git a/changelogs/unreleased/29483-spam-check-only-title-and-description.yml b/changelogs/unreleased/29483-spam-check-only-title-and-description.yml
new file mode 100644
index 0000000000000000000000000000000000000000..de8cacb250d77ae1541b8f821fd33ad36e42ee1d
--- /dev/null
+++ b/changelogs/unreleased/29483-spam-check-only-title-and-description.yml
@@ -0,0 +1,4 @@
+---
+title: Spam check only when spammable attributes have changed
+merge_request:
+author:
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 73977d031f9d803fb5dc12b5c4574a475e7c3a6b..b8584301baadcc06fa752a589dd0922173b244de 100644
--- a/spec/models/issue_spec.rb
+++ b/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
diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb
index 219ab1989ea125c46f96ae4c1f4ea1cdcbbe5a4c..8095d01b69eceaaf7520cae35d27052793e0c6a3 100644
--- a/spec/models/snippet_spec.rb
+++ b/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
diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb
index 4ce3b95aa871e266c7fb03f043db129fd42267e2..e09c05ccf324594b67f5038823c1ae9dafa231c6 100644
--- a/spec/services/spam_service_spec.rb
+++ b/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