Merge branch '39498-part-3' into 'master'

!21542 Part 2 & 3: Handle edge cases in stage and unstage mutations

See merge request gitlab-org/gitlab!21676

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

@@ -134,28 +134,40 @@ export const scrollToTab = () => {
   });
 };
 
-export const stageAllChanges = ({ state, commit, dispatch }) => {
+export const stageAllChanges = ({ state, commit, dispatch, getters }) => {
   const openFile = state.openFiles[0];
 
   commit(types.SET_LAST_COMMIT_MSG, '');
 
-  state.changedFiles.forEach(file => commit(types.STAGE_CHANGE, file.path));
+  state.changedFiles.forEach(file =>
+    commit(types.STAGE_CHANGE, { path: file.path, diffInfo: getters.getDiffInfo(file.path) }),
+  );
 
-  dispatch('openPendingTab', {
-    file: state.stagedFiles.find(f => f.path === openFile.path),
-    keyPrefix: stageKeys.staged,
-  });
+  const file = getters.getStagedFile(openFile.path);
+
+  if (file) {
+    dispatch('openPendingTab', {
+      file,
+      keyPrefix: stageKeys.staged,
+    });
+  }
 };
 
-export const unstageAllChanges = ({ state, commit, dispatch }) => {
+export const unstageAllChanges = ({ state, commit, dispatch, getters }) => {
   const openFile = state.openFiles[0];
 
-  state.stagedFiles.forEach(file => commit(types.UNSTAGE_CHANGE, file.path));
+  state.stagedFiles.forEach(file =>
+    commit(types.UNSTAGE_CHANGE, { path: file.path, diffInfo: getters.getDiffInfo(file.path) }),
+  );
 
-  dispatch('openPendingTab', {
-    file: state.changedFiles.find(f => f.path === openFile.path),
-    keyPrefix: stageKeys.unstaged,
-  });
+  const file = getters.getChangedFile(openFile.path);
+
+  if (file) {
+    dispatch('openPendingTab', {
+      file,
+      keyPrefix: stageKeys.unstaged,
+    });
+  }
 };
 
 export const updateViewer = ({ commit }, viewer) => {

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

@@ -214,20 +214,20 @@ export const discardFileChanges = ({ dispatch, state, commit, getters }, path) =
   eventHub.$emit(`editor.update.model.dispose.unstaged-${file.key}`, file.content);
 };
 
-export const stageChange = ({ commit, state, dispatch }, path) => {
-  const stagedFile = state.stagedFiles.find(f => f.path === path);
-  const openFile = state.openFiles.find(f => f.path === path);
+export const stageChange = ({ commit, dispatch, getters }, path) => {
+  const stagedFile = getters.getStagedFile(path);
+  const openFile = getters.getOpenFile(path);
 
-  commit(types.STAGE_CHANGE, path);
+  commit(types.STAGE_CHANGE, { path, diffInfo: getters.getDiffInfo(path) });
   commit(types.SET_LAST_COMMIT_MSG, '');
 
   if (stagedFile) {
     eventHub.$emit(`editor.update.model.new.content.staged-${stagedFile.key}`, stagedFile.content);
   }
 
-  if (openFile && openFile.active) {
-    const file = state.stagedFiles.find(f => f.path === path);
+  const file = getters.getStagedFile(path);
 
+  if (openFile && openFile.active && file) {
     dispatch('openPendingTab', {
       file,
       keyPrefix: stageKeys.staged,
@@ -235,14 +235,14 @@ export const stageChange = ({ commit, state, dispatch }, path) => {
   }
 };
 
-export const unstageChange = ({ commit, dispatch, state }, path) => {
-  const openFile = state.openFiles.find(f => f.path === path);
+export const unstageChange = ({ commit, dispatch, getters }, path) => {
+  const openFile = getters.getOpenFile(path);
 
-  commit(types.UNSTAGE_CHANGE, path);
+  commit(types.UNSTAGE_CHANGE, { path, diffInfo: getters.getDiffInfo(path) });
 
-  if (openFile && openFile.active) {
-    const file = state.changedFiles.find(f => f.path === path);
+  const file = getters.getChangedFile(path);
 
+  if (openFile && openFile.active && file) {
     dispatch('openPendingTab', {
       file,
       keyPrefix: stageKeys.unstaged,

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

@@ -64,6 +64,7 @@ export const allBlobs = state =>
 
 export const getChangedFile = state => path => state.changedFiles.find(f => f.path === path);
 export const getStagedFile = state => path => state.stagedFiles.find(f => f.path === path);
+export const getOpenFile = state => path => state.openFiles.find(f => f.path === path);
 
 export const lastOpenedFile = state =>
   [...state.changedFiles, ...state.stagedFiles].sort((a, b) => b.lastOpenedAt - a.lastOpenedAt)[0];

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

@@ -164,31 +164,32 @@ export default {
       changedFiles: state.changedFiles.filter(f => f.path !== path),
     });
   },
-  [types.STAGE_CHANGE](state, path) {
+  [types.STAGE_CHANGE](state, { path, diffInfo }) {
     const stagedFile = state.stagedFiles.find(f => f.path === path);
 
     Object.assign(state, {
       changedFiles: state.changedFiles.filter(f => f.path !== path),
       entries: Object.assign(state.entries, {
         [path]: Object.assign(state.entries[path], {
-          staged: true,
+          staged: diffInfo.exists,
+          changed: diffInfo.changed,
+          tempFile: diffInfo.tempFile,
+          deleted: diffInfo.deleted,
         }),
       }),
     });
 
     if (stagedFile) {
-      Object.assign(stagedFile, {
-        ...state.entries[path],
-      });
+      Object.assign(stagedFile, { ...state.entries[path] });
     } else {
-      Object.assign(state, {
-        stagedFiles: state.stagedFiles.concat({
-          ...state.entries[path],
-        }),
-      });
+      state.stagedFiles = [...state.stagedFiles, { ...state.entries[path] }];
+    }
+
+    if (!diffInfo.exists) {
+      state.stagedFiles = state.stagedFiles.filter(f => f.path !== path);
     }
   },
-  [types.UNSTAGE_CHANGE](state, path) {
+  [types.UNSTAGE_CHANGE](state, { path, diffInfo }) {
     const changedFile = state.changedFiles.find(f => f.path === path);
     const stagedFile = state.stagedFiles.find(f => f.path === path);
 
@@ -201,9 +202,11 @@ export default {
         changed: true,
       });
 
-      Object.assign(state, {
-        changedFiles: state.changedFiles.concat(state.entries[path]),
-      });
+      state.changedFiles = state.changedFiles.concat(state.entries[path]);
+    }
+
+    if (!diffInfo.exists) {
+      state.changedFiles = state.changedFiles.filter(f => f.path !== path);
     }
 
     Object.assign(state, {
@@ -211,6 +214,9 @@ export default {
       entries: Object.assign(state.entries, {
         [path]: Object.assign(state.entries[path], {
           staged: false,
+          changed: diffInfo.changed,
+          tempFile: diffInfo.tempFile,
+          deleted: diffInfo.deleted,
         }),
       }),
     });

changelogs/unreleased/39498-part-3.yml

+---
+title: "!21542 Part 3: Handle edge cases in stage and unstage mutations"
+merge_request: 21676
+author:
+type: fixed

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

 import Vue from 'vue';
 import MockAdapter from 'axios-mock-adapter';
 import axios from '~/lib/utils/axios_utils';
-import store from '~/ide/stores';
+import { createStore } from '~/ide/stores';
 import * as actions from '~/ide/stores/actions/file';
 import * as types from '~/ide/stores/mutation_types';
 import service from '~/ide/services';
 import router from '~/ide/ide_router';
 import eventHub from '~/ide/eventhub';
-import { file, resetStore } from '../../helpers';
-import testAction from '../../../helpers/vuex_action_helper';
+import { file } from '../../helpers';
 
 const RELATIVE_URL_ROOT = '/gitlab';
 
 describe('IDE store file actions', () => {
   let mock;
   let originalGon;
+  let store;
 
   beforeEach(() => {
     mock = new MockAdapter(axios);
@@ -24,12 +24,15 @@ describe('IDE store file actions', () => {
       relative_url_root: RELATIVE_URL_ROOT,
     };
 
+    store = createStore();
+
+    jest.spyOn(store, 'commit');
+    jest.spyOn(store, 'dispatch');
     jest.spyOn(router, 'push').mockImplementation(() => {});
   });
 
   afterEach(() => {
     mock.restore();
-    resetStore(store);
     window.gon = originalGon;
   });
 
@@ -663,30 +666,33 @@ describe('IDE store file actions', () => {
   });
 
   describe('stageChange', () => {
-    it('calls STAGE_CHANGE with file path', done => {
-      testAction(
-        actions.stageChange,
-        'path',
-        store.state,
-        [
-          { type: types.STAGE_CHANGE, payload: 'path' },
-          { type: types.SET_LAST_COMMIT_MSG, payload: '' },
-        ],
-        [],
-        done,
+    it('calls STAGE_CHANGE with file path', () => {
+      const f = { ...file('path'), content: 'old' };
+
+      store.state.entries[f.path] = f;
+
+      actions.stageChange(store, 'path');
+
+      expect(store.commit).toHaveBeenCalledWith(
+        types.STAGE_CHANGE,
+        expect.objectContaining({ path: 'path' }),
       );
+      expect(store.commit).toHaveBeenCalledWith(types.SET_LAST_COMMIT_MSG, '');
     });
   });
 
   describe('unstageChange', () => {
-    it('calls UNSTAGE_CHANGE with file path', done => {
-      testAction(
-        actions.unstageChange,
-        'path',
-        store.state,
-        [{ type: types.UNSTAGE_CHANGE, payload: 'path' }],
-        [],
-        done,
+    it('calls UNSTAGE_CHANGE with file path', () => {
+      const f = { ...file('path'), content: 'old' };
+
+      store.state.entries[f.path] = f;
+      store.state.stagedFiles.push({ f, content: 'new' });
+
+      actions.unstageChange(store, 'path');
+
+      expect(store.commit).toHaveBeenCalledWith(
+        types.UNSTAGE_CHANGE,
+        expect.objectContaining({ path: 'path' }),
       );
     });
   });

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

 import mutations from '~/ide/stores/mutations/file';
-import state from '~/ide/stores/state';
+import { createStore } from '~/ide/stores';
 import { FILE_VIEW_MODE_PREVIEW } from '~/ide/constants';
 import { file } from '../../helpers';
 
 describe('IDE store file mutations', () => {
   let localState;
+  let localStore;
   let localFile;
 
   beforeEach(() => {
-    localState = state();
+    localStore = createStore();
+    localState = localStore.state;
     localFile = { ...file(), type: 'blob' };
 
     localState.entries[localFile.path] = localFile;
@@ -333,44 +335,154 @@ describe('IDE store file mutations', () => {
     });
   });
 
-  describe('STAGE_CHANGE', () => {
-    beforeEach(() => {
-      mutations.STAGE_CHANGE(localState, localFile.path);
-    });
+  describe.each`
+    mutationName        | mutation                    | addedTo           | removedFrom       | staged   | changedFilesCount | stagedFilesCount
+    ${'STAGE_CHANGE'}   | ${mutations.STAGE_CHANGE}   | ${'stagedFiles'}  | ${'changedFiles'} | ${true}  | ${0}              | ${1}
+    ${'UNSTAGE_CHANGE'} | ${mutations.UNSTAGE_CHANGE} | ${'changedFiles'} | ${'stagedFiles'}  | ${false} | ${1}              | ${0}
+  `(
+    '$mutationName',
+    ({ mutation, changedFilesCount, removedFrom, addedTo, staged, stagedFilesCount }) => {
+      let unstagedFile;
+      let stagedFile;
+
+      beforeEach(() => {
+        unstagedFile = {
+          ...file('file'),
+          type: 'blob',
+          raw: 'original content',
+          content: 'changed content',
+        };
+
+        stagedFile = {
+          ...unstagedFile,
+          content: 'staged content',
+          staged: true,
+        };
+
+        localState.changedFiles.push(unstagedFile);
+        localState.stagedFiles.push(stagedFile);
+        localState.entries[unstagedFile.path] = unstagedFile;
+      });
 
-    it('adds file into stagedFiles array', () => {
-      expect(localState.stagedFiles.length).toBe(1);
-      expect(localState.stagedFiles[0]).toEqual(localFile);
-    });
+      it('removes all changes of a file if staged and unstaged change contents are equal', () => {
+        unstagedFile.content = 'original content';
 
-    it('updates stagedFile if it is already staged', () => {
-      localFile.raw = 'testing 123';
+        mutation(localState, {
+          path: unstagedFile.path,
+          diffInfo: localStore.getters.getDiffInfo(unstagedFile.path),
+        });
 
-      mutations.STAGE_CHANGE(localState, localFile.path);
+        expect(localState.entries.file).toEqual(
+          expect.objectContaining({
+            content: 'original content',
+            staged: false,
+            changed: false,
+          }),
+        );
 
-      expect(localState.stagedFiles.length).toBe(1);
-      expect(localState.stagedFiles[0].raw).toEqual('testing 123');
-    });
-  });
+        expect(localState.stagedFiles.length).toBe(0);
+        expect(localState.changedFiles.length).toBe(0);
+      });
 
-  describe('UNSTAGE_CHANGE', () => {
-    let f;
+      it('removes all changes of a file if a file is deleted and a new file with same content is added', () => {
+        stagedFile.deleted = true;
+        unstagedFile.tempFile = true;
+        unstagedFile.content = 'original content';
 
-    beforeEach(() => {
-      f = { ...file(), type: 'blob', staged: true };
+        mutation(localState, {
+          path: unstagedFile.path,
+          diffInfo: localStore.getters.getDiffInfo(unstagedFile.path),
+        });
 
-      localState.stagedFiles.push(f);
-      localState.changedFiles.push(f);
-      localState.entries[f.path] = f;
-    });
+        expect(localState.stagedFiles.length).toBe(0);
+        expect(localState.changedFiles.length).toBe(0);
 
-    it('removes from stagedFiles array', () => {
-      mutations.UNSTAGE_CHANGE(localState, f.path);
+        expect(localState.entries.file).toEqual(
+          expect.objectContaining({
+            content: 'original content',
+            deleted: false,
+            tempFile: false,
+          }),
+        );
+      });
 
-      expect(localState.stagedFiles.length).toBe(0);
-      expect(localState.changedFiles.length).toBe(1);
-    });
-  });
+      it('merges deleted and added file into a changed file if the contents differ', () => {
+        stagedFile.deleted = true;
+        unstagedFile.tempFile = true;
+        unstagedFile.content = 'hello';
+
+        mutation(localState, {
+          path: unstagedFile.path,
+          diffInfo: localStore.getters.getDiffInfo(unstagedFile.path),
+        });
+
+        expect(localState.stagedFiles.length).toBe(stagedFilesCount);
+        expect(localState.changedFiles.length).toBe(changedFilesCount);
+
+        expect(unstagedFile).toEqual(
+          expect.objectContaining({
+            content: 'hello',
+            staged,
+            deleted: false,
+            tempFile: false,
+            changed: true,
+          }),
+        );
+      });
+
+      it('does not remove file from stagedFiles and changedFiles if the file was renamed, even if the contents are equal', () => {
+        unstagedFile.content = 'original content';
+        unstagedFile.prevPath = 'old_file';
+
+        mutation(localState, {
+          path: unstagedFile.path,
+          diffInfo: localStore.getters.getDiffInfo(unstagedFile.path),
+        });
+
+        expect(localState.entries.file).toEqual(
+          expect.objectContaining({
+            content: 'original content',
+            staged,
+            changed: false,
+            prevPath: 'old_file',
+          }),
+        );
+
+        expect(localState.stagedFiles.length).toBe(stagedFilesCount);
+        expect(localState.changedFiles.length).toBe(changedFilesCount);
+      });
+
+      it(`removes file from ${removedFrom} array and adds it into ${addedTo} array`, () => {
+        localState.stagedFiles.length = 0;
+
+        mutation(localState, {
+          path: unstagedFile.path,
+          diffInfo: localStore.getters.getDiffInfo(unstagedFile.path),
+        });
+
+        expect(localState.stagedFiles.length).toBe(stagedFilesCount);
+        expect(localState.changedFiles.length).toBe(changedFilesCount);
+
+        const f = localState.stagedFiles[0] || localState.changedFiles[0];
+        expect(f).toEqual(unstagedFile);
+      });
+
+      it(`updates file in ${addedTo} array if it is was already present in it`, () => {
+        unstagedFile.raw = 'testing 123';
+
+        mutation(localState, {
+          path: unstagedFile.path,
+          diffInfo: localStore.getters.getDiffInfo(unstagedFile.path),
+        });
+
+        expect(localState.stagedFiles.length).toBe(stagedFilesCount);
+        expect(localState.changedFiles.length).toBe(changedFilesCount);
+
+        const f = localState.stagedFiles[0] || localState.changedFiles[0];
+        expect(f.raw).toEqual('testing 123');
+      });
+    },
+  );
 
   describe('TOGGLE_FILE_CHANGED', () => {
     it('updates file changed status', () => {

spec/javascripts/ide/stores/actions_spec.js

@@ -18,19 +18,19 @@ import axios from '~/lib/utils/axios_utils';
 import { createStore } from '~/ide/stores';
 import * as types from '~/ide/stores/mutation_types';
 import router from '~/ide/ide_router';
-import { resetStore, file } from '../helpers';
+import { file } from '../helpers';
 import testAction from '../../helpers/vuex_action_helper';
 import eventHub from '~/ide/eventhub';
 
-const store = createStore();
-
 describe('Multi-file store actions', () => {
+  let store;
+
   beforeEach(() => {
-    spyOn(router, 'push');
-  });
+    store = createStore();
 
-  afterEach(() => {
-    resetStore(store);
+    spyOn(store, 'commit').and.callThrough();
+    spyOn(store, 'dispatch').and.callThrough();
+    spyOn(router, 'push');
   });
 
   describe('redirectToUrl', () => {
@@ -390,58 +390,82 @@ describe('Multi-file store actions', () => {
     });
   });
 
-  describe('stageAllChanges', () => {
-    it('adds all files from changedFiles to stagedFiles', done => {
-      const openFile = { ...file(), path: 'test' };
+  describe('stage/unstageAllChanges', () => {
+    let file1;
+    let file2;
 
-      store.state.openFiles.push(openFile);
-      store.state.stagedFiles.push(openFile);
-      store.state.changedFiles.push(openFile, file('new'));
+    beforeEach(() => {
+      file1 = { ...file('test'), content: 'changed test', raw: 'test' };
+      file2 = { ...file('test2'), content: 'changed test2', raw: 'test2' };
 
-      testAction(
-        stageAllChanges,
-        null,
-        store.state,
-        [
-          { type: types.SET_LAST_COMMIT_MSG, payload: '' },
-          { type: types.STAGE_CHANGE, payload: store.state.changedFiles[0].path },
-          { type: types.STAGE_CHANGE, payload: store.state.changedFiles[1].path },
-        ],
-        [
-          {
-            type: 'openPendingTab',
-            payload: { file: openFile, keyPrefix: 'staged' },
-          },
-        ],
-        done,
-      );
+      store.state.openFiles = [file1];
+      store.state.changedFiles = [file1];
+      store.state.stagedFiles = [{ ...file2, content: 'staged test' }];
+
+      store.state.entries = {
+        [file1.path]: { ...file1 },
+        [file2.path]: { ...file2 },
+      };
     });
-  });
 
-  describe('unstageAllChanges', () => {
-    it('removes all files from stagedFiles after unstaging', done => {
-      const openFile = { ...file(), path: 'test' };
+    describe('stageAllChanges', () => {
+      it('adds all files from changedFiles to stagedFiles', () => {
+        stageAllChanges(store);
 
-      store.state.openFiles.push(openFile);
-      store.state.changedFiles.push(openFile);
-      store.state.stagedFiles.push(openFile, file('new'));
+        expect(store.commit.calls.allArgs()).toEqual([
+          [types.SET_LAST_COMMIT_MSG, ''],
+          [types.STAGE_CHANGE, jasmine.objectContaining({ path: file1.path })],
+        ]);
+      });
 
-      testAction(
-        unstageAllChanges,
-        null,
-        store.state,
-        [
-          { type: types.UNSTAGE_CHANGE, payload: store.state.stagedFiles[0].path },
-          { type: types.UNSTAGE_CHANGE, payload: store.state.stagedFiles[1].path },
-        ],
-        [
-          {
-            type: 'openPendingTab',
-            payload: { file: openFile, keyPrefix: 'unstaged' },
-          },
-        ],
-        done,
-      );
+      it('opens pending tab if a change exists in that file', () => {
+        stageAllChanges(store);
+
+        expect(store.dispatch.calls.allArgs()).toEqual([
+          [
+            'openPendingTab',
+            { file: { ...file1, staged: true, changed: true }, keyPrefix: 'staged' },
+          ],
+        ]);
+      });
+
+      it('does not open pending tab if no change exists in that file', () => {
+        store.state.entries[file1.path].content = 'test';
+        store.state.stagedFiles = [file1];
+        store.state.changedFiles = [store.state.entries[file1.path]];
+
+        stageAllChanges(store);
+
+        expect(store.dispatch).not.toHaveBeenCalled();
+      });
+    });
+
+    describe('unstageAllChanges', () => {
+      it('removes all files from stagedFiles after unstaging', () => {
+        unstageAllChanges(store);
+
+        expect(store.commit.calls.allArgs()).toEqual([
+          [types.UNSTAGE_CHANGE, jasmine.objectContaining({ path: file2.path })],
+        ]);
+      });
+
+      it('opens pending tab if a change exists in that file', () => {
+        unstageAllChanges(store);
+
+        expect(store.dispatch.calls.allArgs()).toEqual([
+          ['openPendingTab', { file: file1, keyPrefix: 'unstaged' }],
+        ]);
+      });
+
+      it('does not open pending tab if no change exists in that file', () => {
+        store.state.entries[file1.path].content = 'test';
+        store.state.stagedFiles = [file1];
+        store.state.changedFiles = [store.state.entries[file1.path]];
+
+        unstageAllChanges(store);
+
+        expect(store.dispatch).not.toHaveBeenCalled();
+      });
     });
   });
 
@@ -752,10 +776,6 @@ describe('Multi-file store actions', () => {
         });
       });
 
-      afterEach(() => {
-        resetStore(store);
-      });
-
       it('by default renames an entry and adds to changed', done => {
         testAction(
           renameEntry,
@@ -966,18 +986,19 @@ describe('Multi-file store actions', () => {
 
     describe('error', () => {
       let dispatch;
-      const callParams = [
-        {
-          commit() {},
-          state: store.state,
-        },
-        {
-          projectId: 'abc/def',
-          branchId: 'master-testing',
-        },
-      ];
+      let callParams;
 
       beforeEach(() => {
+        callParams = [
+          {
+            commit() {},
+            state: store.state,
+          },
+          {
+            projectId: 'abc/def',
+            branchId: 'master-testing',
+          },
+        ];
         dispatch = jasmine.createSpy('dispatchSpy');
         document.body.innerHTML += '<div class="flash-container"></div>';
       });