Commit 5ecfd3cd authored by Eric Eastwood's avatar Eric Eastwood

Fix MR widget with external CI services/integrations

Fix https://gitlab.com/gitlab-org/gitlab-ce/issues/33287

The MR widget was trying to render the pipelines section when
there are no GitLab CI pipelines which was throwing some NPE
errors.
parent f0b089cf
...@@ -12,6 +12,9 @@ export default { ...@@ -12,6 +12,9 @@ export default {
ciIcon, ciIcon,
}, },
computed: { computed: {
hasPipeline() {
return this.mr.pipeline && Object.keys(this.mr.pipeline).length > 0;
},
hasCIError() { hasCIError() {
const { hasCI, ciStatus } = this.mr; const { hasCI, ciStatus } = this.mr;
...@@ -28,7 +31,9 @@ export default { ...@@ -28,7 +31,9 @@ export default {
}, },
}, },
template: ` template: `
<div class="mr-widget-heading"> <div
v-if="hasPipeline || hasCIError"
class="mr-widget-heading">
<div class="ci-widget media"> <div class="ci-widget media">
<template v-if="hasCIError"> <template v-if="hasCIError">
<div class="ci-status-icon ci-status-icon-failed ci-error js-ci-error append-right-10"> <div class="ci-status-icon ci-status-icon-failed ci-error js-ci-error append-right-10">
...@@ -40,7 +45,7 @@ export default { ...@@ -40,7 +45,7 @@ export default {
Could not connect to the CI server. Please check your settings and try again Could not connect to the CI server. Please check your settings and try again
</div> </div>
</template> </template>
<template v-else> <template v-else-if="hasPipeline">
<div class="ci-status-icon append-right-10"> <div class="ci-status-icon append-right-10">
<a <a
class="icon-link" class="icon-link"
......
...@@ -29,6 +29,9 @@ export default { ...@@ -29,6 +29,9 @@ export default {
statusIcon, statusIcon,
}, },
computed: { computed: {
shouldShowMergeWhenPipelineSucceedsText() {
return this.mr.isPipelineActive;
},
commitMessageLinkTitle() { commitMessageLinkTitle() {
const withDesc = 'Include description in commit message'; const withDesc = 'Include description in commit message';
const withoutDesc = "Don't include description in commit message"; const withoutDesc = "Don't include description in commit message";
...@@ -56,7 +59,7 @@ export default { ...@@ -56,7 +59,7 @@ export default {
mergeButtonText() { mergeButtonText() {
if (this.isMergingImmediately) { if (this.isMergingImmediately) {
return 'Merge in progress'; return 'Merge in progress';
} else if (this.mr.isPipelineActive) { } else if (this.shouldShowMergeWhenPipelineSucceedsText) {
return 'Merge when pipeline succeeds'; return 'Merge when pipeline succeeds';
} }
...@@ -68,7 +71,7 @@ export default { ...@@ -68,7 +71,7 @@ export default {
isMergeButtonDisabled() { isMergeButtonDisabled() {
const { commitMessage } = this; const { commitMessage } = this;
return Boolean(!commitMessage.length return Boolean(!commitMessage.length
|| !this.isMergeAllowed() || !this.shouldShowMergeControls()
|| this.isMakingRequest || this.isMakingRequest
|| this.mr.preventMerge); || this.mr.preventMerge);
}, },
...@@ -82,7 +85,12 @@ export default { ...@@ -82,7 +85,12 @@ export default {
}, },
methods: { methods: {
isMergeAllowed() { isMergeAllowed() {
return !(this.mr.onlyAllowMergeIfPipelineSucceeds && this.mr.isPipelineFailed); return !this.mr.onlyAllowMergeIfPipelineSucceeds ||
this.mr.isPipelinePassing ||
this.mr.isPipelineSkipped;
},
shouldShowMergeControls() {
return this.isMergeAllowed() || this.shouldShowMergeWhenPipelineSucceedsText;
}, },
updateCommitMessage() { updateCommitMessage() {
const cmwd = this.mr.commitMessageWithDescription; const cmwd = this.mr.commitMessageWithDescription;
...@@ -261,7 +269,7 @@ export default { ...@@ -261,7 +269,7 @@ export default {
</ul> </ul>
</span> </span>
<div class="media-body-wrap space-children"> <div class="media-body-wrap space-children">
<template v-if="isMergeAllowed()"> <template v-if="shouldShowMergeControls()">
<label> <label>
<input <input
id="remove-source-branch-input" id="remove-source-branch-input"
...@@ -286,7 +294,7 @@ export default { ...@@ -286,7 +294,7 @@ export default {
</template> </template>
<template v-else> <template v-else>
<span class="bold"> <span class="bold">
The pipeline for this merge request failed. Please retry the job or push a new commit to fix the failure The pipeline for this merge request has not succeeded yet
</span> </span>
</template> </template>
</div> </div>
......
...@@ -57,7 +57,7 @@ export default { ...@@ -57,7 +57,7 @@ export default {
return stateMaps.statesToShowHelpWidget.indexOf(this.mr.state) > -1; return stateMaps.statesToShowHelpWidget.indexOf(this.mr.state) > -1;
}, },
shouldRenderPipelines() { shouldRenderPipelines() {
return Object.keys(this.mr.pipeline).length || this.mr.hasCI; return this.mr.hasCI;
}, },
shouldRenderRelatedLinks() { shouldRenderRelatedLinks() {
return this.mr.relatedLinks; return this.mr.relatedLinks;
......
...@@ -85,7 +85,9 @@ export default class MergeRequestStore { ...@@ -85,7 +85,9 @@ export default class MergeRequestStore {
this.ciEnvironmentsStatusPath = data.ci_environments_status_path; this.ciEnvironmentsStatusPath = data.ci_environments_status_path;
this.hasCI = data.has_ci; this.hasCI = data.has_ci;
this.ciStatus = data.ci_status; this.ciStatus = data.ci_status;
this.isPipelineFailed = this.ciStatus ? (this.ciStatus === 'failed' || this.ciStatus === 'canceled') : false; this.isPipelineFailed = this.ciStatus === 'failed' || this.ciStatus === 'canceled';
this.isPipelinePassing = this.ciStatus === 'success' || this.ciStatus === 'success_with_warnings';
this.isPipelineSkipped = this.ciStatus === 'skipped';
this.pipelineDetailedStatus = pipelineStatus; this.pipelineDetailedStatus = pipelineStatus;
this.isPipelineActive = data.pipeline ? data.pipeline.active : false; this.isPipelineActive = data.pipeline ? data.pipeline.active : false;
this.isPipelineBlocked = pipelineStatus ? pipelineStatus.group === 'manual' : false; this.isPipelineBlocked = pipelineStatus ? pipelineStatus.group === 'manual' : false;
......
---
title: Fix errors thrown in merge request widget with external CI service/integration
merge_request:
author:
type: fixed
...@@ -142,6 +142,24 @@ describe 'Merge request', :js do ...@@ -142,6 +142,24 @@ describe 'Merge request', :js do
end end
end end
context 'view merge request where project has CI setup but no CI status' do
before do
pipeline = create(:ci_pipeline, project: project,
sha: merge_request.diff_head_sha,
ref: merge_request.source_branch)
create(:ci_build, pipeline: pipeline)
visit project_merge_request_path(project, merge_request)
end
it 'has pipeline error text' do
# Wait for the `ci_status` and `merge_check` requests
wait_for_requests
expect(page).to have_text('Could not connect to the CI server. Please check your settings and try again')
end
end
context 'view merge request with MWPS enabled but automatically merge fails' do context 'view merge request with MWPS enabled but automatically merge fails' do
before do before do
merge_request.update( merge_request.update(
......
...@@ -37,6 +37,26 @@ describe('MRWidgetPipeline', () => { ...@@ -37,6 +37,26 @@ describe('MRWidgetPipeline', () => {
}); });
}); });
describe('hasPipeline', () => {
it('should return true when there is a pipeline', () => {
expect(Object.keys(mockData.pipeline).length).toBeGreaterThan(0);
const vm = createComponent({
pipeline: mockData.pipeline,
});
expect(vm.hasPipeline).toBeTruthy();
});
it('should return false when there is no pipeline', () => {
const vm = createComponent({
pipeline: null,
});
expect(vm.hasPipeline).toBeFalsy();
});
});
describe('hasCIError', () => { describe('hasCIError', () => {
it('should return false when there is no CI error', () => { it('should return false when there is no CI error', () => {
const vm = createComponent({ const vm = createComponent({
......
...@@ -11,6 +11,7 @@ const createComponent = (customConfig = {}) => { ...@@ -11,6 +11,7 @@ const createComponent = (customConfig = {}) => {
isPipelineActive: false, isPipelineActive: false,
pipeline: null, pipeline: null,
isPipelineFailed: false, isPipelineFailed: false,
isPipelinePassing: false,
onlyAllowMergeIfPipelineSucceeds: false, onlyAllowMergeIfPipelineSucceeds: false,
hasCI: false, hasCI: false,
ciStatus: null, ciStatus: null,
...@@ -68,6 +69,18 @@ describe('MRWidgetReadyToMerge', () => { ...@@ -68,6 +69,18 @@ describe('MRWidgetReadyToMerge', () => {
}); });
describe('computed', () => { describe('computed', () => {
describe('shouldShowMergeWhenPipelineSucceedsText', () => {
it('should return true with active pipeline', () => {
vm.mr.isPipelineActive = true;
expect(vm.shouldShowMergeWhenPipelineSucceedsText).toBeTruthy();
});
it('should return false with inactive pipeline', () => {
vm.mr.isPipelineActive = false;
expect(vm.shouldShowMergeWhenPipelineSucceedsText).toBeFalsy();
});
});
describe('commitMessageLinkTitle', () => { describe('commitMessageLinkTitle', () => {
const withDesc = 'Include description in commit message'; const withDesc = 'Include description in commit message';
const withoutDesc = "Don't include description in commit message"; const withoutDesc = "Don't include description in commit message";
...@@ -203,20 +216,55 @@ describe('MRWidgetReadyToMerge', () => { ...@@ -203,20 +216,55 @@ describe('MRWidgetReadyToMerge', () => {
describe('methods', () => { describe('methods', () => {
describe('isMergeAllowed', () => { describe('isMergeAllowed', () => {
it('should return false with initial data', () => { it('should return true when no pipeline and not required to succeed', () => {
vm.mr.onlyAllowMergeIfPipelineSucceeds = false;
vm.mr.isPipelinePassing = false;
expect(vm.isMergeAllowed()).toBeTruthy(); expect(vm.isMergeAllowed()).toBeTruthy();
}); });
it('should return false when MR is set only merge when pipeline succeeds', () => { it('should return true when pipeline failed and not required to succeed', () => {
vm.mr.onlyAllowMergeIfPipelineSucceeds = true; vm.mr.onlyAllowMergeIfPipelineSucceeds = false;
vm.mr.isPipelinePassing = false;
expect(vm.isMergeAllowed()).toBeTruthy(); expect(vm.isMergeAllowed()).toBeTruthy();
}); });
it('should return true true', () => { it('should return false when pipeline failed and required to succeed', () => {
vm.mr.onlyAllowMergeIfPipelineSucceeds = true; vm.mr.onlyAllowMergeIfPipelineSucceeds = true;
vm.mr.isPipelineFailed = true; vm.mr.isPipelinePassing = false;
expect(vm.isMergeAllowed()).toBeFalsy(); expect(vm.isMergeAllowed()).toBeFalsy();
}); });
it('should return true when pipeline succeeded and required to succeed', () => {
vm.mr.onlyAllowMergeIfPipelineSucceeds = true;
vm.mr.isPipelinePassing = true;
expect(vm.isMergeAllowed()).toBeTruthy();
});
});
describe('shouldShowMergeControls', () => {
it('should return false when an external pipeline is running and required to succeed', () => {
spyOn(vm, 'isMergeAllowed').and.returnValue(false);
vm.mr.isPipelineActive = false;
expect(vm.shouldShowMergeControls()).toBeFalsy();
});
it('should return true when the build succeeded or build not required to succeed', () => {
spyOn(vm, 'isMergeAllowed').and.returnValue(true);
vm.mr.isPipelineActive = false;
expect(vm.shouldShowMergeControls()).toBeTruthy();
});
it('should return true when showing the MWPS button and a pipeline is running that needs to be successful', () => {
spyOn(vm, 'isMergeAllowed').and.returnValue(false);
vm.mr.isPipelineActive = true;
expect(vm.shouldShowMergeControls()).toBeTruthy();
});
it('should return true when showing the MWPS button but not required for the pipeline to succeed', () => {
spyOn(vm, 'isMergeAllowed').and.returnValue(true);
vm.mr.isPipelineActive = true;
expect(vm.shouldShowMergeControls()).toBeTruthy();
});
}); });
describe('updateCommitMessage', () => { describe('updateCommitMessage', () => {
......
...@@ -59,23 +59,15 @@ describe('mrWidgetOptions', () => { ...@@ -59,23 +59,15 @@ describe('mrWidgetOptions', () => {
}); });
describe('shouldRenderPipelines', () => { describe('shouldRenderPipelines', () => {
it('should return true for the initial data', () => { it('should return true when hasCI is true', () => {
expect(vm.shouldRenderPipelines).toBeTruthy(); vm.mr.hasCI = true;
});
it('should return true when pipeline is empty but MR.hasCI is set to true', () => {
vm.mr.pipeline = {};
expect(vm.shouldRenderPipelines).toBeTruthy(); expect(vm.shouldRenderPipelines).toBeTruthy();
}); });
it('should return true when pipeline available', () => { it('should return false when hasCI is false', () => {
vm.mr.hasCI = false; vm.mr.hasCI = false;
expect(vm.shouldRenderPipelines).toBeTruthy();
});
it('should return false when there is no pipeline', () => {
vm.mr.pipeline = {};
vm.mr.hasCI = false;
expect(vm.shouldRenderPipelines).toBeFalsy(); expect(vm.shouldRenderPipelines).toBeFalsy();
}); });
}); });
......
...@@ -18,5 +18,39 @@ describe('MergeRequestStore', () => { ...@@ -18,5 +18,39 @@ describe('MergeRequestStore', () => {
store.setData({ ...mockData, work_in_progress: !mockData.work_in_progress }); store.setData({ ...mockData, work_in_progress: !mockData.work_in_progress });
expect(store.hasSHAChanged).toBe(false); expect(store.hasSHAChanged).toBe(false);
}); });
describe('isPipelinePassing', () => {
it('is true when the CI status is `success`', () => {
store.setData({ ...mockData, ci_status: 'success' });
expect(store.isPipelinePassing).toBe(true);
});
it('is true when the CI status is `success_with_warnings`', () => {
store.setData({ ...mockData, ci_status: 'success_with_warnings' });
expect(store.isPipelinePassing).toBe(true);
});
it('is false when the CI status is `failed`', () => {
store.setData({ ...mockData, ci_status: 'failed' });
expect(store.isPipelinePassing).toBe(false);
});
it('is false when the CI status is anything except `success`', () => {
store.setData({ ...mockData, ci_status: 'foobarbaz' });
expect(store.isPipelinePassing).toBe(false);
});
});
describe('isPipelineSkipped', () => {
it('should set isPipelineSkipped=true when the CI status is `skipped`', () => {
store.setData({ ...mockData, ci_status: 'skipped' });
expect(store.isPipelineSkipped).toBe(true);
});
it('should set isPipelineSkipped=false when the CI status is anything except `skipped`', () => {
store.setData({ ...mockData, ci_status: 'foobarbaz' });
expect(store.isPipelineSkipped).toBe(false);
});
});
}); });
}); });
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment