Merge branch 'id-move-remove-approval-service' into 'master'

[RUN AS-IF-FOSS] Add MergeRequests::RemoveApprovalService to FOSS

See merge request gitlab-org/gitlab!35538

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 approved
+    tag due_date pinned_embed cherry_pick health_status approved unapproved
   ].freeze
 
   validates :note, presence: true

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

@@ -9,26 +9,31 @@ module MergeRequests
 
       approval = merge_request.approvals.where(user: current_user)
 
-      currently_approved = merge_request.approved?
+      trigger_approval_hooks(merge_request) do
+        next unless approval.destroy_all # rubocop: disable Cop/DestroyAll
 
-      if approval.destroy_all # rubocop: disable Cop/DestroyAll
-        merge_request.reset_approval_cache!
+        reset_approvals_cache(merge_request)
         create_note(merge_request)
-
-        if currently_approved
-          notification_service.async.unapprove_mr(merge_request, current_user)
-          execute_hooks(merge_request, 'unapproved')
-        else
-          execute_hooks(merge_request, 'unapproval')
-        end
       end
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     private
 
+    def reset_approvals_cache(merge_request)
+      merge_request.approvals.reset
+    end
+
+    def trigger_approval_hooks(merge_request)
+      yield
+
+      execute_hooks(merge_request, 'unapproved')
+    end
+
     def create_note(merge_request)
       SystemNoteService.unapprove_mr(merge_request, current_user)
     end
   end
 end
+
+MergeRequests::RemoveApprovalService.prepend_if_ee('EE::MergeRequests::RemoveApprovalService')

app/services/system_note_service.rb

@@ -288,6 +288,10 @@ module SystemNoteService
     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
+
   private
 
   def merge_requests_service(noteable, project, author)

app/services/system_notes/merge_requests_service.rb

@@ -163,7 +163,11 @@ module SystemNotes
 
       create_note(NoteSummary.new(noteable, project, author, body, action: 'approved'))
     end
+
+    def unapprove_mr
+      body = "unapproved this merge request"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'unapproved'))
+    end
   end
 end
-
-SystemNotes::MergeRequestsService.prepend_if_ee('::EE::SystemNotes::MergeRequestsService')

ee/app/models/ee/system_note_metadata.rb

@@ -5,7 +5,7 @@ module EE
     extend ::Gitlab::Utils::Override
 
     EE_ICON_TYPES = %w[
-      weight unapproved relate unrelate published
+      weight 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/merge_requests/remove_approval_service.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    module RemoveApprovalService
+      extend ::Gitlab::Utils::Override
+
+      private
+
+      override :reset_approvals_cache
+      def reset_approvals_cache(merge_request)
+        merge_request.reset_approval_cache!
+      end
+
+      override :trigger_approval_hooks
+      def trigger_approval_hooks(merge_request)
+        currently_approved = merge_request.approved?
+
+        yield
+
+        if currently_approved
+          notification_service.async.unapprove_mr(merge_request, current_user)
+          execute_hooks(merge_request, 'unapproved')
+        else
+          execute_hooks(merge_request, 'unapproval')
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/system_note_service.rb

@@ -48,10 +48,6 @@ module EE
       issuables_service(noteable, noteable.project, author).change_iteration(iteration)
     end
 
-    def unapprove_mr(noteable, user)
-      merge_requests_service(noteable, noteable.project, user).unapprove_mr
-    end
-
     # Called when the weight of a Noteable is changed
     #
     # noteable   - Noteable object
@@ -155,10 +151,6 @@ module EE
       EE::SystemNotes::EpicsService.new(noteable: noteable, author: author)
     end
 
-    def merge_requests_service(noteable, project, author)
-      ::SystemNotes::MergeRequestsService.new(noteable: noteable, project: project, author: author)
-    end
-
     def merge_trains_service(noteable, project, author)
       EE::SystemNotes::MergeTrainService.new(noteable: noteable, project: project, author: author)
     end

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

-# frozen_string_literal: true
-
-module EE
-  module SystemNotes
-    module MergeRequestsService
-      def unapprove_mr
-        body = "unapproved this merge request"
-
-        create_note(NoteSummary.new(noteable, project, author, body, action: 'unapproved'))
-      end
-    end
-  end
-end

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

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ::SystemNotes::MergeRequestsService do
-  let(:author) { create(:user) }
-  let(:project) { create(:project) }
-  let(:noteable) { create(:merge_request, source_project: project) }
-
-  describe '#unapprove_mr' do
-    subject { described_class.new(noteable: noteable, project: project, author: author).unapprove_mr }
-
-    it_behaves_like 'a system note', exclude_project: true do
-      let(:action) { 'unapproved' }
-    end
-
-    context 'when merge request approved' do
-      it 'sets the note text' do
-        expect(subject.note).to eq "unapproved this merge request"
-      end
-    end
-  end
-end

ee/spec/services/merge_requests/remove_approval_service_spec.rb

@@ -22,9 +22,7 @@ RSpec.describe MergeRequests::RemoveApprovalService do
       end
 
       it 'removes the approval' do
-        expect(merge_request.approvals.size).to eq 1
-        execute!
-        expect(merge_request.approvals).to be_empty
+        expect { execute! }.to change { merge_request.approvals.size }.from(1).to(0)
       end
 
       it 'creates an unapproval note' do
@@ -60,14 +58,9 @@ RSpec.describe MergeRequests::RemoveApprovalService do
         allow(service).to receive(:notification_service).and_return(notification_service)
       end
 
-      it 'fires an unapproved webhook' do
-        expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved')
-
-        execute!
-      end
-
-      it 'sends a notification' do
+      it 'fires an unapproved webhook and sends a notification' do
         expect(notification_service).to receive_message_chain(:async, :unapprove_mr).with(merge_request, user)
+        expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved')
 
         execute!
       end

ee/spec/services/system_note_service_spec.rb

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

spec/services/merge_requests/remove_approval_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::RemoveApprovalService do
+  describe '#execute' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let!(:existing_approval) { create(:approval, merge_request: merge_request) }
+
+    subject(:service) { described_class.new(project, user) }
+
+    def execute!
+      service.execute(merge_request)
+    end
+
+    before do
+      project.add_developer(user)
+    end
+
+    context 'with a user who has approved' do
+      let!(:approval) { create(:approval, user: user, merge_request: merge_request) }
+
+      it 'removes the approval' do
+        expect { execute! }.to change { merge_request.approvals.size }.from(2).to(1)
+      end
+
+      it 'creates an unapproval note and triggers web hook' do
+        expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved')
+        expect(SystemNoteService).to receive(:unapprove_mr)
+
+        execute!
+      end
+    end
+  end
+end

spec/services/system_note_service_spec.rb

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