Commit d4927ca2 authored by Mehmet Emin INAC's avatar Mehmet Emin INAC

Do not try to send email for vulnerability notes

Since we don't know email configuration for vulnerability notes, we are
getting exceptions on `NotificationService`, therefore, we've decided
to disable calling this service until we have email setup for notes of
vulnerability entities.
parent f8715e0f
...@@ -563,6 +563,10 @@ class Note < ApplicationRecord ...@@ -563,6 +563,10 @@ class Note < ApplicationRecord
noteable.author if for_personal_snippet? noteable.author if for_personal_snippet?
end end
def skip_notification?
review.present?
end
private private
# Using this method followed by a call to `save` may result in ActiveRecord::RecordNotUnique exception # Using this method followed by a call to `save` may result in ActiveRecord::RecordNotUnique exception
......
...@@ -13,17 +13,11 @@ class NewNoteWorker # rubocop:disable Scalability/IdempotentWorker ...@@ -13,17 +13,11 @@ class NewNoteWorker # rubocop:disable Scalability/IdempotentWorker
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def perform(note_id, _params = {}) def perform(note_id, _params = {})
if note = Note.find_by(id: note_id) if note = Note.find_by(id: note_id)
NotificationService.new.new_note(note) unless skip_notification?(note) NotificationService.new.new_note(note) unless note.skip_notification?
Notes::PostProcessService.new(note).execute Notes::PostProcessService.new(note).execute
else else
Gitlab::AppLogger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job") Gitlab::AppLogger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")
end end
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
private
def skip_notification?(note)
note.review.present?
end
end end
...@@ -71,6 +71,11 @@ module EE ...@@ -71,6 +71,11 @@ module EE
group_reporter?(user, noteable.group) group_reporter?(user, noteable.group)
end end
override :skip_notification?
def skip_notification?
for_vulnerability? || super
end
private private
def system_note_for_epic? def system_note_for_epic?
......
...@@ -158,4 +158,36 @@ RSpec.describe Note do ...@@ -158,4 +158,36 @@ RSpec.describe Note do
expect(described_class.count_for_vulnerability_id([vulnerability_1.id, vulnerability_2.id])).to eq(vulnerability_1.id => 1, vulnerability_2.id => 2) expect(described_class.count_for_vulnerability_id([vulnerability_1.id, vulnerability_2.id])).to eq(vulnerability_1.id => 1, vulnerability_2.id => 2)
end end
end end
describe '#skip_notification?' do
subject(:skip_notification?) { note.skip_notification? }
context 'when there is no review' do
context 'when the note is not for vulnerability' do
let(:note) { build(:note) }
it { is_expected.to be_falsey }
end
context 'when the note is for vulnerability' do
let(:note) { build(:note, :on_vulnerability) }
it { is_expected.to be_truthy }
end
end
context 'when the review exists' do
context 'when the note is not for vulnerability' do
let(:note) { build(:note, :with_review) }
it { is_expected.to be_truthy }
end
context 'when the note is for vulnerability' do
let(:note) { build(:note, :with_review, :on_vulnerability) }
it { is_expected.to be_truthy }
end
end
end
end end
...@@ -1416,4 +1416,20 @@ RSpec.describe Note do ...@@ -1416,4 +1416,20 @@ RSpec.describe Note do
expect(note.parent_user).to be_nil expect(note.parent_user).to be_nil
end end
end end
describe '#skip_notification?' do
subject(:skip_notification?) { note.skip_notification? }
context 'when there is no review' do
let(:note) { build(:note) }
it { is_expected.to be_falsey }
end
context 'when the review exists' do
let(:note) { build(:note, :with_review) }
it { is_expected.to be_truthy }
end
end
end end
...@@ -50,10 +50,20 @@ RSpec.describe NewNoteWorker do ...@@ -50,10 +50,20 @@ RSpec.describe NewNoteWorker do
end end
end end
context 'when note is with review' do context 'when note does not require notification' do
it 'does not create a new note notification' do let(:note) { create(:note) }
note = create(:note, :with_review)
before do
# TODO: `allow_next_instance_of` helper method is not working
# because ActiveRecord is directly calling `.allocate` on model
# classes and bypasses the `.new` method call.
# Fix the `allow_next_instance_of` helper and change these to mock
# the next instance of `Note` model class.
allow(Note).to receive(:find_by).with(id: note.id).and_return(note)
allow(note).to receive(:skip_notification?).and_return(true)
end
it 'does not create a new note notification' do
expect_any_instance_of(NotificationService).not_to receive(:new_note) expect_any_instance_of(NotificationService).not_to receive(:new_note)
subject.perform(note.id) subject.perform(note.id)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment