From ee217fc0ebf86b15a18310a6e8ef09acdb8e3e4f Mon Sep 17 00:00:00 2001
From: Paul Slaughter <pslaughter@gitlab.com>
Date: Wed, 6 Mar 2019 13:08:26 -0600
Subject: [PATCH 1/4] Move optional approval text to messages

**Why?**
These messages will be needed by the multiple approval rules component.
---
 .../components/approvals/messages.js                      | 2 ++
 .../components/approvals/single_rule/approvals_body.vue   | 6 +++---
 .../approvals/single_rule/approvals_body_spec.js          | 8 ++++++--
 locale/gitlab.pot                                         | 4 ++--
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/messages.js b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/messages.js
index 0538c38307b420d9..1d9368f71aabbafa 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/messages.js
+++ b/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');
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/single_rule/approvals_body.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/single_rule/approvals_body.vue
index 7cf21b6b3705b362..6c90180ee8a4be0d 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/single_rule/approvals_body.vue
+++ b/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) {
diff --git a/ee/spec/javascripts/vue_mr_widget/components/approvals/single_rule/approvals_body_spec.js b/ee/spec/javascripts/vue_mr_widget/components/approvals/single_rule/approvals_body_spec.js
index b05b5cc7c81466bd..250383bc2f4974b7 100644
--- a/ee/spec/javascripts/vue_mr_widget/components/approvals/single_rule/approvals_body_spec.js
+++ b/ee/spec/javascripts/vue_mr_widget/components/approvals/single_rule/approvals_body_spec.js
@@ -1,5 +1,9 @@
 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;
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index dfa0483fcf1d397a..25abc0a8a8f2b6cc 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -11978,10 +11978,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"
-- 
GitLab


From e7d936f5980943ff3c0abbef327d6d88fb96a618 Mon Sep 17 00:00:00 2001
From: Paul Slaughter <pslaughter@gitlab.com>
Date: Wed, 6 Mar 2019 14:11:01 -0600
Subject: [PATCH 2/4] Add approvals summary optional component

For the most part, this is copied from the old approvals component.
Added unit tests to test show / hide of the normal summary
vs. the optional summary.
---
 .../approvals/multiple_rule/approvals.vue     | 37 +++++---
 .../approvals_summary_optional.vue            | 47 ++++++++++
 .../9963-optional-approval-view.yml           |  5 ++
 .../approvals/multiple_rule/approvals_spec.js | 87 +++++++++++++++++--
 .../approvals_summary_optional_spec.js        | 61 +++++++++++++
 5 files changed, 219 insertions(+), 18 deletions(-)
 create mode 100644 ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue
 create mode 100644 ee/changelogs/unreleased/9963-optional-approval-view.yml
 create mode 100644 ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_summary_optional_spec.js

diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue
index 253a03c2e7004014..ee391113b48b4e5b 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue
+++ b/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,
@@ -44,7 +46,13 @@ export default {
       return this.mr.approvals && this.mr.approvals.has_approval_rules;
     },
     approvedBy() {
-      return this.mr.approvals.approved_by.map(x => x.user);
+      return this.mr.approvals ? this.mr.approvals.approved_by.map(x => x.user) : [];
+    },
+    approvalsRequired() {
+      return this.mr.approvals ? this.mr.approvals.approvals_required : 0;
+    },
+    isOptional() {
+      return !this.approvedBy.length && !this.approvalsRequired;
     },
     userHasApproved() {
       return this.mr.approvals.user_has_approved;
@@ -59,16 +67,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.mr.approvals.approved;
+        const text =
+          this.mr.approvals.approved && this.approvedBy.length
+            ? s__('mrWidget|Approve additionally')
+            : s__('mrWidget|Approve');
+
         return {
-          text: s__('mrWidget|Approve'),
+          text,
+          inverted,
           variant: 'primary',
           action: () => this.approve(),
         };
@@ -83,6 +91,9 @@ export default {
 
       return null;
     },
+    hasAction() {
+      return !!this.action;
+    },
   },
   watch: {
     isExpanded(val) {
@@ -160,7 +171,13 @@ 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
+          v-else
           :approved="mr.approvals.approved"
           :approvals-left="mr.approvals.approvals_left"
           :rules-left="mr.approvals.approvalRuleNamesLeft"
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue
new file mode 100644
index 0000000000000000..d7fec141ee3135ae
--- /dev/null
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals_summary_optional.vue
@@ -0,0 +1,47 @@
+<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>
diff --git a/ee/changelogs/unreleased/9963-optional-approval-view.yml b/ee/changelogs/unreleased/9963-optional-approval-view.yml
new file mode 100644
index 0000000000000000..1b755082d7bd01b6
--- /dev/null
+++ b/ee/changelogs/unreleased/9963-optional-approval-view.yml
@@ -0,0 +1,5 @@
+--- 
+title: Add 'No approvals required' view to approval rules (behind feature flag)
+merge_request: 9899
+author:
+type: fixed
diff --git a/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_spec.js b/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_spec.js
index e239e841738ec0ed..8f966e25d8b6a523 100644
--- a/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_spec.js
+++ b/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({
diff --git a/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_summary_optional_spec.js b/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_summary_optional_spec.js
new file mode 100644
index 0000000000000000..dba0fa8761b84c39
--- /dev/null
+++ b/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_summary_optional_spec.js
@@ -0,0 +1,61 @@
+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);
+    });
+  });
+});
-- 
GitLab


