diff --git a/app/assets/javascripts/ide/components/commit_sidebar/actions.vue b/app/assets/javascripts/ide/components/commit_sidebar/actions.vue index 3276d21a04e1ea122fd9d3a93f3a5bbf053ab638..6a8ea506d1b821ea36d847d2da88164368e7fcd2 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/actions.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/actions.vue @@ -18,7 +18,7 @@ export default { computed: { ...mapState(['currentBranchId', 'changedFiles', 'stagedFiles']), ...mapCommitState(['commitAction']), - ...mapGetters(['currentBranch']), + ...mapGetters(['currentBranch', 'emptyRepo', 'canPushToBranch']), commitToCurrentBranchText() { return sprintf( s__('IDE|Commit to %{branchName} branch'), @@ -29,6 +29,13 @@ export default { containsStagedChanges() { return this.changedFiles.length > 0 && this.stagedFiles.length > 0; }, + shouldDefaultToCurrentBranch() { + if (this.emptyRepo) { + return true; + } + + return this.canPushToBranch && !this.currentBranch?.default; + }, }, watch: { containsStagedChanges() { @@ -43,13 +50,11 @@ export default { methods: { ...mapCommitActions(['updateCommitAction']), updateSelectedCommitAction() { - if (!this.currentBranch) { + if (!this.currentBranch && !this.emptyRepo) { return; } - const { can_push: canPush = false, default: isDefault = false } = this.currentBranch; - - if (canPush && !isDefault) { + if (this.shouldDefaultToCurrentBranch) { this.updateCommitAction(consts.COMMIT_TO_CURRENT_BRANCH); } else { this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH); @@ -68,7 +73,7 @@ export default {
- - +
diff --git a/app/assets/javascripts/ide/constants.js b/app/assets/javascripts/ide/constants.js index e7762f9e0f29609aaca3531dff685c6e8394f84a..fa2672aaececdb809c9577c67f857775b5bfc029 100644 --- a/app/assets/javascripts/ide/constants.js +++ b/app/assets/javascripts/ide/constants.js @@ -10,6 +10,7 @@ export const FILE_VIEW_MODE_PREVIEW = 'preview'; export const PERMISSION_CREATE_MR = 'createMergeRequestIn'; export const PERMISSION_READ_MR = 'readMergeRequest'; +export const PERMISSION_PUSH_CODE = 'pushCode'; export const viewerTypes = { mr: 'mrdiff', diff --git a/app/assets/javascripts/ide/queries/getUserPermissions.query.graphql b/app/assets/javascripts/ide/queries/getUserPermissions.query.graphql index 48f63995f449c3459fea26b16fbaefec0ded7060..2c9013ffa9c98d2c7f56401dee4b480bba18ae30 100644 --- a/app/assets/javascripts/ide/queries/getUserPermissions.query.graphql +++ b/app/assets/javascripts/ide/queries/getUserPermissions.query.graphql @@ -2,7 +2,8 @@ query getUserPermissions($projectPath: ID!) { project(fullPath: $projectPath) { userPermissions { createMergeRequestIn, - readMergeRequest + readMergeRequest, + pushCode } } } diff --git a/app/assets/javascripts/ide/stores/actions/project.js b/app/assets/javascripts/ide/stores/actions/project.js index 0b168009847de43d936ed8b41505d7460a0f6598..ae3829dc35e57a205df38d43c85e0417ba874796 100644 --- a/app/assets/javascripts/ide/stores/actions/project.js +++ b/app/assets/javascripts/ide/stores/actions/project.js @@ -83,10 +83,14 @@ export const showBranchNotFoundError = ({ dispatch }, branchId) => { }); }; -export const showEmptyState = ({ commit, state, dispatch }, { projectId, branchId }) => { +export const loadEmptyBranch = ({ commit, state }, { projectId, branchId }) => { const treePath = `${projectId}/${branchId}`; + const currentTree = state.trees[`${projectId}/${branchId}`]; - dispatch('setCurrentBranchId', branchId); + // If we already have a tree, let's not recreate an empty one + if (currentTree) { + return; + } commit(types.CREATE_TREE, { treePath }); commit(types.TOGGLE_LOADING, { @@ -114,8 +118,16 @@ export const loadFile = ({ dispatch, state }, { basePath }) => { } }; -export const loadBranch = ({ dispatch, getters }, { projectId, branchId }) => - dispatch('getBranchData', { +export const loadBranch = ({ dispatch, getters, state }, { projectId, branchId }) => { + const currentProject = state.projects[projectId]; + + if (currentProject?.branches?.[branchId]) { + return Promise.resolve(); + } else if (getters.emptyRepo) { + return dispatch('loadEmptyBranch', { projectId, branchId }); + } + + return dispatch('getBranchData', { projectId, branchId, }) @@ -137,29 +149,23 @@ export const loadBranch = ({ dispatch, getters }, { projectId, branchId }) => dispatch('showBranchNotFoundError', branchId); throw err; }); +}; -export const openBranch = ({ dispatch, state, getters }, { projectId, branchId, basePath }) => { - const currentProject = state.projects[projectId]; - if (getters.emptyRepo) { - return dispatch('showEmptyState', { projectId, branchId }); - } - if (!currentProject || !currentProject.branches[branchId]) { - dispatch('setCurrentBranchId', branchId); - - return dispatch('loadBranch', { projectId, branchId }) - .then(() => dispatch('loadFile', { basePath })) - .catch( - () => - new Error( - sprintf( - __('An error occurred while getting files for - %{branchId}'), - { - branchId: `${esc(projectId)}/${esc(branchId)}`, - }, - false, - ), +export const openBranch = ({ dispatch }, { projectId, branchId, basePath }) => { + dispatch('setCurrentBranchId', branchId); + + return dispatch('loadBranch', { projectId, branchId }) + .then(() => dispatch('loadFile', { basePath })) + .catch( + () => + new Error( + sprintf( + __('An error occurred while getting files for - %{branchId}'), + { + branchId: `${esc(projectId)}/${esc(branchId)}`, + }, + false, ), - ); - } - return Promise.resolve(dispatch('loadFile', { basePath })); + ), + ); }; diff --git a/app/assets/javascripts/ide/stores/getters.js b/app/assets/javascripts/ide/stores/getters.js index d7ad39019a5296d5731c3e39c745b1e6686b4968..5d0a85709061db679860192a60fbab9e24c9ca47 100644 --- a/app/assets/javascripts/ide/stores/getters.js +++ b/app/assets/javascripts/ide/stores/getters.js @@ -4,6 +4,7 @@ import { packageJsonPath, PERMISSION_READ_MR, PERMISSION_CREATE_MR, + PERMISSION_PUSH_CODE, } from '../constants'; export const activeFile = state => state.openFiles.find(file => file.active) || null; @@ -120,8 +121,9 @@ export const packageJson = state => state.entries[packageJsonPath]; export const isOnDefaultBranch = (_state, getters) => getters.currentProject && getters.currentProject.default_branch === getters.branchName; -export const canPushToBranch = (_state, getters) => - getters.currentBranch && getters.currentBranch.can_push; +export const canPushToBranch = (_state, getters) => { + return Boolean(getters.currentBranch ? getters.currentBranch.can_push : getters.canPushCode); +}; export const isFileDeletedAndReadded = (state, getters) => path => { const stagedFile = getters.getStagedFile(path); @@ -157,5 +159,8 @@ export const canReadMergeRequests = (state, getters) => export const canCreateMergeRequests = (state, getters) => Boolean(getters.findProjectPermissions(state.currentProjectId)[PERMISSION_CREATE_MR]); +export const canPushCode = (state, getters) => + Boolean(getters.findProjectPermissions(state.currentProjectId)[PERMISSION_PUSH_CODE]); + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/ide/stores/modules/commit/actions.js b/app/assets/javascripts/ide/stores/modules/commit/actions.js index 9bf0542cd0bb7612fe4dd6ce44a917a858b8e43b..505daa8834d8d767d4f4bd0e2522e4e4ca48c0a5 100644 --- a/app/assets/javascripts/ide/stores/modules/commit/actions.js +++ b/app/assets/javascripts/ide/stores/modules/commit/actions.js @@ -106,6 +106,9 @@ export const updateFilesAfterCommit = ({ commit, dispatch, rootState, rootGetter }; export const commitChanges = ({ commit, state, getters, dispatch, rootState, rootGetters }) => { + // Pull commit options out because they could change + // During some of the pre and post commit processing + const { shouldCreateMR, isCreatingNewBranch, branchName } = getters; const newBranch = state.commitAction !== consts.COMMIT_TO_CURRENT_BRANCH; const stageFilesPromise = rootState.stagedFiles.length ? Promise.resolve() @@ -116,7 +119,7 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo return stageFilesPromise .then(() => { const payload = createCommitPayload({ - branch: getters.branchName, + branch: branchName, newBranch, getters, state, @@ -149,7 +152,7 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo dispatch('updateCommitMessage', ''); return dispatch('updateFilesAfterCommit', { data, - branch: getters.branchName, + branch: branchName, }) .then(() => { commit(rootTypes.CLEAR_STAGED_CHANGES, null, { root: true }); @@ -158,15 +161,15 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo commit(rootTypes.SET_LAST_COMMIT_MSG, '', { root: true }); }, 5000); - if (getters.shouldCreateMR) { + if (shouldCreateMR) { const { currentProject } = rootGetters; - const targetBranch = getters.isCreatingNewBranch + const targetBranch = isCreatingNewBranch ? rootState.currentBranchId : currentProject.default_branch; dispatch( 'redirectToUrl', - createNewMergeRequestUrl(currentProject.web_url, getters.branchName, targetBranch), + createNewMergeRequestUrl(currentProject.web_url, branchName, targetBranch), { root: true }, ); } @@ -194,7 +197,7 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo if (rootGetters.activeFile) { router.push( - `/project/${rootState.currentProjectId}/blob/${getters.branchName}/-/${rootGetters.activeFile.path}`, + `/project/${rootState.currentProjectId}/blob/${branchName}/-/${rootGetters.activeFile.path}`, ); } } diff --git a/app/assets/javascripts/ide/stores/modules/commit/getters.js b/app/assets/javascripts/ide/stores/modules/commit/getters.js index e421d44b6de4ea12d289286386e915a77acc06c3..413c4b0110da603418b6af8930a4d3848f92b6cf 100644 --- a/app/assets/javascripts/ide/stores/modules/commit/getters.js +++ b/app/assets/javascripts/ide/stores/modules/commit/getters.js @@ -55,7 +55,7 @@ export const shouldHideNewMrOption = (_state, getters, _rootState, rootGetters) rootGetters.canPushToBranch; export const shouldDisableNewMrOption = (state, getters, rootState, rootGetters) => - !rootGetters.canCreateMergeRequests; + !rootGetters.canCreateMergeRequests || rootGetters.emptyRepo; export const shouldCreateMR = (state, getters) => state.shouldCreateMR && !getters.shouldDisableNewMrOption; diff --git a/changelogs/unreleased/27915-fix-ide-empty-repo.yml b/changelogs/unreleased/27915-fix-ide-empty-repo.yml new file mode 100644 index 0000000000000000000000000000000000000000..cffae81354eaab1ab5f2c28b3faad097e583f211 --- /dev/null +++ b/changelogs/unreleased/27915-fix-ide-empty-repo.yml @@ -0,0 +1,5 @@ +--- +title: Fix some Web IDE bugs with empty projects +merge_request: 25463 +author: +type: fixed diff --git a/spec/frontend/ide/components/commit_sidebar/actions_spec.js b/spec/frontend/ide/components/commit_sidebar/actions_spec.js index b3b98a64891fdb8e9ac048cec83d2491a6ef1fac..a303e2b9beec73df1fbe02bd670070051f59811b 100644 --- a/spec/frontend/ide/components/commit_sidebar/actions_spec.js +++ b/spec/frontend/ide/components/commit_sidebar/actions_spec.js @@ -17,7 +17,11 @@ describe('IDE commit sidebar actions', () => { let store; let vm; - const createComponent = ({ hasMR = false, currentBranchId = 'master' } = {}) => { + const createComponent = ({ + hasMR = false, + currentBranchId = 'master', + emptyRepo = false, + } = {}) => { const Component = Vue.extend(commitActions); vm = createComponentWithStore(Component, store); @@ -27,6 +31,7 @@ describe('IDE commit sidebar actions', () => { const proj = { ...projectData }; proj.branches[currentBranchId] = branches.find(branch => branch.name === currentBranchId); + proj.empty_repo = emptyRepo; Vue.set(vm.$store.state.projects, 'abcproject', proj); @@ -52,24 +57,27 @@ describe('IDE commit sidebar actions', () => { vm = null; }); + const findText = () => vm.$el.textContent; + const findRadios = () => Array.from(vm.$el.querySelectorAll('input[type="radio"]')); + it('renders 2 groups', () => { createComponent(); - expect(vm.$el.querySelectorAll('input[type="radio"]').length).toBe(2); + expect(findRadios().length).toBe(2); }); it('renders current branch text', () => { createComponent(); - expect(vm.$el.textContent).toContain('Commit to master branch'); + expect(findText()).toContain('Commit to master branch'); }); it('hides merge request option when project merge requests are disabled', done => { - createComponent({ mergeRequestsEnabled: false }); + createComponent({ hasMR: false }); vm.$nextTick(() => { - expect(vm.$el.querySelectorAll('input[type="radio"]').length).toBe(2); - expect(vm.$el.textContent).not.toContain('Create a new branch and merge request'); + expect(findRadios().length).toBe(2); + expect(findText()).not.toContain('Create a new branch and merge request'); done(); }); @@ -119,6 +127,7 @@ describe('IDE commit sidebar actions', () => { it.each` input | expectedOption ${{ currentBranchId: BRANCH_DEFAULT }} | ${consts.COMMIT_TO_NEW_BRANCH} + ${{ currentBranchId: BRANCH_DEFAULT, emptyRepo: true }} | ${consts.COMMIT_TO_CURRENT_BRANCH} ${{ currentBranchId: BRANCH_PROTECTED, hasMR: true }} | ${consts.COMMIT_TO_CURRENT_BRANCH} ${{ currentBranchId: BRANCH_PROTECTED, hasMR: false }} | ${consts.COMMIT_TO_CURRENT_BRANCH} ${{ currentBranchId: BRANCH_PROTECTED_NO_ACCESS, hasMR: true }} | ${consts.COMMIT_TO_NEW_BRANCH} @@ -138,4 +147,15 @@ describe('IDE commit sidebar actions', () => { }, ); }); + + describe('when empty project', () => { + beforeEach(() => { + createComponent({ emptyRepo: true }); + }); + + it('only renders commit to current branch', () => { + expect(findRadios().length).toBe(1); + expect(findText()).toContain('Commit to master branch'); + }); + }); }); diff --git a/spec/frontend/ide/stores/getters_spec.js b/spec/frontend/ide/stores/getters_spec.js index 011be95c1d23d856e6e449eb8464968fa960605f..408ea2b2939430af4635a82ddac52f6ea3178e50 100644 --- a/spec/frontend/ide/stores/getters_spec.js +++ b/spec/frontend/ide/stores/getters_spec.js @@ -280,39 +280,21 @@ describe('IDE store getters', () => { }); describe('canPushToBranch', () => { - it('returns false when no currentBranch exists', () => { - const localGetters = { - currentProject: undefined, - }; - - expect(getters.canPushToBranch({}, localGetters)).toBeFalsy(); - }); - - it('returns true when can_push to currentBranch', () => { - const localGetters = { - currentProject: { - default_branch: 'master', - }, - currentBranch: { - can_push: true, - }, - }; - - expect(getters.canPushToBranch({}, localGetters)).toBeTruthy(); - }); - - it('returns false when !can_push to currentBranch', () => { - const localGetters = { - currentProject: { - default_branch: 'master', - }, - currentBranch: { - can_push: false, - }, - }; - - expect(getters.canPushToBranch({}, localGetters)).toBeFalsy(); - }); + it.each` + currentBranch | canPushCode | expectedValue + ${undefined} | ${undefined} | ${false} + ${{ can_push: true }} | ${false} | ${true} + ${{ can_push: true }} | ${true} | ${true} + ${{ can_push: false }} | ${false} | ${false} + ${{ can_push: false }} | ${true} | ${false} + ${undefined} | ${true} | ${true} + ${undefined} | ${false} | ${false} + `( + 'with currentBranch ($currentBranch) and canPushCode ($canPushCode), it is $expectedValue', + ({ currentBranch, canPushCode, expectedValue }) => { + expect(getters.canPushToBranch({}, { currentBranch, canPushCode })).toBe(expectedValue); + }, + ); }); describe('isFileDeletedAndReadded', () => { @@ -422,6 +404,7 @@ describe('IDE store getters', () => { getterName | permissionKey ${'canReadMergeRequests'} | ${'readMergeRequest'} ${'canCreateMergeRequests'} | ${'createMergeRequestIn'} + ${'canPushCode'} | ${'pushCode'} `('$getterName', ({ getterName, permissionKey }) => { it.each([true, false])('finds permission for current project (%s)', val => { localState.projects[TEST_PROJECT_ID] = { diff --git a/spec/frontend/ide/stores/modules/commit/getters_spec.js b/spec/frontend/ide/stores/modules/commit/getters_spec.js index 07445c229178dc06f95edff23c8eb9492cc5eeae..adbfd7c6835e96698b22abd425f88f928af619fc 100644 --- a/spec/frontend/ide/stores/modules/commit/getters_spec.js +++ b/spec/frontend/ide/stores/modules/commit/getters_spec.js @@ -292,4 +292,15 @@ describe('IDE commit module getters', () => { expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); }); }); + + describe('shouldDisableNewMrOption', () => { + it.each` + rootGetters | expectedValue + ${{ canCreateMergeRequests: false, emptyRepo: false }} | ${true} + ${{ canCreateMergeRequests: true, emptyRepo: true }} | ${true} + ${{ canCreateMergeRequests: true, emptyRepo: false }} | ${false} + `('with $rootGetters, it is $expectedValue', ({ rootGetters, expectedValue }) => { + expect(getters.shouldDisableNewMrOption(state, getters, {}, rootGetters)).toBe(expectedValue); + }); + }); }); diff --git a/spec/javascripts/ide/stores/actions/project_spec.js b/spec/javascripts/ide/stores/actions/project_spec.js index bd51222ac3c7d336279ea25f0f1c662473685f07..e962224d1ad1bb6ebad9519e653a0f70daece27c 100644 --- a/spec/javascripts/ide/stores/actions/project_spec.js +++ b/spec/javascripts/ide/stores/actions/project_spec.js @@ -4,7 +4,7 @@ import { refreshLastCommitData, showBranchNotFoundError, createNewBranchFromDefault, - showEmptyState, + loadEmptyBranch, openBranch, loadFile, loadBranch, @@ -16,6 +16,8 @@ import router from '~/ide/ide_router'; import { resetStore } from '../../helpers'; import testAction from '../../../helpers/vuex_action_helper'; +const TEST_PROJECT_ID = 'abc/def'; + describe('IDE store project actions', () => { let mock; let store; @@ -24,7 +26,7 @@ describe('IDE store project actions', () => { store = createStore(); mock = new MockAdapter(axios); - store.state.projects['abc/def'] = { + store.state.projects[TEST_PROJECT_ID] = { branches: {}, }; }); @@ -83,7 +85,7 @@ describe('IDE store project actions', () => { { type: 'SET_BRANCH_COMMIT', payload: { - projectId: 'abc/def', + projectId: TEST_PROJECT_ID, branchId: 'master', commit: { id: '123' }, }, @@ -200,17 +202,17 @@ describe('IDE store project actions', () => { }); }); - describe('showEmptyState', () => { + describe('loadEmptyBranch', () => { it('creates a blank tree and sets loading state to false', done => { testAction( - showEmptyState, - { projectId: 'abc/def', branchId: 'master' }, + loadEmptyBranch, + { projectId: TEST_PROJECT_ID, branchId: 'master' }, store.state, [ - { type: 'CREATE_TREE', payload: { treePath: 'abc/def/master' } }, + { type: 'CREATE_TREE', payload: { treePath: `${TEST_PROJECT_ID}/master` } }, { type: 'TOGGLE_LOADING', - payload: { entry: store.state.trees['abc/def/master'], forceValue: false }, + payload: { entry: store.state.trees[`${TEST_PROJECT_ID}/master`], forceValue: false }, }, ], jasmine.any(Object), @@ -218,13 +220,15 @@ describe('IDE store project actions', () => { ); }); - it('sets the currentBranchId to the branchId that was passed', done => { + it('does nothing, if tree already exists', done => { + const trees = { [`${TEST_PROJECT_ID}/master`]: [] }; + testAction( - showEmptyState, - { projectId: 'abc/def', branchId: 'master' }, - store.state, - jasmine.any(Object), - [{ type: 'setCurrentBranchId', payload: 'master' }], + loadEmptyBranch, + { projectId: TEST_PROJECT_ID, branchId: 'master' }, + { trees }, + [], + [], done, ); }); @@ -278,10 +282,29 @@ describe('IDE store project actions', () => { }); describe('loadBranch', () => { - const projectId = 'abc/def'; + const projectId = TEST_PROJECT_ID; const branchId = '123-lorem'; const ref = 'abcd2322'; + it('when empty repo, loads empty branch', done => { + const mockGetters = { emptyRepo: true }; + + testAction( + loadBranch, + { projectId, branchId }, + { ...store.state, ...mockGetters }, + [], + [{ type: 'loadEmptyBranch', payload: { projectId, branchId } }], + done, + ); + }); + + it('when branch already exists, does nothing', done => { + store.state.projects[projectId].branches[branchId] = {}; + + testAction(loadBranch, { projectId, branchId }, store.state, [], [], done); + }); + it('fetches branch data', done => { const mockGetters = { findBranch: () => ({ commit: { id: ref } }) }; spyOn(store, 'dispatch').and.returnValue(Promise.resolve()); @@ -317,7 +340,7 @@ describe('IDE store project actions', () => { }); describe('openBranch', () => { - const projectId = 'abc/def'; + const projectId = TEST_PROJECT_ID; const branchId = '123-lorem'; const branch = { @@ -335,55 +358,6 @@ describe('IDE store project actions', () => { }); }); - it('loads file right away if the branch has already been fetched', done => { - spyOn(store, 'dispatch'); - - Object.assign(store.state, { - projects: { - [projectId]: { - branches: { - [branchId]: { foo: 'bar' }, - }, - }, - }, - }); - - openBranch(store, branch) - .then(() => { - expect(store.dispatch.calls.allArgs()).toEqual([['loadFile', { basePath: undefined }]]); - }) - .then(done) - .catch(done.fail); - }); - - describe('empty repo', () => { - beforeEach(() => { - spyOn(store, 'dispatch').and.returnValue(Promise.resolve()); - - Object.assign(store.state, { - currentProjectId: 'abc/def', - projects: { - 'abc/def': { - empty_repo: true, - }, - }, - }); - }); - - afterEach(() => { - resetStore(store); - }); - - it('dispatches showEmptyState action right away', done => { - openBranch(store, branch) - .then(() => { - expect(store.dispatch.calls.allArgs()).toEqual([['showEmptyState', branch]]); - done(); - }) - .catch(done.fail); - }); - }); - describe('existing branch', () => { beforeEach(() => { spyOn(store, 'dispatch').and.returnValue(Promise.resolve()); @@ -410,11 +384,17 @@ describe('IDE store project actions', () => { it('dispatches correct branch actions', done => { openBranch(store, branch) - .then(() => { + .then(val => { expect(store.dispatch.calls.allArgs()).toEqual([ ['setCurrentBranchId', branchId], ['loadBranch', { projectId, branchId }], ]); + + expect(val).toEqual( + new Error( + `An error occurred while getting files for - ${projectId}/${branchId}`, + ), + ); }) .then(done) .catch(done.fail);