Merge branch 'approval-rules-use-api' into 'master'

Fix merge button disabled when MR is approved

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

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue

@@ -161,6 +161,7 @@ export default {
           {{ action.text }}
         </gl-button>
         <approvals-summary
+          :approved="mr.approvals.approved"
           :approvals-left="mr.approvals.approvals_left"
           :rules-left="mr.approvals.approvalRuleNamesLeft"
           :approvers="approvedBy"

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals_list.vue

@@ -25,9 +25,6 @@ export default {
         total: rule.approvals_required,
       });
     },
-    isApproved(rule) {
-      return rule.approvals_required > 0 && rule.approvals_required <= rule.approved_by.length;
-    },
     summaryText(rule) {
       return rule.approvals_required === 0
         ? this.summaryOptionalText(rule)
@@ -63,7 +60,7 @@ export default {
     </thead>
     <tbody>
       <tr v-for="rule in approvalRules" :key="rule.id">
-        <td class="w-0"><approved-icon :is-approved="isApproved(rule)" /></td>
+        <td class="w-0"><approved-icon :is-approved="rule.approved" /></td>
         <td :colspan="rule.fallback ? 2 : 1">
           <div class="d-none d-sm-block js-name">{{ rule.name }}</div>
           <div class="d-flex d-sm-none flex-column js-summary">

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary.vue

@@ -9,6 +9,10 @@ export default {
     UserAvatarList,
   },
   props: {
+    approved: {
+      type: Boolean,
+      required: true,
+    },
     approvalsLeft: {
       type: Number,
       required: true,
@@ -25,11 +29,8 @@ export default {
     },
   },
   computed: {
-    isApproved() {
-      return this.approvalsLeft <= 0;
-    },
     message() {
-      if (this.isApproved) {
+      if (this.approved) {
         return APPROVED_MESSAGE;
       }
 

ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -55,8 +55,13 @@ export default class MergeRequestStore extends CEMergeRequestStore {
   setApprovals(data) {
     this.approvals = mapApprovalsResponse(data);
     this.approvalsLeft = !!data.approvals_left;
-    this.isApproved = !this.approvalsLeft || false;
-    this.preventMerge = this.approvalsRequired && this.approvalsLeft;
+    if (gon.features.approvalRules) {
+      this.isApproved = data.approved || false;
+      this.preventMerge = !this.isApproved;
+    } else {
+      this.isApproved = !this.approvalsLeft || false;
+      this.preventMerge = this.approvalsRequired && this.approvalsLeft;
+    }
   }
 
   setApprovalRules(data) {

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -9,7 +9,7 @@ module EE
 
       prepended do
         before_action only: [:show] do
-          push_frontend_feature_flag(:approval_rules)
+          push_frontend_feature_flag(:approval_rules, merge_request.project)
         end
 
         before_action :whitelist_query_limiting_ee_merge, only: [:merge]
@@ -65,7 +65,12 @@ module EE
       def render_approvals_json
         respond_to do |format|
           format.json do
-            entity = EE::API::Entities::MergeRequestApprovals.new(merge_request, current_user: current_user)
+            entity = if ::Feature.enabled?(:approval_rules, merge_request.project)
+                       EE::API::Entities::ApprovalState.new(merge_request.approval_state, current_user: current_user)
+                     else
+                       EE::API::Entities::MergeRequestApprovals.new(merge_request, current_user: current_user)
+                     end
+
             render json: entity
           end
         end

ee/app/models/approval_wrapped_rule.rb

@@ -35,7 +35,9 @@ class ApprovalWrappedRule
   # - Additional complexity to add update hooks
   # - DB updating many MRs for one project rule change is inefficient
   def approved_approvers
-    return approval_rule.approved_approvers if merge_request.merged?
+    if merge_request.merged? && merge_request.merge_jid.nil? # merge process completed
+      return approval_rule.approved_approvers
+    end
 
     strong_memoize(:approved_approvers) do
       overall_approver_ids = merge_request.approvals.map(&:user_id)

ee/app/services/approval_rules/finalize_service.rb

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

ee/lib/ee/api/entities.rb

@@ -267,6 +267,7 @@ module EE
         expose :approved_approvers, as: :approved_by, using: ::API::Entities::UserBasic
         expose :code_owner
         expose :source_rule, using: SourceRule
+        expose :approved?, as: :approved
       end
 
       # Decorates ApprovalState

ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_list_spec.js

@@ -13,6 +13,7 @@ const testRuleApproved = () => ({
   approvals_required: 2,
   approved_by: [{ id: 1 }, { id: 2 }, { id: 3 }],
   approvers: testApprovers(),
+  approved: true,
 });
 const testRuleUnapproved = () => ({
   id: 2,
@@ -20,6 +21,7 @@ const testRuleUnapproved = () => ({
   approvals_required: 1,
   approved_by: [],
   approvers: testApprovers(),
+  approved: false,
 });
 const testRuleOptional = () => ({
   id: 3,
@@ -27,6 +29,7 @@ const testRuleOptional = () => ({
   approvals_required: 0,
   approved_by: [{ id: 1 }],
   approvers: testApprovers(),
+  approved: false,
 });
 const testRuleFallback = () => ({
   id: 'fallback',
@@ -35,6 +38,7 @@ const testRuleFallback = () => ({
   approvals_required: 3,
   approved_by: [{ id: 1 }, { id: 2 }],
   approvers: [],
+  approved: false,
 });
 const testRules = () => [testRuleApproved(), testRuleUnapproved(), testRuleOptional()];
 

ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_spec.js

@@ -15,6 +15,7 @@ const localVue = createLocalVue();
 const testApprovedBy = () => [1, 7, 10].map(id => ({ id }));
 const testApprovals = () => ({
   has_approval_rules: true,
+  approved: false,
   approved_by: testApprovedBy().map(user => ({ user })),
   approval_rules_left: [],
   approvals_left: 4,

ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_summary_spec.js

@@ -17,6 +17,7 @@ describe('EE MRWidget approvals summary', () => {
   const createComponent = (props = {}) => {
     wrapper = shallowMount(localVue.extend(ApprovalsSummary), {
       propsData: {
+        approved: false,
         approvers: testApprovers(),
         approvalsLeft: TEST_APPROVALS_LEFT,
         rulesLeft: testRulesLeft(),
@@ -37,7 +38,7 @@ describe('EE MRWidget approvals summary', () => {
   describe('when approved', () => {
     beforeEach(() => {
       createComponent({
-        approvalsLeft: 0,
+        approved: true,
       });
     });