Move review related controllers/workers outside EE

Another step to moving Review feature to Core.

This focuses on moving DraftsController and related workers,
serializers, policies, and controller methods.

This also includes moving `submit_review` quick action outside EE.

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -162,8 +162,13 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   def renderable_notes
     define_diff_comment_vars unless @notes
 
-    @notes
+    draft_notes =
+      if current_user
+        merge_request.draft_notes.authored_by(current_user)
+      else
+        []
       end
-end
 
-Projects::MergeRequests::DiffsController.prepend_if_ee('EE::Projects::MergeRequests::DiffsController')
+    @notes.concat(draft_notes)
+  end
+end

app/workers/new_note_worker.rb

@@ -19,14 +19,11 @@ class NewNoteWorker # rubocop:disable Scalability/IdempotentWorker
       Gitlab::AppLogger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")
     end
   end
+  # rubocop: enable CodeReuse/ActiveRecord
 
   private
 
-  # EE-only method
   def skip_notification?(note)
-    false
+    note.review.present?
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 end
-
-NewNoteWorker.prepend_if_ee('EE::NewNoteWorker')

changelogs/unreleased/28154-move-controllers-outside-ee.yml

+---
+title: Move review related controllers/workers outside EE
+merge_request: 32663
+author:
+type: changed

config/routes/merge_requests.rb

@@ -55,6 +55,15 @@ resources :merge_requests, concerns: :awardable, except: [:new, :create, :show],
       delete :resolve, action: :unresolve
     end
   end
+
+  scope module: :merge_requests do
+    resources :drafts, only: [:index, :update, :create, :destroy] do
+      collection do
+        post :publish
+        delete :discard
+      end
+    end
+  end
 end
 
 scope path: 'merge_requests', controller: 'merge_requests/creations' do

ee/app/controllers/ee/projects/merge_requests/diffs_controller.rb

-# frozen_string_literal: true
-
-module EE
-  module Projects
-    module MergeRequests
-      module DiffsController
-        extend ActiveSupport::Concern
-
-        def renderable_notes
-          draft_notes =
-            if current_user
-              merge_request.draft_notes.authored_by(current_user)
-            else
-              []
-            end
-
-          super.concat(draft_notes)
-        end
-      end
-    end
-  end
-end

ee/app/workers/ee/new_note_worker.rb

-# frozen_string_literal: true
-
-module EE
-  module NewNoteWorker
-    extend ActiveSupport::Concern
-
-    private
-
-    # If a note belongs to a review
-    # We do not want to send a standalone
-    # notification
-    def skip_notification?(note)
-      note.review.present?
-    end
-  end
-end

ee/config/routes/merge_requests.rb

@@ -21,13 +21,4 @@ resources :merge_requests, only: [], constraints: { id: /\d+/ } do
   resources :approvers, only: :destroy
   delete 'approvers', to: 'approvers#destroy_via_user_id', as: :approver_via_user_id
   resources :approver_groups, only: :destroy
-
-  scope module: :merge_requests do
-    resources :drafts, only: [:index, :update, :create, :destroy] do
-      collection do
-        post :publish
-        delete :discard
-      end
-    end
-  end
 end

ee/lib/ee/gitlab/quick_actions/merge_request_actions.rb

@@ -20,23 +20,6 @@ module EE
               @execution_message[:approve] = _('Approved the current merge request.')
             end
           end
-
-          desc _('Submit a review')
-          explanation _('Submit the current review.')
-          types MergeRequest
-          condition do
-            quick_action_target.persisted?
-          end
-          command :submit_review do
-            next if params[:review_id]
-
-            result = DraftNotes::PublishService.new(quick_action_target, current_user).execute
-            @execution_message[:submit_review] = if result[:status] == :success
-                                                   _('Submitted the current review.')
-                                                 else
-                                                   result[:message]
-                                                 end
-          end
         end
       end
     end

ee/spec/factories/notes.rb

@@ -10,10 +10,6 @@ FactoryBot.modify do
     trait :on_vulnerability do
       noteable { association(:vulnerability, project: project) }
     end
-
-    trait :with_review do
-      review
-    end
   end
 end
 

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -765,30 +765,6 @@ describe QuickActions::InterpretService do
       end
     end
 
