Handle edge cases in stage and unstage mutations

Modify stage and unstage mutations to handle edge cases involving
the ability to stage empty content in Web IDE. Also handle cases
of files deleted and readded.

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

@@ -134,12 +134,14 @@ 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),
@@ -147,10 +149,12 @@ export const stageAllChanges = ({ state, commit, dispatch }) => {
   });
 };
 
-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),

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

@@ -214,11 +214,11 @@ 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) => {
+export const stageChange = ({ commit, state, dispatch, getters }, path) => {
   const stagedFile = state.stagedFiles.find(f => f.path === path);
   const openFile = state.openFiles.find(f => f.path === path);
 
-  commit(types.STAGE_CHANGE, path);
+  commit(types.STAGE_CHANGE, { path, diffInfo: getters.getDiffInfo(path) });
   commit(types.SET_LAST_COMMIT_MSG, '');
 
   if (stagedFile) {
@@ -235,10 +235,10 @@ export const stageChange = ({ commit, state, dispatch }, path) => {
   }
 };
 
-export const unstageChange = ({ commit, dispatch, state }, path) => {
+export const unstageChange = ({ commit, dispatch, state, getters }, path) => {
   const openFile = state.openFiles.find(f => f.path === 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);

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,31 +666,32 @@ 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.calls.allArgs()).toEqual([
+        [types.STAGE_CHANGE, jest.objectContaining({ path: 'path' })],
+        [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.calls.allArgs()).toEqual([
+        [types.UNSTAGE_CHANGE, jest.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,43 @@ 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,
-      );
+    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 })],
+        ]);
+      });
     });
   });
 
@@ -752,10 +737,6 @@ describe('Multi-file store actions', () => {
         });
       });
 
-      afterEach(() => {
-        resetStore(store);
-      });
-
       it('by default renames an entry and adds to changed', done => {
         testAction(
           renameEntry,
@@ -966,18 +947,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>';
       });