Merge branch '235184_fix_missing_method_error_for_vulnerability_notifications' into 'master'

Do not try to send email for vulnerability notes

See merge request gitlab-org/gitlab!40545

app/models/note.rb

@@ -563,6 +563,10 @@ class Note < ApplicationRecord
     noteable.author if for_personal_snippet?
   end
 
+  def skip_notification?
+    review.present?
+  end
+
   private
 
   # Using this method followed by a call to `save` may result in ActiveRecord::RecordNotUnique exception

app/workers/new_note_worker.rb

@@ -13,17 +13,11 @@ class NewNoteWorker # rubocop:disable Scalability/IdempotentWorker
   # rubocop: disable CodeReuse/ActiveRecord
   def perform(note_id, _params = {})
     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
     else
       Gitlab::AppLogger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")
     end
   end
   # rubocop: enable CodeReuse/ActiveRecord
-
-  private
-
-  def skip_notification?(note)
-    note.review.present?
-  end
 end

ee/app/models/ee/note.rb

@@ -71,6 +71,11 @@ module EE
       group_reporter?(user, noteable.group)
     end
 
+    override :skip_notification?
+    def skip_notification?
+      for_vulnerability? || super
+    end
+
     private
 
     def system_note_for_epic?

ee/spec/models/note_spec.rb

@@ -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)
     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

spec/models/note_spec.rb

@@ -1416,4 +1416,20 @@ RSpec.describe Note do
       expect(note.parent_user).to be_nil
     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

spec/workers/new_note_worker_spec.rb

@@ -50,10 +50,20 @@ RSpec.describe NewNoteWorker do
     end
   end
 
-  context 'when note is with review' do
-    it 'does not create a new note notification' do
-      note = create(:note, :with_review)
+  context 'when note does not require notification' do
+    let(:note) { create(:note) }
 
+    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)
 
       subject.perform(note.id)