Merge branch 'id-move-approval-services' into 'master'

Add MergeRequests::ApprovalService to FOSS

See merge request gitlab-org/gitlab!35529

app/models/system_note_metadata.rb

@@ -18,7 +18,7 @@ class SystemNoteMetadata < ApplicationRecord
     designs_added designs_modified designs_removed designs_discussion_added
     title time_tracking branch milestone discussion task moved
     opened closed merged duplicate locked unlocked outdated
-    tag due_date pinned_embed cherry_pick health_status
+    tag due_date pinned_embed cherry_pick health_status approved
   ].freeze
 
   validates :note, presence: true

app/services/event_create_service.rb

@@ -138,6 +138,10 @@ class EventCreateService
     event
   end
 
+  def approve_mr(merge_request, current_user)
+    create_record_event(merge_request, current_user, :approved)
+  end
+
   private
 
   def existing_wiki_event(wiki_page_meta, action)

ee/app/services/merge_requests/approval_service.rb → app/services/merge_requests/approval_service.rb

@@ -2,35 +2,27 @@
 
 module MergeRequests
   class ApprovalService < MergeRequests::BaseService
-    IncorrectApprovalPasswordError = Class.new(StandardError)
-
     def execute(merge_request)
-      if incorrect_approval_password?(merge_request)
-        raise IncorrectApprovalPasswordError
-      end
-
       approval = merge_request.approvals.new(user: current_user)
 
-      if save_approval(approval)
-        merge_request.reset_approval_cache!
+      return unless save_approval(approval)
+
+      reset_approvals_cache(merge_request)
       create_event(merge_request)
       create_approval_note(merge_request)
       mark_pending_todos_as_done(merge_request)
-
-        if merge_request.approvals_left.zero?
-          notification_service.async.approve_mr(merge_request, current_user)
-          execute_hooks(merge_request, 'approved')
-        else
-          execute_hooks(merge_request, 'approval')
-        end
-      end
+      execute_approval_hooks(merge_request, current_user)
     end
 
     private
 
-    def incorrect_approval_password?(merge_request)
-      merge_request.project.require_password_to_approve? &&
-        !Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password])
+    def reset_approvals_cache(merge_request)
+      merge_request.approvals.reset
+    end
+
+    def execute_approval_hooks(merge_request, current_user)
+      # Only one approval is required for a merge request to be approved
+      execute_hooks(merge_request, 'approved')
     end
 
     def save_approval(approval)
@@ -47,19 +39,10 @@ module MergeRequests
       todo_service.resolve_todos_for_target(merge_request, current_user)
     end
 
-    def calculate_approvals_metrics(merge_request)
-      return unless merge_request.project.feature_available?(:code_review_analytics)
-
-      ::Analytics::RefreshApprovalsData.new(merge_request).execute
-    end
-
     def create_event(merge_request)
-      # Making sure MergeRequest::Metrics updates are in sync with
-      # Event creation.
-      Event.transaction do
       event_service.approve_mr(merge_request, current_user)
-        calculate_approvals_metrics(merge_request)
-      end
     end
   end
 end
+
+MergeRequests::ApprovalService.prepend_if_ee('EE::MergeRequests::ApprovalService')

app/services/system_note_service.rb

@@ -273,6 +273,26 @@ module SystemNoteService
 
     ::SystemNotes::DesignManagementService.new(noteable: design.issue, project: design.project, author: discussion_note.author).design_discussion_added(discussion_note)
   end
+
+  # Called when the merge request is approved by user
+  #
+  # noteable - Noteable object
+  # user     - User performing approve
+  #
+  # Example Note text:
+  #
+  #   "approved this merge request"
+  #
+  # Returns the created Note object
+  def approve_mr(noteable, user)
+    merge_requests_service(noteable, noteable.project, user).approve_mr
+  end
+
+  private
+
+  def merge_requests_service(noteable, project, author)
+    ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author)
+  end
 end
 
 SystemNoteService.prepend_if_ee('EE::SystemNoteService')

app/services/system_notes/merge_requests_service.rb

@@ -150,6 +150,19 @@ module SystemNotes
 
       create_note(summary)
     end
+
+    # Called when the merge request is approved by user
+    #
+    # Example Note text:
+    #
+    #   "approved this merge request"
+    #
+    # Returns the created Note object
+    def approve_mr
+      body = "approved this merge request"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'approved'))
+    end
   end
 end
 