From b51bcb3597d6533d7fc2858e26c933109f0ae10d Mon Sep 17 00:00:00 2001
From: Paul Slaughter <pslaughter@gitlab.com>
Date: Wed, 6 Mar 2019 22:51:23 -0600
Subject: [PATCH 3/4] Fix use of deprecated hasClass in spec

---
 .../components/approvals/multiple_rule/approvals_list_spec.js   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_list_spec.js b/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_list_spec.js
index 485f1a7d7e7f22ff..0208d080a75b861e 100644
--- a/ee/spec/javascripts/vue_mr_widget/components/approvals/multiple_rule/approvals_list_spec.js
+++ b/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);
     });
   });
-- 
GitLab


From d9ba6c774d5140c470e239efa50f8aec1dd0c8d3 Mon Sep 17 00:00:00 2001
From: Paul Slaughter <pslaughter@gitlab.com>
Date: Wed, 6 Mar 2019 23:59:59 -0600
Subject: [PATCH 4/4] Add default approvals obj to rules mr component

**Why?**
This helps eliminate a handful of nested null checks since the
computed property defaults to an empty object.

Also, transform some of the computed props to use `!!` to
ensure a Boolean representation.
---
 .../approvals/multiple_rule/approvals.vue     | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue
index ee391113b48b4e5b..1224907b4533918a 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/multiple_rule/approvals.vue
@@ -42,23 +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 ? 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.mr.approvals ? this.mr.approvals.approvals_required : 0;
+      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;
@@ -68,9 +74,9 @@ export default {
     },
     action() {
       if (this.showApprove) {
-        const inverted = this.mr.approvals.approved;
+        const inverted = this.isApproved;
         const text =
-          this.mr.approvals.approved && this.approvedBy.length
+          this.isApproved && this.approvedBy.length > 0
             ? s__('mrWidget|Approve additionally')
             : s__('mrWidget|Approve');
 
@@ -178,9 +184,9 @@ export default {
         />
         <approvals-summary
           v-else
-          :approved="mr.approvals.approved"
-          :approvals-left="mr.approvals.approvals_left"
-          :rules-left="mr.approvals.approvalRuleNamesLeft"
+          :approved="isApproved"
+          :approvals-left="approvals.approvals_left"
+          :rules-left="approvals.approvalRuleNamesLeft"
           :approvers="approvedBy"
         />
       </template>
@@ -189,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"
     />
-- 
GitLab