Merge branch...

Merge branch '350503-frontend-browser-popup-appears-when-updating-ci-yml-content-and-submit-a-merge-request' into 'master'

Fix unnecessary navigation modal in pipeline editor

See merge request gitlab-org/gitlab!79032

app/assets/javascripts/pipeline_editor/components/commit/commit_section.vue

 <script>
-import { mergeUrlParams, redirectTo } from '~/lib/utils/url_utility';
 import { __, s__, sprintf } from '~/locale';
 import {
   COMMIT_ACTION_CREATE,
   COMMIT_ACTION_UPDATE,
   COMMIT_FAILURE,
   COMMIT_SUCCESS,
+  COMMIT_SUCCESS_WITH_REDIRECT,
 } from '../../constants';
 import commitCIFile from '../../graphql/mutations/commit_ci_file.mutation.graphql';
 import updateCurrentBranchMutation from '../../graphql/mutations/client/update_current_branch.mutation.graphql';
@@ -15,9 +15,6 @@ import getCurrentBranch from '../../graphql/queries/client/current_branch.query.
 
 import CommitForm from './commit_form.vue';
 
-const MR_SOURCE_BRANCH = 'merge_request[source_branch]';
-const MR_TARGET_BRANCH = 'merge_request[target_branch]';
-
 export default {
   alertTexts: {
     [COMMIT_FAILURE]: s__('Pipelines|The GitLab CI configuration could not be updated.'),
@@ -29,7 +26,7 @@ export default {
   components: {
     CommitForm,
   },
-  inject: ['projectFullPath', 'ciConfigPath', 'newMergeRequestPath'],
+  inject: ['projectFullPath', 'ciConfigPath'],
   props: {
     ciFileContent: {
       type: String,
@@ -74,16 +71,6 @@ export default {
     },
   },
   methods: {
-    redirectToNewMergeRequest(sourceBranch) {
-      const url = mergeUrlParams(
-        {
-          [MR_SOURCE_BRANCH]: sourceBranch,
-          [MR_TARGET_BRANCH]: this.currentBranch,
-        },
-        this.newMergeRequestPath,
-      );
-      redirectTo(url);
-    },
     async onCommitSubmit({ message, targetBranch, openMergeRequest }) {
       this.isSaving = true;
 
@@ -112,12 +99,25 @@ export default {
 
         if (errors?.length) {
           this.$emit('showError', { type: COMMIT_FAILURE, reasons: errors });
-        } else if (openMergeRequest) {
-          this.redirectToNewMergeRequest(targetBranch);
         } else {
-          this.$emit('commit', { type: COMMIT_SUCCESS });
+          const commitBranch = targetBranch;
+          const params = openMergeRequest
+            ? {
+                type: COMMIT_SUCCESS_WITH_REDIRECT,
+                params: {
+                  sourceBranch: commitBranch,
+                  targetBranch: this.currentBranch,
+                },
+              }
+            : { type: COMMIT_SUCCESS };
+
+          this.$emit('commit', {
+            ...params,
+          });
+
           this.updateLastCommitBranch(targetBranch);
           this.updateCurrentBranch(targetBranch);
+
           if (this.currentBranch === targetBranch) {
             this.$emit('updateCommitSha');
           }

app/assets/javascripts/pipeline_editor/components/ui/pipeline_editor_messages.vue

@@ -5,6 +5,7 @@ import { __, s__ } from '~/locale';
 import {
   COMMIT_FAILURE,
   COMMIT_SUCCESS,
+  COMMIT_SUCCESS_WITH_REDIRECT,
   DEFAULT_FAILURE,
   DEFAULT_SUCCESS,
   LOAD_FAILURE_UNKNOWN,
@@ -21,14 +22,18 @@ export default {
     GlAlert,
     CodeSnippetAlert,
   },
-  errorTexts: {
+
+  errors: {
     [COMMIT_FAILURE]: s__('Pipelines|The GitLab CI configuration could not be updated.'),
     [DEFAULT_FAILURE]: __('Something went wrong on our end.'),
     [LOAD_FAILURE_UNKNOWN]: s__('Pipelines|The CI configuration was not loaded, please try again.'),
     [PIPELINE_FAILURE]: s__('Pipelines|There was a problem with loading the pipeline data.'),
   },
-  successTexts: {
+  success: {
     [COMMIT_SUCCESS]: __('Your changes have been successfully committed.'),
+    [COMMIT_SUCCESS_WITH_REDIRECT]: s__(
+      'Pipelines|Your changes have been successfully committed. Now redirecting to the new merge request page.',
+    ),
     [DEFAULT_SUCCESS]: __('Your action succeeded.'),
   },
   props: {
@@ -65,42 +70,20 @@ export default {
   },
   computed: {
     failure() {
-      switch (this.failureType) {
-        case LOAD_FAILURE_UNKNOWN:
-          return {
-            text: this.$options.errorTexts[LOAD_FAILURE_UNKNOWN],
-            variant: 'danger',
-          };
-        case COMMIT_FAILURE:
-          return {
-            text: this.$options.errorTexts[COMMIT_FAILURE],
-            variant: 'danger',
-          };
-        case PIPELINE_FAILURE:
-          return {
-            text: this.$options.errorTexts[PIPELINE_FAILURE],
-            variant: 'danger',
-          };
-        default:
-          return {
-            text: this.$options.errorTexts[DEFAULT_FAILURE],
-            variant: 'danger',
-          };
-      }
+      const { errors } = this.$options;
+
+      return {
+        text: errors[this.failureType] ?? errors[DEFAULT_FAILURE],
+        variant: 'danger',
+      };
     },
     success() {
-      switch (this.successType) {
-        case COMMIT_SUCCESS:
-          return {
-            text: this.$options.successTexts[COMMIT_SUCCESS],
-            variant: 'info',
-          };
-        default:
-          return {
-            text: this.$options.successTexts[DEFAULT_SUCCESS],
-            variant: 'info',
-          };
-      }
+      const { success } = this.$options;
+
+      return {
+        text: success[this.successType] ?? success[DEFAULT_SUCCESS],
+        variant: 'info',
+      };
     },
   },
   created() {

app/assets/javascripts/pipeline_editor/constants.js

@@ -20,6 +20,7 @@ export const EDITOR_APP_VALID_STATUSES = [
 
 export const COMMIT_FAILURE = 'COMMIT_FAILURE';
 export const COMMIT_SUCCESS = 'COMMIT_SUCCESS';
+export const COMMIT_SUCCESS_WITH_REDIRECT = 'COMMIT_SUCCESS_WITH_REDIRECT';
 
 export const DEFAULT_FAILURE = 'DEFAULT_FAILURE';
 export const DEFAULT_SUCCESS = 'DEFAULT_SUCCESS';

app/assets/javascripts/pipeline_editor/pipeline_editor_app.vue

 <script>
 import { GlLoadingIcon, GlModal } from '@gitlab/ui';
 import { fetchPolicies } from '~/lib/graphql';
-import { queryToObject } from '~/lib/utils/url_utility';
+import { mergeUrlParams, queryToObject, redirectTo } from '~/lib/utils/url_utility';
 import { __, s__ } from '~/locale';
 
 import { unwrapStagesWithNeeds } from '~/pipelines/components/unwrapping_utils';
@@ -11,6 +11,7 @@ import PipelineEditorEmptyState from './components/ui/pipeline_editor_empty_stat
 import PipelineEditorMessages from './components/ui/pipeline_editor_messages.vue';
 import {
   COMMIT_SHA_POLL_INTERVAL,
+  COMMIT_SUCCESS_WITH_REDIRECT,
   EDITOR_APP_STATUS_EMPTY,
   EDITOR_APP_STATUS_LOADING,
   EDITOR_APP_STATUS_LINT_UNAVAILABLE,
@@ -27,6 +28,9 @@ import getTemplate from './graphql/queries/get_starter_template.query.graphql';
 import getLatestCommitShaQuery from './graphql/queries/latest_commit_sha.query.graphql';
 import PipelineEditorHome from './pipeline_editor_home.vue';
 
+const MR_SOURCE_BRANCH = 'merge_request[source_branch]';
+const MR_TARGET_BRANCH = 'merge_request[target_branch]';
+
 export default {
   components: {
     ConfirmUnsavedChangesDialog,
@@ -36,14 +40,7 @@ export default {
     PipelineEditorHome,
     PipelineEditorMessages,
   },
-  inject: {
-    ciConfigPath: {
-      default: '',
-    },
-    projectFullPath: {
-      default: '',
-    },
-  },
+  inject: ['ciConfigPath', 'newMergeRequestPath', 'projectFullPath'],
   data() {
     return {
       ciConfigData: {},
@@ -57,7 +54,7 @@ export default {
       lastCommittedContent: '',
       shouldSkipStartScreen: false,
       showFailure: false,
-      showResetComfirmationModal: false,
+      showResetConfirmationModal: false,
       showStartScreen: false,
       showSuccess: false,
       starterTemplate: '',
@@ -271,17 +268,39 @@ export default {
     this.checkShouldSkipStartScreen();
   },
   methods: {
+    checkShouldSkipStartScreen() {
+      const params = queryToObject(window.location.search);
+      this.shouldSkipStartScreen = Boolean(params?.add_new_config_file);
+    },
+    confirmReset() {
+      if (this.hasUnsavedChanges) {
+        this.showResetConfirmationModal = true;
+      }
+    },
     hideFailure() {
       this.showFailure = false;
     },
     hideSuccess() {
       this.showSuccess = false;
     },
-    confirmReset() {
-      if (this.hasUnsavedChanges) {
-        this.showResetComfirmationModal = true;
+    loadTemplateFromURL() {
+      const templateName = queryToObject(window.location.search)?.template;
+
+      if (templateName) {
+        this.starterTemplateName = templateName;
+        this.setNewEmptyCiConfigFile();
       }
     },
+    redirectToNewMergeRequest(sourceBranch, targetBranch) {
+      const url = mergeUrlParams(
+        {
+          [MR_SOURCE_BRANCH]: sourceBranch,
+          [MR_TARGET_BRANCH]: targetBranch,
+        },
+        this.newMergeRequestPath,
+      );
+      redirectTo(url);
+    },
     async refetchContent() {
       this.$apollo.queries.initialCiFileContent.skip = false;
       await this.$apollo.queries.initialCiFileContent.refetch();
@@ -298,7 +317,7 @@ export default {
       this.successType = type;
     },
     resetContent() {
-      this.showResetComfirmationModal = false;
+      this.showResetConfirmationModal = false;
       this.currentCiFileContent = this.lastCommittedContent;
     },
     setAppStatus(appStatus) {
@@ -323,7 +342,7 @@ export default {
       this.isFetchingCommitSha = true;
       this.$apollo.queries.commitSha.refetch();
     },
-    updateOnCommit({ type }) {
+    async updateOnCommit({ type, params = {} }) {
       this.reportSuccess(type);
 
       if (this.isNewCiConfigFile) {
@@ -333,19 +352,17 @@ export default {
       // Keep track of the latest committed content to know
       // if the user has made changes to the file that are unsaved.
       this.lastCommittedContent = this.currentCiFileContent;
-    },
-    loadTemplateFromURL() {
-      const templateName = queryToObject(window.location.search)?.template;
 
-      if (templateName) {
-        this.starterTemplateName = templateName;
-        this.setNewEmptyCiConfigFile();
+      if (type === COMMIT_SUCCESS_WITH_REDIRECT) {
+        const { sourceBranch, targetBranch } = params;
+        // This force update does 2 things for us:
+        // 1. It make sure `hasUnsavedChanges` is updated so
+        // we don't show a modal when the user creates an MR
+        // 2. Ensure the commit success banner is visible.
+        await this.$forceUpdate();
+        this.redirectToNewMergeRequest(sourceBranch, targetBranch);
       }
     },
-    checkShouldSkipStartScreen() {
-      const params = queryToObject(window.location.search);
-      this.shouldSkipStartScreen = Boolean(params?.add_new_config_file);
-    },
   },
 };
 </script>
@@ -382,7 +399,7 @@ export default {
         @updateCommitSha="updateCommitSha"
       />
       <gl-modal
-        v-model="showResetComfirmationModal"
+        v-model="showResetConfirmationModal"
         modal-id="reset-content"
         :title="$options.i18n.resetModal.title"
         :action-cancel="$options.i18n.resetModal.actionCancel"

locale/gitlab.pot

@@ -26486,6 +26486,9 @@ msgstr ""
 msgid "Pipelines|Visualize"
 msgstr ""
 
+msgid "Pipelines|Your changes have been successfully committed. Now redirecting to the new merge request page."
+msgstr ""
+
 msgid "Pipelines|detached"
 msgstr ""
 

spec/features/projects/ci/editor_spec.rb

@@ -26,7 +26,7 @@ RSpec.describe 'Pipeline Editor', :js do
     expect(page).to have_content('Pipeline Editor')
   end
 
-  context 'branch switcher' do
+  describe 'Branch switcher' do
     def switch_to_branch(branch)
       find('[data-testid="branch-selector"]').click
 
@@ -64,7 +64,64 @@ RSpec.describe 'Pipeline Editor', :js do
     end
   end
 
-  context 'Editor content' do
+  describe 'Editor navigation' do
+    context 'when no change is made' do
+      it 'user can navigate away without a browser alert' do
+        expect(page).to have_content('Pipeline Editor')
+
+        click_link 'Pipelines'
+
+        expect(page).not_to have_content('Pipeline Editor')
+      end
+    end
+
+    context 'when a change is made' do
+      before do
+        click_button 'Collapse'
+
+        page.within('#source-editor-') do
+          find('textarea').send_keys '123'
+          # It takes some time after sending keys for the vue
+          # component to update
+          sleep 1
+        end
+      end
+
+      it 'user who tries to navigate away can cancel the action and keep their changes' do
+        click_link 'Pipelines'
+
+        page.driver.browser.switch_to.alert.dismiss
+
+        expect(page).to have_content('Pipeline Editor')
+
+        page.within('#source-editor-') do
+          expect(page).to have_content('Default Content123')
+        end
+      end
+
+      it 'user who tries to navigate away can confirm the action and discard their change' do
+        click_link 'Pipelines'
+
+        page.driver.browser.switch_to.alert.accept
+
+        expect(page).not_to have_content('Pipeline Editor')
+      end
+
+      it 'user who creates a MR is taken to the merge request page without warnings' do
+        expect(page).not_to have_content('New merge request')
+
+        find_field('Target Branch').set 'new_branch'
+        find_field('Start a new merge request with these changes').click
+
+        click_button 'Commit changes'
+
+        expect(page).not_to have_content('Pipeline Editor')
+        expect(page).to have_content('New merge request')
+      end
+    end
+  end
+
+  describe 'Editor content' do
     it 'user can reset their CI configuration' do
       click_button 'Collapse'
 

spec/frontend/pipeline_editor/components/commit/commit_section_spec.js

@@ -4,13 +4,13 @@ import { mount } from '@vue/test-utils';
 import Vue from 'vue';
 import createMockApollo from 'helpers/mock_apollo_helper';
 import waitForPromises from 'helpers/wait_for_promises';
-import { objectToQuery, redirectTo } from '~/lib/utils/url_utility';
 import CommitForm from '~/pipeline_editor/components/commit/commit_form.vue';
 import CommitSection from '~/pipeline_editor/components/commit/commit_section.vue';
 import {
   COMMIT_ACTION_CREATE,
   COMMIT_ACTION_UPDATE,
   COMMIT_SUCCESS,
+  COMMIT_SUCCESS_WITH_REDIRECT,
 } from '~/pipeline_editor/constants';
 import commitCreate from '~/pipeline_editor/graphql/mutations/commit_ci_file.mutation.graphql';
 import updatePipelineEtag from '~/pipeline_editor/graphql/mutations/client/update_pipeline_etag.mutation.graphql';
@@ -24,16 +24,8 @@ import {
   mockCommitMessage,
   mockDefaultBranch,
   mockProjectFullPath,
-  mockNewMergeRequestPath,
 } from '../../mock_data';
 
-jest.mock('~/lib/utils/url_utility', () => ({
-  redirectTo: jest.fn(),
-  refreshCurrentPage: jest.fn(),
-  objectToQuery: jest.requireActual('~/lib/utils/url_utility').objectToQuery,
-  mergeUrlParams: jest.requireActual('~/lib/utils/url_utility').mergeUrlParams,
-}));
-
 const mockVariables = {
   action: COMMIT_ACTION_UPDATE,
   projectPath: mockProjectFullPath,
@@ -47,7 +39,6 @@ const mockVariables = {
 const mockProvide = {
   ciConfigPath: mockCiConfigPath,
   projectFullPath: mockProjectFullPath,
-  newMergeRequestPath: mockNewMergeRequestPath,
 };
 
 describe('Pipeline Editor | Commit section', () => {
@@ -208,19 +199,21 @@ describe('Pipeline Editor | Commit section', () => {
 
     beforeEach(async () => {
       createComponentWithApollo();
+      mockMutateCommitData.mockResolvedValue(mockCommitCreateResponse);
       await submitCommit({
         branch: newBranch,
         openMergeRequest: true,
       });
     });
 
-    it('redirects to the merge request page with source and target branches', () => {
-      const branchesQuery = objectToQuery({
-        'merge_request[source_branch]': newBranch,
-        'merge_request[target_branch]': mockDefaultBranch,
-      });
-
-      expect(redirectTo).toHaveBeenCalledWith(`${mockNewMergeRequestPath}?${branchesQuery}`);
+    it('emits a commit event with the right type, sourceBranch and targetBranch', () => {
+      expect(wrapper.emitted('commit')).toBeTruthy();
+      expect(wrapper.emitted('commit')[0]).toMatchObject([
+        {
+          type: COMMIT_SUCCESS_WITH_REDIRECT,
+          params: { sourceBranch: newBranch, targetBranch: mockDefaultBranch },
+        },
+      ]);
     });
   });
 

spec/frontend/pipeline_editor/components/ui/pipeline_editor_messages_spec.js

@@ -8,6 +8,7 @@ import PipelineEditorMessages from '~/pipeline_editor/components/ui/pipeline_edi
 import {
   COMMIT_FAILURE,
   COMMIT_SUCCESS,
+  COMMIT_SUCCESS_WITH_REDIRECT,
   DEFAULT_FAILURE,
   DEFAULT_SUCCESS,
   LOAD_FAILURE_UNKNOWN,
@@ -34,7 +35,13 @@ describe('Pipeline Editor messages', () => {
     it('shows a message for successful commit type', () => {
       createComponent({ successType: COMMIT_SUCCESS, showSuccess: true });
 
-      expect(findAlert().text()).toBe(wrapper.vm.$options.successTexts[COMMIT_SUCCESS]);
+      expect(findAlert().text()).toBe(wrapper.vm.$options.success[COMMIT_SUCCESS]);
+    });
+
+    it('shows a message for successful commit with redirect type', () => {
+      createComponent({ successType: COMMIT_SUCCESS_WITH_REDIRECT, showSuccess: true });
+
+      expect(findAlert().text()).toBe(wrapper.vm.$options.success[COMMIT_SUCCESS_WITH_REDIRECT]);
     });
 
     it('does not show alert when there is a successType but visibility is off', () => {
@@ -46,7 +53,7 @@ describe('Pipeline Editor messages', () => {
     it('shows a success alert with default copy if `showSuccess` is true and the `successType` is not valid,', () => {
       createComponent({ successType: 'random', showSuccess: true });
 
-      expect(findAlert().text()).toBe(wrapper.vm.$options.successTexts[DEFAULT_SUCCESS]);
+      expect(findAlert().text()).toBe(wrapper.vm.$options.success[DEFAULT_SUCCESS]);
     });
 
     it('emit `hide-success` event when clicking on the dismiss button', async () => {
@@ -71,7 +78,7 @@ describe('Pipeline Editor messages', () => {
     `('shows a message for $message', ({ failureType, expectedFailureType }) => {
       createComponent({ failureType, showFailure: true });
 
-      expect(findAlert().text()).toBe(wrapper.vm.$options.errorTexts[expectedFailureType]);
+      expect(findAlert().text()).toBe(wrapper.vm.$options.errors[expectedFailureType]);
     });
 
     it('show failure reasons when there are some', () => {

spec/frontend/pipeline_editor/pipeline_editor_app_spec.js

@@ -5,6 +5,7 @@ import createMockApollo from 'helpers/mock_apollo_helper';
 import setWindowLocation from 'helpers/set_window_location_helper';
 import waitForPromises from 'helpers/wait_for_promises';
 
+import { objectToQuery, redirectTo } from '~/lib/utils/url_utility';
 import { resolvers } from '~/pipeline_editor/graphql/resolvers';
 import PipelineEditorTabs from '~/pipeline_editor/components/pipeline_editor_tabs.vue';
 import PipelineEditorEmptyState from '~/pipeline_editor/components/ui/pipeline_editor_empty_state.vue';
@@ -13,7 +14,11 @@ import PipelineEditorHeader from '~/pipeline_editor/components/header/pipeline_e
 import ValidationSegment, {
   i18n as validationSegmenti18n,
 } from '~/pipeline_editor/components/header/validation_segment.vue';
-import { COMMIT_SUCCESS, COMMIT_FAILURE } from '~/pipeline_editor/constants';
+import {
+  COMMIT_SUCCESS,
+  COMMIT_SUCCESS_WITH_REDIRECT,
+  COMMIT_FAILURE,
+} from '~/pipeline_editor/constants';
 import getBlobContent from '~/pipeline_editor/graphql/queries/blob_content.query.graphql';
 import getCiConfigData from '~/pipeline_editor/graphql/queries/ci_config.query.graphql';
 import getTemplate from '~/pipeline_editor/graphql/queries/get_starter_template.query.graphql';
@@ -35,15 +40,22 @@ import {
   mockDefaultBranch,
   mockEmptyCommitShaResults,
   mockNewCommitShaResults,
+  mockNewMergeRequestPath,
   mockProjectFullPath,
 } from './mock_data';
 
+jest.mock('~/lib/utils/url_utility', () => ({
+  ...jest.requireActual('~/lib/utils/url_utility'),
+  redirectTo: jest.fn(),
+}));
+
 const localVue = createLocalVue();
 localVue.use(VueApollo);
 
 const mockProvide = {
   ciConfigPath: mockCiConfigPath,
   defaultBranch: mockDefaultBranch,
+  newMergeRequestPath: mockNewMergeRequestPath,
   projectFullPath: mockProjectFullPath,
 };
 
@@ -311,6 +323,28 @@ describe('Pipeline editor app component', () => {
         });
       });
 
+      describe('when the commit succeeds with a redirect', () => {
+        const newBranch = 'new-branch';
+
+        beforeEach(async () => {
+          await createComponentWithApollo({ stubs: { PipelineEditorMessages } });
+
+          findEditorHome().vm.$emit('commit', {
+            type: COMMIT_SUCCESS_WITH_REDIRECT,
+            params: { sourceBranch: newBranch, targetBranch: mockDefaultBranch },
+          });
+        });
+
+        it('redirects to the merge request page with source and target branches', () => {
+          const branchesQuery = objectToQuery({
+            'merge_request[source_branch]': newBranch,
+            'merge_request[target_branch]': mockDefaultBranch,
+          });
+
+          expect(redirectTo).toHaveBeenCalledWith(`${mockNewMergeRequestPath}?${branchesQuery}`);
+        });
+      });
+
       describe('and the commit mutation fails', () => {
         const commitFailedReasons = ['Commit failed'];