Merge branch '9963-optional-approval-view' into 'master'

Resolve "Add "No approval required" state to approval rules MR component"

Closes #9963

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

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/messages.js

@@ -7,3 +7,5 @@ export const FETCH_ERROR = s__(
 export const APPROVE_ERROR = s__('mrWidget|An error occurred while submitting your approval.');
 export const UNAPPROVE_ERROR = s__('mrWidget|An error occurred while removing your approval.');
 export const APPROVED_MESSAGE = s__('mrWidget|Merge request approved.');
+export const OPTIONAL_CAN_APPROVE = s__('mrWidget|No approval required; you can still approve');
+export const OPTIONAL = s__('mrWidget|No approval required');

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

@@ -7,6 +7,7 @@ import eventHub from '~/vue_merge_request_widget/event_hub';
 import MrWidgetContainer from '~/vue_merge_request_widget/components/mr_widget_container.vue';
 import MrWidgetIcon from '~/vue_merge_request_widget/components/mr_widget_icon.vue';
 import ApprovalsSummary from './approvals_summary.vue';
+import ApprovalsSummaryOptional from './approvals_summary_optional.vue';
 import ApprovalsFooter from './approvals_footer.vue';
 import { FETCH_LOADING, FETCH_ERROR, APPROVE_ERROR, UNAPPROVE_ERROR } from '../messages';
 
@@ -17,6 +18,7 @@ export default {
     MrWidgetContainer,
     MrWidgetIcon,
     ApprovalsSummary,
+    ApprovalsSummaryOptional,
     ApprovalsFooter,
     GlButton,
     GlLoadingIcon,
@@ -40,17 +42,29 @@ export default {
     };
   },
   computed: {
+    approvals() {
+      return this.mr.approvals || {};
+    },
     hasFooter() {
-      return this.mr.approvals && this.mr.approvals.has_approval_rules;
+      return !!this.approvals.has_approval_rules;
     },
     approvedBy() {
-      return this.mr.approvals.approved_by.map(x => x.user);
+      return this.approvals.approved_by ? this.approvals.approved_by.map(x => x.user) : [];
+    },
+    isApproved() {
+      return !!this.approvals.approved;
+    },
+    approvalsRequired() {
+      return this.approvals.approvals_required || 0;
+    },
+    isOptional() {
+      return !this.approvedBy.length && !this.approvalsRequired;
     },
     userHasApproved() {
-      return this.mr.approvals.user_has_approved;
+      return !!this.approvals.user_has_approved;
     },
     userCanApprove() {
-      return this.mr.approvals.user_can_approve;
+      return !!this.approvals.user_can_approve;
     },
     showApprove() {
       return !this.userHasApproved && this.userCanApprove && this.mr.isOpen;
@@ -59,16 +73,16 @@ export default {
       return this.userHasApproved && !this.userCanApprove && this.mr.state !== 'merged';
     },
     action() {
-      if (this.showApprove && this.mr.approvals.approved) {
-        return {
-          text: s__('mrWidget|Approve additionally'),
-          variant: 'primary',
-          inverted: true,
-          action: () => this.approve(),
-        };
-      } else if (this.showApprove) {
+      if (this.showApprove) {
+        const inverted = this.isApproved;
+        const text =
+          this.isApproved && this.approvedBy.length > 0
+            ? s__('mrWidget|Approve additionally')
+            : s__('mrWidget|Approve');
+
         return {
-          text: s__('mrWidget|Approve'),
+          text,
+          inverted,
           variant: 'primary',
           action: () => this.approve(),
         };
@@ -83,6 +97,9 @@ export default {
 
       return null;
     },
+    hasAction() {
+      return !!this.action;
+    },
   },
   watch: {
     isExpanded(val) {
@@ -160,10 +177,16 @@ export default {
           <gl-loading-icon v-if="isApproving" inline />
           {{ action.text }}
         </gl-button>
+        <approvals-summary-optional
+          v-if="isOptional"
+          :can-approve="hasAction"
+          :help-path="mr.approvalsHelpPath"
+        />
         <approvals-summary
-          :approved="mr.approvals.approved"
-          :approvals-left="mr.approvals.approvals_left"
-          :rules-left="mr.approvals.approvalRuleNamesLeft"
+          v-else
+          :approved="isApproved"
+          :approvals-left="approvals.approvals_left"
+          :rules-left="approvals.approvalRuleNamesLeft"
           :approvers="approvedBy"
         />
       </template>
@@ -172,7 +195,7 @@ export default {
       v-if="hasFooter"
       slot="footer"
       v-model="isExpanded"
-      :suggested-approvers="mr.approvals.suggested_approvers"
+      :suggested-approvers="approvals.suggested_approvers"
       :approval-rules="mr.approvalRules"
       :is-loading-rules="isLoadingRules"
     />

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

+<script>
+import { GlTooltipDirective, GlLink } from '@gitlab/ui';
+import Icon from '~/vue_shared/components/icon.vue';
+import { OPTIONAL, OPTIONAL_CAN_APPROVE } from '../messages';
+
+export default {
+  components: {
+    GlLink,
+    Icon,
+  },
+  directives: {
+    GlTooltip: GlTooltipDirective,
+  },
+  props: {
+    canApprove: {
+      type: Boolean,
+      required: true,
+    },
+    helpPath: {
+      type: String,
+      required: false,
+      default: '',
+    },
+  },
+  computed: {
+    message() {
+      return this.canApprove ? OPTIONAL_CAN_APPROVE : OPTIONAL;
+    },
+  },
+};
+</script>
+
+<template>
+  <div class="d-flex align-items-center">
+    <span class="text-muted">{{ message }}</span>
+    <gl-link
+      v-if="canApprove && helpPath"
+      v-gl-tooltip
+      :href="helpPath"
+      :title="__('About this feature')"
+      target="_blank"
+      class="d-flex-center pl-1"
+    >
+      <icon name="question-o" />
+    </gl-link>
+  </div>
+</template>

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/single_rule/approvals_body.vue

@@ -5,7 +5,7 @@ import Icon from '~/vue_shared/components/icon.vue';
 import MrWidgetAuthor from '~/vue_merge_request_widget/components/mr_widget_author.vue';
 import tooltip from '~/vue_shared/directives/tooltip';
 import eventHub from '~/vue_merge_request_widget/event_hub';
-import { APPROVE_ERROR } from '../messages';
+import { APPROVE_ERROR, OPTIONAL_CAN_APPROVE, OPTIONAL } from '../messages';
 
 export default {
   name: 'ApprovalsBody',
@@ -65,10 +65,10 @@ export default {
     approvalsRequiredStringified() {
       if (this.approvalsOptional) {
         if (this.userCanApprove) {
-          return s__('mrWidget|No Approval required; you can still approve');
+          return OPTIONAL_CAN_APPROVE;
         }
 
-        return s__('mrWidget|No Approval required');
+        return OPTIONAL;
       }
 
       if (this.approvalsLeft === 0) {

ee/changelogs/unreleased/9963-optional-approval-view.yml

+--- 
+title: Add 'No approvals required' view to approval rules (behind feature flag)
+merge_request: 9899
+author:
+type: fixed

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

@@ -308,7 +308,7 @@ describe('EE MRWidget approvals list', () => {
     it('renders the name in a monospace font', () => {
       const codeOwnerRow = findRowElement(row, 'name');
 
-      expect(codeOwnerRow.hasClass('monospace')).toEqual(true);
+      expect(codeOwnerRow.classes('monospace')).toEqual(true);
       expect(codeOwnerRow.text()).toEqual(rule.name);
     });
   });

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

@@ -3,6 +3,7 @@ import { GlButton, GlLoadingIcon } from '@gitlab/ui';
 import eventHub from '~/vue_merge_request_widget/event_hub';
 import Approvals from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue';
 import ApprovalsSummary from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary.vue';
+import ApprovalsSummaryOptional from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue';
 import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_footer.vue';
 import {
   FETCH_LOADING,
@@ -12,6 +13,7 @@ import {
 } from 'ee/vue_merge_request_widget/components/approvals/messages';
 
 const localVue = createLocalVue();
+const TEST_HELP_PATH = 'help/path';
 const testApprovedBy = () => [1, 7, 10].map(id => ({ id }));
 const testApprovals = () => ({
   has_approval_rules: true,
@@ -64,6 +66,7 @@ describe('EE MRWidget approvals', () => {
         };
   };
   const findSummary = () => wrapper.find(ApprovalsSummary);
+  const findOptionalSummary = () => wrapper.find(ApprovalsSummaryOptional);
   const findFooter = () => wrapper.find(ApprovalsFooter);
 
   beforeEach(() => {
@@ -75,6 +78,7 @@ describe('EE MRWidget approvals', () => {
     });
     mr = {
       ...jasmine.createSpyObj('Store', ['setApprovals', 'setApprovalRules']),
+      approvalsHelpPath: TEST_HELP_PATH,
       approvals: testApprovals(),
       approvalRules: [],
       isOpen: true,
@@ -185,17 +189,39 @@ describe('EE MRWidget approvals', () => {
       });
 
       describe('and MR is approved', () => {
-        beforeEach(done => {
+        beforeEach(() => {
           mr.approvals.approved = true;
-          createComponent();
-          waitForTick(done);
         });
 
-        it('approve additionally action is rendered', () => {
-          expect(findActionData()).toEqual({
-            variant: 'primary',
-            text: 'Approve additionally',
-            inverted: true,
+        describe('with no approvers', () => {
+          beforeEach(done => {
+            mr.approvals.approved_by = [];
+            createComponent();
+            waitForTick(done);
+          });
+
+          it('approve action (with inverted) is rendered', () => {
+            expect(findActionData()).toEqual({
+              variant: 'primary',
+              text: 'Approve',
+              inverted: true,
+            });
+          });
+        });
+
+        describe('with approvers', () => {
+          beforeEach(done => {
+            mr.approvals.approved_by = [{ user: { id: 7 } }];
+            createComponent();
+            waitForTick(done);
+          });
+
+          it('approve additionally action is rendered', () => {
+            expect(findActionData()).toEqual({
+              variant: 'primary',
+              text: 'Approve additionally',
+              inverted: true,
+            });
           });
         });
       });
@@ -315,6 +341,50 @@ describe('EE MRWidget approvals', () => {
     });
   });
 
+  describe('approvals optional summary', () => {
+    describe('when no approvals required and no approvers', () => {
+      beforeEach(() => {
+        mr.approvals.approved_by = [];
+        mr.approvals.approvals_required = 0;
+        mr.approvals.user_has_approved = false;
+      });
+
+      describe('and can approve', () => {
+        beforeEach(done => {
+          mr.approvals.user_can_approve = true;
+
+          createComponent();
+          waitForTick(done);
+        });
+
+        it('is shown', () => {
+          expect(findSummary().exists()).toBe(false);
+          expect(findOptionalSummary().props()).toEqual({
+            canApprove: true,
+            helpPath: TEST_HELP_PATH,
+          });
+        });
+      });
+
+      describe('and cannot approve', () => {
+        beforeEach(done => {
+          mr.approvals.user_can_approve = false;
+
+          createComponent();
+          waitForTick(done);
+        });
+
+        it('is shown', () => {
+          expect(findSummary().exists()).toBe(false);
+          expect(findOptionalSummary().props()).toEqual({
+            canApprove: false,
+            helpPath: TEST_HELP_PATH,
+          });
+        });
+      });
+    });
+  });
+
   describe('approvals summary', () => {
     beforeEach(done => {
       createComponent();
@@ -325,6 +395,7 @@ describe('EE MRWidget approvals', () => {
       const expected = testApprovals();
       const summary = findSummary();
 
+      expect(findOptionalSummary().exists()).toBe(false);
       expect(summary.exists()).toBe(true);
       expect(summary.props()).toEqual(
         jasmine.objectContaining({

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

+import { createLocalVue, shallowMount } from '@vue/test-utils';
+import { GlLink } from '@gitlab/ui';
+import {
+  OPTIONAL,
+  OPTIONAL_CAN_APPROVE,
+} from 'ee/vue_merge_request_widget/components/approvals/messages';
+import ApprovalsSummaryOptional from 'ee/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue';
+
+const localVue = createLocalVue();
+
+const TEST_HELP_PATH = 'help/path';
+
+describe('EE MRWidget approvals summary optional', () => {
+  let wrapper;
+
+  const createComponent = (props = {}) => {
+    wrapper = shallowMount(localVue.extend(ApprovalsSummaryOptional), {
+      propsData: props,
+      sync: false,
+      localVue,
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+    wrapper = null;
+  });
+
+  const findHelpLink = () => wrapper.find(GlLink);
+
+  describe('when can approve', () => {
+    beforeEach(() => {
+      createComponent({ canApprove: true, helpPath: TEST_HELP_PATH });
+    });
+
+    it('shows optional can approve message', () => {
+      expect(wrapper.text()).toEqual(OPTIONAL_CAN_APPROVE);
+    });
+
+    it('shows help link', () => {
+      const link = findHelpLink();
+
+      expect(link.exists()).toBe(true);
+      expect(link.attributes('href')).toBe(TEST_HELP_PATH);
+    });
+  });
+
+  describe('when cannot approve', () => {
+    beforeEach(() => {
+      createComponent({ canApprove: false, helpPath: TEST_HELP_PATH });
+    });
+
+    it('shows optional message', () => {
+      expect(wrapper.text()).toEqual(OPTIONAL);
+    });
+
+    it('does not show help link', () => {
+      expect(findHelpLink().exists()).toBe(false);
+    });
+  });
+});

ee/spec/javascripts/vue_mr_widget/components/approvals/single_rule/approvals_body_spec.js

 import Vue from 'vue';
 import ApprovalsBody from 'ee/vue_merge_request_widget/components/approvals/single_rule/approvals_body.vue';
+import {
+  OPTIONAL,
+  OPTIONAL_CAN_APPROVE,
+} from 'ee/vue_merge_request_widget/components/approvals/messages';
 
 describe('Approvals Body Component', () => {
   let vm;
@@ -62,7 +66,7 @@ describe('Approvals Body Component', () => {
       });
 
       it('should display the correct string for 0 approvals required', done => {
-        const correctText = 'No Approval required';
+        const correctText = OPTIONAL;
 
         vm.approvalsOptional = true;
 
@@ -73,7 +77,7 @@ describe('Approvals Body Component', () => {
       });
 
       it('should display the correct string for 0 approvals required and if the user is able to approve', done => {
-        const correctText = 'No Approval required; you can still approve';
+        const correctText = OPTIONAL_CAN_APPROVE;
 
         vm.approvalsOptional = true;
         vm.userCanApprove = true;

locale/gitlab.pot

@@ -12050,10 +12050,10 @@ msgstr ""
 msgid "mrWidget|Merged by"
 msgstr ""
 
-msgid "mrWidget|No Approval required"
+msgid "mrWidget|No approval required"
 msgstr ""
 
-msgid "mrWidget|No Approval required; you can still approve"
+msgid "mrWidget|No approval required; you can still approve"
 msgstr ""
 
 msgid "mrWidget|Open in Web IDE"