Merge branch '33287-fix-mr-widget-errors-with-external-services' into 'master'

Fix MR widget with external CI services/integrations

Closes #33287

See merge request gitlab-org/gitlab-ce!13977

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"

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>

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;

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;

changelogs/unreleased/33287-fix-mr-widget-errors-with-external-services.yml

+---
+title: Fix errors thrown in merge request widget with external CI service/integration
+merge_request:
+author:
+type: fixed

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(

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({

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', () => {

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

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