Revert "Merge branch '48817-fix-mr-changes-discussion-navigation' into 'master'"

This reverts commit ced005f3, reversing
changes made to 9b01b293.

app/assets/javascripts/diffs/components/diff_discussions.vue

@@ -30,7 +30,6 @@ export default {
           :render-header="false"
           :render-diff-file="false"
           :always-expanded="true"
-          :discussions-by-diff-order="true"
         />
       </ul>
     </div>

app/assets/javascripts/notes/components/discussion_counter.vue

@@ -5,20 +5,19 @@ import resolvedSvg from 'icons/_icon_status_success_solid.svg';
 import mrIssueSvg from 'icons/_icon_mr_issue.svg';
 import nextDiscussionSvg from 'icons/_next_discussion.svg';
 import { pluralize } from '../../lib/utils/text_utility';
-import discussionNavigation from '../mixins/discussion_navigation';
+import { scrollToElement } from '../../lib/utils/common_utils';
 import tooltip from '../../vue_shared/directives/tooltip';
 
 export default {
   directives: {
     tooltip,
   },
-  mixins: [discussionNavigation],
   computed: {
     ...mapGetters([
       'getUserData',
       'getNoteableData',
       'discussionCount',
-      'firstUnresolvedDiscussionId',
+      'unresolvedDiscussions',
       'resolvedDiscussionCount',
     ]),
     isLoggedIn() {
@@ -36,6 +35,11 @@ export default {
     resolveAllDiscussionsIssuePath() {
       return this.getNoteableData.create_issue_to_resolve_discussions_path;
     },
+    firstUnresolvedDiscussionId() {
+      const item = this.unresolvedDiscussions[0] || {};
+
+      return item.id;
+    },
   },
   created() {
     this.resolveSvg = resolveSvg;
@@ -46,10 +50,22 @@ export default {
   methods: {
     ...mapActions(['expandDiscussion']),
     jumpToFirstUnresolvedDiscussion() {
-      const diffTab = window.mrTabs.currentAction === 'diffs';
-      const discussionId = this.firstUnresolvedDiscussionId(diffTab);
+      const discussionId = this.firstUnresolvedDiscussionId;
+      if (!discussionId) {
+        return;
+      }
+
+      const el = document.querySelector(`[data-discussion-id="${discussionId}"]`);
+      const activeTab = window.mrTabs.currentAction;
+
+      if (activeTab === 'commits' || activeTab === 'pipelines') {
+        window.mrTabs.activateTab('show');
+      }
 
-      this.jumpToDiscussion(discussionId);
+      if (el) {
+        this.expandDiscussion({ discussionId });
+        scrollToElement(el);
+      }
     },
   },
 };

app/assets/javascripts/notes/components/noteable_discussion.vue

 <script>
+import _ from 'underscore';
 import { mapActions, mapGetters } from 'vuex';
 import resolveDiscussionsSvg from 'icons/_icon_mr_issue.svg';
 import nextDiscussionsSvg from 'icons/_next_discussion.svg';
-import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
+import { convertObjectPropsToCamelCase, scrollToElement } from '~/lib/utils/common_utils';
 import { truncateSha } from '~/lib/utils/text_utility';
 import systemNote from '~/vue_shared/components/notes/system_note.vue';
 import { s__ } from '~/locale';
@@ -20,7 +21,6 @@ import placeholderSystemNote from '../../vue_shared/components/notes/placeholder
 import autosave from '../mixins/autosave';
 import noteable from '../mixins/noteable';
 import resolvable from '../mixins/resolvable';
-import discussionNavigation from '../mixins/discussion_navigation';
 import tooltip from '../../vue_shared/directives/tooltip';
 
 export default {
@@ -40,7 +40,7 @@ export default {
   directives: {
     tooltip,
   },
-  mixins: [autosave, noteable, resolvable, discussionNavigation],
+  mixins: [autosave, noteable, resolvable],
   props: {
     discussion: {
       type: Object,
@@ -61,11 +61,6 @@ export default {
       required: false,
       default: false,
     },
-    discussionsByDiffOrder: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
   },
   data() {
     return {
@@ -80,12 +75,7 @@ export default {
       'discussionCount',
       'resolvedDiscussionCount',
       'allDiscussions',
-      'unresolvedDiscussionsIdsByDiff',
-      'unresolvedDiscussionsIdsByDate',
       'unresolvedDiscussions',
-      'unresolvedDiscussionsIdsOrdered',
-      'nextUnresolvedDiscussionId',
-      'isLastUnresolvedDiscussion',
     ]),
     transformedDiscussion() {
       return {
@@ -136,10 +126,6 @@ export default {
     hasMultipleUnresolvedDiscussions() {
       return this.unresolvedDiscussions.length > 1;
     },
-    showJumpToNextDiscussion() {
-      return this.hasMultipleUnresolvedDiscussions &&
-        !this.isLastUnresolvedDiscussion(this.discussion.id, this.discussionsByDiffOrder);
-    },
     shouldRenderDiffs() {
       const { diffDiscussion, diffFile } = this.transformedDiscussion;
 
@@ -256,10 +242,21 @@ Please check your network connection and try again.`;
         });
     },
     jumpToNextDiscussion() {
-      const nextId =
-        this.nextUnresolvedDiscussionId(this.discussion.id, this.discussionsByDiffOrder);
+      const discussionIds = this.allDiscussions.map(d => d.id);
+      const unresolvedIds = this.unresolvedDiscussions.map(d => d.id);
+      const currentIndex = discussionIds.indexOf(this.discussion.id);
+      const remainingAfterCurrent = discussionIds.slice(currentIndex + 1);
+      const nextIndex = _.findIndex(remainingAfterCurrent, id => unresolvedIds.indexOf(id) > -1);
+
+      if (nextIndex > -1) {
+        const nextId = remainingAfterCurrent[nextIndex];
+        const el = document.querySelector(`[data-discussion-id="${nextId}"]`);
 
-      this.jumpToDiscussion(nextId);
+        if (el) {
+          this.expandDiscussion({ discussionId: nextId });
+          scrollToElement(el);
+        }
+      }
     },
   },
 };
@@ -401,7 +398,7 @@ Please check your network connection and try again.`;
                           </a>
                         </div>
                         <div
-                          v-if="showJumpToNextDiscussion"
+                          v-if="hasMultipleUnresolvedDiscussions"
                           class="btn-group"
                           role="group">
                           <button

app/assets/javascripts/notes/mixins/discussion_navigation.js

-import { scrollToElement } from '~/lib/utils/common_utils';
-
-export default {
-  methods: {
-    jumpToDiscussion(id) {
-      if (id) {
-        const activeTab = window.mrTabs.currentAction;
-        const selector =
-          activeTab === 'diffs'
-            ? `ul.notes[data-discussion-id="${id}"]`
-            : `div.discussion[data-discussion-id="${id}"]`;
-        const el = document.querySelector(selector);
-
-        if (activeTab === 'commits' || activeTab === 'pipelines') {
-          window.mrTabs.activateTab('show');
-        }
-
-        if (el) {
-          this.expandDiscussion({ discussionId: id });
-
-          scrollToElement(el);
-          return true;
-        }
-      }
-
-      return false;
-    },
-  },
-};

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

@@ -70,9 +70,6 @@ export const allDiscussions = (state, getters) => {
   return Object.values(resolved).concat(unresolved);
 };
 
-export const allResolvableDiscussions = (state, getters) =>
-  getters.allDiscussions.filter(d => !d.individual_note && d.resolvable);
-
 export const resolvedDiscussionsById = state => {
   const map = {};
 
@@ -89,51 +86,6 @@ export const resolvedDiscussionsById = state => {
   return map;
 };
 
-// Gets Discussions IDs ordered by the date of their initial note
-export const unresolvedDiscussionsIdsByDate = (state, getters) =>
-  getters.allResolvableDiscussions
-    .filter(d => !d.resolved)
-    .sort((a, b) => {
-      const aDate = new Date(a.notes[0].created_at);
-      const bDate = new Date(b.notes[0].created_at);
-
-      if (aDate < bDate) {
-        return -1;
-      }
-
-      return aDate === bDate ? 0 : 1;
-    })
-    .map(d => d.id);
-
-// Gets Discussions IDs ordered by their position in the diff
-//
-// Sorts the array of resolvable yet unresolved discussions by
-// comparing file names first. If file names are the same, compares
-// line numbers.
-export const unresolvedDiscussionsIdsByDiff = (state, getters) =>
-  getters.allResolvableDiscussions
-    .filter(d => !d.resolved)
-    .sort((a, b) => {
-      if (!a.diff_file || !b.diff_file) {
-        return 0;
-      }
-
-      // Get file names comparison result
-      const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path);
-
-      // Get the line numbers, to compare within the same file
-      const aLines = [a.position.formatter.new_line, a.position.formatter.old_line];
-      const bLines = [b.position.formatter.new_line, b.position.formatter.old_line];
-
-      return filenameComparison < 0 ||
-        (filenameComparison === 0 &&
-          // .max() because one of them might be zero (if removed/added)
-          Math.max(aLines[0], aLines[1]) < Math.max(bLines[0], bLines[1]))
-        ? -1
-        : 1;
-    })
-    .map(d => d.id);
-
 export const resolvedDiscussionCount = (state, getters) => {
   const resolvedMap = getters.resolvedDiscussionsById;
 
@@ -150,42 +102,5 @@ export const discussionTabCounter = state => {
   return all.length;
 };
 
-// Returns the list of discussion IDs ordered according to given parameter
-// @param {Boolean} diffOrder - is ordered by diff?
-export const unresolvedDiscussionsIdsOrdered = (state, getters) => diffOrder => {
-  if (diffOrder) {
-    return getters.unresolvedDiscussionsIdsByDiff;
-  }
-  return getters.unresolvedDiscussionsIdsByDate;
-};
-
-// Checks if a given discussion is the last in the current order (diff or date)
-// @param {Boolean} discussionId - id of the discussion
-// @param {Boolean} diffOrder - is ordered by diff?
-export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, diffOrder) => {
-  const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder);
-  const lastDiscussionId = idsOrdered[idsOrdered.length - 1];
-
-  return lastDiscussionId === discussionId;
-};
-
-// Gets the ID of the discussion following the one provided, respecting order (diff or date)
-// @param {Boolean} discussionId - id of the current discussion
-// @param {Boolean} diffOrder - is ordered by diff?
-export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => {
-  const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder);
-  const currentIndex = idsOrdered.indexOf(discussionId);
-
-  return idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0];
-};
-
-// @param {Boolean} diffOrder - is ordered by diff?
-export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => {
-  if (diffOrder) {
-    return getters.unresolvedDiscussionsIdsByDiff[0];
-  }
-  return getters.unresolvedDiscussionsIdsByDate[0];
-};
-
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -342,9 +342,8 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
         end
       end
 
-      it 'shows jump to next discussion button, apart from the last one' do
-        expect(page).to have_selector('.discussion-reply-holder', count: 2)
-        expect(page).to have_selector('.discussion-reply-holder .discussion-next-btn', count: 1)
+      it 'shows jump to next discussion button' do
+        expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn'))
       end
 
       it 'displays next discussion even if hidden' do

spec/javascripts/notes/components/discussion_counter_spec.js

@@ -46,7 +46,7 @@ describe('DiscussionCounter component', () => {
           discussions,
         });
         setFixtures(`
-          <div class="discussion" data-discussion-id="${firstDiscussionId}"></div>
+          <div data-discussion-id="${firstDiscussionId}"></div>
         `);
 
         vm.jumpToFirstUnresolvedDiscussion();

spec/javascripts/notes/components/noteable_discussion_spec.js

@@ -14,7 +14,6 @@ describe('noteable_discussion component', () => {
   preloadFixtures(discussionWithTwoUnresolvedNotes);
 
   beforeEach(() => {
-    window.mrTabs = {};
     store = createStore();
     store.dispatch('setNoteableData', noteableDataMock);
     store.dispatch('setNotesData', notesDataMock);
@@ -107,29 +106,33 @@ describe('noteable_discussion component', () => {
 
   describe('methods', () => {
     describe('jumpToNextDiscussion', () => {
-      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>
-            `);
+      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>
+        `);
 
-            vm.jumpToNextDiscussion();
+        vm.jumpToNextDiscussion();
 
-            expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId });
-          })
-          .then(done)
-          .catch(done.fail);
+        expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId });
       });
     });
   });

spec/javascripts/notes/mock_data.js

@@ -1168,87 +1168,3 @@ 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,
-};

spec/javascripts/notes/stores/getters_spec.js

@@ -5,11 +5,6 @@ import {
   noteableDataMock,
   individualNote,
   collapseNotesMock,
-  discussion1,
-  discussion2,
-  discussion3,
-  resolvedDiscussion1,
-  unresolvableDiscussion,
 } from '../mock_data';
 
 const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json';
@@ -114,154 +109,4 @@ 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();
-    });
-  });
 });