Merge branch '27915-fix-ide-empty-repo' into 'master'

Fix IDE on empty repo

Closes #27915

See merge request gitlab-org/gitlab!25463

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 {
   <div class="append-bottom-15 ide-commit-options">
     <radio-group
       :value="$options.commitToCurrentBranch"
-      :disabled="currentBranch && !currentBranch.can_push"
+      :disabled="!canPushToBranch"
       :title="$options.currentBranchPermissionsTooltip"
     >
       <span
@@ -77,11 +82,13 @@ export default {
         v-html="commitToCurrentBranchText"
       ></span>
     </radio-group>
-    <radio-group
-      :value="$options.commitToNewBranch"
-      :label="__('Create a new branch')"
-      :show-input="true"
-    />
-    <new-merge-request-option />
+    <template v-if="!emptyRepo">
+      <radio-group
+        :value="$options.commitToNewBranch"
+        :label="__('Create a new branch')"
+        :show-input="true"
+      />
+      <new-merge-request-option />
+    </template>
   </div>
 </template>

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',

app/assets/javascripts/ide/queries/getUserPermissions.query.graphql

@@ -2,7 +2,8 @@ query getUserPermissions($projectPath: ID!) {
   project(fullPath: $projectPath) {
     userPermissions {
       createMergeRequestIn,
-      readMergeRequest
+      readMergeRequest,
+      pushCode
     }
   }
 }

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: `<strong>${esc(projectId)}/${esc(branchId)}</strong>`,
-              },
-              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: `<strong>${esc(projectId)}/${esc(branchId)}</strong>`,
+            },
+            false,
           ),
-      );
-  }
-  return Promise.resolve(dispatch('loadFile', { basePath }));
+        ),
+    );
 };

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 () => {};

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}`,
               );
             }
           }

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;

changelogs/unreleased/27915-fix-ide-empty-repo.yml

+---
+title: Fix some Web IDE bugs with empty projects
+merge_request: 25463
+author:
+type: fixed

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');
+    });
+  });
 });

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] = {

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);
+    });
+  });
 });

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 - <strong>${projectId}/${branchId}</strong>`,
+              ),
+            );
           })
           .then(done)
           .catch(done.fail);