Merge branch '9889-fix-finalize' into 'master'

Compute approved approver for merged MR if it is absent

Closes #9866, #9889, and #9913

See merge request gitlab-org/gitlab-ee!9648

ee/app/models/approval_merge_request_rule.rb

@@ -12,7 +12,7 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   validates :name, uniqueness: { scope: [:merge_request, :code_owner] }
 
-  belongs_to :merge_request
+  belongs_to :merge_request, inverse_of: :approval_rules
 
   # approved_approvers is only populated after MR is merged
   has_and_belongs_to_many :approved_approvers, class_name: 'User', join_table: :approval_merge_request_rules_approved_approvers

ee/app/models/approval_wrapped_rule.rb

@@ -25,17 +25,17 @@ class ApprovalWrappedRule
 
   # @return [Array<User>] all approvers related to this rule
   #
-  # This is dynamically calculated when MR is open, but is persisted in DB after MR is merged.
+  # This is dynamically calculated unless it is persisted as `approved_approvers`.
   #
   # After merge, the approval state should no longer change.
-  # Persisting instead of recomputing dynamically guarantees this even
-  # if project level rule ever adds/removes approvers after the merge.
+  # We persist this so if project level rule is changed in the future,
+  # return result won't be affected.
   #
-  # For open MRs, it is still dynamically calculated instead of persisted because
+  # For open MRs, it is dynamically calculated because:
   # - Additional complexity to add update hooks
   # - DB updating many MRs for one project rule change is inefficient
   def approved_approvers
-    if merge_request.merged? && merge_request.merge_jid.nil? # merge process completed
+    if merge_request.merged? && approval_rule.is_a?(ApprovalMergeRequestRule) && approval_rule.approved_approvers.present?
       return approval_rule.approved_approvers
     end
 

ee/app/models/ee/merge_request.rb

@@ -18,7 +18,7 @@ module EE
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
-      has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
+      has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request
       has_many :draft_notes
 
       validate :validate_approvals_before_merge, unless: :importing?

ee/app/services/approval_rules/finalize_service.rb

@@ -18,8 +18,7 @@ module ApprovalRules
           copy_project_approval_rules
         end
 
-        # TODO: https://gitlab.com/gitlab-org/gitlab-ee/issues/9866
-        merge_request.approval_rules.reload.each(&:sync_approved_approvers)
+        merge_request.approval_rules.each(&:sync_approved_approvers)
       end
     end
 

ee/changelogs/unreleased/9889-fix-finalize.yml

+---
+title: Fix 500 error when visiting merged merge request
+merge_request: 9648
+author:
+type: fixed

ee/spec/models/approval_wrapped_rule_spec.rb

@@ -103,6 +103,37 @@ describe ApprovalWrappedRule do
         expect(subject.approved_approvers).to contain_exactly(approver3)
       end
     end
+
+    context 'when merged but without materialized approved_approvers' do
+      let(:merge_request) { create(:merged_merge_request) }
+
+      before do
+        create(:approval, merge_request: merge_request, user: approver1)
+        create(:approval, merge_request: merge_request, user: approver2)
+
+        rule.users = [approver1, approver3]
+      end
+
+      it 'returns computed approved approvers' do
+        expect(subject.approved_approvers).to contain_exactly(approver1)
+      end
+    end
+
+    context 'when project rule' do
+      let(:rule) { create(:approval_project_rule, project: merge_request.project, approvals_required: approvals_required) }
+      let(:merge_request) { create(:merged_merge_request) }
+
+      before do
+        create(:approval, merge_request: merge_request, user: approver1)
+        create(:approval, merge_request: merge_request, user: approver2)
+
+        rule.users = [approver1, approver3]
+      end
+
+      it 'returns computed approved approvers' do
+        expect(subject.approved_approvers).to contain_exactly(approver1)
+      end
+    end
   end
 
   describe '#unactioned_approvers' do

ee/spec/services/merge_requests/merge_service_spec.rb

@@ -10,9 +10,9 @@ describe MergeRequests::MergeService do
   end
 
   describe '#execute' do
-    context 'project has exceeded size limit' do
-      let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
+    let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
 
+    context 'project has exceeded size limit' do
       before do
         allow(project).to receive(:above_size_limit?).and_return(true)
 
@@ -25,6 +25,25 @@ describe MergeRequests::MergeService do
         expect(merge_request.merge_error).to include('This merge request cannot be merged')
       end
     end
+
+    context 'when merge request rule exists' do
+      let(:approver) { create(:user) }
+      let!(:approval_rule) { create :approval_merge_request_rule, merge_request: merge_request, users: [approver] }
+      let!(:approval) { create :approval, merge_request: merge_request, user: approver }
+
+      it 'creates approved_approvers' do
+        allow(service).to receive(:execute_hooks)
+
+        perform_enqueued_jobs do
+          service.execute(merge_request)
+        end
+        merge_request.reload
+        rule = merge_request.approval_rules.first
+
+        expect(merge_request.merged?).to eq(true)
+        expect(rule.approved_approvers).to contain_exactly(approver)
+      end
+    end
   end
 
   describe '#hooks_validation_pass?' do