-    context 'submit_review command' do
-      using RSpec::Parameterized::TableSyntax
-
-      where(:note) do
-        [
-          'I like it',
-          '/submit_review'
-        ]
-      end
-
-      with_them do
-        let(:merge_request) { create(:merge_request, source_project: project) }
-        let(:content) { '/submit_review' }
-        let!(:draft_note) { create(:draft_note, note: note, merge_request: merge_request, author: current_user) }
-
-        it 'submits the users current review' do
-          _, _, message = service.execute(content, merge_request)
-
-          expect { draft_note.reload }.to raise_error(ActiveRecord::RecordNotFound)
-          expect(message).to eq('Submitted the current review.')
-        end
-      end
-    end
-
     shared_examples 'weight command' do
       it 'populates weight specified by the /weight command' do
         _, updates = service.execute(content, issuable)

ee/spec/workers/new_note_worker_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe NewNoteWorker do
-  context 'when skip_notification' do
-    it 'does not create a new note notification' do
-      note = create(:note, :with_review)
-
-      expect_any_instance_of(NotificationService).not_to receive(:new_note)
-
-      subject.perform(note.id)
-    end
-  end
-end

lib/gitlab/quick_actions/merge_request_actions.rb

@@ -104,6 +104,23 @@ module Gitlab
         command :target_branch do |branch_name|
           @updates[:target_branch] = branch_name if project.repository.branch_exists?(branch_name)
         end
+
+        desc _('Submit a review')
+        explanation _('Submit the current review.')
+        types MergeRequest
+        condition do
+          quick_action_target.persisted?
+        end
+        command :submit_review do
+          next if params[:review_id]
+
+          result = DraftNotes::PublishService.new(quick_action_target, current_user).execute
+          @execution_message[:submit_review] = if result[:status] == :success
+                                                 _('Submitted the current review.')
+                                               else
+                                                 result[:message]
+                                               end
+        end
       end
 
       def merge_orchestration_service

ee/spec/controllers/projects/merge_requests/drafts_controller_spec.rb → spec/controllers/projects/merge_requests/drafts_controller_spec.rb

@@ -300,7 +300,7 @@ describe Projects::MergeRequests::DraftsController do
     it 'publishes a draft note with quick actions and applies them' do
       project.add_developer(user2)
       create(:draft_note, merge_request: merge_request, author: user,
-                          note: "/assign #{user.to_reference} #{user2.to_reference}")
+                          note: "/assign #{user2.to_reference}")
 
       expect(merge_request.assignees).to be_empty
 
@@ -308,7 +308,7 @@ describe Projects::MergeRequests::DraftsController do
         .and change { DraftNote.count }.by(-1)
 
       expect(response).to have_gitlab_http_status(:ok)
-      expect(merge_request.reload.assignee_ids).to match_array([user.id, user2.id])
+      expect(merge_request.reload.assignee_ids).to match_array([user2.id])
       expect(Note.last.system?).to be true
     end
 

spec/factories/notes.rb

@@ -183,6 +183,10 @@ FactoryBot.define do
       confidential { true }
     end
 
+    trait :with_review do
+      review
+    end
+
     transient do
       in_reply_to { nil }
     end

spec/services/quick_actions/interpret_service_spec.rb

@@ -1621,6 +1621,29 @@ describe QuickActions::InterpretService do
         expect(message).to eq("Created branch '#{branch_name}' and a merge request to resolve this issue.")
       end
     end
+
+    context 'submit_review command' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:note) do
+        [
+          'I like it',
+          '/submit_review'
+        ]
+      end
+
+      with_them do
+        let(:content) { '/submit_review' }
+        let!(:draft_note) { create(:draft_note, note: note, merge_request: merge_request, author: developer) }
+
+        it 'submits the users current review' do
+          _, _, message = service.execute(content, merge_request)
+
+          expect { draft_note.reload }.to raise_error(ActiveRecord::RecordNotFound)
+          expect(message).to eq('Submitted the current review.')
+        end
+      end
+    end
   end
 
   describe '#explain' do

spec/workers/new_note_worker_spec.rb

@@ -49,4 +49,14 @@ describe NewNoteWorker do
       described_class.new.perform(unexistent_note_id)
     end
   end
+
+  context 'when note is with review' do
+    it 'does not create a new note notification' do
+      note = create(:note, :with_review)
+
+      expect_any_instance_of(NotificationService).not_to receive(:new_note)
+
+      subject.perform(note.id)
+    end
+  end
 end