Merge branch '11-1-3-stable-fixes-mr-refactor-regressions' into '11-1-stable-patch-3'11-1-stable-patch-3

MR refactor fixes for 11.1.3

See merge request gitlab-org/gitlab-ce!20878

11 files changed, 379 insertions, 44 deletions

diff --git a/spec/javascripts/autosave_spec.js b/spec/javascripts/autosave_spec.js
index 38ae5b7e00c..dcb1c781591 100644
--- a/spec/javascripts/autosave_spec.js
+++ b/spec/javascripts/autosave_spec.js
@@ -59,12 +59,10 @@ describe('Autosave', () => {
 
         Autosave.prototype.restore.call(autosave);
 
-        expect(
-          field.trigger,
-        ).toHaveBeenCalled();
+        expect(field.trigger).toHaveBeenCalled();
       });
 
-      it('triggers native event', (done) => {
+      it('triggers native event', done => {
         autosave.field.get(0).addEventListener('change', () => {
           done();
         });
@@ -81,9 +79,7 @@ describe('Autosave', () => {
       it('does not trigger event', () => {
         spyOn(field, 'trigger').and.callThrough();
 
-        expect(
-          field.trigger,
-        ).not.toHaveBeenCalled();
+        expect(field.trigger).not.toHaveBeenCalled();
       });
     });
   });
diff --git a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js
index 2d136a63c52..bdc94131fc2 100644
--- a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js
+++ b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js
@@ -18,10 +18,12 @@ describe('DiffLineGutterContent', () => {
   };
   const setDiscussions = component => {
     component.$store.dispatch('setInitialNotes', getDiscussionsMockData());
+    component.$store.commit('diffs/SET_DIFF_DATA', { diffFiles: [getDiffFileMock()] });
   };
 
   const resetDiscussions = component => {
     component.$store.dispatch('setInitialNotes', []);
+    component.$store.commit('diffs/SET_DIFF_DATA', {});
   };
 
   describe('computed', () => {
@@ -48,7 +50,11 @@ describe('DiffLineGutterContent', () => {
 
       it('should return discussions for the given lineCode', () => {
         const { lineCode } = getDiffFileMock().highlightedDiffLines[1];
-        const component = createComponent({ lineCode, showCommentButton: true });
+        const component = createComponent({
+          lineCode,
+          showCommentButton: true,
+          discussions: getDiscussionsMockData(),
+        });
 
         setDiscussions(component);
 
diff --git a/spec/javascripts/diffs/components/diff_line_note_form_spec.js b/spec/javascripts/diffs/components/diff_line_note_form_spec.js
index 4600aaea70b..6fe5fdaf7f9 100644
--- a/spec/javascripts/diffs/components/diff_line_note_form_spec.js
+++ b/spec/javascripts/diffs/components/diff_line_note_form_spec.js
@@ -3,6 +3,7 @@ import DiffLineNoteForm from '~/diffs/components/diff_line_note_form.vue';
 import store from '~/mr_notes/stores';
 import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
 import diffFileMockData from '../mock_data/diff_file';
+import { noteableDataMock } from '../../notes/mock_data';
 
 describe('DiffLineNoteForm', () => {
   let component;
@@ -21,10 +22,9 @@ describe('DiffLineNoteForm', () => {
       noteTargetLine: diffLines[0],
     });
 
-    Object.defineProperty(component, 'isLoggedIn', {
-      get() {
-        return true;
-      },
+    Object.defineProperties(component, {
+      noteableData: { value: noteableDataMock },
+      isLoggedIn: { value: true },
     });
 
     component.$mount();
@@ -32,12 +32,37 @@ describe('DiffLineNoteForm', () => {
 
   describe('methods', () => {
     describe('handleCancelCommentForm', () => {
-      it('should call cancelCommentForm with lineCode', () => {
+      it('should ask for confirmation when shouldConfirm and isDirty passed as truthy', () => {
+        spyOn(window, 'confirm').and.returnValue(false);
+
+        component.handleCancelCommentForm(true, true);
+        expect(window.confirm).toHaveBeenCalled();
+      });
+
+      it('should ask for confirmation when one of the params false', () => {
+        spyOn(window, 'confirm').and.returnValue(false);
+
+        component.handleCancelCommentForm(true, false);
+        expect(window.confirm).not.toHaveBeenCalled();
+
+        component.handleCancelCommentForm(false, true);
+        expect(window.confirm).not.toHaveBeenCalled();
+      });
+
+      it('should call cancelCommentForm with lineCode', done => {
+        spyOn(window, 'confirm');
         spyOn(component, 'cancelCommentForm');
+        spyOn(component, 'resetAutoSave');
         component.handleCancelCommentForm();
 
-        expect(component.cancelCommentForm).toHaveBeenCalledWith({
-          lineCode: diffLines[0].lineCode,
+        expect(window.confirm).not.toHaveBeenCalled();
+        component.$nextTick(() => {
+          expect(component.cancelCommentForm).toHaveBeenCalledWith({
+            lineCode: diffLines[0].lineCode,
+          });
+          expect(component.resetAutoSave).toHaveBeenCalled();
+
+          done();
         });
       });
     });
@@ -66,7 +91,7 @@ describe('DiffLineNoteForm', () => {
 
   describe('mounted', () => {
     it('should init autosave', () => {
-      const key = 'autosave/Note/issue///DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1';
+      const key = 'autosave/Note/Issue/98//DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1';
 
       expect(component.autosave).toBeDefined();
       expect(component.autosave.key).toEqual(key);
diff --git a/spec/javascripts/diffs/components/inline_diff_view_spec.js b/spec/javascripts/diffs/components/inline_diff_view_spec.js
index b02328dd359..90dfa5c5a58 100644
--- a/spec/javascripts/diffs/components/inline_diff_view_spec.js
+++ b/spec/javascripts/diffs/components/inline_diff_view_spec.js
@@ -33,6 +33,7 @@ describe('InlineDiffView', () => {
     it('should render discussions', done => {
       const el = component.$el;
       component.$store.dispatch('setInitialNotes', getDiscussionsMockData());
+      component.$store.commit('diffs/SET_DIFF_DATA', { diffFiles: [getDiffFileMock()] });
 
       Vue.nextTick(() => {
         expect(el.querySelectorAll('.notes_holder').length).toEqual(1);
diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js
index 41d0dfd8939..8cd57d2248b 100644
--- a/spec/javascripts/diffs/mock_data/diff_discussions.js
+++ b/spec/javascripts/diffs/mock_data/diff_discussions.js
@@ -12,6 +12,17 @@ export default {
       head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
     },
   },
+  original_position: {
+    formatter: {
+      old_line: null,
+      new_line: 2,
+      old_path: 'CHANGELOG',
+      new_path: 'CHANGELOG',
+      base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a',
+      start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962',
+      head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
+    },
+  },
   line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2',
   expanded: true,
   notes: [
diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js
index 6210d4a7124..f5628a01a55 100644
--- a/spec/javascripts/diffs/store/getters_spec.js
+++ b/spec/javascripts/diffs/store/getters_spec.js
@@ -2,6 +2,7 @@ import * as getters from '~/diffs/store/getters';
 import state from '~/diffs/store/modules/diff_state';
 import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants';
 import discussion from '../mock_data/diff_discussions';
+import diffFile from '../mock_data/diff_file';
 
 describe('Diffs Module Getters', () => {
   let localState;
@@ -203,4 +204,38 @@ describe('Diffs Module Getters', () => {
       expect(getters.getDiffFileByHash(localState)('123')).toBeUndefined();
     });
   });
+
+  describe('discussionsByLineCode', () => {
+    let mockState;
+
+    beforeEach(() => {
+      mockState = { diffFiles: [diffFile] };
+    });
+
+    it('should return a map of diff lines with their line codes', () => {
+      const mockGetters = { discussions: [discussionMock] };
+
+      const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters);
+      expect(map['1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2']).toBeDefined();
+      expect(Object.keys(map).length).toEqual(1);
+    });
+
+    it('should have the diff discussion on the map if the original position matches', () => {
+      discussionMock.position.formatter.base_sha = 'ff9200';
+      const mockGetters = { discussions: [discussionMock] };
+
+      const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters);
+      expect(map['1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2']).toBeDefined();
+      expect(Object.keys(map).length).toEqual(1);
+    });
+
+    it('should not add an outdated diff discussion to the returned map', () => {
+      discussionMock.position.formatter.base_sha = 'ff9200';
+      discussionMock.original_position.formatter.base_sha = 'ff9200';
+      const mockGetters = { discussions: [discussionMock] };
+
+      const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters);
+      expect(Object.keys(map).length).toEqual(0);
+    });
+  });
 });
diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js
index 32136d9ebff..8e7bd8afca4 100644
--- a/spec/javascripts/diffs/store/utils_spec.js
+++ b/spec/javascripts/diffs/store/utils_spec.js
@@ -207,4 +207,24 @@ describe('DiffsStoreUtils', () => {
       expect(utils.trimFirstCharOfLineContent()).toEqual({});
     });
   });
+
+  describe('getDiffRefsByLineCode', () => {
+    it('should return diffRefs for all highlightedDiffLines', () => {
+      const diffFile = getDiffFileMock();
+      const map = utils.getDiffRefsByLineCode([diffFile]);
+      const { highlightedDiffLines } = diffFile;
+      const lineCodeCount = highlightedDiffLines.reduce(
+        (acc, line) => (line.lineCode ? acc + 1 : acc),
+        0,
+      );
+
+      const { baseSha, headSha, startSha } = diffFile.diffRefs;
+      const targetLine = map[highlightedDiffLines[4].lineCode];
+
+      expect(Object.keys(map).length).toEqual(lineCodeCount);
+      expect(targetLine.baseSha).toEqual(baseSha);
+      expect(targetLine.headSha).toEqual(headSha);
+      expect(targetLine.startSha).toEqual(startSha);
+    });
+  });
 });
diff --git a/spec/javascripts/notes/components/discussion_counter_spec.js b/spec/javascripts/notes/components/discussion_counter_spec.js
index a3869cc6498..d09bc5037ef 100644
--- a/spec/javascripts/notes/components/discussion_counter_spec.js
+++ b/spec/javascripts/notes/components/discussion_counter_spec.js
@@ -46,7 +46,7 @@ describe('DiscussionCounter component', () => {
           discussions,
         });
         setFixtures(`
-          <div data-discussion-id="${firstDiscussionId}"></div>
+          <div class="discussion" data-discussion-id="${firstDiscussionId}"></div>
         `);
 
         vm.jumpToFirstUnresolvedDiscussion();
diff --git a/spec/javascripts/notes/components/noteable_discussion_spec.js b/spec/javascripts/notes/components/noteable_discussion_spec.js
index 7da931fd9cb..2a01bd85520 100644
--- a/spec/javascripts/notes/components/noteable_discussion_spec.js
+++ b/spec/javascripts/notes/components/noteable_discussion_spec.js
@@ -14,6 +14,7 @@ describe('noteable_discussion component', () => {
   preloadFixtures(discussionWithTwoUnresolvedNotes);
 
   beforeEach(() => {
+    window.mrTabs = {};
     store = createStore();
     store.dispatch('setNoteableData', noteableDataMock);
     store.dispatch('setNotesData', notesDataMock);
@@ -46,10 +47,15 @@ describe('noteable_discussion component', () => {
 
     it('should toggle reply form', done => {
       vm.$el.querySelector('.js-vue-discussion-reply').click();
+
       Vue.nextTick(() => {
-        expect(vm.$refs.noteForm).not.toBeNull();
         expect(vm.isReplying).toEqual(true);
-        done();
+
+        // There is a watcher for `isReplying` which will init autosave in the next tick
+        Vue.nextTick(() => {
+          expect(vm.$refs.noteForm).not.toBeNull();
+          done();
+        });
       });
     });
 
@@ -101,33 +107,29 @@ describe('noteable_discussion component', () => {
 
   describe('methods', () => {
     describe('jumpToNextDiscussion', () => {
-      it('expands next unresolved discussion', () => {
-        spyOn(vm, 'expandDiscussion').and.stub();
-        const discussions = [
-          discussionMock,
-          {
-            ...discussionMock,
-            id: discussionMock.id + 1,
-            notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }],
-          },
-          {
-            ...discussionMock,
-            id: discussionMock.id + 2,
-            notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }],
-          },
-        ];
-        const nextDiscussionId = discussionMock.id + 2;
-        store.replaceState({
-          ...store.state,
-          discussions,
-        });
-        setFixtures(`
-          <div data-discussion-id="${nextDiscussionId}"></div>
-        `);
+      it('expands next unresolved discussion', done => {
+        const discussion2 = getJSONFixture(discussionWithTwoUnresolvedNotes)[0];
+        discussion2.resolved = false;
+        discussion2.id = 'next'; // prepare this for being identified as next one (to be jumped to)
+        vm.$store.dispatch('setInitialNotes', [discussionMock, discussion2]);
+        window.mrTabs.currentAction = 'show';
+
+        Vue.nextTick()
+          .then(() => {
+            spyOn(vm, 'expandDiscussion').and.stub();
+
+            const nextDiscussionId = discussion2.id;
+
+            setFixtures(`
+              <div class="discussion" data-discussion-id="${nextDiscussionId}"></div>
+            `);
 
-        vm.jumpToNextDiscussion();
+            vm.jumpToNextDiscussion();
 
-        expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId });
+            expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId });
+          })
+          .then(done)
+          .catch(done.fail);
       });
     });
   });
diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js
index be2a8ba67fe..67f6a9629d9 100644
--- a/spec/javascripts/notes/mock_data.js
+++ b/spec/javascripts/notes/mock_data.js
@@ -1168,3 +1168,87 @@ export const collapsedSystemNotes = [
     diff_discussion: false,
   },
 ];
+
+export const discussion1 = {
+  id: 'abc1',
+  resolvable: true,
+  resolved: false,
+  diff_file: {
+    file_path: 'about.md',
+  },
+  position: {
+    formatter: {
+      new_line: 50,
+      old_line: null,
+    },
+  },
+  notes: [
+    {
+      created_at: '2018-07-04T16:25:41.749Z',
+    },
+  ],
+};
+
+export const resolvedDiscussion1 = {
+  id: 'abc1',
+  resolvable: true,
+  resolved: true,
+  diff_file: {
+    file_path: 'about.md',
+  },
+  position: {
+    formatter: {
+      new_line: 50,
+      old_line: null,
+    },
+  },
+  notes: [
+    {
+      created_at: '2018-07-04T16:25:41.749Z',
+    },
+  ],
+};
+
+export const discussion2 = {
+  id: 'abc2',
+  resolvable: true,
+  resolved: false,
+  diff_file: {
+    file_path: 'README.md',
+  },
+  position: {
+    formatter: {
+      new_line: null,
+      old_line: 20,
+    },
+  },
+  notes: [
+    {
+      created_at: '2018-07-04T12:05:41.749Z',
+    },
+  ],
+};
+
+export const discussion3 = {
+  id: 'abc3',
+  resolvable: true,
+  resolved: false,
+  diff_file: {
+    file_path: 'README.md',
+  },
+  position: {
+    formatter: {
+      new_line: 21,
+      old_line: null,
+    },
+  },
+  notes: [
+    {
+      created_at: '2018-07-05T17:25:41.749Z',
+    },
+  ],
+};
+
+export const unresolvableDiscussion = {
+  resolvable: false,
+};
diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js
index 41599e00122..7f8ede51508 100644
--- a/spec/javascripts/notes/stores/getters_spec.js
+++ b/spec/javascripts/notes/stores/getters_spec.js
@@ -5,6 +5,11 @@ import {
   noteableDataMock,
   individualNote,
   collapseNotesMock,
+  discussion1,
+  discussion2,
+  discussion3,
+  resolvedDiscussion1,
+  unresolvableDiscussion,
 } from '../mock_data';
 
 const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json';
@@ -109,4 +114,154 @@ describe('Getters Notes Store', () => {
       expect(getters.isNotesFetched(state)).toBeFalsy();
     });
   });
+
+  describe('allResolvableDiscussions', () => {
+    it('should return only resolvable discussions in same order', () => {
+      const localGetters = {
+        allDiscussions: [
+          discussion3,
+          unresolvableDiscussion,
+          discussion1,
+          unresolvableDiscussion,
+          discussion2,
+        ],
+      };
+
+      expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([
+        discussion3,
+        discussion1,
+        discussion2,
+      ]);
+    });
+
+    it('should return empty array if there are no resolvable discussions', () => {
+      const localGetters = {
+        allDiscussions: [unresolvableDiscussion, unresolvableDiscussion],
+      };
+
+      expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([]);
+    });
+  });
+
+  describe('unresolvedDiscussionsIdsByDiff', () => {
+    it('should return all discussions IDs in diff order', () => {
+      const localGetters = {
+        allResolvableDiscussions: [discussion3, discussion1, discussion2],
+      };
+
+      expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([
+        'abc1',
+        'abc2',
+        'abc3',
+      ]);
+    });
+
+    it('should return empty array if all discussions have been resolved', () => {
+      const localGetters = {
+        allResolvableDiscussions: [resolvedDiscussion1],
+      };
+
+      expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([]);
+    });
+  });
+
+  describe('unresolvedDiscussionsIdsByDate', () => {
+    it('should return all discussions in date ascending order', () => {
+      const localGetters = {
+        allResolvableDiscussions: [discussion3, discussion1, discussion2],
+      };
+
+      expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([
+        'abc2',
+        'abc1',
+        'abc3',
+      ]);
+    });
+
+    it('should return empty array if all discussions have been resolved', () => {
+      const localGetters = {
+        allResolvableDiscussions: [resolvedDiscussion1],
+      };
+
+      expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([]);
+    });
+  });
+
+  describe('unresolvedDiscussionsIdsOrdered', () => {
+    const localGetters = {
+      unresolvedDiscussionsIdsByDate: ['123', '456'],
+      unresolvedDiscussionsIdsByDiff: ['abc', 'def'],
+    };
+
+    it('should return IDs ordered by diff when diffOrder param is true', () => {
+      expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(true)).toEqual([
+        'abc',
+        'def',
+      ]);
+    });
+
+    it('should return IDs ordered by date when diffOrder param is not true', () => {
+      expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(false)).toEqual([
+        '123',
+        '456',
+      ]);
+      expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(undefined)).toEqual([
+        '123',
+        '456',
+      ]);
+    });
+  });
+
+  describe('isLastUnresolvedDiscussion', () => {
+    const localGetters = {
+      unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'],
+    };
+
+    it('should return true if the discussion id provided is the last', () => {
+      expect(getters.isLastUnresolvedDiscussion(state, localGetters)('789')).toBe(true);
+    });
+
+    it('should return false if the discussion id provided is not the last', () => {
+      expect(getters.isLastUnresolvedDiscussion(state, localGetters)('123')).toBe(false);
+      expect(getters.isLastUnresolvedDiscussion(state, localGetters)('456')).toBe(false);
+    });
+  });
+
+  describe('nextUnresolvedDiscussionId', () => {
+    const localGetters = {
+      unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'],
+    };
+
+    it('should return the ID of the discussion after the ID provided', () => {
+      expect(getters.nextUnresolvedDiscussionId(state, localGetters)('123')).toBe('456');
+      expect(getters.nextUnresolvedDiscussionId(state, localGetters)('456')).toBe('789');
+      expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe(undefined);
+    });
+  });
+
+  describe('firstUnresolvedDiscussionId', () => {
+    const localGetters = {
+      unresolvedDiscussionsIdsByDate: ['123', '456'],
+      unresolvedDiscussionsIdsByDiff: ['abc', 'def'],
+    };
+
+    it('should return the first discussion id by diff when diffOrder param is true', () => {
+      expect(getters.firstUnresolvedDiscussionId(state, localGetters)(true)).toBe('abc');
+    });
+
+    it('should return the first discussion id by date when diffOrder param is not true', () => {
+      expect(getters.firstUnresolvedDiscussionId(state, localGetters)(false)).toBe('123');
+      expect(getters.firstUnresolvedDiscussionId(state, localGetters)(undefined)).toBe('123');
+    });
+
+    it('should be falsy if all discussions are resolved', () => {
+      const localGettersFalsy = {
+        unresolvedDiscussionsIdsByDiff: [],
+        unresolvedDiscussionsIdsByDate: [],
+      };
+
+      expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(true)).toBeFalsy();
+      expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(false)).toBeFalsy();
+    });
+  });
 });