Merge branch 'fe-ide-clean-up-discard-duplication' into 'master'

Clean up discard duplication in IDE

See merge request gitlab-org/gitlab!22572

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

@@ -16,21 +16,7 @@ export const redirectToUrl = (self, url) => visitUrl(url);
 export const setInitialData = ({ commit }, data) => commit(types.SET_INITIAL_DATA, data);
 
 export const discardAllChanges = ({ state, commit, dispatch }) => {
-  state.changedFiles.forEach(file => {
-    if (file.tempFile || file.prevPath) dispatch('closeFile', file);
-
-    if (file.tempFile) {
-      dispatch('deleteEntry', file.path);
-    } else if (file.prevPath) {
-      dispatch('renameEntry', {
-        path: file.path,
-        name: file.prevName,
-        parentPath: file.prevParentPath,
-      });
-    } else {
-      commit(types.DISCARD_FILE_CHANGES, file.path);
-    }
-  });
+  state.changedFiles.forEach(file => dispatch('restoreOriginalFile', file.path));
 
   commit(types.REMOVE_ALL_CHANGES_FILES);
 };

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

@@ -191,38 +191,47 @@ export const setFileViewMode = ({ commit }, { file, viewMode }) => {
   commit(types.SET_FILE_VIEWMODE, { file, viewMode });
 };
 
