Merge branch '330851-improve-failed-merge-check-ux' into 'master'

Add confirmation modal for merge button when pipeline has failed

See merge request gitlab-org/gitlab!78722

app/assets/javascripts/vue_merge_request_widget/components/states/merge_failed_pipeline_confirmation_dialog.vue

+<script>
+import { GlModal, GlButton } from '@gitlab/ui';
+import { __ } from '~/locale';
+
+export default {
+  name: 'MergeFailedPipelineConfirmationDialog',
+  i18n: {
+    primary: __('Merge unverified changes'),
+    cancel: __('Cancel'),
+    info: __(
+      'The latest pipeline for this merge request did not succeed. The latest changes are unverified.',
+    ),
+    confirmation: __('Are you sure you want to attempt to merge?'),
+    title: __('Merge unverified changes?'),
+  },
+  components: {
+    GlModal,
+    GlButton,
+  },
+  props: {
+    visible: {
+      type: Boolean,
+      required: true,
+    },
+  },
+  methods: {
+    hide() {
+      this.$refs.modal.hide();
+    },
+    cancel() {
+      this.hide();
+      this.$emit('cancel');
+    },
+    focusCancelButton() {
+      this.$refs.cancelButton.$el.focus();
+    },
+    mergeChanges() {
+      this.$emit('mergeWithFailedPipeline');
+      this.hide();
+    },
+  },
+};
+</script>
+<template>
+  <gl-modal
+    ref="modal"
+    size="sm"
+    modal-id="merge-train-failed-pipeline-confirmation-dialog"
+    :title="$options.i18n.title"
+    :visible="visible"
+    data-testid="merge-failed-pipeline-confirmation-dialog"
+    @shown="focusCancelButton"
+    @hide="$emit('cancel')"
+  >
+    <p>{{ $options.i18n.info }}</p>
+    <p>{{ $options.i18n.confirmation }}</p>
+    <template #modal-footer>
+      <gl-button ref="cancelButton" data-testid="merge-cancel-btn" @click="cancel">{{
+        $options.i18n.cancel
+      }}</gl-button>
+      <gl-button variant="danger" data-testid="merge-unverified-changes" @click="mergeChanges">
+        {{ $options.i18n.primary }}
+      </gl-button>
+    </template>
+  </gl-modal>
+</template>

app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue

