Fix MR widget with external CI services/integrations33287-fix-mr-widget-errors-with-external-services

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.

10 files changed, 157 insertions, 25 deletions

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.js b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.js
index 6c2e9ba1d30..c79b5c720eb 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.js
@@ -12,6 +12,9 @@ export default {
     ciIcon,
   },
   computed: {
+    hasPipeline() {
+      return this.mr.pipeline && Object.keys(this.mr.pipeline).length > 0;
+    },
     hasCIError() {
       const { hasCI, ciStatus } = this.mr;
 
@@ -28,7 +31,9 @@ export default {
     },
   },
   template: `
-    <div class="mr-widget-heading">
+    <div
+      v-if="hasPipeline || hasCIError"
+      class="mr-widget-heading">
       <div class="ci-widget media">
         <template v-if="hasCIError">
           <div class="ci-status-icon ci-status-icon-failed ci-error js-ci-error append-right-10">
@@ -40,7 +45,7 @@ export default {
             Could not connect to the CI server. Please check your settings and try again
           </div>
         </template>
-        <template v-else>
+        <template v-else-if="hasPipeline">
           <div class="ci-status-icon append-right-10">
             <a
               class="icon-link"
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js
index 1fb7150d20e..557b89b2b84 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js
@@ -29,6 +29,9 @@ export default {
     statusIcon,
   },
   computed: {
+    shouldShowMergeWhenPipelineSucceedsText() {
+      return this.mr.isPipelineActive;
+    },
     commitMessageLinkTitle() {
       const withDesc = 'Include description in commit message';
       const withoutDesc = "Don't include description in commit message";
@@ -56,7 +59,7 @@ export default {
     mergeButtonText() {
       if (this.isMergingImmediately) {
         return 'Merge in progress';
-      } else if (this.mr.isPipelineActive) {
+      } else if (this.shouldShowMergeWhenPipelineSucceedsText) {
         return 'Merge when pipeline succeeds';
       }
 
@@ -68,7 +71,7 @@ export default {
     isMergeButtonDisabled() {
       const { commitMessage } = this;
       return Boolean(!commitMessage.length
-        || !this.isMergeAllowed()
+        || !this.shouldShowMergeControls()
         || this.isMakingRequest
         || this.mr.preventMerge);
     },
@@ -82,7 +85,12 @@ export default {
   },
   methods: {
     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() {
       const cmwd = this.mr.commitMessageWithDescription;
@@ -261,7 +269,7 @@ export default {
             </ul>
           </span>
           <div class="media-body-wrap space-children">
-            <template v-if="isMergeAllowed()">
+            <template v-if="shouldShowMergeControls()">
               <label>
                 <input
                   id="remove-source-branch-input"
@@ -286,7 +294,7 @@ export default {
             </template>
             <template v-else>
               <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>
             </template>
           </div>
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 0042c48816f..2f237262028 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
@@ -57,7 +57,7 @@ export default {
       return stateMaps.statesToShowHelpWidget.indexOf(this.mr.state) > -1;
     },
     shouldRenderPipelines() {
-      return Object.keys(this.mr.pipeline).length || this.mr.hasCI;
+      return this.mr.hasCI;
     },
     shouldRenderRelatedLinks() {
       return this.mr.relatedLinks;
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 fbea764b739..29464662578 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
@@ -85,7 +85,9 @@ export default class MergeRequestStore {
     this.ciEnvironmentsStatusPath = data.ci_environments_status_path;
     this.hasCI = data.has_ci;
     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.isPipelineActive = data.pipeline ? data.pipeline.active : false;
     this.isPipelineBlocked = pipelineStatus ? pipelineStatus.group === 'manual' : false;
diff --git a/changelogs/unreleased/33287-fix-mr-widget-errors-with-external-services.yml b/changelogs/unreleased/33287-fix-mr-widget-errors-with-external-services.yml
new file mode 100644
index 00000000000..f0c76060781
--- /dev/null
+++ b/changelogs/unreleased/33287-fix-mr-widget-errors-with-external-services.yml
@@ -0,0 +1,5 @@
+---
+title: Fix errors thrown in merge request widget with external CI service/integration
+merge_request:
+author:
+type: fixed
diff --git a/spec/features/merge_requests/widget_spec.rb b/spec/features/merge_requests/widget_spec.rb
index fd991293ee9..443b596b3c6 100644
--- a/spec/features/merge_requests/widget_spec.rb
+++ b/spec/features/merge_requests/widget_spec.rb
@@ -142,6 +142,24 @@ describe 'Merge request', :js do
     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
     before do
       merge_request.update(
diff --git a/spec/javascripts/vue_mr_widget/components/mr_widget_pipeline_spec.js b/spec/javascripts/vue_mr_widget/components/mr_widget_pipeline_spec.js
index c763487d12f..690665ae12c 100644
--- a/spec/javascripts/vue_mr_widget/components/mr_widget_pipeline_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/mr_widget_pipeline_spec.js
@@ -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', () => {
       it('should return false when there is no CI error', () => {
         const vm = createComponent({
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js
index c607c9746a4..8f69458ffb0 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js
@@ -11,6 +11,7 @@ const createComponent = (customConfig = {}) => {
     isPipelineActive: false,
     pipeline: null,
     isPipelineFailed: false,
+    isPipelinePassing: false,
     onlyAllowMergeIfPipelineSucceeds: false,
     hasCI: false,
     ciStatus: null,
@@ -68,6 +69,18 @@ describe('MRWidgetReadyToMerge', () => {
   });
 
   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', () => {
       const withDesc = 'Include description in commit message';
       const withoutDesc = "Don't include description in commit message";
@@ -203,20 +216,55 @@ describe('MRWidgetReadyToMerge', () => {
 
   describe('methods', () => {
     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();
       });
 
-      it('should return false when MR is set only merge when pipeline succeeds', () => {
-        vm.mr.onlyAllowMergeIfPipelineSucceeds = true;
+      it('should return true when pipeline failed and not required to succeed', () => {
+        vm.mr.onlyAllowMergeIfPipelineSucceeds = false;
+        vm.mr.isPipelinePassing = false;
         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.isPipelineFailed = true;
+        vm.mr.isPipelinePassing = false;
         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', () => {
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 669ee248bf1..da66c7504cb 100644
--- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js
+++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js
@@ -59,23 +59,15 @@ describe('mrWidgetOptions', () => {
     });
 
     describe('shouldRenderPipelines', () => {
-      it('should return true for the initial data', () => {
-        expect(vm.shouldRenderPipelines).toBeTruthy();
-      });
+      it('should return true when hasCI is true', () => {
+        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();
       });
 
-      it('should return true when pipeline available', () => {
+      it('should return false when hasCI is 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();
       });
     });
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..8e5614b20f0 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,39 @@ describe('MergeRequestStore', () => {
       store.setData({ ...mockData, work_in_progress: !mockData.work_in_progress });
       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);
+      });
+    });
   });
 });