ee/app/models/ee/system_note_metadata.rb

@@ -5,7 +5,7 @@ module EE
     extend ::Gitlab::Utils::Override
 
     EE_ICON_TYPES = %w[
-      weight approved unapproved relate unrelate published
+      weight unapproved relate unrelate published
       epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
       epic_issue_moved issue_changed_epic epic_date_changed relate_epic unrelate_epic
       vulnerability_confirmed vulnerability_dismissed vulnerability_resolved

ee/app/services/ee/event_create_service.rb

@@ -13,9 +13,5 @@ module EE
     def reopen_epic(epic, current_user)
       create_record_event(epic, current_user, :reopened)
     end
-
-    def approve_mr(merge_request, current_user)
-      create_record_event(merge_request, current_user, :approved)
-    end
   end
 end

ee/app/services/ee/merge_requests/approval_service.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    module ApprovalService
+      extend ::Gitlab::Utils::Override
+
+      IncorrectApprovalPasswordError = Class.new(StandardError)
+
+      override :execute
+      def execute(merge_request)
+        if incorrect_approval_password?(merge_request)
+          raise IncorrectApprovalPasswordError
+        end
+
+        super
+      end
+
+      private
+
+      override :reset_approvals_cache
+      def reset_approvals_cache(merge_request)
+        merge_request.reset_approval_cache!
+      end
+
+      override :execute_approval_hooks
+      def execute_approval_hooks(merge_request, current_user)
+        if merge_request.approvals_left.zero?
+          notification_service.async.approve_mr(merge_request, current_user)
+          execute_hooks(merge_request, 'approved')
+        else
+          execute_hooks(merge_request, 'approval')
+        end
+      end
+
+      override :create_event
+      def create_event(merge_request)
+        # Making sure MergeRequest::Metrics updates are in sync with
+        # Event creation.
+        ::Event.transaction do
+          event_service.approve_mr(merge_request, current_user)
+          calculate_approvals_metrics(merge_request)
+        end
+      end
+
+      def incorrect_approval_password?(merge_request)
+        merge_request.project.require_password_to_approve? &&
+          !::Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password])
+      end
+
+      def calculate_approvals_metrics(merge_request)
+        return unless merge_request.project.feature_available?(:code_review_analytics)
+
+        ::Analytics::RefreshApprovalsData.new(merge_request).execute
+      end
+    end
+  end
+end

ee/app/services/ee/system_note_service.rb

@@ -48,20 +48,6 @@ module EE
       issuables_service(noteable, noteable.project, author).change_iteration(iteration)
     end
 
-    # Called when the merge request is approved by user
-    #
-    # noteable - Noteable object
-    # user     - User performing approve
-    #
-    # Example Note text:
-    #
-    #   "approved this merge request"
-    #
-    # Returns the created Note object
-    def approve_mr(noteable, user)
-      merge_requests_service(noteable, noteable.project, user).approve_mr
-    end
-
     def unapprove_mr(noteable, user)
       merge_requests_service(noteable, noteable.project, user).unapprove_mr
     end

ee/app/services/ee/system_notes/merge_requests_service.rb

@@ -3,19 +3,6 @@
 module EE
   module SystemNotes
     module MergeRequestsService
-      # Called when the merge request is approved by user
-      #
-      # Example Note text:
-      #
-      #   "approved this merge request"
-      #
-      # Returns the created Note object
-      def approve_mr
-        body = "approved this merge request"
-
-        create_note(NoteSummary.new(noteable, project, author, body, action: 'approved'))
-      end
-
       def unapprove_mr
         body = "unapproved this merge request"
 

ee/spec/services/ee/event_create_service_spec.rb

@@ -49,19 +49,4 @@ RSpec.describe EventCreateService do
       expect(event.group_id).to eq epic.group_id
     end
   end
-
-  describe 'Merge Requests' do
-    let_it_be(:user) { create(:user) }
-    let(:merge_request) { create(:merge_request) }
-
-    describe '#approve_mr' do
-      it { expect(service.approve_mr(merge_request, user)).to be_truthy }
-
-      it 'creates new event' do
-        service.approve_mr(merge_request, user)
-
-        change { Event.approved_action.where(target: merge_request).count }.by(1)
-      end
-    end
-  end
 end

ee/spec/services/ee/system_notes/merge_requests_service_spec.rb

