diff options
-rw-r--r-- | app/models/issue.rb | 3 | ||||
-rw-r--r-- | app/models/snippet.rb | 3 | ||||
-rw-r--r-- | app/services/spam_check_service.rb | 3 | ||||
-rw-r--r-- | changelogs/unreleased/29483-spam-check-only-title-and-description.yml | 4 | ||||
-rw-r--r-- | spec/models/issue_spec.rb | 37 | ||||
-rw-r--r-- | spec/models/snippet_spec.rb | 43 | ||||
-rw-r--r-- | spec/services/spam_service_spec.rb | 71 |
7 files changed, 138 insertions, 26 deletions
diff --git a/app/models/issue.rb b/app/models/issue.rb index 602eed86d9e..10a5d9d2a24 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 dbd564e5e7d..30aca62499c 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 023e0824e85..11030bee8f1 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 00000000000..de8cacb250d --- /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 73977d031f9..b8584301baa 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 219ab1989ea..8095d01b69e 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 4ce3b95aa87..e09c05ccf32 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 |