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