@@ -7,20 +7,6 @@ RSpec.describe ::SystemNotes::MergeRequestsService do
   let(:project) { create(:project) }
   let(:noteable) { create(:merge_request, source_project: project) }
 
-  describe '#approve_mr' do
-    subject { described_class.new(noteable: noteable, project: project, author: author).approve_mr }
-
-    it_behaves_like 'a system note' do
-      let(:action) { 'approved' }
-    end
-
-    context 'when merge request approved' do
-      it 'sets the note text' do
-        expect(subject.note).to eq "approved this merge request"
-      end
-    end
-  end
-
   describe '#unapprove_mr' do
     subject { described_class.new(noteable: noteable, project: project, author: author).unapprove_mr }
 

ee/spec/services/system_note_service_spec.rb

@@ -48,16 +48,6 @@ RSpec.describe SystemNoteService do
     end
   end
 
-  describe '.approve_mr' do
-    it 'calls MergeRequestsService' do
-      expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service|
-        expect(service).to receive(:approve_mr)
-      end
-
-      described_class.approve_mr(noteable, author)
-    end
-  end
-
   describe '.unapprove_mr' do
     it 'calls MergeRequestsService' do
       expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service|

spec/services/event_create_service_spec.rb

@@ -87,6 +87,18 @@ RSpec.describe EventCreateService do
         expect { service.reopen_mr(merge_request, merge_request.author) }.to change { ResourceStateEvent.count }
       end
     end
+
+    describe '#approve_mr' do
+      let(:merge_request) { create(:merge_request) }
+
+      it { expect(service.approve_mr(merge_request, user)).to be_truthy }
+
+      it 'creates new event' do
+        service.approve_mr(merge_request, user)
+
+        change { Event.approved_action.where(target: merge_request).count }.by(1)
+      end
+    end
   end
 
   describe 'Milestone' do

spec/services/merge_requests/approval_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::ApprovalService do
+  describe '#execute' do
+    let(:user)          { create(:user) }
+    let(:merge_request) { create(:merge_request) }
+    let(:project)       { merge_request.project }
+    let!(:todo)         { create(:todo, user: user, project: project, target: merge_request) }
+
+    subject(:service) { described_class.new(project, user) }
+
+    context 'with invalid approval' do
+      before do
+        allow(merge_request.approvals).to receive(:new).and_return(double(save: false))
+      end
+
+      it 'does not create an approval note' do
+        expect(SystemNoteService).not_to receive(:approve_mr)
+
+        service.execute(merge_request)
+      end
+
+      it 'does not mark pending todos as done' do
+        service.execute(merge_request)
+
+        expect(todo.reload).to be_pending
+      end
+    end
+
+    context 'with valid approval' do
+      it 'creates an approval note and marks pending todos as done' do
+        expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user)
+        expect(merge_request.approvals).to receive(:reset)
+
+        service.execute(merge_request)
+
+        expect(todo.reload).to be_done
+      end
+
+      it 'creates approve MR event' do
+        expect_next_instance_of(EventCreateService) do |instance|
+          expect(instance).to receive(:approve_mr)
+            .with(merge_request, user)
+        end
+
+        service.execute(merge_request)
+      end
+
+      context 'with remaining approvals' do
+        it 'fires an approval webhook' do
+          expect(service).to receive(:execute_hooks).with(merge_request, 'approved')
+
+          service.execute(merge_request)
+        end
+      end
+    end
+  end
+end

spec/services/system_note_service_spec.rb

@@ -661,4 +661,14 @@ RSpec.describe SystemNoteService do
       described_class.design_discussion_added(discussion_note)
     end
   end
+
+  describe '.approve_mr' do
+    it 'calls MergeRequestsService' do
+      expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service|
+        expect(service).to receive(:approve_mr)
+      end
+
+      described_class.approve_mr(noteable, author)
+    end
+  end
 end

spec/services/system_notes/merge_requests_service_spec.rb

@@ -261,4 +261,18 @@ RSpec.describe ::SystemNotes::MergeRequestsService do
       expect(subject.commit_id).to eq(commit_sha)
     end
   end
+
+  describe '#approve_mr' do
+    subject { described_class.new(noteable: noteable, project: project, author: author).approve_mr }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'approved' }
+    end
+
+    context 'when merge request approved' do
+      it 'sets the note text' do
+        expect(subject.note).to eq "approved this merge request"
+      end
+    end
+  end
 end