Merge branch 'new-note-worker-record-not-found-fix' into 'master'

Fix record not found error on NewNoteWorker processing

Resolves #22678 

See merge request !6863

app/models/note.rb

@@ -7,6 +7,7 @@ class Note < ActiveRecord::Base
   include Importable
   include FasterCacheKeys
   include CacheMarkdownField
+  include AfterCommitQueue
 
   cache_markdown_field :note, pipeline: :note
 

app/services/notes/create_service.rb

@@ -26,9 +26,12 @@ module Notes
         note.note = content
       end
 
-      if !only_commands && note.save
+      note.run_after_commit do
         # Finish the harder work in the background
-        NewNoteWorker.perform_in(2.seconds, note.id, params)
+        NewNoteWorker.perform_async(note.id)
+      end
+
+      if !only_commands && note.save
         todo_service.new_note(note, current_user)
       end
 

app/workers/new_note_worker.rb

@@ -2,10 +2,12 @@ class NewNoteWorker
   include Sidekiq::Worker
   include DedicatedSidekiqQueue
 
-  def perform(note_id, note_params)
-    note = Note.find(note_id)
-
+  def perform(note_id)
+    if note = Note.find_by(id: note_id)
       NotificationService.new.new_note(note)
       Notes::PostProcessService.new(note).execute
+    else
+      Rails.logger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")
+    end
   end
 end

changelogs/unreleased/new-note-worker-record-not-found-fix.yml

+---
+title: Fix record not found error on NewNoteWorker processing
+merge_request: 6863
+author: Oswaldo Ferreira

spec/services/notes/create_service_spec.rb

@@ -14,12 +14,41 @@ describe Notes::CreateService, services: true do
     end
 
     context "valid params" do
-      before do
-        @note = Notes::CreateService.new(project, user, opts).execute
+      it 'returns a valid note' do
+        note = Notes::CreateService.new(project, user, opts).execute
+
+        expect(note).to be_valid
+      end
+
+      it 'returns a persisted note' do
+        note = Notes::CreateService.new(project, user, opts).execute
+
+        expect(note).to be_persisted
+      end
+
+      it 'note has valid content' do
+        note = Notes::CreateService.new(project, user, opts).execute
+
+        expect(note.note).to eq(opts[:note])
       end
 
-      it { expect(@note).to be_valid }
-      it { expect(@note.note).to eq(opts[:note]) }
+      it 'TodoService#new_note is called' do
+        note = build(:note)
+        allow(project).to receive_message_chain(:notes, :new).with(opts) { note }
+
+        expect_any_instance_of(TodoService).to receive(:new_note).with(note, user)
+
+        Notes::CreateService.new(project, user, opts).execute
+      end
+
+      it 'enqueues NewNoteWorker' do
+        note = build(:note, id: 999)
+        allow(project).to receive_message_chain(:notes, :new).with(opts) { note }
+
+        expect(NewNoteWorker).to receive(:perform_async).with(note.id)
+
+        Notes::CreateService.new(project, user, opts).execute
+      end
     end
 
     describe 'note with commands' do

spec/workers/new_note_worker_spec.rb

+require "spec_helper"
+
+describe NewNoteWorker do
+  context 'when Note found' do
+    let(:note) { create(:note) }
+
+    it "calls NotificationService#new_note" do
+      expect_any_instance_of(NotificationService).to receive(:new_note).with(note)
+
+      described_class.new.perform(note.id)
+    end
+
+    it "calls Notes::PostProcessService#execute" do
+      notes_post_process_service = double(Notes::PostProcessService)
+      allow(Notes::PostProcessService).to receive(:new).with(note) { notes_post_process_service }
+
+      expect(notes_post_process_service).to receive(:execute)
+
+      described_class.new.perform(note.id)
+    end
+  end
+
+  context 'when Note not found' do
+    let(:unexistent_note_id) { 999 }
+
+    it 'logs NewNoteWorker process skipping' do
+      expect(Rails.logger).to receive(:error).
+        with("NewNoteWorker: couldn't find note with ID=999, skipping job")
+
+      described_class.new.perform(unexistent_note_id)
+    end
+
+    it 'does not raise errors' do
+      expect { described_class.new.perform(unexistent_note_id) }.not_to raise_error
+    end
+
+    it "does not call NotificationService#new_note" do
+      expect_any_instance_of(NotificationService).not_to receive(:new_note)
+
+      described_class.new.perform(unexistent_note_id)
+    end
+
+    it "does not call Notes::PostProcessService#execute" do
+      expect_any_instance_of(Notes::PostProcessService).not_to receive(:execute)
+
+      described_class.new.perform(unexistent_note_id)
+    end
+  end
+end