diff options
10 files changed, 262 insertions, 111 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_related_links.js b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_related_links.js index 205804670fa..686cb38cbb1 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_related_links.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_related_links.js @@ -1,42 +1,63 @@ export default { name: 'MRWidgetRelatedLinks', props: { + isMerged: { type: Boolean, required: true }, relatedLinks: { type: Object, required: true }, }, computed: { + // TODO: the following should be handled by i18n + closingText() { + if (this.isMerged) { + return `Closed ${this.issueLabel('closing')}`; + } + + return `Closes ${this.issueLabel('closing')}`; + }, hasLinks() { const { closing, mentioned, assignToMe } = this.relatedLinks; return closing || mentioned || assignToMe; }, + // TODO: the following should be handled by i18n + mentionedText() { + if (this.isMerged) { + if (this.hasMultipleIssues(this.relatedLinks.mentioned)) { + return 'are mentioned but were not closed'; + } + + return 'is mentioned but was not closed'; + } + + if (this.hasMultipleIssues(this.relatedLinks.mentioned)) { + return 'are mentioned but will not be closed'; + } + + return 'is mentioned but will not be closed'; + }, }, methods: { hasMultipleIssues(text) { - return !text ? false : text.match(/<\/a> and <a/); + return /<\/a>,? and <a/.test(text); }, + // TODO: the following should be handled by i18n issueLabel(field) { return this.hasMultipleIssues(this.relatedLinks[field]) ? 'issues' : 'issue'; }, - verbLabel(field) { - return this.hasMultipleIssues(this.relatedLinks[field]) ? 'are' : 'is'; - }, }, template: ` - <section - v-if="hasLinks" - class="mr-info-list mr-links"> + <div v-if="hasLinks"> <div class="legend"></div> <p v-if="relatedLinks.closing"> - Closes {{issueLabel('closing')}} + {{closingText}} <span v-html="relatedLinks.closing"></span>. </p> <p v-if="relatedLinks.mentioned"> <span class="capitalize">{{issueLabel('mentioned')}}</span> <span v-html="relatedLinks.mentioned"></span> - {{verbLabel('mentioned')}} mentioned but will not be closed. + {{mentionedText}} </p> <p v-if="relatedLinks.assignToMe"> <span v-html="relatedLinks.assignToMe"></span> </p> - </section> + </div> `, }; diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js index c7d32d18141..9b8eed9016d 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js @@ -1,7 +1,9 @@ /* global Flash */ import mrWidgetAuthorTime from '../../components/mr_widget_author_time'; +import mrWidgetRelatedLinks from '../../components/mr_widget_related_links'; import eventHub from '../../event_hub'; +import '../../../flash'; export default { name: 'MRWidgetMerged', @@ -11,6 +13,7 @@ export default { }, components: { 'mr-widget-author-and-time': mrWidgetAuthorTime, + 'mr-widget-related-links': mrWidgetRelatedLinks, }, data() { return { @@ -18,6 +21,9 @@ export default { }; }, computed: { + shouldRenderRelatedLinks() { + return this.mr.relatedLinks && this.mr.isMerged; + }, shouldShowRemoveSourceBranch() { const { sourceBranchRemoved, isRemovingSourceBranch, canRemoveSourceBranch } = this.mr; @@ -86,6 +92,10 @@ export default { aria-hidden="true" /> The source branch is being removed. </p> + <mr-widget-related-links + v-if="shouldRenderRelatedLinks" + :is-merged="mr.isMerged()" + :related-links="mr.relatedLinks" /> </section> <div v-if="shouldShowMergedButtons" diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 2339a00ddd0..222d0b7f79e 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -48,7 +48,7 @@ export default { return stateMaps.stateToComponentMap[this.mr.state]; }, shouldRenderMergeHelp() { - return stateMaps.statesToShowHelpWidget.indexOf(this.mr.state) > -1; + return !this.mr.isMerged; }, shouldRenderPipelines() { return Object.keys(this.mr.pipeline).length || this.mr.hasCI; @@ -238,9 +238,14 @@ export default { :is="componentName" :mr="mr" :service="service" /> - <mr-widget-related-links + <section v-if="shouldRenderRelatedLinks" - :related-links="mr.relatedLinks" /> + class="mr-info-list mr-links"> + <div class="legend"></div> + <mr-widget-related-links + :is-merged="mr.isMerged" + :related-links="mr.relatedLinks" /> + </section> <mr-widget-merge-help v-if="shouldRenderMergeHelp" /> </div> `, diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 69bc1436284..ad73efb37e1 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -1,6 +1,18 @@ import Timeago from 'timeago.js'; import { getStateKey } from '../dependencies'; +const unmergedStates = [ + 'locked', + 'conflicts', + 'workInProgress', + 'readyToMerge', + 'checking', + 'unresolvedDiscussions', + 'pipelineFailed', + 'pipelineBlocked', + 'autoMergeFailed', +]; + export default class MergeRequestStore { constructor(data) { @@ -65,6 +77,7 @@ export default class MergeRequestStore { this.mergeActionsContentPath = data.commit_change_content_path; this.isRemovingSourceBranch = this.isRemovingSourceBranch || false; this.isOpen = data.state === 'opened' || data.state === 'reopened' || false; + this.isMerged = unmergedStates.indexOf(data.state) === -1; this.hasMergeableDiscussionsState = data.mergeable_discussions_state === false; this.canRemoveSourceBranch = currentUser.can_remove_source_branch || false; this.canMerge = !!data.merge_path; diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js index 605dd3a1ff4..dd939d98d0f 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js @@ -19,19 +19,6 @@ const stateToComponentMap = { shaMismatch: 'mr-widget-sha-mismatch', }; -const statesToShowHelpWidget = [ - 'locked', - 'conflicts', - 'workInProgress', - 'readyToMerge', - 'checking', - 'unresolvedDiscussions', - 'pipelineFailed', - 'pipelineBlocked', - 'autoMergeFailed', -]; - export default { stateToComponentMap, - statesToShowHelpWidget, }; diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 2dc7f73a295..c0bd045f1fc 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -372,6 +372,10 @@ margin-left: 12px; } + &.mr-state-locked + .mr-info-list.mr-links { + margin-top: -16px; + } + &.empty-state { .artwork { margin-bottom: $gl-padding; diff --git a/spec/features/merge_requests/closes_issues_spec.rb b/spec/features/merge_requests/closes_issues_spec.rb index e627618042a..26444bb7a55 100644 --- a/spec/features/merge_requests/closes_issues_spec.rb +++ b/spec/features/merge_requests/closes_issues_spec.rb @@ -36,7 +36,7 @@ feature 'Merge Request closing issues message', feature: true, js: true do let(:merge_request_description) { "Description\n\nclosing #{issue_1.to_reference}, #{issue_2.to_reference}" } it 'does not display closing issue message' do - expect(page).to have_content("Closes issues #{issue_1.to_reference} and #{issue_2.to_reference}") + expect(page).to have_content("Closed issues #{issue_1.to_reference} and #{issue_2.to_reference}") end end @@ -44,7 +44,7 @@ feature 'Merge Request closing issues message', feature: true, js: true do let(:merge_request_description) { "Description\n\nRefers to #{issue_1.to_reference} and #{issue_2.to_reference}" } it 'does not display closing issue message' do - expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but will not be closed.") + expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but were not closed") end end @@ -52,8 +52,8 @@ feature 'Merge Request closing issues message', feature: true, js: true do let(:merge_request_title) { "closes #{issue_1.to_reference}\n\n refers to #{issue_2.to_reference}" } it 'does not display closing issue message' do - expect(page).to have_content("Closes issue #{issue_1.to_reference}.") - expect(page).to have_content("Issue #{issue_2.to_reference} is mentioned but will not be closed.") + expect(page).to have_content("Closed issue #{issue_1.to_reference}") + expect(page).to have_content("Issue #{issue_2.to_reference} is mentioned but was not closed") end end @@ -61,7 +61,7 @@ feature 'Merge Request closing issues message', feature: true, js: true do let(:merge_request_title) { "closing #{issue_1.to_reference}, #{issue_2.to_reference}" } it 'does not display closing issue message' do - expect(page).to have_content("Closes issues #{issue_1.to_reference} and #{issue_2.to_reference}") + expect(page).to have_content("Closed issues #{issue_1.to_reference} and #{issue_2.to_reference}") end end @@ -69,7 +69,7 @@ feature 'Merge Request closing issues message', feature: true, js: true do let(:merge_request_title) { "Refers to #{issue_1.to_reference} and #{issue_2.to_reference}" } it 'does not display closing issue message' do - expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but will not be closed.") + expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but were not closed") end end @@ -77,8 +77,8 @@ feature 'Merge Request closing issues message', feature: true, js: true do let(:merge_request_title) { "closes #{issue_1.to_reference}\n\n refers to #{issue_2.to_reference}" } it 'does not display closing issue message' do - expect(page).to have_content("Closes issue #{issue_1.to_reference}. Issue #{issue_2.to_reference} is mentioned but will not be closed.") - expect(page).to have_content("Issue #{issue_2.to_reference} is mentioned but will not be closed.") + expect(page).to have_content("Closed issue #{issue_1.to_reference}") + expect(page).to have_content("Issue #{issue_2.to_reference} is mentioned but was not closed") end end end diff --git a/spec/javascripts/vue_mr_widget/components/mr_widget_related_links_spec.js b/spec/javascripts/vue_mr_widget/components/mr_widget_related_links_spec.js index f6e0c3dfb74..6a44c54cdee 100644 --- a/spec/javascripts/vue_mr_widget/components/mr_widget_related_links_spec.js +++ b/spec/javascripts/vue_mr_widget/components/mr_widget_related_links_spec.js @@ -1,20 +1,31 @@ import Vue from 'vue'; -import relatedLinksComponent from '~/vue_merge_request_widget/components/mr_widget_related_links'; +import MRWidgetRelatedLinks from '~/vue_merge_request_widget/components/mr_widget_related_links'; -const createComponent = (data) => { - const Component = Vue.extend(relatedLinksComponent); +describe('MRWidgetRelatedLinks', () => { + let vm; + + beforeEach(() => { + const Component = Vue.extend(MRWidgetRelatedLinks); + vm = new Component({ + el: document.createElement('div'), + propsData: { + isMerged: false, + relatedLinks: {}, + }, + }); + }); - return new Component({ - el: document.createElement('div'), - propsData: data, + afterEach(() => { + vm.$destroy(); }); -}; -describe('MRWidgetRelatedLinks', () => { describe('props', () => { it('should have props', () => { - const { relatedLinks } = relatedLinksComponent.props; + const { isMerged, relatedLinks } = MRWidgetRelatedLinks.props; + expect(isMerged).toBeDefined(); + expect(isMerged.type).toBe(Boolean); + expect(isMerged.required).toBeTruthy(); expect(relatedLinks).toBeDefined(); expect(relatedLinks.type instanceof Object).toBeTruthy(); expect(relatedLinks.required).toBeTruthy(); @@ -22,16 +33,38 @@ describe('MRWidgetRelatedLinks', () => { }); describe('computed', () => { + describe('closingText', () => { + const dummyIssueLabel = 'dummy label'; + + beforeEach(() => { + spyOn(vm, 'issueLabel').and.returnValue(dummyIssueLabel); + }); + + it('outputs text for closing issues', () => { + vm.isMerged = false; + + const text = vm.closingText; + + expect(text).toBe(`Closes ${dummyIssueLabel}`); + }); + + it('outputs text for closed issues', () => { + vm.isMerged = true; + + const text = vm.closingText; + + expect(text).toBe(`Closed ${dummyIssueLabel}`); + }); + }); + describe('hasLinks', () => { it('should return correct value when we have links reference', () => { - const data = { - relatedLinks: { - closing: '/foo', - mentioned: '/foo', - assignToMe: '/foo', - }, + vm.relatedLinks = { + closing: '/foo', + mentioned: '/foo', + assignToMe: '/foo', }; - const vm = createComponent(data); + expect(vm.hasLinks).toBeTruthy(); vm.relatedLinks.closing = null; @@ -44,95 +77,160 @@ describe('MRWidgetRelatedLinks', () => { expect(vm.hasLinks).toBeFalsy(); }); }); - }); - describe('methods', () => { - const data = { - relatedLinks: { - closing: '<a href="#">#23</a> and <a>#42</a>', - mentioned: '<a href="#">#7</a>', - }, - }; - const vm = createComponent(data); + describe('mentionedText', () => { + it('outputs text for one mentioned issue before merging', () => { + vm.isMerged = false; + spyOn(vm, 'hasMultipleIssues').and.returnValue(false); - describe('hasMultipleIssues', () => { - it('should return true if the given text has multiple issues', () => { - expect(vm.hasMultipleIssues(data.relatedLinks.closing)).toBeTruthy(); + const text = vm.mentionedText; + + expect(text).toBe('is mentioned but will not be closed'); }); - it('should return false if the given text has one issue', () => { - expect(vm.hasMultipleIssues(data.relatedLinks.mentioned)).toBeFalsy(); + it('outputs text for one mentioned issue after merging', () => { + vm.isMerged = true; + spyOn(vm, 'hasMultipleIssues').and.returnValue(false); + + const text = vm.mentionedText; + + expect(text).toBe('is mentioned but was not closed'); + }); + + it('outputs text for multiple mentioned issue before merging', () => { + vm.isMerged = false; + spyOn(vm, 'hasMultipleIssues').and.returnValue(true); + + const text = vm.mentionedText; + + expect(text).toBe('are mentioned but will not be closed'); + }); + + it('outputs text for multiple mentioned issue after merging', () => { + vm.isMerged = true; + spyOn(vm, 'hasMultipleIssues').and.returnValue(true); + + const text = vm.mentionedText; + + expect(text).toBe('are mentioned but were not closed'); }); }); + }); - describe('issueLabel', () => { + describe('methods', () => { + const relatedLinks = { + oneIssue: '<a href="#">#7</a>', + twoIssues: '<a href="#">#23</a> and <a>#42</a>', + threeIssues: '<a href="#">#1</a>, <a>#2</a>, and <a>#3</a>', + }; + + beforeEach(() => { + vm.relatedLinks = relatedLinks; + }); + + describe('hasMultipleIssues', () => { it('should return true if the given text has multiple issues', () => { - expect(vm.issueLabel('closing')).toEqual('issues'); + expect(vm.hasMultipleIssues(relatedLinks.twoIssues)).toBeTruthy(); + expect(vm.hasMultipleIssues(relatedLinks.threeIssues)).toBeTruthy(); }); it('should return false if the given text has one issue', () => { - expect(vm.issueLabel('mentioned')).toEqual('issue'); + expect(vm.hasMultipleIssues(relatedLinks.oneIssue)).toBeFalsy(); }); }); - describe('verbLabel', () => { + describe('issueLabel', () => { it('should return true if the given text has multiple issues', () => { - expect(vm.verbLabel('closing')).toEqual('are'); + expect(vm.issueLabel('twoIssues')).toEqual('issues'); + expect(vm.issueLabel('threeIssues')).toEqual('issues'); }); it('should return false if the given text has one issue', () => { - expect(vm.verbLabel('mentioned')).toEqual('is'); + expect(vm.issueLabel('oneIssue')).toEqual('issue'); }); }); }); describe('template', () => { - it('should have only have closing issues text', () => { - const vm = createComponent({ - relatedLinks: { - closing: '<a href="#">#23</a> and <a>#42</a>', - }, - }); - const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim(); - - expect(content).toContain('Closes issues #23 and #42'); - expect(content).not.toContain('mentioned'); - }); + it('should have only have closing issues text', (done) => { + vm.relatedLinks = { + closing: '<a href="#">#23</a> and <a>#42</a>', + }; - it('should have only have mentioned issues text', () => { - const vm = createComponent({ - relatedLinks: { - mentioned: '<a href="#">#7</a>', - }, - }); + Vue.nextTick() + .then(() => { + const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim(); - expect(vm.$el.innerText).toContain('issue #7'); - expect(vm.$el.innerText).toContain('is mentioned but will not be closed.'); - expect(vm.$el.innerText).not.toContain('Closes'); + expect(content).toContain('Closes issues #23 and #42'); + expect(content).not.toContain('mentioned'); + }) + .then(done) + .catch(done.fail); }); - it('should have closing and mentioned issues at the same time', () => { - const vm = createComponent({ - relatedLinks: { - closing: '<a href="#">#7</a>', - mentioned: '<a href="#">#23</a> and <a>#42</a>', - }, - }); - const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim(); + it('should have only have mentioned issues text', (done) => { + vm.relatedLinks = { + mentioned: '<a href="#">#7</a>', + }; + + Vue.nextTick() + .then(() => { + expect(vm.$el.innerText).toContain('issue #7'); + expect(vm.$el.innerText).toContain('is mentioned but will not be closed'); + expect(vm.$el.innerText).not.toContain('Closes'); + }) + .then(done) + .catch(done.fail); + }); - expect(content).toContain('Closes issue #7.'); - expect(content).toContain('issues #23 and #42'); - expect(content).toContain('are mentioned but will not be closed.'); + it('should have closing and mentioned issues at the same time', (done) => { + vm.relatedLinks = { + closing: '<a href="#">#7</a>', + mentioned: '<a href="#">#23</a> and <a>#42</a>', + }; + + Vue.nextTick() + .then(() => { + const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim(); + + expect(content).toContain('Closes issue #7.'); + expect(content).toContain('issues #23 and #42'); + expect(content).toContain('are mentioned but will not be closed'); + }) + .then(done) + .catch(done.fail); }); - it('should have assing issues link', () => { - const vm = createComponent({ - relatedLinks: { - assignToMe: '<a href="#">Assign yourself to these issues</a>', - }, - }); + it('should have assing issues link', (done) => { + vm.relatedLinks = { + assignToMe: '<a href="#">Assign yourself to these issues</a>', + }; + + Vue.nextTick() + .then(() => { + expect(vm.$el.innerText).toContain('Assign yourself to these issues'); + }) + .then(done) + .catch(done.fail); + }); - expect(vm.$el.innerText).toContain('Assign yourself to these issues'); + it('should use different wording after merging', (done) => { + vm.isMerged = true; + vm.relatedLinks = { + closing: '<a href="#">#7</a>', + mentioned: '<a href="#">#23</a> and <a>#42</a>', + }; + + Vue.nextTick() + .then(() => { + const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim(); + + expect(content).toContain('Closed issue #7.'); + expect(content).toContain('issues #23 and #42'); + expect(content).toContain('are mentioned but were not closed'); + }) + .then(done) + .catch(done.fail); }); }); }); diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js index 3a0c50b750f..425dff89439 100644 --- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js +++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js @@ -48,12 +48,13 @@ describe('mrWidgetOptions', () => { }); describe('shouldRenderMergeHelp', () => { - it('should return false for the initial merged state', () => { + it('should return false after merging', () => { + vm.mr.isMerged = true; expect(vm.shouldRenderMergeHelp).toBeFalsy(); }); - it('should return true for a state which requires help widget', () => { - vm.mr.state = 'conflicts'; + it('should return true before merging', () => { + vm.mr.isMerged = false; expect(vm.shouldRenderMergeHelp).toBeTruthy(); }); }); diff --git a/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js b/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js index 56dd0198ae2..71285866302 100644 --- a/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js +++ b/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js @@ -18,5 +18,17 @@ describe('MergeRequestStore', () => { store.setData({ ...mockData, work_in_progress: !mockData.work_in_progress }); expect(store.hasSHAChanged).toBe(false); }); + + it('sets isMerged to true for merged state', () => { + store.setData({ ...mockData, state: 'merged' }); + + expect(store.isMerged).toBe(true); + }); + + it('sets isMerged to false for readyToMerge state', () => { + store.setData({ ...mockData, state: 'readyToMerge' }); + + expect(store.isMerged).toBe(false); + }); }); }); |