kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Adjust position and wording for related issues in merge requests

Browse source
This commit is contained in:
Winnie Hellmann 2017-06-19 22:17:00 +00:00 committed by Fatih Acet
parent 025cbc2ad4
commit ef4bc6df04
10 changed files with 262 additions and 111 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

41
app/assets/javascripts/vue_merge_request_widget/components/mr_widget_related_links.js
View file

@ -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>
`,
};

10
app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js
View file

@ -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"

11
app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js
View file

@ -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>
`,

13
app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
View file

@ -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;

13
app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js
View file

@ -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,
};

4
app/assets/stylesheets/pages/merge_requests.scss
View file

@ -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;

16
spec/features/merge_requests/closes_issues_spec.rb
View file

@ -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

246
spec/javascripts/vue_mr_widget/components/mr_widget_related_links_spec.js
View file

@ -1,20 +1,31 @@
import Vue from 'vue';
import relatedLinksComponent from '~/vue_merge_request_widget/components/mr_widget_related_links';
const createComponent = (data) => {
const Component = Vue.extend(relatedLinksComponent);
return new Component({
el: document.createElement('div'),
propsData: data,
});
};
import MRWidgetRelatedLinks from '~/vue_merge_request_widget/components/mr_widget_related_links';
describe('MRWidgetRelatedLinks', () => {
let vm;
beforeEach(() => {
const Component = Vue.extend(MRWidgetRelatedLinks);
vm = new Component({
el: document.createElement('div'),
propsData: {
isMerged: false,
relatedLinks: {},
},
});
});
afterEach(() => {
vm.$destroy();
});
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('mentionedText', () => {
it('outputs text for one mentioned issue before merging', () => {
vm.isMerged = false;
spyOn(vm, 'hasMultipleIssues').and.returnValue(false);
const text = vm.mentionedText;
expect(text).toBe('is mentioned but will not be closed');
});
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('methods', () => {
const data = {
relatedLinks: {
closing: '<a href="#">#23</a> and <a>#42</a>',
mentioned: '<a href="#">#7</a>',
},
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>',
};
const vm = createComponent(data);
beforeEach(() => {
vm.relatedLinks = relatedLinks;
});
describe('hasMultipleIssues', () => {
it('should return true if the given text has multiple issues', () => {
expect(vm.hasMultipleIssues(data.relatedLinks.closing)).toBeTruthy();
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.hasMultipleIssues(data.relatedLinks.mentioned)).toBeFalsy();
expect(vm.hasMultipleIssues(relatedLinks.oneIssue)).toBeFalsy();
});
});
describe('issueLabel', () => {
it('should return true if the given text has multiple issues', () => {
expect(vm.issueLabel('closing')).toEqual('issues');
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.issueLabel('mentioned')).toEqual('issue');
});
});
describe('verbLabel', () => {
it('should return true if the given text has multiple issues', () => {
expect(vm.verbLabel('closing')).toEqual('are');
});
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();
it('should have only have closing issues text', (done) => {
vm.relatedLinks = {
closing: '<a href="#">#23</a> and <a>#42</a>',
};
expect(content).toContain('Closes issues #23 and #42');
expect(content).not.toContain('mentioned');
Vue.nextTick()
.then(() => {
const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim();
expect(content).toContain('Closes issues #23 and #42');
expect(content).not.toContain('mentioned');
})
.then(done)
.catch(done.fail);
});
it('should have only have mentioned issues text', () => {
const vm = createComponent({
relatedLinks: {
mentioned: '<a href="#">#7</a>',
},
});
it('should have only have mentioned issues text', (done) => {
vm.relatedLinks = {
mentioned: '<a href="#">#7</a>',
};
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');
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);
});
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 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>',
};
expect(content).toContain('Closes issue #7.');
expect(content).toContain('issues #23 and #42');
expect(content).toContain('are mentioned but will not be closed.');
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>',
};
expect(vm.$el.innerText).toContain('Assign yourself to these issues');
Vue.nextTick()
.then(() => {
expect(vm.$el.innerText).toContain('Assign yourself to these issues');
})
.then(done)
.catch(done.fail);
});
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);
});
});
});

7
spec/javascripts/vue_mr_widget/mr_widget_options_spec.js
View file

@ -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();
});
});

12
spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js
View file

@ -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);
});
});
});