diff --git a/app/assets/javascripts/repository/components/blob_button_group.vue b/app/assets/javascripts/repository/components/blob_button_group.vue index ed7212eb9a646a8f7a4cafbb28e33fdfdc9df98b..3aec35ee96000bcb34e8f8e4e6d6b60254c41450 100644 --- a/app/assets/javascripts/repository/components/blob_button_group.vue +++ b/app/assets/javascripts/repository/components/blob_button_group.vue @@ -4,7 +4,7 @@ import { uniqueId } from 'lodash'; import { sprintf, __ } from '~/locale'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import getRefMixin from '../mixins/get_ref'; -import DeleteBlobModal from './delete_blob_modal.vue'; +import CommitChangesModal from './commit_changes_modal.vue'; import UploadBlobModal from './upload_blob_modal.vue'; const REPLACE_BLOB_MODAL_ID = 'modal-replace-blob'; @@ -19,7 +19,7 @@ export default { GlButtonGroup, GlButton, UploadBlobModal, - DeleteBlobModal, + CommitChangesModal, LockButton: () => import('ee_component/repository/components/lock_button.vue'), }, mixins: [getRefMixin, glFeatureFlagMixin()], @@ -89,7 +89,7 @@ export default { deleteModalId() { return uniqueId('delete-modal'); }, - deleteModalTitle() { + deleteModalCommitMessage() { return sprintf(__('Delete %{name}'), { name: this.name }); }, lockBtnTestId() { @@ -141,12 +141,11 @@ export default { :replace-path="replacePath" :primary-btn-text="$options.i18n.replacePrimaryBtnText" /> - import { GlModal, + GlFormCheckbox, GlFormGroup, GlFormInput, GlFormTextarea, - GlToggle, + GlFormRadio, + GlFormRadioGroup, GlForm, GlSprintf, GlLink, } from '@gitlab/ui'; import csrf from '~/lib/utils/csrf'; import { __, s__ } from '~/locale'; -import validation from '~/vue_shared/directives/validation'; +import validation, { initFormField } from '~/vue_shared/directives/validation'; import { helpPagePath } from '~/helpers/help_page_helper'; import { SECONDARY_OPTIONS_TEXT, COMMIT_LABEL, - TARGET_BRANCH_LABEL, - TOGGLE_CREATE_MR_LABEL, COMMIT_MESSAGE_SUBJECT_MAX_LENGTH, COMMIT_MESSAGE_BODY_MAX_LENGTH, } from '../constants'; -const initFormField = ({ value, required = true, skipValidation = false }) => ({ - value, - required, - state: skipValidation ? true : null, - feedback: null, -}); - export default { csrf, components: { GlModal, + GlFormCheckbox, GlFormGroup, GlFormInput, + GlFormRadio, + GlFormRadioGroup, GlFormTextarea, - GlToggle, GlForm, GlSprintf, GlLink, }, i18n: { + BRANCH: __('Branch'), + CURRENT_BRANCH_LABEL: __('Commit to the current %{branchName} branch'), + COMMIT_CHANGES: __('Commit changes'), + COMMIT_LABEL, + COMMIT_MESSAGE_HINT: __( + 'Try to keep the first line under 52 characters and the others under 72.', + ), + NEW_BRANCH_LABEl: __('Commit to a new branch'), + CREATE_MR_LABEL: __('Create a merge request for this change'), LFS_WARNING_TITLE: __("The file you're about to delete is tracked by LFS"), LFS_WARNING_PRIMARY_CONTENT: s__( 'BlobViewer|If you delete the file, it will be removed from the branch %{branch}.', @@ -51,14 +55,7 @@ export default { ), LFS_CONTINUE_TEXT: __('Continue…'), LFS_CANCEL_TEXT: __('Cancel'), - PRIMARY_OPTIONS_TEXT: __('Delete file'), SECONDARY_OPTIONS_TEXT, - COMMIT_LABEL, - TARGET_BRANCH_LABEL, - TOGGLE_CREATE_MR_LABEL, - COMMIT_MESSAGE_HINT: __( - 'Try to keep the first line under 52 characters and the others under 72.', - ), }, directives: { validation: validation(), @@ -68,10 +65,6 @@ export default { type: String, required: true, }, - modalTitle: { - type: String, - required: true, - }, deletePath: { type: String, required: true, @@ -113,23 +106,29 @@ export default { fields: { // fields key must match case of form name for validation directive to work commit_message: initFormField({ value: this.commitMessage }), - branch_name: initFormField({ value: this.targetBranch, skipValidation: !this.canPushCode }), + branch_name: initFormField({ + value: this.canPushToBranch ? this.originalBranch : '', + // Branch name is pre-filled with the current branch name in two scenarios and therefore doesn't need validation: + // 1. When the user doesn't have permission to push to the repo (e.g., guest user) + // 2. When the user can push directly to the current branch + skipValidation: !this.canPushCode || this.canPushToBranch, + }), }, }; return { lfsWarningDismissed: false, loading: false, + createNewBranch: false, createNewMr: true, - error: '', form, }; }, computed: { primaryOptions() { const defaultOptions = { - text: this.$options.i18n.PRIMARY_OPTIONS_TEXT, + text: this.$options.i18n.COMMIT_CHANGES, attributes: { - variant: 'danger', + variant: 'confirm', loading: this.loading, disabled: this.loading || !this.form.state, }, @@ -150,12 +149,6 @@ export default { }, }; }, - showCreateNewMrToggle() { - return this.canPushCode && this.form.fields.branch_name.value !== this.originalBranch; - }, - formCompleted() { - return this.form.fields.commit_message.value && this.form.fields.branch_name.value; - }, showHint() { const splitCommitMessageByLineBreak = this.form.fields.commit_message.value .trim() @@ -177,12 +170,29 @@ export default { return this.isUsingLfs && !this.lfsWarningDismissed; }, title() { - return this.showLfsWarning ? this.$options.i18n.LFS_WARNING_TITLE : this.modalTitle; + return this.showLfsWarning + ? this.$options.i18n.LFS_WARNING_TITLE + : this.$options.i18n.COMMIT_CHANGES; }, showDeleteForm() { return !this.isUsingLfs || (this.isUsingLfs && this.lfsWarningDismissed); }, }, + watch: { + createNewBranch: { + handler(newValue) { + if (newValue) { + this.form.fields.branch_name.value = ''; + } else { + this.form.fields.branch_name = { + ...this.form.fields.branch_name, + value: this.originalBranch, + state: true, + }; + } + }, + }, + }, methods: { show() { this.$refs[this.modalId].show(); @@ -259,12 +269,7 @@ export default { diff --git a/app/assets/javascripts/vue_shared/directives/validation.js b/app/assets/javascripts/vue_shared/directives/validation.js index c129aff9236dd4ef8431d86835e1066ee58cc006..2aeaacc2ab14422dff3b55e199484a528d0f68fe 100644 --- a/app/assets/javascripts/vue_shared/directives/validation.js +++ b/app/assets/javascripts/vue_shared/directives/validation.js @@ -154,7 +154,7 @@ export default function initValidation(customFeedbackMap = {}) { * @param {*} fieldValues * @returns formObject */ -const initFormField = ({ value, required = true, skipValidation = false }) => ({ +export const initFormField = ({ value, required = true, skipValidation = false }) => ({ value, required, state: skipValidation ? true : null, diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d77469f88565c52ce04d9a75336531bd406cc523..8eeb5aaa0ce383cf1ad0e682581b63dc8dd15eca 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -13852,6 +13852,12 @@ msgstr "" msgid "Commit statistics for %{ref} %{start_time} - %{end_time}" msgstr "" +msgid "Commit to a new branch" +msgstr "" + +msgid "Commit to the current %{branchName} branch" +msgstr "" + msgid "CommitBoxTitle|Commit" msgstr "" @@ -16207,6 +16213,9 @@ msgstr "" msgid "Create a merge request branch target." msgstr "" +msgid "Create a merge request for this change" +msgstr "" + msgid "Create a merge request, or edit your criteria and try again." msgstr "" @@ -18234,9 +18243,6 @@ msgstr "" msgid "Delete epic" msgstr "" -msgid "Delete file" -msgstr "" - msgid "Delete group" msgstr "" @@ -64934,6 +64940,9 @@ msgstr "" msgid "event" msgstr "" +msgid "example-branch-name" +msgstr "" + msgid "example.com" msgstr "" diff --git a/qa/qa/page/file/shared/commit_message.rb b/qa/qa/page/file/shared/commit_message.rb index 32e1abf5590264130241bd6ee28779df47889640..3372c67bd1164fa064a71c1d6099f78306b6f9fa 100644 --- a/qa/qa/page/file/shared/commit_message.rb +++ b/qa/qa/page/file/shared/commit_message.rb @@ -10,7 +10,7 @@ module CommitMessage def self.included(base) super - base.view 'app/assets/javascripts/repository/components/delete_blob_modal.vue' do + base.view 'app/assets/javascripts/repository/components/commit_changes_modal.vue' do element 'commit-message-field' end diff --git a/qa/qa/page/file/show.rb b/qa/qa/page/file/show.rb index 5e1593ac11768d1dbd568fcf6090c2e6f56f25e0..4637cedad418a5e7dec6d53f56c596c833c963f2 100644 --- a/qa/qa/page/file/show.rb +++ b/qa/qa/page/file/show.rb @@ -35,8 +35,8 @@ def explain_code click_element('question-icon') end - def click_delete_file - click_on 'Delete file' + def click_commit_changes + click_on 'Commit changes' end end end diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/file/delete_file_via_web_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/file/delete_file_via_web_spec.rb index 305056f2277fbfe63e1fde6163b4480f5c890233..b00a34de3432005d04e0922e82be224a618cd55b 100644 --- a/qa/qa/specs/features/browser_ui/3_create/repository/file/delete_file_via_web_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/repository/file/delete_file_via_web_spec.rb @@ -16,7 +16,7 @@ module QA Page::File::Show.perform do |file| file.click_delete file.add_commit_message(commit_message_for_delete) - file.click_delete_file + file.click_commit_changes end Page::Project::Show.perform do |project| diff --git a/spec/features/projects/files/user_deletes_files_spec.rb b/spec/features/projects/files/user_deletes_files_spec.rb index 68a996451f60649e219746414c677db35c89ce01..b34b8e8d566a4ab6dff822361480b594b290c5b3 100644 --- a/spec/features/projects/files/user_deletes_files_spec.rb +++ b/spec/features/projects/files/user_deletes_files_spec.rb @@ -32,7 +32,7 @@ click_on('Delete') fill_in(:commit_message, with: 'New commit message', visible: true) - click_button('Delete file') + click_button('Commit changes') expect(page).to have_current_path(project_tree_path(project, 'master/'), ignore_query: true) expect(page).not_to have_content('.gitignore') @@ -62,7 +62,7 @@ click_on('Delete') fill_in(:commit_message, with: 'New commit message', visible: true) - click_button('Delete file') + click_button('Commit changes') fork = user.fork_of(project2.reload) diff --git a/spec/frontend/repository/components/blob_button_group_spec.js b/spec/frontend/repository/components/blob_button_group_spec.js index 1a077028704d77aacb6096718cc87d405df4e540..42aed016f40f1b2f5b0444ad832f95319ad43d12 100644 --- a/spec/frontend/repository/components/blob_button_group_spec.js +++ b/spec/frontend/repository/components/blob_button_group_spec.js @@ -2,7 +2,7 @@ import { GlButton } from '@gitlab/ui'; import { mountExtended } from 'helpers/vue_test_utils_helper'; import { stubComponent } from 'helpers/stub_component'; import BlobButtonGroup from '~/repository/components/blob_button_group.vue'; -import DeleteBlobModal from '~/repository/components/delete_blob_modal.vue'; +import CommitChangesModal from '~/repository/components/commit_changes_modal.vue'; import UploadBlobModal from '~/repository/components/upload_blob_modal.vue'; const DEFAULT_PROPS = { @@ -40,7 +40,7 @@ describe('BlobButtonGroup component', () => { show: showUploadBlobModalMock, }, }); - const DeleteBlobModalStub = stubComponent(DeleteBlobModal, { + const DeleteBlobModalStub = stubComponent(CommitChangesModal, { methods: { show: showDeleteBlobModalMock, }, @@ -56,12 +56,12 @@ describe('BlobButtonGroup component', () => { }, stubs: { UploadBlobModal: UploadBlobModalStub, - DeleteBlobModal: DeleteBlobModalStub, + CommitChangesModal: DeleteBlobModalStub, }, }); }; - const findDeleteBlobModal = () => wrapper.findComponent(DeleteBlobModal); + const findDeleteBlobModal = () => wrapper.findComponent(CommitChangesModal); const findUploadBlobModal = () => wrapper.findComponent(UploadBlobModal); const findDeleteButton = () => wrapper.findByTestId('delete'); const findReplaceButton = () => wrapper.findByTestId('replace'); @@ -92,13 +92,13 @@ describe('BlobButtonGroup component', () => { }); it('triggers the UploadBlobModal from the replace button', () => { - findReplaceButton().trigger('click'); + findReplaceButton().vm.$emit('click'); expect(showUploadBlobModalMock).toHaveBeenCalled(); }); - it('triggers the DeleteBlobModal from the delete button', () => { - findDeleteButton().trigger('click'); + it('triggers the CommitChangesModal from the delete button', () => { + findDeleteButton().vm.$emit('click'); expect(showDeleteBlobModalMock).toHaveBeenCalled(); }); @@ -109,14 +109,14 @@ describe('BlobButtonGroup component', () => { }); it('does not trigger the UploadBlobModal from the replace button', () => { - findReplaceButton().trigger('click'); + findReplaceButton().vm.$emit('click'); expect(showUploadBlobModalMock).not.toHaveBeenCalled(); expect(wrapper.emitted().fork).toHaveLength(1); }); it('does not trigger the DeleteBlobModal from the delete button', () => { - findDeleteButton().trigger('click'); + findDeleteButton().vm.$emit('click'); expect(showDeleteBlobModalMock).not.toHaveBeenCalled(); expect(wrapper.emitted().fork).toHaveLength(1); @@ -143,7 +143,7 @@ describe('BlobButtonGroup component', () => { }); }); - it('renders DeleteBlobModel', () => { + it('renders CommitChangesModal for delete', () => { createComponent(); const { targetBranch, originalBranch } = DEFAULT_INJECT; @@ -151,7 +151,6 @@ describe('BlobButtonGroup component', () => { const title = `Delete ${name}`; expect(findDeleteBlobModal().props()).toMatchObject({ - modalTitle: title, commitMessage: title, targetBranch, originalBranch, diff --git a/spec/frontend/repository/components/delete_blob_modal_spec.js b/spec/frontend/repository/components/commit_changes_modal_spec.js similarity index 73% rename from spec/frontend/repository/components/delete_blob_modal_spec.js rename to spec/frontend/repository/components/commit_changes_modal_spec.js index d3af39425db627a34b9cb14053ebfe95dde62602..e945f2cc37f683929f63920cdb90e70ea9dea7e4 100644 --- a/spec/frontend/repository/components/delete_blob_modal_spec.js +++ b/spec/frontend/repository/components/commit_changes_modal_spec.js @@ -1,16 +1,24 @@ -import { GlFormTextarea, GlModal, GlFormInput, GlToggle, GlForm, GlSprintf } from '@gitlab/ui'; +import { + GlFormTextarea, + GlModal, + GlFormCheckbox, + GlFormInput, + GlFormRadioGroup, + GlForm, + GlSprintf, + GlFormRadio, +} from '@gitlab/ui'; import { mount } from '@vue/test-utils'; import { nextTick } from 'vue'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import { RENDER_ALL_SLOTS_TEMPLATE, stubComponent } from 'helpers/stub_component'; -import DeleteBlobModal from '~/repository/components/delete_blob_modal.vue'; +import CommitChangesModal from '~/repository/components/commit_changes_modal.vue'; import { sprintf } from '~/locale'; jest.mock('~/lib/utils/csrf', () => ({ token: 'mock-csrf-token' })); const initialProps = { modalId: 'Delete-blob', - modalTitle: 'Delete File', deletePath: 'some/path', commitMessage: 'Delete File', targetBranch: 'some-target-branch', @@ -20,15 +28,15 @@ const initialProps = { emptyRepo: false, }; -const { i18n } = DeleteBlobModal; +const { i18n } = CommitChangesModal; -describe('DeleteBlobModal', () => { +describe('CommitChangesModal', () => { let wrapper; const createComponentFactory = (mountFn) => (props = {}) => { - wrapper = mountFn(DeleteBlobModal, { + wrapper = mountFn(CommitChangesModal, { propsData: { ...initialProps, ...props, @@ -50,6 +58,11 @@ describe('DeleteBlobModal', () => { const findModal = () => wrapper.findComponent(GlModal); const findForm = () => findModal().findComponent(GlForm); const findCommitTextarea = () => findForm().findComponent(GlFormTextarea); + const findFormRadioGroup = () => findForm().findComponent(GlFormRadioGroup); + const findRadioGroup = () => findForm().findAllComponents(GlFormRadio); + const findCurrentBranchRadioOption = () => findRadioGroup().at(0); + const findNewBranchRadioOption = () => findRadioGroup().at(1); + const findCreateMrCheckbox = () => findForm().findComponent(GlFormCheckbox); const findTargetInput = () => findForm().findComponent(GlFormInput); const findCommitHint = () => wrapper.find('[data-testid="hint"]'); @@ -92,13 +105,10 @@ describe('DeleteBlobModal', () => { it('renders Modal component', () => { createComponent(); - const { modalTitle: title } = initialProps; - expect(findModal().props()).toMatchObject({ - title, size: 'md', actionPrimary: { - text: 'Delete file', + text: 'Commit changes', }, actionCancel: { text: 'Cancel', @@ -112,32 +122,53 @@ describe('DeleteBlobModal', () => { expect(findForm().attributes('action')).toBe(initialProps.deletePath); }); - it.each` - component | defaultValue | canPushCode | targetBranch | originalBranch | exist - ${GlFormTextarea} | ${initialProps.commitMessage} | ${true} | ${initialProps.targetBranch} | ${initialProps.originalBranch} | ${true} - ${GlFormInput} | ${initialProps.targetBranch} | ${true} | ${initialProps.targetBranch} | ${initialProps.originalBranch} | ${true} - ${GlFormInput} | ${undefined} | ${false} | ${initialProps.targetBranch} | ${initialProps.originalBranch} | ${false} - ${GlToggle} | ${'true'} | ${true} | ${initialProps.targetBranch} | ${initialProps.originalBranch} | ${true} - ${GlToggle} | ${undefined} | ${true} | ${'same-branch'} | ${'same-branch'} | ${false} - `( - 'has the correct form fields', - ({ component, defaultValue, canPushCode, targetBranch, originalBranch, exist }) => { - createComponent({ - canPushCode, - targetBranch, - originalBranch, - }); - const formField = wrapper.findComponent(component); + it('shows the correct form fields when commit to current branch', () => { + createComponent(); + expect(findCommitTextarea().exists()).toBe(true); + expect(findRadioGroup()).toHaveLength(2); + expect(findCurrentBranchRadioOption().text()).toContain(initialProps.originalBranch); + expect(findNewBranchRadioOption().text()).toBe('Commit to a new branch'); + }); - if (!exist) { - expect(formField.exists()).toBe(false); - return; - } + it('shows the correct form fields when commit to new branch', async () => { + createComponent(); + expect(findTargetInput().exists()).toBe(false); - expect(formField.exists()).toBe(true); - expect(formField.attributes('value')).toBe(defaultValue); - }, - ); + findFormRadioGroup().vm.$emit('input', true); + await nextTick(); + + expect(findTargetInput().exists()).toBe(true); + expect(findCreateMrCheckbox().text()).toBe('Create a merge request for this change'); + }); + + it('shows the correct form fields when `canPushToBranch` is `false`', () => { + createComponent({ canPushToBranch: false, canPushCode: true }); + expect(wrapper.vm.$data.form.fields.branch_name.value).toBe(''); + expect(findCommitTextarea().exists()).toBe(true); + expect(findRadioGroup().exists()).toBe(false); + expect(findTargetInput().exists()).toBe(true); + expect(findCreateMrCheckbox().text()).toBe('Create a merge request for this change'); + }); + + it('clear branch name when new branch option is selected', async () => { + createComponent(); + expect(wrapper.vm.$data.form.fields.branch_name).toEqual({ + feedback: null, + required: true, + state: true, + value: 'main', + }); + + findFormRadioGroup().vm.$emit('input', true); + await nextTick(); + + expect(wrapper.vm.$data.form.fields.branch_name).toEqual({ + feedback: null, + required: true, + state: true, + value: '', + }); + }); it.each` input | value | emptyRepo | canPushCode | canPushToBranch | exist @@ -181,6 +212,7 @@ describe('DeleteBlobModal', () => { beforeEach(async () => { createFullComponent(); + findFormRadioGroup().vm.$emit('input', true); await nextTick(); }); @@ -219,6 +251,9 @@ describe('DeleteBlobModal', () => { describe('invalid form', () => { beforeEach(async () => { + findFormRadioGroup().vm.$emit('input', true); + await nextTick(); + await fillForm({ targetText: '', commitText: '' }); }); @@ -236,6 +271,8 @@ describe('DeleteBlobModal', () => { describe('valid form', () => { beforeEach(async () => { + findFormRadioGroup().vm.$emit('input', true); + await nextTick(); await fillForm({ targetText: 'some valid target branch', commitText: 'some valid commit message',