From 0898efbf85d4981553f0e661221d0aa05b0a6b89 Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Tue, 30 Sep 2025 18:05:58 +0200 Subject: [PATCH 1/5] Recreate default success message after commiting change --- .../pages/projects/blob/show/index.js | 2 + .../local_storage_alert/constants.js | 1 + .../save_alert_to_local_storage.js | 8 ++ .../show_alert_from_local_storage.js | 19 +++++ .../repository/pages/blob_edit_header.vue | 35 ++++++++- locale/gitlab.pot | 3 + .../source_editor_toolbar_spec.rb | 8 +- spec/features/projects/blobs/edit_spec.rb | 1 - .../projects/files/user_edits_files_spec.rb | 1 - .../local_storage_alert/constants_spec.js | 7 ++ .../save_alert_to_local_storage_spec.js | 48 ++++++++++++ .../show_alert_from_local_storage_spec.js | 77 +++++++++++++++++++ .../repository/pages/blob_edit_header_spec.js | 74 +++++++++++++++++- 13 files changed, 272 insertions(+), 12 deletions(-) create mode 100644 app/assets/javascripts/repository/local_storage_alert/constants.js create mode 100644 app/assets/javascripts/repository/local_storage_alert/save_alert_to_local_storage.js create mode 100644 app/assets/javascripts/repository/local_storage_alert/show_alert_from_local_storage.js create mode 100644 spec/frontend/repository/local_storage_alert/constants_spec.js create mode 100644 spec/frontend/repository/local_storage_alert/save_alert_to_local_storage_spec.js create mode 100644 spec/frontend/repository/local_storage_alert/show_alert_from_local_storage_spec.js diff --git a/app/assets/javascripts/pages/projects/blob/show/index.js b/app/assets/javascripts/pages/projects/blob/show/index.js index 3a8375fbee47fa..6e245486182d76 100644 --- a/app/assets/javascripts/pages/projects/blob/show/index.js +++ b/app/assets/javascripts/pages/projects/blob/show/index.js @@ -29,6 +29,7 @@ import initFileTreeBrowser from '~/repository/file_tree_browser'; import LastCommit from '~/repository/components/last_commit.vue'; import projectPathQuery from '~/repository/queries/project_path.query.graphql'; import refsQuery from '~/repository/queries/ref.query.graphql'; +import { showAlertFromLocalStorage } from '~/repository/local_storage_alert/show_alert_from_local_storage'; import PerformancePlugin from '~/performance/vue_performance_plugin'; @@ -74,6 +75,7 @@ const initLastCommitApp = (router) => { initAmbiguousRefModal(); initFindFileShortcut(); +showAlertFromLocalStorage(); if (viewBlobEl) { const { diff --git a/app/assets/javascripts/repository/local_storage_alert/constants.js b/app/assets/javascripts/repository/local_storage_alert/constants.js new file mode 100644 index 00000000000000..d54f883a3c13f2 --- /dev/null +++ b/app/assets/javascripts/repository/local_storage_alert/constants.js @@ -0,0 +1 @@ +export const LOCAL_STORAGE_ALERT_KEY = 'repository_alert'; diff --git a/app/assets/javascripts/repository/local_storage_alert/save_alert_to_local_storage.js b/app/assets/javascripts/repository/local_storage_alert/save_alert_to_local_storage.js new file mode 100644 index 00000000000000..ca7c627459a5a3 --- /dev/null +++ b/app/assets/javascripts/repository/local_storage_alert/save_alert_to_local_storage.js @@ -0,0 +1,8 @@ +import AccessorUtilities from '~/lib/utils/accessor'; +import { LOCAL_STORAGE_ALERT_KEY } from './constants'; + +export const saveAlertToLocalStorage = (alertOptions) => { + if (AccessorUtilities.canUseLocalStorage()) { + localStorage.setItem(LOCAL_STORAGE_ALERT_KEY, JSON.stringify(alertOptions)); + } +}; diff --git a/app/assets/javascripts/repository/local_storage_alert/show_alert_from_local_storage.js b/app/assets/javascripts/repository/local_storage_alert/show_alert_from_local_storage.js new file mode 100644 index 00000000000000..43a4ab881821f0 --- /dev/null +++ b/app/assets/javascripts/repository/local_storage_alert/show_alert_from_local_storage.js @@ -0,0 +1,19 @@ +import AccessorUtilities from '~/lib/utils/accessor'; +import * as Sentry from '~/sentry/sentry_browser_wrapper'; +import { LOCAL_STORAGE_ALERT_KEY } from './constants'; + +export const showAlertFromLocalStorage = async () => { + if (AccessorUtilities.canUseLocalStorage()) { + const alertOptions = localStorage.getItem(LOCAL_STORAGE_ALERT_KEY); + + if (alertOptions) { + try { + const { createAlert } = await import('~/alert'); + createAlert(JSON.parse(alertOptions)); + } catch (error) { + Sentry.captureException(error); + } + } + localStorage.removeItem(LOCAL_STORAGE_ALERT_KEY); + } +}; diff --git a/app/assets/javascripts/repository/pages/blob_edit_header.vue b/app/assets/javascripts/repository/pages/blob_edit_header.vue index 6ac28c7d339581..cf97266cacd0eb 100644 --- a/app/assets/javascripts/repository/pages/blob_edit_header.vue +++ b/app/assets/javascripts/repository/pages/blob_edit_header.vue @@ -7,7 +7,9 @@ import PageHeading from '~/vue_shared/components/page_heading.vue'; import CommitChangesModal from '~/repository/components/commit_changes_modal.vue'; import { getParameterByName, visitUrl, joinPaths, mergeUrlParams } from '~/lib/utils/url_utility'; import { buildApiUrl } from '~/api/api_utils'; +import { VARIANT_SUCCESS } from '~/alert'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; +import { saveAlertToLocalStorage } from '../local_storage_alert/save_alert_to_local_storage'; import getRefMixin from '../mixins/get_ref'; const MR_SOURCE_BRANCH = 'merge_request[source_branch]'; @@ -70,6 +72,25 @@ export default { ? __('An error occurred editing the blob') : __('An error occurred creating the blob'); }, + successMessageForAlert() { + return (isNewBranch, createMergeRequestNotChosen) => { + let message = __( + 'Your %{changesLinkStart}changes%{changesLinkEnd} have been committed successfully.', + ); + + if (isNewBranch && createMergeRequestNotChosen) { + // Use canPushToBranch to determine if user is working on a fork + const mrMessage = this.canPushToBranch + ? __('You can now submit a merge request to get this change into the original branch.') + : __( + 'You can now submit a merge request to get this change into the original project.', + ); + message += ` ${mrMessage}`; + } + + return message; + }; + }, }, methods: { handleCancelButtonClick() { @@ -218,7 +239,19 @@ export default { ); visitUrl(mrUrl); } else { - visitUrl(this.getUpdatePath(responseData.branch, responseData.file_path)); + const successPath = this.getUpdatePath(responseData.branch, responseData.file_path); + const isNewBranch = this.originalBranch !== responseData.branch; + const createMergeRequestNotChosen = !formData.create_merge_request; + + const message = this.successMessageForAlert(isNewBranch, createMergeRequestNotChosen); + + saveAlertToLocalStorage({ + message, + messageLinks: { changesLink: successPath }, + variant: VARIANT_SUCCESS, + }); + + visitUrl(successPath); } }, handleControllerSuccess(responseData) { diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 312ec5c5569ff6..68caded2396361 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -76397,6 +76397,9 @@ msgstr "" msgid "YouTube" msgstr "" +msgid "Your %{changesLinkStart}changes%{changesLinkEnd} have been committed successfully." +msgstr "" + msgid "Your %{changes_link} have been committed successfully." msgstr "" diff --git a/qa/qa/specs/features/browser_ui/3_create/source_editor/source_editor_toolbar_spec.rb b/qa/qa/specs/features/browser_ui/3_create/source_editor/source_editor_toolbar_spec.rb index 858e063309bee5..0aab3aeda2953a 100644 --- a/qa/qa/specs/features/browser_ui/3_create/source_editor/source_editor_toolbar_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/source_editor/source_editor_toolbar_spec.rb @@ -1,9 +1,7 @@ # frozen_string_literal: true module QA - RSpec.describe 'Create', feature_category: :source_code_management, feature_flag: { - name: :blob_edit_refactor - } do + RSpec.describe 'Create', feature_category: :source_code_management do describe 'Source editor toolbar preview' do let(:project) { create(:project, :with_readme, name: 'empty-project-with-md') } let(:edited_readme_content) { 'Here is the edited content.' } @@ -14,10 +12,6 @@ module QA it 'can preview markdown side-by-side while editing', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/367749' do - # Skip test when blob_edit_refactor feature flag is enabled as it is WIP - # https://gitlab.com/gitlab-org/gitlab/-/issues/509968 - skip 'blob_edit_refactor feature flag is WIP' if Runtime::Feature.enabled?(:blob_edit_refactor) - project.visit! Page::Project::Show.perform do |project| project.click_file('README.md') diff --git a/spec/features/projects/blobs/edit_spec.rb b/spec/features/projects/blobs/edit_spec.rb index 38b8aa46aaf519..051f6f8ab86cee 100644 --- a/spec/features/projects/blobs/edit_spec.rb +++ b/spec/features/projects/blobs/edit_spec.rb @@ -115,7 +115,6 @@ def has_toolbar_buttons context 'from blob file path' do before do - stub_feature_flags(blob_edit_refactor: false) visit project_blob_path(project, tree_join(branch, file_path)) end diff --git a/spec/features/projects/files/user_edits_files_spec.rb b/spec/features/projects/files/user_edits_files_spec.rb index 5735be77460939..f0b06d7f622b17 100644 --- a/spec/features/projects/files/user_edits_files_spec.rb +++ b/spec/features/projects/files/user_edits_files_spec.rb @@ -48,7 +48,6 @@ context 'when an user has write access', :js do before do - stub_feature_flags(blob_edit_refactor: false) project.add_maintainer(user) visit(project_tree_path_root_ref) wait_for_requests diff --git a/spec/frontend/repository/local_storage_alert/constants_spec.js b/spec/frontend/repository/local_storage_alert/constants_spec.js new file mode 100644 index 00000000000000..cd895c741e78a9 --- /dev/null +++ b/spec/frontend/repository/local_storage_alert/constants_spec.js @@ -0,0 +1,7 @@ +import { LOCAL_STORAGE_ALERT_KEY } from '~/repository/local_storage_alert/constants'; + +describe('LOCAL_STORAGE_ALERT_KEY', () => { + it('exports the correct localStorage key', () => { + expect(LOCAL_STORAGE_ALERT_KEY).toBe('repository_alert'); + }); +}); diff --git a/spec/frontend/repository/local_storage_alert/save_alert_to_local_storage_spec.js b/spec/frontend/repository/local_storage_alert/save_alert_to_local_storage_spec.js new file mode 100644 index 00000000000000..2d777cb43d3830 --- /dev/null +++ b/spec/frontend/repository/local_storage_alert/save_alert_to_local_storage_spec.js @@ -0,0 +1,48 @@ +import AccessorUtilities from '~/lib/utils/accessor'; +import { saveAlertToLocalStorage } from '~/repository/local_storage_alert/save_alert_to_local_storage'; +import { LOCAL_STORAGE_ALERT_KEY } from '~/repository/local_storage_alert/constants'; +import { useLocalStorageSpy } from 'helpers/local_storage_helper'; + +const mockAlert = { message: 'Message!' }; + +describe('saveAlertToLocalStorage', () => { + useLocalStorageSpy(); + + beforeEach(() => { + jest.spyOn(AccessorUtilities, 'canUseLocalStorage').mockReturnValue(true); + }); + + it('saves message to local storage', () => { + saveAlertToLocalStorage(mockAlert); + + expect(localStorage.setItem).toHaveBeenCalledTimes(1); + expect(localStorage.setItem).toHaveBeenCalledWith( + LOCAL_STORAGE_ALERT_KEY, + JSON.stringify(mockAlert), + ); + }); + + it('does not save to local storage when localStorage is not available', () => { + jest.spyOn(AccessorUtilities, 'canUseLocalStorage').mockReturnValue(false); + + saveAlertToLocalStorage(mockAlert); + + expect(localStorage.setItem).not.toHaveBeenCalled(); + }); + + it('saves complex alert options to local storage', () => { + const complexAlert = { + message: 'Your changes have been committed successfully.', + variant: 'success', + renderMessageHTML: true, + }; + + saveAlertToLocalStorage(complexAlert); + + expect(localStorage.setItem).toHaveBeenCalledTimes(1); + expect(localStorage.setItem).toHaveBeenCalledWith( + LOCAL_STORAGE_ALERT_KEY, + JSON.stringify(complexAlert), + ); + }); +}); diff --git a/spec/frontend/repository/local_storage_alert/show_alert_from_local_storage_spec.js b/spec/frontend/repository/local_storage_alert/show_alert_from_local_storage_spec.js new file mode 100644 index 00000000000000..010bfcd71de6f3 --- /dev/null +++ b/spec/frontend/repository/local_storage_alert/show_alert_from_local_storage_spec.js @@ -0,0 +1,77 @@ +import AccessorUtilities from '~/lib/utils/accessor'; +import * as Sentry from '~/sentry/sentry_browser_wrapper'; +import { showAlertFromLocalStorage } from '~/repository/local_storage_alert/show_alert_from_local_storage'; +import { LOCAL_STORAGE_ALERT_KEY } from '~/repository/local_storage_alert/constants'; +import { useLocalStorageSpy } from 'helpers/local_storage_helper'; +import { createAlert } from '~/alert'; + +jest.mock('~/alert'); +jest.mock('~/sentry/sentry_browser_wrapper'); + +describe('showAlertFromLocalStorage', () => { + useLocalStorageSpy(); + + beforeEach(() => { + jest.spyOn(AccessorUtilities, 'canUseLocalStorage').mockReturnValue(true); + }); + + it('retrieves alert options from local storage and displays them', async () => { + const complexAlert = { + message: 'Your changes have been committed successfully.', + variant: 'success', + renderMessageHTML: true, + }; + + localStorage.getItem.mockReturnValueOnce(JSON.stringify(complexAlert)); + + await showAlertFromLocalStorage(); + + expect(createAlert).toHaveBeenCalledTimes(1); + expect(createAlert).toHaveBeenCalledWith(complexAlert); + + expect(localStorage.removeItem).toHaveBeenCalledTimes(1); + expect(localStorage.removeItem).toHaveBeenCalledWith(LOCAL_STORAGE_ALERT_KEY); + }); + + it.each(['not a json string', null])('does not fail when stored message is %o', async (item) => { + localStorage.getItem.mockReturnValueOnce(item); + + await showAlertFromLocalStorage(); + + expect(createAlert).not.toHaveBeenCalled(); + + expect(localStorage.removeItem).toHaveBeenCalledTimes(1); + expect(localStorage.removeItem).toHaveBeenCalledWith(LOCAL_STORAGE_ALERT_KEY); + }); + + it('does not show alert when localStorage is not available', async () => { + jest.spyOn(AccessorUtilities, 'canUseLocalStorage').mockReturnValue(false); + + await showAlertFromLocalStorage(); + + expect(localStorage.getItem).not.toHaveBeenCalled(); + expect(createAlert).not.toHaveBeenCalled(); + expect(localStorage.removeItem).not.toHaveBeenCalled(); + }); + + it('removes item from localStorage even when no alert is stored', async () => { + localStorage.getItem.mockReturnValueOnce(null); + + await showAlertFromLocalStorage(); + + expect(createAlert).not.toHaveBeenCalled(); + expect(localStorage.removeItem).toHaveBeenCalledTimes(1); + expect(localStorage.removeItem).toHaveBeenCalledWith(LOCAL_STORAGE_ALERT_KEY); + }); + + it('handles JSON parsing errors gracefully and logs error to Sentry', async () => { + localStorage.getItem.mockReturnValueOnce('invalid json {'); + + await showAlertFromLocalStorage(); + + expect(createAlert).not.toHaveBeenCalled(); + expect(localStorage.removeItem).toHaveBeenCalledTimes(1); + expect(localStorage.removeItem).toHaveBeenCalledWith(LOCAL_STORAGE_ALERT_KEY); + expect(Sentry.captureException).toHaveBeenCalledWith(expect.any(Error)); + }); +}); diff --git a/spec/frontend/repository/pages/blob_edit_header_spec.js b/spec/frontend/repository/pages/blob_edit_header_spec.js index c2f8b1a5d8537d..3e158f6d5b733c 100644 --- a/spec/frontend/repository/pages/blob_edit_header_spec.js +++ b/spec/frontend/repository/pages/blob_edit_header_spec.js @@ -11,11 +11,13 @@ import { import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import CommitChangesModal from '~/repository/components/commit_changes_modal.vue'; import BlobEditHeader from '~/repository/pages/blob_edit_header.vue'; +import { saveAlertToLocalStorage } from '~/repository/local_storage_alert/save_alert_to_local_storage'; import PageHeading from '~/vue_shared/components/page_heading.vue'; import { stubComponent } from 'helpers/stub_component'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; jest.mock('~/alert'); +jest.mock('~/repository/local_storage_alert/save_alert_to_local_storage'); jest.mock('lodash/uniqueId', () => { return jest.fn((input) => `${input}1`); }); @@ -36,7 +38,7 @@ describe('BlobEditHeader', () => { }, }; - const createWrapper = ({ action = 'update', glFeatures = {} } = {}) => { + const createWrapper = ({ action = 'update', glFeatures = {}, provided = {} } = {}) => { return shallowMountExtended(BlobEditHeader, { provide: { action, @@ -54,6 +56,7 @@ describe('BlobEditHeader', () => { projectId: 123, projectPath: 'gitlab-org/gitlab', newMergeRequestPath: 'merge_request/new/123', + ...provided, glFeatures: { blobEditRefactor: true, ...glFeatures }, }, mixins: [glFeatureFlagMixin()], @@ -138,7 +141,7 @@ describe('BlobEditHeader', () => { ); }); - it('on submit, redirects to the updated file', async () => { + it('on submit, saves success message to localStorage and redirects to the updated file', async () => { // First click the commit button to open the modal and set up the file content clickCommitChangesButton(); mock.onPut().replyOnce(HTTP_STATUS_OK, { @@ -147,6 +150,15 @@ describe('BlobEditHeader', () => { }); await submitForm(); + expect(saveAlertToLocalStorage).toHaveBeenCalledWith({ + message: + 'Your %{changesLinkStart}changes%{changesLinkEnd} have been committed successfully. You can now submit a merge request to get this change into the original branch.', + messageLinks: { + changesLink: 'http://test.host/gitlab-org/gitlab/-/blob/feature/test.js', + }, + variant: 'success', + }); + expect(mock.history.put).toHaveLength(1); expect(mock.history.put[0].url).toBe('/api/v4/projects/123/repository/files/test.js'); const putData = JSON.parse(mock.history.put[0].data); @@ -156,6 +168,64 @@ describe('BlobEditHeader', () => { ); }); + it('on submit to same branch, saves shorter success message to localStorage', async () => { + // First click the commit button to open the modal and set up the file content + clickCommitChangesButton(); + mock.onPut().replyOnce(HTTP_STATUS_OK, { + branch: 'main', // Same as originalBranch + file_path: 'test.js', + }); + + const formData = new FormData(); + formData.append('commit_message', 'Test commit'); + formData.append('branch_name', 'main'); + formData.append('original_branch', 'main'); + + findCommitChangesModal().vm.$emit('submit-form', formData); + await axios.waitForAll(); + + expect(saveAlertToLocalStorage).toHaveBeenCalledWith({ + message: + 'Your %{changesLinkStart}changes%{changesLinkEnd} have been committed successfully.', + messageLinks: { + changesLink: 'http://test.host/gitlab-org/gitlab/-/blob/main/test.js', + }, + variant: 'success', + }); + + expect(visitUrlSpy).toHaveBeenCalledWith( + 'http://test.host/gitlab-org/gitlab/-/blob/main/test.js', + ); + }); + + it('on submit to new branch without push access, saves success message with "original project" text', async () => { + // Create wrapper with canPushToBranch: false to simulate fork scenario + wrapper = createWrapper({ + glFeatures: { blobEditRefactor: true }, + provided: { canPushToBranch: false }, + }); + + clickCommitChangesButton(); + mock.onPut().replyOnce(HTTP_STATUS_OK, { + branch: 'feature', + file_path: 'test.js', + }); + await submitForm(); + + expect(saveAlertToLocalStorage).toHaveBeenCalledWith({ + message: + 'Your %{changesLinkStart}changes%{changesLinkEnd} have been committed successfully. You can now submit a merge request to get this change into the original project.', + messageLinks: { + changesLink: 'http://test.host/gitlab-org/gitlab/-/blob/feature/test.js', + }, + variant: 'success', + }); + + expect(visitUrlSpy).toHaveBeenCalledWith( + 'http://test.host/gitlab-org/gitlab/-/blob/feature/test.js', + ); + }); + describe('error handling', () => { const errorMessage = 'Custom error message'; -- GitLab From 2917fa6b5d3c787df62edf7c43e1e35cc4151fa7 Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Thu, 2 Oct 2025 13:09:32 +0200 Subject: [PATCH 2/5] Extract form data utils --- .../repository/pages/blob_edit_header.vue | 47 ++++--- .../repository/utils/edit_form_data_utils.js | 81 ++++++++++++ .../utils/edit_form_data_utils_spec.js | 116 ++++++++++++++++++ 3 files changed, 219 insertions(+), 25 deletions(-) create mode 100644 app/assets/javascripts/repository/utils/edit_form_data_utils.js create mode 100644 spec/frontend/repository/utils/edit_form_data_utils_spec.js diff --git a/app/assets/javascripts/repository/pages/blob_edit_header.vue b/app/assets/javascripts/repository/pages/blob_edit_header.vue index cf97266cacd0eb..c4bdcee63b5849 100644 --- a/app/assets/javascripts/repository/pages/blob_edit_header.vue +++ b/app/assets/javascripts/repository/pages/blob_edit_header.vue @@ -11,6 +11,11 @@ import { VARIANT_SUCCESS } from '~/alert'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { saveAlertToLocalStorage } from '../local_storage_alert/save_alert_to_local_storage'; import getRefMixin from '../mixins/get_ref'; +import { + prepareEditFormData, + prepareControllerFormData, + prepareCreateFormData, +} from '../utils/edit_form_data_utils'; const MR_SOURCE_BRANCH = 'merge_request[source_branch]'; @@ -108,22 +113,6 @@ export default { this.originalFilePath = this.editor.getOriginalFilePath(); this.$refs[this.updateModalId].show(); }, - prepareFormData(formData) { - formData.append('file', this.fileContent); - formData.append('file_path', this.filePath); - formData.append('last_commit_sha', this.lastCommitSha); - formData.append('from_merge_request_iid', this.fromMergeRequestIid); - - return Object.fromEntries(formData); - }, - prepareControllerFormData(formData) { - formData.append('file', this.fileContent); - formData.append('file_path', this.filePath); - formData.append('last_commit_sha', this.lastCommitSha); - formData.append('from_merge_request_iid', this.fromMergeRequestIid); - - return Object.fromEntries(formData); - }, handleError(message, errorCode = null) { if (!message) return; // Returns generic '403 Forbidden' error message @@ -141,7 +130,19 @@ export default { } }, async handleEditFormSubmit(formData) { - const originalFormData = this.prepareFormData(formData); + const originalFormData = this.glFeatures.blobEditRefactor + ? prepareEditFormData(formData, { + fileContent: this.fileContent, + filePath: this.filePath, + lastCommitSha: this.lastCommitSha, + fromMergeRequestIid: this.fromMergeRequestIid, + }) + : prepareControllerFormData(formData, { + fileContent: this.fileContent, + filePath: this.filePath, + lastCommitSha: this.lastCommitSha, + fromMergeRequestIid: this.fromMergeRequestIid, + }); try { const response = this.glFeatures.blobEditRefactor @@ -168,14 +169,10 @@ export default { } }, async handleCreateFormSubmit(formData) { - formData.append('file_name', this.filePath); - formData.append('content', this.fileContent); - - // Object.fromEntries is used here to handle potential line ending mutations in `FormData`. - // `FormData` uses the "multipart/form-data" format (RFC 2388), which follows MIME data stream rules (RFC 2046). - // These specifications require line breaks to be represented as CRLF sequences in the canonical form. - // See https://stackoverflow.com/questions/69835705/formdata-textarea-puts-r-carriage-return-when-sent-with-post for more details. - const originalFormData = Object.fromEntries(formData); + const originalFormData = prepareCreateFormData(formData, { + filePath: this.filePath, + fileContent: this.fileContent, + }); try { const response = await axios({ diff --git a/app/assets/javascripts/repository/utils/edit_form_data_utils.js b/app/assets/javascripts/repository/utils/edit_form_data_utils.js new file mode 100644 index 00000000000000..4a358aa84d62eb --- /dev/null +++ b/app/assets/javascripts/repository/utils/edit_form_data_utils.js @@ -0,0 +1,81 @@ +/** + * Prepares form data for blob editing operations by appending required fields + * and converting FormData to a plain object to handle potential line ending mutations. + * + * Object.fromEntries is used here to handle potential line ending mutations in FormData. + * FormData uses the "multipart/form-data" format (RFC 2388), which follows MIME data + * stream rules (RFC 2046). These specifications require line breaks to be represented + * as CRLF sequences in the canonical form. + * See https://stackoverflow.com/questions/69835705/formdata-textarea-puts-r-carriage-return-when-sent-with-post for more details. + * + * @param {FormData} formData - The original form data + * @param {Object} params - Parameters object + * @param {string} params.fileContent - The file content to be committed + * @param {string} params.filePath - The path of the file being edited + * @param {string} params.lastCommitSha - The SHA of the last commit + * @param {string} params.fromMergeRequestIid - The merge request IID if editing from an MR + * @returns {Object} Plain object with form data entries + */ +export const prepareEditFormData = ( + formData, + { fileContent, filePath, lastCommitSha, fromMergeRequestIid }, +) => { + formData.append('file', fileContent); + formData.append('file_path', filePath); + formData.append('last_commit_sha', lastCommitSha); + formData.append('from_merge_request_iid', fromMergeRequestIid); + + return Object.fromEntries(formData); +}; + +/** + * Prepares form data for controller-based blob editing operations. + * This is used when the blobEditRefactor feature flag is disabled. + * + * Object.fromEntries is used here to handle potential line ending mutations in FormData. + * FormData uses the "multipart/form-data" format (RFC 2388), which follows MIME data + * stream rules (RFC 2046). These specifications require line breaks to be represented + * as CRLF sequences in the canonical form. + * See https://stackoverflow.com/questions/69835705/formdata-textarea-puts-r-carriage-return-when-sent-with-post for more details. + * + * @param {FormData} formData - The original form data + * @param {Object} params - Parameters object + * @param {string} params.fileContent - The file content to be committed + * @param {string} params.filePath - The path of the file being edited + * @param {string} params.lastCommitSha - The SHA of the last commit + * @param {string} params.fromMergeRequestIid - The merge request IID if editing from an MR + * @returns {Object} Plain object with form data entries + */ +export const prepareControllerFormData = ( + formData, + { fileContent, filePath, lastCommitSha, fromMergeRequestIid }, +) => { + formData.append('file', fileContent); + formData.append('file_path', filePath); + formData.append('last_commit_sha', lastCommitSha); + formData.append('from_merge_request_iid', fromMergeRequestIid); + + return Object.fromEntries(formData); +}; + +/** + * Prepares form data for creating new blobs by appending file name and content. + * + * Object.fromEntries is used here to handle potential line ending mutations in FormData. + * FormData uses the "multipart/form-data" format (RFC 2388), which follows MIME data + * stream rules (RFC 2046). These specifications require line breaks to be represented + * as CRLF sequences in the canonical form. + * See https://stackoverflow.com/questions/69835705/formdata-textarea-puts-r-carriage-return-when-sent-with-post for more details. + * + * @param {FormData} formData - The original form data + * @param {Object} params - Parameters object + * @param {string} params.filePath - The path/name of the new file + * @param {string} params.fileContent - The content of the new file + * @returns {Object} Plain object with form data entries + */ +export const prepareCreateFormData = (formData, { filePath, fileContent }) => { + formData.append('file_name', filePath); + formData.append('content', fileContent); + + return Object.fromEntries(formData); +}; diff --git a/spec/frontend/repository/utils/edit_form_data_utils_spec.js b/spec/frontend/repository/utils/edit_form_data_utils_spec.js new file mode 100644 index 00000000000000..0c8e4c2426d5e3 --- /dev/null +++ b/spec/frontend/repository/utils/edit_form_data_utils_spec.js @@ -0,0 +1,116 @@ +import { + prepareEditFormData, + prepareControllerFormData, + prepareCreateFormData, +} from '~/repository/utils/edit_form_data_utils'; + +describe('edit_form_data_utils', () => { + let formData; + + beforeEach(() => { + formData = new FormData(); + }); + + describe('prepareEditFormData', () => { + const params = { + fileContent: 'console.log("Hello World");', + filePath: 'src/test.js', + lastCommitSha: 'abc123def456', + fromMergeRequestIid: '42', + }; + + it('appends all required fields to FormData and returns plain object', () => { + const result = prepareEditFormData(formData, params); + + expect(result).toEqual({ + file: params.fileContent, + file_path: params.filePath, + last_commit_sha: params.lastCommitSha, + from_merge_request_iid: params.fromMergeRequestIid, + }); + }); + }); + + describe('prepareControllerFormData', () => { + const params = { + fileContent: 'const x = 1;', + filePath: 'lib/utils.js', + lastCommitSha: 'def456abc789', + fromMergeRequestIid: '123', + }; + + it('appends all required fields to FormData and returns plain object', () => { + const result = prepareControllerFormData(formData, params); + + expect(result).toEqual({ + file: params.fileContent, + file_path: params.filePath, + last_commit_sha: params.lastCommitSha, + from_merge_request_iid: params.fromMergeRequestIid, + }); + }); + + it('handles null values gracefully', () => { + const nullParams = { + fileContent: params.fileContent, + filePath: params.filePath, + lastCommitSha: null, + fromMergeRequestIid: null, + }; + const result = prepareControllerFormData(formData, nullParams); + + expect(result).toEqual({ + file: params.fileContent, + file_path: params.filePath, + last_commit_sha: 'null', + from_merge_request_iid: 'null', + }); + }); + }); + + describe('prepareCreateFormData', () => { + const params = { + filePath: 'new-file.md', + fileContent: '# New File\n\nThis is a new file.', + }; + + it('appends file_name and content to FormData and returns plain object', () => { + const result = prepareCreateFormData(formData, params); + + expect(result).toEqual({ + file_name: params.filePath, + content: params.fileContent, + }); + }); + + it('handles empty file content', () => { + const emptyParams = { ...params, fileContent: '' }; + const result = prepareCreateFormData(formData, emptyParams); + + expect(result).toEqual({ + file_name: params.filePath, + content: '', + }); + }); + + it('handles file paths with special characters', () => { + const specialParams = { + ...params, + filePath: 'docs/special file (with spaces & symbols).txt', + }; + const result = prepareCreateFormData(formData, specialParams); + + expect(result.file_name).toBe(specialParams.filePath); + }); + + it('handles multiline content with different line endings', () => { + const multilineParams = { + ...params, + fileContent: 'Line 1\nLine 2\r\nLine 3\rLine 4', + }; + const result = prepareCreateFormData(formData, multilineParams); + + expect(result.content).toBe(multilineParams.fileContent); + }); + }); +}); -- GitLab From 72bdad4559e0472e313f6b1ba82a39609f51619a Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Thu, 2 Oct 2025 15:05:03 +0200 Subject: [PATCH 3/5] Use correct alert variant --- .../javascripts/repository/pages/blob_edit_header.vue | 4 ++-- spec/frontend/repository/pages/blob_edit_header_spec.js | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/repository/pages/blob_edit_header.vue b/app/assets/javascripts/repository/pages/blob_edit_header.vue index c4bdcee63b5849..a861a8ab2737c0 100644 --- a/app/assets/javascripts/repository/pages/blob_edit_header.vue +++ b/app/assets/javascripts/repository/pages/blob_edit_header.vue @@ -7,7 +7,7 @@ import PageHeading from '~/vue_shared/components/page_heading.vue'; import CommitChangesModal from '~/repository/components/commit_changes_modal.vue'; import { getParameterByName, visitUrl, joinPaths, mergeUrlParams } from '~/lib/utils/url_utility'; import { buildApiUrl } from '~/api/api_utils'; -import { VARIANT_SUCCESS } from '~/alert'; +import { VARIANT_INFO } from '~/alert'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { saveAlertToLocalStorage } from '../local_storage_alert/save_alert_to_local_storage'; import getRefMixin from '../mixins/get_ref'; @@ -245,7 +245,7 @@ export default { saveAlertToLocalStorage({ message, messageLinks: { changesLink: successPath }, - variant: VARIANT_SUCCESS, + variant: VARIANT_INFO, }); visitUrl(successPath); diff --git a/spec/frontend/repository/pages/blob_edit_header_spec.js b/spec/frontend/repository/pages/blob_edit_header_spec.js index 3e158f6d5b733c..9914b6a037b797 100644 --- a/spec/frontend/repository/pages/blob_edit_header_spec.js +++ b/spec/frontend/repository/pages/blob_edit_header_spec.js @@ -156,7 +156,7 @@ describe('BlobEditHeader', () => { messageLinks: { changesLink: 'http://test.host/gitlab-org/gitlab/-/blob/feature/test.js', }, - variant: 'success', + variant: 'info', }); expect(mock.history.put).toHaveLength(1); @@ -190,7 +190,7 @@ describe('BlobEditHeader', () => { messageLinks: { changesLink: 'http://test.host/gitlab-org/gitlab/-/blob/main/test.js', }, - variant: 'success', + variant: 'info', }); expect(visitUrlSpy).toHaveBeenCalledWith( @@ -218,7 +218,7 @@ describe('BlobEditHeader', () => { messageLinks: { changesLink: 'http://test.host/gitlab-org/gitlab/-/blob/feature/test.js', }, - variant: 'success', + variant: 'info', }); expect(visitUrlSpy).toHaveBeenCalledWith( -- GitLab From e47f72cf4de44c7138adc18b3426e18822fa3760 Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Fri, 3 Oct 2025 10:18:53 +0200 Subject: [PATCH 4/5] Apply reviewer feedback --- spec/frontend/repository/pages/blob_edit_header_spec.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/spec/frontend/repository/pages/blob_edit_header_spec.js b/spec/frontend/repository/pages/blob_edit_header_spec.js index 9914b6a037b797..b8898915863675 100644 --- a/spec/frontend/repository/pages/blob_edit_header_spec.js +++ b/spec/frontend/repository/pages/blob_edit_header_spec.js @@ -169,8 +169,6 @@ describe('BlobEditHeader', () => { }); it('on submit to same branch, saves shorter success message to localStorage', async () => { - // First click the commit button to open the modal and set up the file content - clickCommitChangesButton(); mock.onPut().replyOnce(HTTP_STATUS_OK, { branch: 'main', // Same as originalBranch file_path: 'test.js', @@ -198,14 +196,13 @@ describe('BlobEditHeader', () => { ); }); - it('on submit to new branch without push access, saves success message with "original project" text', async () => { + it('on submit to new branch to a fork repo, saves success message with "original project" text', async () => { // Create wrapper with canPushToBranch: false to simulate fork scenario wrapper = createWrapper({ glFeatures: { blobEditRefactor: true }, provided: { canPushToBranch: false }, }); - clickCommitChangesButton(); mock.onPut().replyOnce(HTTP_STATUS_OK, { branch: 'feature', file_path: 'test.js', -- GitLab From 0211044f415122d945d468606424e9958c163989 Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Mon, 6 Oct 2025 12:56:37 +0200 Subject: [PATCH 5/5] Apply reviewers feedback --- .../repository/pages/blob_edit_header.vue | 8 +- .../repository/utils/edit_form_data_utils.js | 30 ------- .../repository/pages/blob_edit_header_spec.js | 90 +++++++------------ .../utils/edit_form_data_utils_spec.js | 38 -------- 4 files changed, 33 insertions(+), 133 deletions(-) diff --git a/app/assets/javascripts/repository/pages/blob_edit_header.vue b/app/assets/javascripts/repository/pages/blob_edit_header.vue index a861a8ab2737c0..9f396571fc5b11 100644 --- a/app/assets/javascripts/repository/pages/blob_edit_header.vue +++ b/app/assets/javascripts/repository/pages/blob_edit_header.vue @@ -11,11 +11,7 @@ import { VARIANT_INFO } from '~/alert'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { saveAlertToLocalStorage } from '../local_storage_alert/save_alert_to_local_storage'; import getRefMixin from '../mixins/get_ref'; -import { - prepareEditFormData, - prepareControllerFormData, - prepareCreateFormData, -} from '../utils/edit_form_data_utils'; +import { prepareEditFormData, prepareCreateFormData } from '../utils/edit_form_data_utils'; const MR_SOURCE_BRANCH = 'merge_request[source_branch]'; @@ -137,7 +133,7 @@ export default { lastCommitSha: this.lastCommitSha, fromMergeRequestIid: this.fromMergeRequestIid, }) - : prepareControllerFormData(formData, { + : prepareEditFormData(formData, { fileContent: this.fileContent, filePath: this.filePath, lastCommitSha: this.lastCommitSha, diff --git a/app/assets/javascripts/repository/utils/edit_form_data_utils.js b/app/assets/javascripts/repository/utils/edit_form_data_utils.js index 4a358aa84d62eb..f4bbc651247fb8 100644 --- a/app/assets/javascripts/repository/utils/edit_form_data_utils.js +++ b/app/assets/javascripts/repository/utils/edit_form_data_utils.js @@ -28,36 +28,6 @@ export const prepareEditFormData = ( return Object.fromEntries(formData); }; -/** - * Prepares form data for controller-based blob editing operations. - * This is used when the blobEditRefactor feature flag is disabled. - * - * Object.fromEntries is used here to handle potential line ending mutations in FormData. - * FormData uses the "multipart/form-data" format (RFC 2388), which follows MIME data - * stream rules (RFC 2046). These specifications require line breaks to be represented - * as CRLF sequences in the canonical form. - * See https://stackoverflow.com/questions/69835705/formdata-textarea-puts-r-carriage-return-when-sent-with-post for more details. - * - * @param {FormData} formData - The original form data - * @param {Object} params - Parameters object - * @param {string} params.fileContent - The file content to be committed - * @param {string} params.filePath - The path of the file being edited - * @param {string} params.lastCommitSha - The SHA of the last commit - * @param {string} params.fromMergeRequestIid - The merge request IID if editing from an MR - * @returns {Object} Plain object with form data entries - */ -export const prepareControllerFormData = ( - formData, - { fileContent, filePath, lastCommitSha, fromMergeRequestIid }, -) => { - formData.append('file', fileContent); - formData.append('file_path', filePath); - formData.append('last_commit_sha', lastCommitSha); - formData.append('from_merge_request_iid', fromMergeRequestIid); - - return Object.fromEntries(formData); -}; - /** * Prepares form data for creating new blobs by appending file name and content. * diff --git a/spec/frontend/repository/pages/blob_edit_header_spec.js b/spec/frontend/repository/pages/blob_edit_header_spec.js index b8898915863675..035eee70232f26 100644 --- a/spec/frontend/repository/pages/blob_edit_header_spec.js +++ b/spec/frontend/repository/pages/blob_edit_header_spec.js @@ -106,35 +106,38 @@ describe('BlobEditHeader', () => { await axios.waitForAll(); }; - describe('for edit blob', () => { - describe('when blobEditRefactor is enabled', () => { - it('renders title with two buttons', () => { - expect(findTitle().text()).toBe('Edit file'); - const buttons = findButtons(); - expect(buttons).toHaveLength(2); - expect(buttons.at(0).text()).toBe('Cancel'); - expect(buttons.at(1).text()).toBe('Commit changes'); - }); + it('renders title with two buttons', () => { + expect(findTitle().text()).toBe('Edit file'); + const buttons = findButtons(); + expect(buttons).toHaveLength(2); + expect(buttons.at(0).text()).toBe('Cancel'); + expect(buttons.at(1).text()).toBe('Commit changes'); + }); - it('opens commit changes modal with correct props', () => { - clickCommitChangesButton(); - expect(mockEditor.getFileContent).toHaveBeenCalled(); - expect(findCommitChangesModal().props()).toEqual({ - modalId: 'update-modal1', - canPushCode: true, - canPushToBranch: true, - commitMessage: 'Edit test.js', - emptyRepo: false, - isUsingLfs: false, - originalBranch: 'main', - targetBranch: 'feature', - loading: false, - branchAllowsCollaboration: false, - valid: true, - error: null, - }); - }); + it('retrieves edit content, when opening the modal', () => { + clickCommitChangesButton(); + expect(mockEditor.getFileContent).toHaveBeenCalled(); + }); + + it('opens commit changes modal with correct props', () => { + expect(findCommitChangesModal().props()).toEqual({ + modalId: 'update-modal1', + canPushCode: true, + canPushToBranch: true, + commitMessage: 'Edit test.js', + emptyRepo: false, + isUsingLfs: false, + originalBranch: 'main', + targetBranch: 'feature', + loading: false, + branchAllowsCollaboration: false, + valid: true, + error: null, + }); + }); + describe('for edit blob', () => { + describe('when blobEditRefactor is enabled', () => { it('shows confirmation message on cancel button', () => { expect(findCancelButton().attributes('data-confirm')).toBe( 'Leave edit mode? All unsaved changes will be lost.', @@ -226,10 +229,6 @@ describe('BlobEditHeader', () => { describe('error handling', () => { const errorMessage = 'Custom error message'; - beforeEach(() => { - clickCommitChangesButton(); - }); - it('shows error message in modal when response contains error', async () => { mock.onPut().replyOnce(HTTP_STATUS_OK, { error: errorMessage }); await submitForm(); @@ -360,33 +359,6 @@ describe('BlobEditHeader', () => { wrapper = createWrapper({ action: 'create' }); }); - it('renders title with two buttons', () => { - expect(findTitle().text()).toBe('New file'); - const buttons = findButtons(); - expect(buttons).toHaveLength(2); - expect(buttons.at(0).text()).toBe('Cancel'); - expect(buttons.at(1).text()).toBe('Commit changes'); - }); - - it('opens commit changes modal with correct props', () => { - clickCommitChangesButton(); - expect(mockEditor.getFileContent).toHaveBeenCalled(); - expect(findCommitChangesModal().props()).toEqual({ - modalId: 'update-modal1', - canPushCode: true, - canPushToBranch: true, - commitMessage: 'Add new file', - originalBranch: 'main', - targetBranch: 'feature', - isUsingLfs: false, - emptyRepo: false, - branchAllowsCollaboration: false, - loading: false, - valid: true, - error: null, - }); - }); - it('shows confirmation message on cancel button', () => { expect(findCancelButton().attributes('data-confirm')).toBe( 'Leave edit mode? All unsaved changes will be lost.', @@ -408,7 +380,7 @@ describe('BlobEditHeader', () => { describe('validation', () => { it('toggles validation error when filename is empty', () => { mockEditor.filepathFormMediator.$filenameInput.val.mockReturnValue(null); - wrapper = createWrapper(); + createWrapper(); clickCommitChangesButton(); expect(mockEditor.filepathFormMediator.toggleValidationError).toHaveBeenCalledWith(true); diff --git a/spec/frontend/repository/utils/edit_form_data_utils_spec.js b/spec/frontend/repository/utils/edit_form_data_utils_spec.js index 0c8e4c2426d5e3..5aec1772658142 100644 --- a/spec/frontend/repository/utils/edit_form_data_utils_spec.js +++ b/spec/frontend/repository/utils/edit_form_data_utils_spec.js @@ -1,6 +1,5 @@ import { prepareEditFormData, - prepareControllerFormData, prepareCreateFormData, } from '~/repository/utils/edit_form_data_utils'; @@ -31,43 +30,6 @@ describe('edit_form_data_utils', () => { }); }); - describe('prepareControllerFormData', () => { - const params = { - fileContent: 'const x = 1;', - filePath: 'lib/utils.js', - lastCommitSha: 'def456abc789', - fromMergeRequestIid: '123', - }; - - it('appends all required fields to FormData and returns plain object', () => { - const result = prepareControllerFormData(formData, params); - - expect(result).toEqual({ - file: params.fileContent, - file_path: params.filePath, - last_commit_sha: params.lastCommitSha, - from_merge_request_iid: params.fromMergeRequestIid, - }); - }); - - it('handles null values gracefully', () => { - const nullParams = { - fileContent: params.fileContent, - filePath: params.filePath, - lastCommitSha: null, - fromMergeRequestIid: null, - }; - const result = prepareControllerFormData(formData, nullParams); - - expect(result).toEqual({ - file: params.fileContent, - file_path: params.filePath, - last_commit_sha: 'null', - from_merge_request_iid: 'null', - }); - }); - }); - describe('prepareCreateFormData', () => { const params = { filePath: 'new-file.md', -- GitLab