@@ -25,8 +25,6 @@ import { helpPagePath } from '~/helpers/help_page_helper';
 import MergeRequest from '../../../merge_request';
 import {
   AUTO_MERGE_STRATEGIES,
-  DANGER,
-  CONFIRM,
   WARNING,
   MT_MERGE_STRATEGY,
   PIPELINE_FAILED_STATE,
@@ -42,6 +40,7 @@ import CommitEdit from './commit_edit.vue';
 import CommitMessageDropdown from './commit_message_dropdown.vue';
 import CommitsHeader from './commits_header.vue';
 import SquashBeforeMerge from './squash_before_merge.vue';
+import MergeFailedPipelineConfirmationDialog from './merge_failed_pipeline_confirmation_dialog.vue';
 
 const PIPELINE_RUNNING_STATE = 'running';
 const PIPELINE_PENDING_STATE = 'pending';
@@ -106,6 +105,7 @@ export default {
     GlDropdownItem,
     GlFormCheckbox,
     GlSkeletonLoader,
+    MergeFailedPipelineConfirmationDialog,
     MergeTrainHelperIcon: () =>
       import('ee_component/vue_merge_request_widget/components/merge_train_helper_icon.vue'),
     MergeImmediatelyConfirmationDialog: () =>
@@ -138,7 +138,8 @@ export default {
       squashBeforeMerge: this.mr.squashIsSelected,
       isSquashReadOnly: this.mr.squashIsReadonly,
       squashCommitMessage: this.mr.squashCommitMessage,
-      isPipelineFailedModalVisible: false,
+      isPipelineFailedModalVisibleMergeTrain: false,
+      isPipelineFailedModalVisibleNormalMerge: false,
       editCommitMessage: false,
     };
   },
@@ -166,6 +167,9 @@ export default {
 
       return this.mr.isPipelineFailed;
     },
+    showMergeFailedPipelineConfirmationDialog() {
+      return this.status === PIPELINE_FAILED_STATE && this.isPipelineFailed;
+    },
     isMergeAllowed() {
       if (this.glFeatures.mergeRequestWidgetGraphql) {
         return this.state.mergeable;
@@ -248,13 +252,6 @@ export default {
 
       return PIPELINE_SUCCESS_STATE;
     },
-    mergeButtonVariant() {
-      if (this.status === PIPELINE_FAILED_STATE || this.isPipelineFailed) {
-        return DANGER;
-      }
-
-      return CONFIRM;
-    },
     iconClass() {
       if (this.shouldRenderMergeTrainHelperIcon && !this.mr.preventMerge) {
         return PIPELINE_RUNNING_STATE;
@@ -279,6 +276,10 @@ export default {
         return this.autoMergeText;
       }
 
+      if (this.status === PIPELINE_FAILED_STATE || this.isPipelineFailed) {
+        return __('Merge...');
+      }
+
       return __('Merge');
     },
     hasPipelineMustSucceedConflict() {
@@ -361,8 +362,13 @@ export default {
       return this.$apollo.queries.state.refetch();
     },
     handleMergeButtonClick(useAutoMerge, mergeImmediately = false, confirmationClicked = false) {
-      if (this.showFailedPipelineModal && !confirmationClicked) {
-        this.isPipelineFailedModalVisible = true;
+      if (this.showMergeFailedPipelineConfirmationDialog && !confirmationClicked) {
+        this.isPipelineFailedModalVisibleNormalMerge = true;
+        return;
+      }
+
+      if (this.showFailedPipelineModalMergeTrain && !confirmationClicked) {
+        this.isPipelineFailedModalVisibleMergeTrain = true;
         return;
       }
 
@@ -434,6 +440,9 @@ export default {
     onMergeImmediatelyConfirmation() {
       this.handleMergeButtonClick(false, true, true);
     },
+    onMergeWithFailedPipelineConfirmation() {
+      this.handleMergeButtonClick(false, true, true);
+    },
     initiateMergePolling() {
       simplePoll(
         (continuePolling, stopPolling) => {
@@ -559,7 +568,7 @@ export default {
                 category="primary"
                 class="accept-merge-request"
                 data-testid="merge-button"
-                :variant="mergeButtonVariant"
+                variant="confirm"
                 :disabled="isMergeButtonDisabled"
                 :loading="isMakingRequest"
                 data-qa-selector="merge_button"
@@ -570,7 +579,7 @@ export default {
                 v-if="shouldShowMergeImmediatelyDropdown"
                 v-gl-tooltip.hover.focus="__('Select merge moment')"
                 :disabled="isMergeButtonDisabled"
-                :variant="mergeButtonVariant"
+                variant="confirm"
                 data-qa-selector="merge_moment_dropdown"
                 toggle-class="btn-icon js-merge-moment"
               >
@@ -593,9 +602,14 @@ export default {
                 />
               </gl-dropdown>
               <merge-train-failed-pipeline-confirmation-dialog
-                :visible="isPipelineFailedModalVisible"
+                :visible="isPipelineFailedModalVisibleMergeTrain"
                 @startMergeTrain="onStartMergeTrainConfirmation"
-                @cancel="isPipelineFailedModalVisible = false"
+                @cancel="isPipelineFailedModalVisibleMergeTrain = false"
+              />
+              <merge-failed-pipeline-confirmation-dialog
+                :visible="isPipelineFailedModalVisibleNormalMerge"
+                @mergeWithFailedPipeline="onMergeWithFailedPipelineConfirmation"
+                @cancel="isPipelineFailedModalVisibleNormalMerge = false"
               />
             </gl-button-group>
             <merge-train-helper-icon v-if="shouldRenderMergeTrainHelperIcon" class="gl-mx-3" />

app/assets/javascripts/vue_merge_request_widget/i18n.js

@@ -20,3 +20,8 @@ export const I18N_SHA_MISMATCH = {
   warningMessage: __('Merge blocked: new changes were just added.'),
   actionButtonLabel: __('Review changes'),
 };
+
+export const MERGE_TRAIN_BUTTON_TEXT = {
+  failed: __('Start merge train...'),
+  passed: __('Start merge train'),
+};

app/assets/javascripts/vue_merge_request_widget/mixins/ready_to_merge.js

@@ -48,7 +48,7 @@ export default {
     pipelineId() {
       return this.pipeline.id;
     },
-    showFailedPipelineModal() {
+    showFailedPipelineModalMergeTrain() {
       return false;
     },
   },

ee/app/assets/javascripts/vue_merge_request_widget/mixins/ready_to_merge.js

@@ -7,6 +7,7 @@ import {
   PIPELINE_FAILED_STATE,
 } from '~/vue_merge_request_widget/constants';
 import base from '~/vue_merge_request_widget/mixins/ready_to_merge';
+import { MERGE_TRAIN_BUTTON_TEXT } from '~/vue_merge_request_widget/i18n';
 
 export const MERGE_DISABLED_TEXT_UNAPPROVED = s__(
   'mrWidget|Merge blocked: this merge request must be approved.',
@@ -48,7 +49,9 @@ export default {
         return __('Add to merge train when pipeline succeeds');
       } else if (this.preferredAutoMergeStrategy === MT_MERGE_STRATEGY) {
         if (this.stateData.mergeTrainsCount === 0) {
-          return __('Start merge train');
+          const pipelineFailed = this.status === PIPELINE_FAILED_STATE || this.isPipelineFailed;
+
+          return pipelineFailed ? MERGE_TRAIN_BUTTON_TEXT.failed : MERGE_TRAIN_BUTTON_TEXT.passed;
         }
         return __('Add to merge train');
       }
@@ -80,7 +83,7 @@ export default {
     isMergeImmediatelyDangerous() {
       return [MT_MERGE_STRATEGY, MTWPS_MERGE_STRATEGY].includes(this.preferredAutoMergeStrategy);
     },
-    showFailedPipelineModal() {
+    showFailedPipelineModalMergeTrain() {
       const pipelineFailed = this.status === PIPELINE_FAILED_STATE || this.isPipelineFailed;
       const mergeStrateyMergeTrain = this.preferredAutoMergeStrategy === MT_MERGE_STRATEGY;
 

ee/spec/frontend/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js

@@ -11,6 +11,7 @@ import {
   MT_MERGE_STRATEGY,
   MTWPS_MERGE_STRATEGY,
 } from '~/vue_merge_request_widget/constants';
+import { MERGE_TRAIN_BUTTON_TEXT } from '~/vue_merge_request_widget/i18n';
 import {
   MERGE_DISABLED_TEXT,
   MERGE_DISABLED_SKIPPED_PIPELINE_TEXT,
@@ -405,31 +406,45 @@ describe('ReadyToMerge', () => {
     });
   });
 
-  describe('Merge button variant', () => {
-    it('danger variant and failed text should show if pipeline failed', () => {
-      factory({
-        isPipelineFailed: true,
-        preferredAutoMergeStrategy: MT_MERGE_STRATEGY,
-        availableAutoMergeStrategies: [MT_MERGE_STRATEGY],
-        hasCI: true,
-        onlyAllowMergeIfPipelineSucceeds: false,
+  describe('Merge train text', () => {
+    describe('pipeline failed', () => {
+      beforeEach(() => {
+        factory({
+          isPipelineFailed: true,
+          preferredAutoMergeStrategy: MT_MERGE_STRATEGY,
+          availableAutoMergeStrategies: [MT_MERGE_STRATEGY],
+          hasCI: true,
+          onlyAllowMergeIfPipelineSucceeds: false,
+        });
       });
 
-      expect(findMergeButton().attributes('variant')).toBe('danger');
-      expect(findFailedPipelineMergeTrainText().exists()).toBe(true);
+      it('failed merge train text should show if pipeline failed', () => {
+        expect(findFailedPipelineMergeTrainText().exists()).toBe(true);
+      });
+
+      it('merge button text should contain ellipsis', () => {
+        expect(findMergeButton().text()).toBe(MERGE_TRAIN_BUTTON_TEXT.failed);
+      });
     });
 
-    it('confirm variant and failed text should not show if pipeline passed', () => {
-      factory({
-        preferredAutoMergeStrategy: MT_MERGE_STRATEGY,
-        availableAutoMergeStrategies: [MT_MERGE_STRATEGY],
-        hasCI: true,
-        onlyAllowMergeIfPipelineSucceeds: false,
-        ciStatus: 'success',
+    describe('pipeline passed', () => {
+      beforeEach(() => {
+        factory({
+          preferredAutoMergeStrategy: MT_MERGE_STRATEGY,
+          availableAutoMergeStrategies: [MT_MERGE_STRATEGY],
+          hasCI: true,
+          onlyAllowMergeIfPipelineSucceeds: false,
+          ciStatus: 'success',
+        });
       });
 
-      expect(findMergeButton().attributes('variant')).toBe('confirm');
-      expect(findFailedPipelineMergeTrainText().exists()).toBe(false);
+      it('merge button text should not contain ellipsis', () => {
+        expect(findMergeButton().text()).toBe(MERGE_TRAIN_BUTTON_TEXT.passed);
+      });
+
+      it('failed merge train text should not show if pipeline passed', () => {
+        expect(findFailedPipelineMergeTrainText().exists()).toBe(false);
+      });
     });
   });
 });

locale/gitlab.pot

@@ -22417,9 +22417,18 @@ msgstr ""
 msgid "Merge unavailable: merge requests are read-only in a secondary Geo node."
 msgstr ""
 
+msgid "Merge unverified changes"
+msgstr ""
+
+msgid "Merge unverified changes?"
+msgstr ""
+
 msgid "Merge when pipeline succeeds"
 msgstr ""
 
+msgid "Merge..."
+msgstr ""
+
 msgid "MergeConflict|Commit to source branch"
 msgstr ""
 
@@ -34033,6 +34042,9 @@ msgstr ""
 msgid "Start merge train when pipeline succeeds"
 msgstr ""
 
+msgid "Start merge train..."
+msgstr ""
+
 msgid "Start search"
 msgstr ""
 
@@ -35774,6 +35786,9 @@ msgstr ""
 msgid "The latest pipeline for this merge request did not complete successfully."
 msgstr ""
 
+msgid "The latest pipeline for this merge request did not succeed. The latest changes are unverified."
+msgstr ""
+
 msgid "The latest pipeline for this merge request has failed."
 msgstr ""
 

spec/features/merge_request/user_sees_merge_widget_spec.rb

@@ -104,10 +104,11 @@ RSpec.describe 'Merge request > User sees merge widget', :js do
       visit project_merge_request_path(project, merge_request)
     end
 
-    it 'has danger button while waiting for external CI status' do
+    it 'has merge button with confirm variant while waiting for external CI status' do
       # Wait for the `ci_status` and `merge_check` requests
       wait_for_requests
-      expect(page).to have_selector('.accept-merge-request.btn-danger')
+
+      expect(page).to have_selector('.accept-merge-request.btn-confirm')
     end
   end
 
@@ -125,10 +126,27 @@ RSpec.describe 'Merge request > User sees merge widget', :js do
       visit project_merge_request_path(project, merge_request)
     end
 
-    it 'has danger button when not succeeded' do
+    it 'has merge button that shows modal when pipeline does not succeeded' do
       # Wait for the `ci_status` and `merge_check` requests
       wait_for_requests
-      expect(page).to have_selector('.accept-merge-request.btn-danger')
+
+      click_button 'Merge...'
+
+      expect(page).to have_selector('[data-testid="merge-failed-pipeline-confirmation-dialog"]', visible: true)
+    end
+
+    it 'allows me to merge with a failed pipeline' do
+      modal_selector = '[data-testid="merge-failed-pipeline-confirmation-dialog"]'
+
+      wait_for_requests
+
+      click_button 'Merge...'
+
+      page.within(modal_selector) do
+        click_button 'Merge unverified changes'
+      end
+
+      expect(find('.media-body h4')).to have_content('Merging!')
     end
   end
 

spec/frontend/vue_mr_widget/components/states/merge_failed_pipeline_confirmation_dialog_spec.js

+import { shallowMount } from '@vue/test-utils';
+import MergeFailedPipelineConfirmationDialog from '~/vue_merge_request_widget/components/states/merge_failed_pipeline_confirmation_dialog.vue';
+import { trimText } from 'helpers/text_helper';
+
+describe('MergeFailedPipelineConfirmationDialog', () => {
+  let wrapper;
+
+  const GlModal = {
+    template: `
+      <div>
+        <slot></slot>
+        <slot name="modal-footer"></slot>
+      </div>
+    `,
+    methods: {
+      hide: jest.fn(),
+    },
+  };
+
+  const createComponent = () => {
+    wrapper = shallowMount(MergeFailedPipelineConfirmationDialog, {
+      propsData: {
+        visible: true,
+      },
+      stubs: {
+        GlModal,
+      },
+      attachTo: document.body,
+    });
+  };
+
+  const findModal = () => wrapper.findComponent(GlModal);
+  const findMergeBtn = () => wrapper.find('[data-testid="merge-unverified-changes"]');
+  const findCancelBtn = () => wrapper.find('[data-testid="merge-cancel-btn"]');
+
+  beforeEach(() => {
+    createComponent();
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  it('should render informational text explaining why merging immediately can be dangerous', () => {
+    expect(trimText(wrapper.text())).toContain(
+      'The latest pipeline for this merge request did not succeed. The latest changes are unverified. Are you sure you want to attempt to merge?',
+    );
+  });
+
+  it('should emit the mergeWithFailedPipeline event', () => {
+    findMergeBtn().vm.$emit('click');
+
+    expect(wrapper.emitted('mergeWithFailedPipeline')).toBeTruthy();
+  });
+
+  it('when the cancel button is clicked should emit cancel and call hide', () => {
+    jest.spyOn(findModal().vm, 'hide');
+
+    findCancelBtn().vm.$emit('click');
+
+    expect(wrapper.emitted('cancel')).toBeTruthy();
+    expect(findModal().vm.hide).toHaveBeenCalled();
+  });
+
+  it('should emit cancel when the hide event is emitted', () => {
+    findModal().vm.$emit('hide');
+
+    expect(wrapper.emitted('cancel')).toBeTruthy();
+  });
+
+  it('when modal is shown it will focus the cancel button', () => {
+    jest.spyOn(findCancelBtn().element, 'focus');
+
+    findModal().vm.$emit('shown');
+
+    expect(findCancelBtn().element.focus).toHaveBeenCalled();
+  });
+});

spec/frontend/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js

@@ -7,6 +7,7 @@ import CommitMessageDropdown from '~/vue_merge_request_widget/components/states/
 import CommitsHeader from '~/vue_merge_request_widget/components/states/commits_header.vue';
 import ReadyToMerge from '~/vue_merge_request_widget/components/states/ready_to_merge.vue';
 import SquashBeforeMerge from '~/vue_merge_request_widget/components/states/squash_before_merge.vue';
+import MergeFailedPipelineConfirmationDialog from '~/vue_merge_request_widget/components/states/merge_failed_pipeline_confirmation_dialog.vue';
 import { MWPS_MERGE_STRATEGY } from '~/vue_merge_request_widget/constants';
 import eventHub from '~/vue_merge_request_widget/event_hub';
 
@@ -61,6 +62,11 @@ const createTestService = () => ({
 });
 
 let wrapper;
+
+const findMergeButton = () => wrapper.find('[data-testid="merge-button"]');
+const findPipelineFailedConfirmModal = () =>
+  wrapper.findComponent(MergeFailedPipelineConfirmationDialog);
+
 const createComponent = (customConfig = {}, mergeRequestWidgetGraphql = false) => {
   wrapper = shallowMount(ReadyToMerge, {
     propsData: {
@@ -132,33 +138,13 @@ describe('ReadyToMerge', () => {
       });
     });
 
-    describe('mergeButtonVariant', () => {
+    describe('Merge Button Variant', () => {
       it('defaults to confirm class', () => {
         createComponent({
           mr: { availableAutoMergeStrategies: [] },
         });
 
-        expect(wrapper.vm.mergeButtonVariant).toEqual('confirm');
-      });
-
-      it('returns confirm class for success status', () => {
-        createComponent({
-          mr: { availableAutoMergeStrategies: [], pipeline: true },
-        });
-
-        expect(wrapper.vm.mergeButtonVariant).toEqual('confirm');
-      });
-
-      it('returns confirm class for pending status', () => {
-        createComponent();
-
-        expect(wrapper.vm.mergeButtonVariant).toEqual('confirm');
-      });
-
-      it('returns danger class for failed status', () => {
-        createComponent({ mr: { hasCI: true } });
-
-        expect(wrapper.vm.mergeButtonVariant).toEqual('danger');
+        expect(findMergeButton().attributes('variant')).toBe('confirm');
       });
     });
 
@@ -794,4 +780,24 @@ describe('ReadyToMerge', () => {
       });
     });
   });
+
+  describe('Merge button when pipeline has failed', () => {
+    beforeEach(() => {
+      createComponent({
+        mr: { pipeline: {}, isPipelineFailed: true, availableAutoMergeStrategies: [] },
+      });
+    });
+
+    it('should display the correct merge text', () => {
+      expect(findMergeButton().text()).toBe('Merge...');
+    });
+
+    it('should display confirmation modal when merge button is clicked', async () => {
+      expect(findPipelineFailedConfirmModal().props()).toEqual({ visible: false });
+
+      await findMergeButton().vm.$emit('click');
+
+      expect(findPipelineFailedConfirmModal().props()).toEqual({ visible: true });
+    });
+  });
 });