From 08e69d1560e98aa1d7ce9558326ccf41722c0c23 Mon Sep 17 00:00:00 2001 From: Chaoyue Zhao Date: Fri, 11 Oct 2024 11:07:33 -0400 Subject: [PATCH 1/6] Update blob delete modal Changelog: changed --- .../components/blob_button_group.vue | 11 +- ...lob_modal.vue => commit_changes_modal.vue} | 133 +++++++++++------- .../vue_shared/directives/validation.js | 2 +- locale/gitlab.pot | 12 +- .../components/blob_button_group_spec.js | 21 ++- ...l_spec.js => commit_changes_modal_spec.js} | 77 +++++----- 6 files changed, 147 insertions(+), 109 deletions(-) rename app/assets/javascripts/repository/components/{delete_blob_modal.vue => commit_changes_modal.vue} (72%) rename spec/frontend/repository/components/{delete_blob_modal_spec.js => commit_changes_modal_spec.js} (78%) diff --git a/app/assets/javascripts/repository/components/blob_button_group.vue b/app/assets/javascripts/repository/components/blob_button_group.vue index ed7212eb9a646a..3aec35ee96000b 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" /> - ({ - value, - required, - state: skipValidation ? true : null, - feedback: null, -}); - export default { csrf, components: { GlModal, GlFormGroup, GlFormInput, + GlFormRadio, + GlFormRadioGroup, GlFormTextarea, - GlToggle, GlForm, GlSprintf, GlLink, }, i18n: { + BRANCH: __('Branch'), + COMMIT_CHANGES: __('Commit changes'), + COMMIT_LABEL, + COMMIT_MESSAGE_HINT: __( + 'Try to keep the first line under 52 characters and the others under 72.', + ), + CURRENT_BRANCH_LABEl: __('Commit to current branch:'), + NEW_BRANCH_LABEl: __('Commit to new branch and create a merge request'), 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 +52,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 +62,6 @@ export default { type: String, required: true, }, - modalTitle: { - type: String, - required: true, - }, deletePath: { type: String, required: true, @@ -113,23 +103,25 @@ 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.targetBranch, + skipValidation: true, + }), }, }; return { lfsWarningDismissed: false, loading: false, - createNewMr: true, - error: '', + createNewMr: false, 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 +142,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 +163,25 @@ 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: { + createNewMr: { + handler(newValue) { + if (newValue) { + this.form.fields.branch_name.value = ''; + } else { + this.form.fields.branch_name.value = this.originalBranch; + } + }, + }, + }, methods: { show() { this.$refs[this.modalId].show(); @@ -288,26 +287,54 @@ export default { - + + + - diff --git a/app/assets/javascripts/vue_shared/directives/validation.js b/app/assets/javascripts/vue_shared/directives/validation.js index c129aff9236dd4..2aeaacc2ab1442 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 d77469f88565c5..e21e2ee05c345f 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 current branch:" +msgstr "" + +msgid "Commit to new branch and create a merge request" +msgstr "" + msgid "CommitBoxTitle|Commit" msgstr "" @@ -18234,9 +18240,6 @@ msgstr "" msgid "Delete epic" msgstr "" -msgid "Delete file" -msgstr "" - msgid "Delete group" msgstr "" @@ -64934,6 +64937,9 @@ msgstr "" msgid "event" msgstr "" +msgid "example-branch-name" +msgstr "" + msgid "example.com" msgstr "" diff --git a/spec/frontend/repository/components/blob_button_group_spec.js b/spec/frontend/repository/components/blob_button_group_spec.js index 1a077028704d77..42aed016f40f1b 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 78% rename from spec/frontend/repository/components/delete_blob_modal_spec.js rename to spec/frontend/repository/components/commit_changes_modal_spec.js index d3af39425db627..a991e170ce4d22 100644 --- a/spec/frontend/repository/components/delete_blob_modal_spec.js +++ b/spec/frontend/repository/components/commit_changes_modal_spec.js @@ -1,16 +1,15 @@ -import { GlFormTextarea, GlModal, GlFormInput, GlToggle, GlForm, GlSprintf } from '@gitlab/ui'; +import { GlFormTextarea, GlModal, GlFormInput, 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 +19,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 +49,9 @@ describe('DeleteBlobModal', () => { const findModal = () => wrapper.findComponent(GlModal); const findForm = () => findModal().findComponent(GlForm); const findCommitTextarea = () => findForm().findComponent(GlFormTextarea); + const findRadioGroup = () => wrapper.findAllComponents(GlFormRadio); + const findCurrentBranchRadioOption = () => findRadioGroup().at(0); + const findNewBranchRadioOption = () => findRadioGroup().at(1); const findTargetInput = () => findForm().findComponent(GlFormInput); const findCommitHint = () => wrapper.find('[data-testid="hint"]'); @@ -92,13 +94,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 +111,31 @@ 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 new branch and create a merge request', + ); + }); - 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); - }, - ); + // eslint-disable-next-line no-restricted-syntax + await wrapper.setData({ createNewMr: true }); + expect(findTargetInput().exists()).toBe(true); + }); + + it('shows the correct form fields when `canPushToBranch` is `false`', () => { + createComponent({ canPushToBranch: false, canPushCode: true }); + expect(findCommitTextarea().exists()).toBe(true); + expect(findRadioGroup().exists()).toBe(false); + expect(findTargetInput().exists()).toBe(true); + }); it.each` input | value | emptyRepo | canPushCode | canPushToBranch | exist @@ -154,13 +152,16 @@ describe('DeleteBlobModal', () => { ${'create_merge_request'} | ${undefined} | ${true} | ${false} | ${true} | ${false} `( 'passes $input as a hidden input with the correct value', - ({ input, value, emptyRepo, canPushCode, canPushToBranch, exist }) => { + async ({ input, value, emptyRepo, canPushCode, canPushToBranch, exist }) => { createComponent({ emptyRepo, canPushCode, canPushToBranch, }); + // eslint-disable-next-line no-restricted-syntax + await wrapper.setData({ createNewMr: true }); + const inputMethod = findForm().find(`input[name="${input}"]`); if (!exist) { @@ -182,6 +183,8 @@ describe('DeleteBlobModal', () => { beforeEach(async () => { createFullComponent(); await nextTick(); + // eslint-disable-next-line no-restricted-syntax + await wrapper.setData({ createNewMr: true }); }); it.each` @@ -219,6 +222,8 @@ describe('DeleteBlobModal', () => { describe('invalid form', () => { beforeEach(async () => { + // eslint-disable-next-line no-restricted-syntax + await wrapper.setData({ createNewMr: true }); await fillForm({ targetText: '', commitText: '' }); }); @@ -236,6 +241,8 @@ describe('DeleteBlobModal', () => { describe('valid form', () => { beforeEach(async () => { + // eslint-disable-next-line no-restricted-syntax + await wrapper.setData({ createNewMr: true }); await fillForm({ targetText: 'some valid target branch', commitText: 'some valid commit message', -- GitLab From 5cb78317804c4a615753555a477efc337379c7b1 Mon Sep 17 00:00:00 2001 From: Chaoyue Zhao Date: Tue, 15 Oct 2024 15:36:40 -0400 Subject: [PATCH 2/6] Code/Design review comments: - Add checkbox - Update copy - Remove `setData` in tests - Fix qa tests failure --- .../components/commit_changes_modal.vue | 75 +++++++++++-------- locale/gitlab.pot | 7 +- qa/qa/page/file/shared/commit_message.rb | 2 +- .../components/commit_changes_modal_spec.js | 64 +++++++++++----- 4 files changed, 98 insertions(+), 50 deletions(-) diff --git a/app/assets/javascripts/repository/components/commit_changes_modal.vue b/app/assets/javascripts/repository/components/commit_changes_modal.vue index 80b633b1feb5a3..00f79d4350fc5a 100644 --- a/app/assets/javascripts/repository/components/commit_changes_modal.vue +++ b/app/assets/javascripts/repository/components/commit_changes_modal.vue @@ -1,6 +1,7 @@