Merge branch '195776-fix-discard-rename-web-ide' into 'master'

Fix discarding renamed directories in Web IDE

See merge request gitlab-org/gitlab!22943

app/assets/javascripts/ide/components/new_dropdown/modal.vue

@@ -67,8 +67,8 @@ export default {
       if (this.entryModal.type === modalTypes.rename) {
         if (this.entries[this.entryName] && !this.entries[this.entryName].deleted) {
           flash(
-            sprintf(s__('The name %{entryName} is already taken in this directory.'), {
-              entryName: this.entryName,
+            sprintf(s__('The name "%{name}" is already taken in this directory.'), {
+              name: this.entryName,
             }),
             'alert',
             document,
@@ -81,22 +81,11 @@ export default {
           const entryName = parentPath.pop();
           parentPath = parentPath.join('/');
 
-          const createPromise =
-            parentPath && !this.entries[parentPath]
-              ? this.createTempEntry({ name: parentPath, type: 'tree' })
-              : Promise.resolve();
-
-          createPromise
-            .then(() =>
-              this.renameEntry({
-                path: this.entryModal.entry.path,
-                name: entryName,
-                parentPath,
-              }),
-            )
-            .catch(() =>
-              flash(__('Error creating a new path'), 'alert', document, null, false, true),
-            );
+          this.renameEntry({
+            path: this.entryModal.entry.path,
+            name: entryName,
+            parentPath,
+          });
         }
       } else {
         this.createTempEntry({

app/assets/javascripts/ide/stores/actions.js

@@ -53,60 +53,55 @@ export const setResizingStatus = ({ commit }, resizing) => {
 export const createTempEntry = (
   { state, commit, dispatch },
   { name, type, content = '', base64 = false, binary = false, rawPath = '' },
-) =>
-  new Promise(resolve => {
-    const fullName = name.slice(-1) !== '/' && type === 'tree' ? `${name}/` : name;
-
-    if (state.entries[name] && !state.entries[name].deleted) {
-      flash(
-        `The name "${name.split('/').pop()}" is already taken in this directory.`,
-        'alert',
-        document,
-        null,
-        false,
-        true,
-      );
-
-      resolve();
-
-      return null;
-    }
-
-    const data = decorateFiles({
-      data: [fullName],
-      projectId: state.currentProjectId,
-      branchId: state.currentBranchId,
-      type,
-      tempFile: true,
-      content,
-      base64,
-      binary,
-      rawPath,
-    });
-    const { file, parentPath } = data;
+) => {
+  const fullName = name.slice(-1) !== '/' && type === 'tree' ? `${name}/` : name;
+
+  if (state.entries[name] && !state.entries[name].deleted) {
+    flash(
+      sprintf(__('The name "%{name}" is already taken in this directory.'), {
+        name: name.split('/').pop(),
+      }),
+      'alert',
+      document,
+      null,
+      false,
+      true,
+    );
 
-    commit(types.CREATE_TMP_ENTRY, {
-      data,
-      projectId: state.currentProjectId,
-      branchId: state.currentBranchId,
-    });
+    return;
+  }
 
-    if (type === 'blob') {
-      commit(types.TOGGLE_FILE_OPEN, file.path);
-      commit(types.ADD_FILE_TO_CHANGED, file.path);
-      dispatch('setFileActive', file.path);
-      dispatch('triggerFilesChange');
-      dispatch('burstUnusedSeal');
-    }
+  const data = decorateFiles({
+    data: [fullName],
+    projectId: state.currentProjectId,
+    branchId: state.currentBranchId,
+    type,
+    tempFile: true,
+    content,
+    base64,
+    binary,
+    rawPath,
+  });
+  const { file, parentPath } = data;
 
-    if (parentPath && !state.entries[parentPath].opened) {
-      commit(types.TOGGLE_TREE_OPEN, parentPath);
-    }
+  commit(types.CREATE_TMP_ENTRY, {
+    data,
+    projectId: state.currentProjectId,
+    branchId: state.currentBranchId,
+  });
 
-    resolve(file);
+  if (type === 'blob') {
+    commit(types.TOGGLE_FILE_OPEN, file.path);
+    commit(types.ADD_FILE_TO_CHANGED, file.path);
+    dispatch('setFileActive', file.path);
+    dispatch('triggerFilesChange');
+    dispatch('burstUnusedSeal');
+  }
 
-    return null;
-  });
+  if (parentPath && !state.entries[parentPath].opened) {
+    commit(types.TOGGLE_TREE_OPEN, parentPath);
+  }
+};
 
 export const scrollToTab = () => {
   Vue.nextTick(() => {
@@ -211,8 +206,9 @@ export const deleteEntry = ({ commit, dispatch, state }, path) => {
   const entry = state.entries[path];
   const { prevPath, prevName, prevParentPath } = entry;
   const isTree = entry.type === 'tree';
+  const prevEntry = prevPath && state.entries[prevPath];
 
-  if (prevPath) {
+  if (prevPath && (!prevEntry || prevEntry.deleted)) {
     dispatch('renameEntry', {
       path,
       name: prevName,
@@ -245,6 +241,11 @@ export const resetOpenFiles = ({ commit }) => commit(types.RESET_OPEN_FILES);
 export const renameEntry = ({ dispatch, commit, state }, { path, name, parentPath }) => {
   const entry = state.entries[path];
   const newPath = parentPath ? `${parentPath}/${name}` : name;
+  const existingParent = parentPath && state.entries[parentPath];
+
+  if (parentPath && (!existingParent || existingParent.deleted)) {
+    dispatch('createTempEntry', { name: parentPath, type: 'tree' });
+  }
 
   commit(types.RENAME_ENTRY, { path, name, parentPath });
 

changelogs/unreleased/195776-fix-discard-rename-web-ide.yml

+---
+title: Fix discarding renamed directories in Web IDE
+merge_request: 22943
+author:
+type: fixed

locale/gitlab.pot

@@ -7209,9 +7209,6 @@ msgstr ""
 msgid "Error Tracking"
 msgstr ""
 
-msgid "Error creating a new path"
-msgstr ""
-
 msgid "Error creating epic"
 msgstr ""
 
@@ -18263,7 +18260,7 @@ msgstr ""
 msgid "The merge request can now be merged."
 msgstr ""
 
-msgid "The name %{entryName} is already taken in this directory."
+msgid "The name \"%{name}\" is already taken in this directory."
 msgstr ""
 
 msgid "The number of changes to be fetched from GitLab when cloning a repository. This can speed up Pipelines execution. Keep empty or set to 0 to disable shallow clone by default and make GitLab CI fetch all branches and tags each time."

spec/javascripts/ide/components/new_dropdown/modal_spec.js

@@ -52,19 +52,6 @@ describe('new file modal component', () => {
           expect(templateFilesEl instanceof Element).toBeTruthy();
         }
       });
-
-      describe('createEntryInStore', () => {
-        it('$emits create', () => {
-          spyOn(vm, 'createTempEntry');
-
-          vm.submitForm();
-
-          expect(vm.createTempEntry).toHaveBeenCalledWith({
-            name: 'testing',
-            type,
-          });
-        });
-      });
     });
   });
 
@@ -145,31 +132,19 @@ describe('new file modal component', () => {
 
       vm = createComponentWithStore(Component, store).$mount();
       const flashSpy = spyOnDependency(modal, 'flash');
-      vm.submitForm();
 
-      expect(flashSpy).toHaveBeenCalled();
-    });
+      expect(flashSpy).not.toHaveBeenCalled();
 
-    it('calls createTempEntry when target path does not exist', () => {
-      const store = createStore();
-      store.state.entryModal = {
-        type: 'rename',
-        path: 'test-path/test',
-        entry: {
-          name: 'test',
-          type: 'blob',
-          path: 'test-path1/test',
-        },
-      };
-
-      vm = createComponentWithStore(Component, store).$mount();
-      spyOn(vm, 'createTempEntry').and.callFake(() => Promise.resolve());
       vm.submitForm();
 
-      expect(vm.createTempEntry).toHaveBeenCalledWith({
-        name: 'test-path1',
-        type: 'tree',
-      });
+      expect(flashSpy).toHaveBeenCalledWith(
+        'The name "test-path/test" is already taken in this directory.',
+        'alert',
+        jasmine.anything(),
+        null,
+        false,
+        true,
+      );
     });
   });
 });

spec/javascripts/ide/stores/actions_spec.js

@@ -206,13 +206,17 @@ describe('Multi-file store actions', () => {
 
     describe('blob', () => {
       it('creates temp file', done => {
+        const name = 'test';
+
         store
           .dispatch('createTempEntry', {
-            name: 'test',
+            name,
             branchId: 'mybranch',
             type: 'blob',
           })
-          .then(f => {
+          .then(() => {
+            const f = store.state.entries[name];
+
             expect(f.tempFile).toBeTruthy();
             expect(store.state.trees['abcproject/mybranch'].tree.length).toBe(1);
 
@@ -222,13 +226,17 @@ describe('Multi-file store actions', () => {
       });
 
       it('adds tmp file to open files', done => {
+        const name = 'test';
+
         store
           .dispatch('createTempEntry', {
-            name: 'test',
+            name,
             branchId: 'mybranch',
             type: 'blob',
           })
-          .then(f => {
+          .then(() => {
+            const f = store.state.entries[name];
+
             expect(store.state.openFiles.length).toBe(1);
             expect(store.state.openFiles[0].name).toBe(f.name);
 
@@ -238,13 +246,17 @@ describe('Multi-file store actions', () => {
       });
 
       it('adds tmp file to changed files', done => {
+        const name = 'test';
+
         store
           .dispatch('createTempEntry', {
-            name: 'test',
+            name,
             branchId: 'mybranch',
             type: 'blob',
           })
-          .then(f => {
+          .then(() => {
+            const f = store.state.entries[name];
+
             expect(store.state.changedFiles.length).toBe(1);
             expect(store.state.changedFiles[0].name).toBe(f.name);
 
@@ -292,7 +304,9 @@ describe('Multi-file store actions', () => {
             type: 'blob',
           })
           .then(() => {
-            expect(document.querySelector('.flash-alert')).not.toBeNull();
+            expect(document.querySelector('.flash-alert')?.textContent.trim()).toEqual(
+              `The name "${f.name}" is already taken in this directory.`,
+            );
 
             done();
           })
@@ -604,36 +618,98 @@ describe('Multi-file store actions', () => {
       );
     });
 
-    it('if renamed, reverts the rename before deleting', () => {
-      const testEntry = {
-        path: 'test',
-        name: 'test',
-        prevPath: 'lorem/ipsum',
-        prevName: 'ipsum',
-        prevParentPath: 'lorem',
-      };
+    describe('when renamed', () => {
+      let testEntry;
 
-      store.state.entries = { test: testEntry };
-      testAction(
-        deleteEntry,
-        testEntry.path,
-        store.state,
-        [],
-        [
-          {
-            type: 'renameEntry',
-            payload: {
-              path: testEntry.path,
-              name: testEntry.prevName,
-              parentPath: testEntry.prevParentPath,
-            },
-          },
-          {
-            type: 'deleteEntry',
-            payload: testEntry.prevPath,
-          },
-        ],
-      );
+      beforeEach(() => {
+        testEntry = {
+          path: 'test',
+          name: 'test',
+          prevPath: 'test_old',
+          prevName: 'test_old',
+          prevParentPath: '',
+        };
+
+        store.state.entries = { test: testEntry };
+      });
+
+      describe('and previous does not exist', () => {
+        it('reverts the rename before deleting', done => {
+          testAction(
+            deleteEntry,
+            testEntry.path,
+            store.state,
+            [],
+            [
+              {
+                type: 'renameEntry',
+                payload: {
+                  path: testEntry.path,
+                  name: testEntry.prevName,
+                  parentPath: testEntry.prevParentPath,
+                },
+              },
+              {
+                type: 'deleteEntry',
+                payload: testEntry.prevPath,
+              },
+            ],
+            done,
+          );
+        });
+      });
+
+      describe('and previous exists', () => {
+        beforeEach(() => {
+          const oldEntry = {
+            path: testEntry.prevPath,
+            name: testEntry.prevName,
+          };
+
+          store.state.entries[oldEntry.path] = oldEntry;
+        });
+
+        it('does not revert rename before deleting', done => {
+          testAction(
+            deleteEntry,
+            testEntry.path,
+            store.state,
+            [{ type: types.DELETE_ENTRY, payload: testEntry.path }],
+            [
+              { type: 'burstUnusedSeal' },
+              { type: 'stageChange', payload: testEntry.path },
+              { type: 'triggerFilesChange' },
+            ],
+            done,
+          );
+        });
+
+        it('when previous is deleted, it reverts rename before deleting', done => {
+          store.state.entries[testEntry.prevPath].deleted = true;
+
+          testAction(
+            deleteEntry,
+            testEntry.path,
+            store.state,
+            [],
+            [
+              {
+                type: 'renameEntry',
+                payload: {
+                  path: testEntry.path,
+                  name: testEntry.prevName,
+                  parentPath: testEntry.prevParentPath,
+                },
+              },
+              {
+                type: 'deleteEntry',
+                payload: testEntry.prevPath,
+              },
+            ],
+            done,
+          );
+        });
+      });
     });
 
     it('bursts unused seal', done => {
@@ -918,6 +994,103 @@ describe('Multi-file store actions', () => {
           .then(done)
           .catch(done.fail);
       });
+
+      describe('with file in directory', () => {
+        const parentPath = 'original-dir';
+        const newParentPath = 'new-dir';
+        const fileName = 'test.md';
+        const filePath = `${parentPath}/${fileName}`;
+
+        let rootDir;
+
+        beforeEach(() => {
+          const parentEntry = file(parentPath, parentPath, 'tree');
+          const fileEntry = file(filePath, filePath, 'blob', parentEntry);
+          rootDir = {
+            tree: [],
+          };
+
+          Object.assign(store.state, {
+            entries: {
+              [parentPath]: {
+                ...parentEntry,
+                tree: [fileEntry],
+              },
+              [filePath]: fileEntry,
+            },
+            trees: {
+              '/': rootDir,
+            },
+          });
+        });
+
+        it('creates new directory', done => {
+          expect(store.state.entries[newParentPath]).toBeUndefined();
+
+          store
+            .dispatch('renameEntry', { path: filePath, name: fileName, parentPath: newParentPath })
+            .then(() => {
+              expect(store.state.entries[newParentPath]).toEqual(
+                jasmine.objectContaining({
+                  path: newParentPath,
+                  type: 'tree',
+                  tree: jasmine.arrayContaining([
+                    store.state.entries[`${newParentPath}/${fileName}`],
+                  ]),
+                }),
+              );
+            })
+            .then(done)
+            .catch(done.fail);
+        });
+
+        describe('when new directory exists', () => {
+          let newDir;
+
+          beforeEach(() => {
+            newDir = file(newParentPath, newParentPath, 'tree');
+
+            store.state.entries[newDir.path] = newDir;
+            rootDir.tree.push(newDir);
+          });
+
+          it('inserts in new directory', done => {
+            expect(newDir.tree).toEqual([]);
+
+            store
+              .dispatch('renameEntry', {
+                path: filePath,
+                name: fileName,
+                parentPath: newParentPath,
+              })
+              .then(() => {
+                expect(newDir.tree).toEqual([store.state.entries[`${newParentPath}/${fileName}`]]);
+              })
+              .then(done)
+              .catch(done.fail);
+          });
+
+          it('when new directory is deleted, it undeletes it', done => {
+            store.dispatch('deleteEntry', newParentPath);
+
+            expect(store.state.entries[newParentPath].deleted).toBe(true);
+            expect(rootDir.tree.some(x => x.path === newParentPath)).toBe(false);
+
+            store
+              .dispatch('renameEntry', {
+                path: filePath,
+                name: fileName,
+                parentPath: newParentPath,
+              })
+              .then(() => {
+                expect(store.state.entries[newParentPath].deleted).toBe(false);
+                expect(rootDir.tree.some(x => x.path === newParentPath)).toBe(true);
+              })
+              .then(done)
+              .catch(done.fail);
+          });
+        });
+      });
     });
   });