-export const discardFileChanges = ({ dispatch, state, commit, getters }, path) => {
+export const restoreOriginalFile = ({ dispatch, state, commit }, path) => {
   const file = state.entries[path];
+  const isDestructiveDiscard = file.tempFile || file.prevPath;
 
   if (file.deleted && file.parentPath) {
     dispatch('restoreTree', file.parentPath);
   }
 
-  if (file.tempFile || file.prevPath) {
+  if (isDestructiveDiscard) {
     dispatch('closeFile', file);
+  }
 
-    if (file.tempFile) {
-      dispatch('deleteEntry', file.path);
-    } else {
-      commit(types.DISCARD_FILE_CHANGES, file.path);
-      dispatch('renameEntry', {
-        path: file.path,
-        name: file.prevName,
-        parentPath: file.prevParentPath,
-      });
-    }
+  if (file.tempFile) {
+    dispatch('deleteEntry', file.path);
   } else {
-    commit(types.DISCARD_FILE_CHANGES, path);
-
-    if (getters.activeFile && file.path === getters.activeFile.path) {
-      dispatch('updateDelayViewerUpdated', true)
-        .then(() => {
-          router.push(`/project${file.url}`);
-        })
-        .catch(e => {
-          throw e;
-        });
-    }
+    commit(types.DISCARD_FILE_CHANGES, file.path);
+  }
+
+  if (file.prevPath) {
+    dispatch('renameEntry', {
+      path: file.path,
+      name: file.prevName,
+      parentPath: file.prevParentPath,
+    });
+  }
+};
+
+export const discardFileChanges = ({ dispatch, state, commit, getters }, path) => {
+  const file = state.entries[path];
+  const isDestructiveDiscard = file.tempFile || file.prevPath;
+
+  dispatch('restoreOriginalFile', path);
+
+  if (!isDestructiveDiscard && file.path === getters.activeFile?.path) {
+    dispatch('updateDelayViewerUpdated', true)
+      .then(() => {
+        router.push(`/project${file.url}`);
+      })
+      .catch(e => {
+        throw e;
+      });
   }
 
   commit(types.REMOVE_FILE_FROM_CHANGED, path);

changelogs/unreleased/fe-ide-clean-up-discard-duplication.yml

+---
+title: Fix discard all to behave like discard single file in Web IDE
+merge_request: 22572
+author:
+type: fixed

spec/frontend/ide/stores/actions/file_spec.js

@@ -619,107 +619,113 @@ describe('IDE store file actions', () => {
     });
   });
 
-  describe('discardFileChanges', () => {
+  describe('with changed file', () => {
     let tmpFile;
 
     beforeEach(() => {
-      jest.spyOn(eventHub, '$on').mockImplementation(() => {});
-      jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
-
       tmpFile = file('tempFile');
       tmpFile.content = 'testing';
       tmpFile.raw = ORIGINAL_CONTENT;
 
       store.state.changedFiles.push(tmpFile);
       store.state.entries[tmpFile.path] = tmpFile;
-
-      jest.spyOn(store, 'dispatch');
     });
 
-    it('resets file content', done => {
-      store
-        .dispatch('discardFileChanges', tmpFile.path)
-        .then(() => {
+    describe('restoreOriginalFile', () => {
+      it('resets file content', () =>
+        store.dispatch('restoreOriginalFile', tmpFile.path).then(() => {
           expect(tmpFile.content).toBe(ORIGINAL_CONTENT);
+        }));
 
-          done();
-        })
-        .catch(done.fail);
-    });
+      it('closes temp file and deletes it', () => {
+        tmpFile.tempFile = true;
+        tmpFile.opened = true;
+        tmpFile.parentPath = 'parentFile';
+        store.state.entries.parentFile = file('parentFile');
 
-    it('removes file from changedFiles array', done => {
-      store
-        .dispatch('discardFileChanges', tmpFile.path)
-        .then(() => {
-          expect(store.state.changedFiles.length).toBe(0);
+        actions.restoreOriginalFile(store, tmpFile.path);
 
-          done();
-        })
-        .catch(done.fail);
-    });
+        expect(store.dispatch).toHaveBeenCalledWith('closeFile', tmpFile);
+        expect(store.dispatch).toHaveBeenCalledWith('deleteEntry', tmpFile.path);
+      });
 
-    it('closes temp file and deletes it', () => {
-      tmpFile.tempFile = true;
-      tmpFile.opened = true;
-      tmpFile.parentPath = 'parentFile';
-      store.state.entries.parentFile = file('parentFile');
+      describe('with renamed file', () => {
+        beforeEach(() => {
+          Object.assign(tmpFile, {
+            prevPath: 'parentPath/old_name',
+            prevName: 'old_name',
+            prevParentPath: 'parentPath',
+          });
 
-      actions.discardFileChanges(store, tmpFile.path);
+          store.state.entries.parentPath = file('parentPath');
 
-      expect(store.dispatch).toHaveBeenCalledWith('closeFile', tmpFile);
-      expect(store.dispatch).toHaveBeenCalledWith('deleteEntry', tmpFile.path);
-    });
+          actions.restoreOriginalFile(store, tmpFile.path);
+        });
 
-    describe('with renamed file', () => {
-      beforeEach(() => {
-        Object.assign(tmpFile, {
-          prevPath: 'parentPath/old_name',
-          prevName: 'old_name',
-          prevParentPath: 'parentPath',
+        it('renames the file to its original name and closes it if it was open', () => {
+          expect(store.dispatch).toHaveBeenCalledWith('closeFile', tmpFile);
+          expect(store.dispatch).toHaveBeenCalledWith('renameEntry', {
+            path: 'tempFile',
+            name: 'old_name',
+            parentPath: 'parentPath',
+          });
         });
 
-        store.state.entries.parentPath = file('parentPath');
+        it('resets file content', () => {
+          expect(tmpFile.content).toBe(ORIGINAL_CONTENT);
+        });
+      });
+    });
 
-        actions.discardFileChanges(store, tmpFile.path);
+    describe('discardFileChanges', () => {
+      beforeEach(() => {
+        jest.spyOn(eventHub, '$on').mockImplementation(() => {});
+        jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
       });
 
-      it('renames the file to its original name and closes it if it was open', () => {
-        expect(store.dispatch).toHaveBeenCalledWith('closeFile', tmpFile);
-        expect(store.dispatch).toHaveBeenCalledWith('renameEntry', {
-          path: 'tempFile',
-          name: 'old_name',
-          parentPath: 'parentPath',
+      describe('with regular file', () => {
+        beforeEach(() => {
+          actions.discardFileChanges(store, tmpFile.path);
         });
-      });
 
-      it('resets file content', () => {
-        expect(tmpFile.content).toBe(ORIGINAL_CONTENT);
-      });
-    });
+        it('restores original file', () => {
+          expect(store.dispatch).toHaveBeenCalledWith('restoreOriginalFile', tmpFile.path);
+        });
 
-    it('pushes route for active file', done => {
-      tmpFile.active = true;
-      store.state.openFiles.push(tmpFile);
+        it('removes file from changedFiles array', () => {
+          expect(store.state.changedFiles.length).toBe(0);
+        });
 
-      store
-        .dispatch('discardFileChanges', tmpFile.path)
-        .then(() => {
-          expect(router.push).toHaveBeenCalledWith(`/project${tmpFile.url}`);
+        it('does not push a new route', () => {
+          expect(router.push).not.toHaveBeenCalled();
+        });
 
-          done();
-        })
-        .catch(done.fail);
-    });
+        it('emits eventHub event to dispose cached model', () => {
+          actions.discardFileChanges(store, tmpFile.path);
+
+          expect(eventHub.$emit).toHaveBeenCalledWith(
+            `editor.update.model.new.content.${tmpFile.key}`,
+            ORIGINAL_CONTENT,
+          );
+          expect(eventHub.$emit).toHaveBeenCalledWith(
+            `editor.update.model.dispose.unstaged-${tmpFile.key}`,
+            ORIGINAL_CONTENT,
+          );
+        });
+      });
 
-    it('emits eventHub event to dispose cached model', done => {
-      store
-        .dispatch('discardFileChanges', tmpFile.path)
-        .then(() => {
-          expect(eventHub.$emit).toHaveBeenCalled();
+      describe('with active file', () => {
+        beforeEach(() => {
+          tmpFile.active = true;
+          store.state.openFiles.push(tmpFile);
 
-          done();
-        })
-        .catch(done.fail);
+          actions.discardFileChanges(store, tmpFile.path);
+        });
+
+        it('pushes route for active file', () => {
+          expect(router.push).toHaveBeenCalledWith(`/project${tmpFile.url}`);
+        });
+      });
     });
   });
 

spec/javascripts/ide/stores/actions_spec.js

@@ -61,24 +61,25 @@ describe('Multi-file store actions', () => {
   });
 
   describe('discardAllChanges', () => {
-    let f;
+    const paths = ['to_discard', 'another_one_to_discard'];
+
     beforeEach(() => {
-      f = file('discardAll');
-      f.changed = true;
+      paths.forEach(path => {
+        const f = file(path);
+        f.changed = true;
 
-      store.state.openFiles.push(f);
-      store.state.changedFiles.push(f);
-      store.state.entries[f.path] = f;
+        store.state.openFiles.push(f);
+        store.state.changedFiles.push(f);
+        store.state.entries[f.path] = f;
+      });
     });
 
-    it('discards changes in file', done => {
-      store
-        .dispatch('discardAllChanges')
-        .then(() => {
-          expect(store.state.openFiles.changed).toBeFalsy();
-        })
-        .then(done)
-        .catch(done.fail);
+    it('discards all changes in file', () => {
+      const expectedCalls = paths.map(path => ['restoreOriginalFile', path]);
+
+      discardAllChanges(store);
+
+      expect(store.dispatch.calls.allArgs()).toEqual(jasmine.arrayContaining(expectedCalls));
     });
 
     it('removes all files from changedFiles state', done => {
@@ -86,64 +87,11 @@ describe('Multi-file store actions', () => {
         .dispatch('discardAllChanges')
         .then(() => {
           expect(store.state.changedFiles.length).toBe(0);
-          expect(store.state.openFiles.length).toBe(1);
+          expect(store.state.openFiles.length).toBe(2);
         })
         .then(done)
         .catch(done.fail);
     });
-
-    it('closes the temp file and deletes it if it was open', done => {
-      f.tempFile = true;
-
-      testAction(
-        discardAllChanges,
-        undefined,
-        store.state,
-        [{ type: types.REMOVE_ALL_CHANGES_FILES }],
-        [
-          { type: 'closeFile', payload: jasmine.objectContaining({ path: 'discardAll' }) },
-          { type: 'deleteEntry', payload: 'discardAll' },
-        ],
-        done,
-      );
-    });
-
-    it('renames the file to its original name and closes it if it was open', done => {
-      Object.assign(f, {
-        prevPath: 'parent/path/old_name',
-        prevName: 'old_name',
-        prevParentPath: 'parent/path',
-      });
-
-      testAction(
-        discardAllChanges,
-        undefined,
-        store.state,
-        [{ type: types.REMOVE_ALL_CHANGES_FILES }],
-        [
-          { type: 'closeFile', payload: jasmine.objectContaining({ path: 'discardAll' }) },
-          {
-            type: 'renameEntry',
-            payload: { path: 'discardAll', name: 'old_name', parentPath: 'parent/path' },
-          },
-        ],
-        done,
-      );
-    });
-
-    it('discards file changes on all other files', done => {
-      testAction(
-        discardAllChanges,
-        undefined,
-        store.state,
-        [
-          { type: types.DISCARD_FILE_CHANGES, payload: 'discardAll' },
-          { type: types.REMOVE_ALL_CHANGES_FILES },
-        ],
-        [],
-        done,
-      );
-    });
   });
 
   describe('closeAllFiles', () => {