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

MR refactor fixes for 11.1.3

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

app/assets/javascripts/autosave.js

@@ -53,4 +53,8 @@ export default class Autosave {
 
     return window.localStorage.removeItem(this.key);
   }
+
+  dispose() {
+    this.field.off('input');
+  }
 }

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

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

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

@@ -71,13 +71,18 @@ export default {
       required: false,
       default: false,
     },
+    discussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   computed: {
     ...mapState({
       diffViewType: state => state.diffs.diffViewType,
       diffFiles: state => state.diffs.diffFiles,
     }),
-    ...mapGetters(['isLoggedIn', 'discussionsByLineCode']),
+    ...mapGetters(['isLoggedIn']),
     lineHref() {
       return this.lineCode ? `#${this.lineCode}` : '#';
     },
@@ -87,24 +92,19 @@ export default {
         this.showCommentButton &&
         !this.isMatchLine &&
         !this.isContextLine &&
-        !this.hasDiscussions &&
-        !this.isMetaLine
+        !this.isMetaLine &&
+        !this.hasDiscussions
       );
     },
-    discussions() {
-      return this.discussionsByLineCode[this.lineCode] || [];
-    },
     hasDiscussions() {
       return this.discussions.length > 0;
     },
     shouldShowAvatarsOnGutter() {
-      let render = this.hasDiscussions && this.showCommentButton;
-
       if (!this.lineType && this.linePosition === LINE_POSITION_RIGHT) {
-        render = false;
+        return false;
       }
 
-      return render;
+      return this.hasDiscussions && this.showCommentButton;
     },
   },
   methods: {
@@ -189,7 +189,6 @@ export default {
       </button>
       <a
         v-if="lineNumber"
-        v-once
         :data-linenumber="lineNumber"
         :href="lineHref"
       >

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

 <script>
-import $ from 'jquery';
 import { mapState, mapGetters, mapActions } from 'vuex';
 import createFlash from '~/flash';
 import { s__ } from '~/locale';
 import noteForm from '../../notes/components/note_form.vue';
 import { getNoteFormData } from '../store/utils';
-import Autosave from '../../autosave';
-import { DIFF_NOTE_TYPE, NOTE_TYPE } from '../constants';
+import autosave from '../../notes/mixins/autosave';
+import { DIFF_NOTE_TYPE } from '../constants';
 
 export default {
   components: {
     noteForm,
   },
+  mixins: [autosave],
   props: {
     diffFileHash: {
       type: String,
@@ -41,28 +41,35 @@ export default {
   },
   mounted() {
     if (this.isLoggedIn) {
-      const noteableData = this.getNoteableData;
       const keys = [
-        NOTE_TYPE,
-        this.noteableType,
-        noteableData.id,
-        noteableData.diff_head_sha,
+        this.noteableData.diff_head_sha,
         DIFF_NOTE_TYPE,
-        noteableData.source_project_id,
+        this.noteableData.source_project_id,
         this.line.lineCode,
       ];
 
-      this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys);
+      this.initAutoSave(this.noteableData, keys);
     }
   },
   methods: {
     ...mapActions('diffs', ['cancelCommentForm']),
     ...mapActions(['saveNote', 'refetchDiscussionById']),
-    handleCancelCommentForm() {
-      this.autosave.reset();
+    handleCancelCommentForm(shouldConfirm, isDirty) {
+      if (shouldConfirm && isDirty) {
+        const msg = s__('Notes|Are you sure you want to cancel creating this comment?');
+
+        // eslint-disable-next-line no-alert
+        if (!window.confirm(msg)) {
+          return;
+        }
+      }
+
       this.cancelCommentForm({
         lineCode: this.line.lineCode,
       });
+      this.$nextTick(() => {
+        this.resetAutoSave();
+      });
     },
     handleSaveNote(note) {
       const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash);

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

@@ -67,6 +67,11 @@ export default {
       required: false,
       default: false,
     },
+    discussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   computed: {
     ...mapGetters(['isLoggedIn']),
@@ -136,6 +141,7 @@ export default {
       :is-match-line="isMatchLine"
       :is-context-line="isContentLine"
       :is-meta-line="isMetaLine"
+      :discussions="discussions"
     />
   </td>
 </template>

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

 <script>
-import { mapState, mapGetters } from 'vuex';
+import { mapState } from 'vuex';
 import diffDiscussions from './diff_discussions.vue';
 import diffLineNoteForm from './diff_line_note_form.vue';
 
@@ -21,18 +21,22 @@ export default {
       type: Number,
       required: true,
     },
+    discussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   computed: {
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
-    ...mapGetters(['discussionsByLineCode']),
-    discussions() {
-      return this.discussionsByLineCode[this.line.lineCode] || [];
-    },
     className() {
       return this.discussions.length ? '' : 'js-temp-notes-holder';
     },
+    hasCommentForm() {
+      return this.diffLineCommentForms[this.line.lineCode];
+    },
   },
 };
 </script>
@@ -53,7 +57,7 @@ export default {
           :discussions="discussions"
         />
         <diff-line-note-form
-          v-if="diffLineCommentForms[line.lineCode]"
+          v-if="hasCommentForm"
           :diff-file-hash="diffFileHash"
           :line="line"
           :note-target-line="line"

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

@@ -33,6 +33,11 @@ export default {
       required: false,
       default: false,
     },
+    discussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   data() {
     return {
@@ -89,6 +94,7 @@ export default {
       :is-bottom="isBottom"
       :is-hover="isHover"
       :show-comment-button="true"
+      :discussions="discussions"
       class="diff-line-num old_line"
     />
     <diff-table-cell
@@ -98,10 +104,10 @@ export default {
       :line-type="newLineType"
       :is-bottom="isBottom"
       :is-hover="isHover"
+      :discussions="discussions"
       class="diff-line-num new_line"
     />
     <td
-      v-once
       :class="line.type"
       class="line_content"
       v-html="line.richText"

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

@@ -20,8 +20,11 @@ export default {
     },
   },
   computed: {
-    ...mapGetters('diffs', ['commitId']),
-    ...mapGetters(['discussionsByLineCode']),
+    ...mapGetters('diffs', [
+      'commitId',
+      'shouldRenderInlineCommentRow',
+      'singleDiscussionByLineCode',
+    ]),
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
@@ -35,18 +38,7 @@ export default {
       return window.gon.user_color_scheme;
     },
   },
-  methods: {
-    shouldRenderCommentRow(line) {
-      if (this.diffLineCommentForms[line.lineCode]) return true;
-
-      const lineDiscussions = this.discussionsByLineCode[line.lineCode];
-      if (lineDiscussions === undefined) {
-        return false;
-      }
-
-      return lineDiscussions.every(discussion => discussion.expanded);
-    },
-  },
+  methods: {},
 };
 </script>
 
@@ -65,13 +57,15 @@ export default {
           :line="line"
           :is-bottom="index + 1 === diffLinesLength"
           :key="line.lineCode"
+          :discussions="singleDiscussionByLineCode(line.lineCode)"
         />
         <inline-diff-comment-row
-          v-if="shouldRenderCommentRow(line)"
+          v-if="shouldRenderInlineCommentRow(line)"
           :diff-file-hash="diffFile.fileHash"
           :line="line"
           :line-index="index"
           :key="index"
+          :discussions="singleDiscussionByLineCode(line.lineCode)"
         />
       </template>
     </tbody>

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

 <script>
-import { mapState, mapGetters } from 'vuex';
+import { mapState } from 'vuex';
 import diffDiscussions from './diff_discussions.vue';
 import diffLineNoteForm from './diff_line_note_form.vue';
 
@@ -21,48 +21,51 @@ export default {
       type: Number,
       required: true,
     },
+    leftDiscussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
+    rightDiscussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   computed: {
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
-    ...mapGetters(['discussionsByLineCode']),
     leftLineCode() {
       return this.line.left.lineCode;
     },
     rightLineCode() {
       return this.line.right.lineCode;
     },
-    hasDiscussion() {
-      const discussions = this.discussionsByLineCode;
-
-      return discussions[this.leftLineCode] || discussions[this.rightLineCode];
-    },
     hasExpandedDiscussionOnLeft() {
-      const discussions = this.discussionsByLineCode[this.leftLineCode];
-
+      const discussions = this.leftDiscussions;
       return discussions ? discussions.every(discussion => discussion.expanded) : false;
     },
     hasExpandedDiscussionOnRight() {
-      const discussions = this.discussionsByLineCode[this.rightLineCode];
-
+      const discussions = this.rightDiscussions;
       return discussions ? discussions.every(discussion => discussion.expanded) : false;
     },
     hasAnyExpandedDiscussion() {
       return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight;
     },
     shouldRenderDiscussionsOnLeft() {
-      return this.discussionsByLineCode[this.leftLineCode] && this.hasExpandedDiscussionOnLeft;
+      return this.leftDiscussions && this.hasExpandedDiscussionOnLeft;
     },
     shouldRenderDiscussionsOnRight() {
-      return (
-        this.discussionsByLineCode[this.rightLineCode] &&
-        this.hasExpandedDiscussionOnRight &&
-        this.line.right.type
-      );
+      return this.rightDiscussions && this.hasExpandedDiscussionOnRight && this.line.right.type;
+    },
+    showRightSideCommentForm() {
+      return this.line.right.type && this.diffLineCommentForms[this.rightLineCode];
     },
     className() {
-      return this.hasDiscussion ? '' : 'js-temp-notes-holder';
+      return this.leftDiscussions.length > 0 || this.rightDiscussions.length > 0
+        ? ''
+        : 'js-temp-notes-holder';
     },
   },
 };
@@ -80,13 +83,12 @@ export default {
         class="content"
       >
         <diff-discussions
-          v-if="discussionsByLineCode[leftLineCode].length"
-          :discussions="discussionsByLineCode[leftLineCode]"
+          v-if="leftDiscussions.length"
+          :discussions="leftDiscussions"
         />
       </div>
       <diff-line-note-form
-        v-if="diffLineCommentForms[leftLineCode] &&
-        diffLineCommentForms[leftLineCode]"
+        v-if="diffLineCommentForms[leftLineCode]"
         :diff-file-hash="diffFileHash"
         :line="line.left"
         :note-target-line="line.left"
@@ -100,13 +102,12 @@ export default {
         class="content"
       >
         <diff-discussions
-          v-if="discussionsByLineCode[rightLineCode].length"
-          :discussions="discussionsByLineCode[rightLineCode]"
+          v-if="rightDiscussions.length"
+          :discussions="rightDiscussions"
         />
       </div>
       <diff-line-note-form
-        v-if="diffLineCommentForms[rightLineCode] &&
-        diffLineCommentForms[rightLineCode] && line.right.type"
+        v-if="showRightSideCommentForm"
         :diff-file-hash="diffFileHash"
         :line="line.right"
         :note-target-line="line.right"

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

@@ -36,6 +36,16 @@ export default {
       required: false,
       default: false,
     },
+    leftDiscussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
+    rightDiscussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   data() {
     return {
@@ -116,10 +126,10 @@ export default {
       :is-hover="isLeftHover"
       :show-comment-button="true"
       :diff-view-type="parallelDiffViewType"
+      :discussions="leftDiscussions"
       class="diff-line-num old_line"
     />
     <td
-      v-once
       :id="line.left.lineCode"
       :class="parallelViewLeftLineType"
       class="line_content parallel left-side"
@@ -137,10 +147,10 @@ export default {
       :is-hover="isRightHover"
       :show-comment-button="true"
       :diff-view-type="parallelDiffViewType"
+      :discussions="rightDiscussions"
       class="diff-line-num new_line"
     />
     <td
-      v-once
       :id="line.right.lineCode"
       :class="line.right.type"
       class="line_content parallel right-side"

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

@@ -21,8 +21,11 @@ export default {
     },
   },
   computed: {
-    ...mapGetters('diffs', ['commitId']),
-    ...mapGetters(['discussionsByLineCode']),
+    ...mapGetters('diffs', [
+      'commitId',
+      'singleDiscussionByLineCode',
+      'shouldRenderParallelCommentRow',
+    ]),
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
@@ -52,32 +55,6 @@ export default {
       return window.gon.user_color_scheme;
     },
   },
-  methods: {
-    shouldRenderCommentRow(line) {
-      const leftLineCode = line.left.lineCode;
-      const rightLineCode = line.right.lineCode;
-      const discussions = this.discussionsByLineCode;
-      const leftDiscussions = discussions[leftLineCode];
-      const rightDiscussions = discussions[rightLineCode];
-      const hasDiscussion = leftDiscussions || rightDiscussions;
-
-      const hasExpandedDiscussionOnLeft = leftDiscussions
-        ? leftDiscussions.every(discussion => discussion.expanded)
-        : false;
-      const hasExpandedDiscussionOnRight = rightDiscussions
-        ? rightDiscussions.every(discussion => discussion.expanded)
-        : false;
-
-      if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) {
-        return true;
-      }
-
-      const hasCommentFormOnLeft = this.diffLineCommentForms[leftLineCode];
-      const hasCommentFormOnRight = this.diffLineCommentForms[rightLineCode];
-
-      return hasCommentFormOnLeft || hasCommentFormOnRight;
-    },
-  },
 };
 </script>
 
@@ -98,13 +75,17 @@ export default {
             :line="line"
             :is-bottom="index + 1 === diffLinesLength"
             :key="index"
+            :left-discussions="singleDiscussionByLineCode(line.left.lineCode)"
+            :right-discussions="singleDiscussionByLineCode(line.right.lineCode)"
           />
           <parallel-diff-comment-row
-            v-if="shouldRenderCommentRow(line)"
+            v-if="shouldRenderParallelCommentRow(line)"
             :key="`dcr-${index}`"
             :line="line"
             :diff-file-hash="diffFile.fileHash"
             :line-index="index"
+            :left-discussions="singleDiscussionByLineCode(line.left.lineCode)"
+            :right-discussions="singleDiscussionByLineCode(line.right.lineCode)"
           />
         </template>
       </tbody>

app/assets/javascripts/diffs/store/getters.js

 import _ from 'underscore';
+import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
 import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants';
+import { getDiffRefsByLineCode } from './utils';
 
 export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE;
 
@@ -56,6 +58,87 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) =
       discussion.diff_discussion && _.isEqual(discussion.diff_file.file_hash, diff.fileHash),
   ) || [];
 
+/**
+ * Returns an Object with discussions by their diff line code
+ * To avoid rendering outdated discussions on the Changes tab we should do a bunch of SHA
+ * comparisions. `note.position.formatter` have the current version diff refs but
+ * `note.original_position.formatter` will have the first version's diff refs.
+ * If line diff refs matches with one of them, we should render it as a discussion on Changes tab.
+ *
+ * @param {Object} diff
+ * @returns {Array}
+ */
+export const discussionsByLineCode = (state, getters, rootState, rootGetters) => {
+  const diffRefsByLineCode = getDiffRefsByLineCode(state.diffFiles);
+
+  return rootGetters.discussions.reduce((acc, note) => {
+    const isDiffDiscussion = note.diff_discussion;
+    const hasLineCode = note.line_code;
+    const isResolvable = note.resolvable;
+
+    if (isDiffDiscussion && hasLineCode && isResolvable) {
+      const diffRefs = diffRefsByLineCode[note.line_code];
+      if (diffRefs) {
+        const refs = convertObjectPropsToCamelCase(note.position.formatter);
+        const originalRefs = convertObjectPropsToCamelCase(note.original_position.formatter);
+
+        if (_.isEqual(refs, diffRefs) || _.isEqual(originalRefs, diffRefs)) {
+          const lineCode = note.line_code;
+
+          if (acc[lineCode]) {
+            acc[lineCode].push(note);
+          } else {
+            acc[lineCode] = [note];
+          }
+        }
+      }
+    }
+
+    return acc;
+  }, {});
+};
+
+export const singleDiscussionByLineCode = (state, getters) => lineCode => {
+  if (!lineCode) return [];
+  const discussions = getters.discussionsByLineCode;
+  return discussions[lineCode] || [];
+};
+
+export const shouldRenderParallelCommentRow = (state, getters) => line => {
+  const leftLineCode = line.left.lineCode;
+  const rightLineCode = line.right.lineCode;
+  const leftDiscussions = getters.singleDiscussionByLineCode(leftLineCode);
+  const rightDiscussions = getters.singleDiscussionByLineCode(rightLineCode);
+  const hasDiscussion = leftDiscussions.length || rightDiscussions.length;
+
+  const hasExpandedDiscussionOnLeft = leftDiscussions.length
+    ? leftDiscussions.every(discussion => discussion.expanded)
+    : false;
+  const hasExpandedDiscussionOnRight = rightDiscussions.length
+    ? rightDiscussions.every(discussion => discussion.expanded)
+    : false;
+
+  if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) {
+    return true;
+  }
+
+  const hasCommentFormOnLeft = state.diffLineCommentForms[leftLineCode];
+  const hasCommentFormOnRight = state.diffLineCommentForms[rightLineCode];
+
+  return hasCommentFormOnLeft || hasCommentFormOnRight;
+};
+
+export const shouldRenderInlineCommentRow = (state, getters) => line => {
+  if (state.diffLineCommentForms[line.lineCode]) return true;
+
+  const lineDiscussions = getters.singleDiscussionByLineCode(line.lineCode);
+  if (lineDiscussions.length === 0) {
+    return false;
+  }
+
+  return lineDiscussions.every(discussion => discussion.expanded);
+};
+
 // prevent babel-plugin-rewire from generating an invalid default during karma∂ tests
 export const getDiffFileByHash = state => fileHash =>
   state.diffFiles.find(file => file.fileHash === fileHash);

app/assets/javascripts/diffs/store/utils.js

@@ -173,3 +173,24 @@ export function trimFirstCharOfLineContent(line = {}) {
 
   return parsedLine;
 }
+
+export function getDiffRefsByLineCode(diffFiles) {
+  return diffFiles.reduce((acc, diffFile) => {
+    const { baseSha, headSha, startSha } = diffFile.diffRefs;
+    const { newPath, oldPath } = diffFile;
+
+    // We can only use highlightedDiffLines to create the map of diff lines because
+    // highlightedDiffLines will also include every parallel diff line in it.
+    if (diffFile.highlightedDiffLines) {
+      diffFile.highlightedDiffLines.forEach(line => {
+        const { lineCode, oldLine, newLine } = line;
+
+        if (lineCode) {
+          acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine };
+        }
+      });
+    }
+
+    return acc;
+  }, {});
+}

app/assets/javascripts/lib/utils/poll.js

@@ -38,7 +38,7 @@ import { normalizeHeaders } from './common_utils';
  *  } else {
  *   poll.stop();
  *  }
-* });
+ * });
  *
  * 1. Checks for response and headers before start polling
  * 2. Interval is provided by `Poll-Interval` header.
@@ -51,8 +51,8 @@ export default class Poll {
   constructor(options = {}) {
     this.options = options;
     this.options.data = options.data || {};
-    this.options.notificationCallback = options.notificationCallback ||
-      function notificationCallback() {};
+    this.options.notificationCallback =
+      options.notificationCallback || function notificationCallback() {};
 
     this.intervalHeader = 'POLL-INTERVAL';
     this.timeoutID = null;
@@ -63,6 +63,7 @@ export default class Poll {
     const headers = normalizeHeaders(response.headers);
     const pollInterval = parseInt(headers[this.intervalHeader], 10);
     if (pollInterval > 0 && response.status === httpStatusCodes.OK && this.canPoll) {
+      clearTimeout(this.timeoutID);
       this.timeoutID = setTimeout(() => {
         this.makeRequest();
       }, pollInterval);
@@ -77,11 +78,11 @@ export default class Poll {
     notificationCallback(true);
 
     return resource[method](data)
-      .then((response) => {
+      .then(response => {
         this.checkConditions(response);
         notificationCallback(false);
       })
-      .catch((error) => {
+      .catch(error => {
         notificationCallback(false);
         if (error.status === httpStatusCodes.ABORTED) {
           return;

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

@@ -5,19 +5,20 @@ 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 { scrollToElement } from '../../lib/utils/common_utils';
+import discussionNavigation from '../mixins/discussion_navigation';
 import tooltip from '../../vue_shared/directives/tooltip';
 
 export default {
   directives: {
     tooltip,
   },
+  mixins: [discussionNavigation],
   computed: {
     ...mapGetters([
       'getUserData',
       'getNoteableData',
       'discussionCount',
-      'unresolvedDiscussions',
+      'firstUnresolvedDiscussionId',
       'resolvedDiscussionCount',
     ]),
     isLoggedIn() {
@@ -35,11 +36,6 @@ 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;
@@ -50,22 +46,10 @@ export default {
   methods: {
     ...mapActions(['expandDiscussion']),
     jumpToFirstUnresolvedDiscussion() {
-      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');
-      }
+      const diffTab = window.mrTabs.currentAction === 'diffs';
+      const discussionId = this.firstUnresolvedDiscussionId(diffTab);
 
-      if (el) {
-        this.expandDiscussion({ discussionId });
-        scrollToElement(el);
-      }
+      this.jumpToDiscussion(discussionId);
     },
   },
 };

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

@@ -7,7 +7,7 @@ import issuableStateMixin from '../mixins/issuable_state';
 import resolvable from '../mixins/resolvable';
 
 export default {
-  name: 'IssueNoteForm',
+  name: 'NoteForm',
   components: {
     issueWarning,
     markdownField,

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, scrollToElement } from '~/lib/utils/common_utils';
+import { convertObjectPropsToCamelCase } 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';
 import Flash from '../../flash';
 import { SYSTEM_NOTE } from '../constants';
 import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue';
@@ -20,6 +20,7 @@ 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 {
@@ -39,7 +40,7 @@ export default {
   directives: {
     tooltip,
   },
-  mixins: [autosave, noteable, resolvable],
+  mixins: [autosave, noteable, resolvable, discussionNavigation],
   props: {
     discussion: {
       type: Object,
@@ -60,6 +61,11 @@ export default {
       required: false,
       default: false,
     },
+    discussionsByDiffOrder: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -74,7 +80,12 @@ export default {
       'discussionCount',
       'resolvedDiscussionCount',
       'allDiscussions',
+      'unresolvedDiscussionsIdsByDiff',
+      'unresolvedDiscussionsIdsByDate',
       'unresolvedDiscussions',
+      'unresolvedDiscussionsIdsOrdered',
+      'nextUnresolvedDiscussionId',
+      'isLastUnresolvedDiscussion',
     ]),
     transformedDiscussion() {
       return {
@@ -125,6 +136,10 @@ 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;
 
@@ -144,19 +159,17 @@ export default {
       return this.isDiffDiscussion ? '' : 'card discussion-wrapper';
     },
   },
-  mounted() {
-    if (this.isReplying) {
-      this.initAutoSave(this.transformedDiscussion);
-    }
-  },
-  updated() {
-    if (this.isReplying) {
-      if (!this.autosave) {
-        this.initAutoSave(this.transformedDiscussion);
+  watch: {
+    isReplying() {
+      if (this.isReplying) {
+        this.$nextTick(() => {
+          // Pass an extra key to separate reply and note edit forms
+          this.initAutoSave(this.transformedDiscussion, ['Reply']);
+        });
       } else {
-        this.setAutoSave();
+        this.disposeAutoSave();
       }
-    }
+    },
   },
   created() {
     this.resolveDiscussionsSvg = resolveDiscussionsSvg;
@@ -194,16 +207,18 @@ export default {
     showReplyForm() {
       this.isReplying = true;
     },
-    cancelReplyForm(shouldConfirm) {
-      if (shouldConfirm && this.$refs.noteForm.isDirty) {
+    cancelReplyForm(shouldConfirm, isDirty) {
+      if (shouldConfirm && isDirty) {
+        const msg = s__('Notes|Are you sure you want to cancel creating this comment?');
+
         // eslint-disable-next-line no-alert
-        if (!window.confirm('Are you sure you want to cancel creating this comment?')) {
+        if (!window.confirm(msg)) {
           return;
         }
       }
 
-      this.resetAutoSave();
       this.isReplying = false;
+      this.resetAutoSave();
     },
     saveReply(noteText, form, callback) {
       const postData = {
@@ -241,21 +256,10 @@ Please check your network connection and try again.`;
         });
     },
     jumpToNextDiscussion() {
-      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}"]`);
+      const nextId =
+        this.nextUnresolvedDiscussionId(this.discussion.id, this.discussionsByDiffOrder);
 
-        if (el) {
-          this.expandDiscussion({ discussionId: nextId });
-          scrollToElement(el);
-        }
-      }
+      this.jumpToDiscussion(nextId);
     },
   },
 };
@@ -397,7 +401,7 @@ Please check your network connection and try again.`;
                           </a>
                         </div>
                         <div
-                          v-if="hasMultipleUnresolvedDiscussions"
+                          v-if="showJumpToNextDiscussion"
                           class="btn-group"
                           role="group">
                           <button
@@ -420,7 +424,8 @@ Please check your network connection and try again.`;
                     :is-editing="false"
                     save-button-title="Comment"
                     @handleFormUpdate="saveReply"
-                    @cancelForm="cancelReplyForm" />
+                    @cancelForm="cancelReplyForm"
+                  />
                   <note-signed-out-widget v-if="!canReply" />
                 </div>
               </div>

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

@@ -4,12 +4,18 @@ import { capitalizeFirstCharacter } from '../../lib/utils/text_utility';
 
 export default {
   methods: {
-    initAutoSave(noteable) {
-      this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), [
+    initAutoSave(noteable, extraKeys = []) {
+      let keys = [
         'Note',
-        capitalizeFirstCharacter(noteable.noteable_type),
+        capitalizeFirstCharacter(noteable.noteable_type || noteable.noteableType),
         noteable.id,
-      ]);
+      ];
+
+      if (extraKeys) {
+        keys = keys.concat(extraKeys);
+      }
+
+      this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys);
     },
     resetAutoSave() {
       this.autosave.reset();
@@ -17,5 +23,8 @@ export default {
     setAutoSave() {
       this.autosave.save();
     },
+    disposeAutoSave() {
+      this.autosave.dispose();
+    },
   },
 };

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

@@ -28,18 +28,6 @@ export const notesById = state =>
     return acc;
   }, {});
 
-export const discussionsByLineCode = state =>
-  state.discussions.reduce((acc, note) => {
-    if (note.diff_discussion && note.line_code && note.resolvable) {
-      // For context about line notes: there might be multiple notes with the same line code
-      const items = acc[note.line_code] || [];
-      items.push(note);
-
-      Object.assign(acc, { [note.line_code]: items });
-    }
-    return acc;
-  }, {});
-
 export const noteableType = state => {
   const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants;
 
@@ -82,6 +70,9 @@ 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 = {};
 
@@ -98,6 +89,51 @@ 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;
 
@@ -114,5 +150,42 @@ 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 () => {};

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

@@ -174,27 +174,19 @@ export default {
 
   [types.UPDATE_NOTE](state, note) {
     const noteObj = utils.findNoteObjectById(state.discussions, note.discussion_id);
-
     if (noteObj.individual_note) {
       noteObj.notes.splice(0, 1, note);
     } else {
       const comment = utils.findNoteObjectById(noteObj.notes, note.id);
-      noteObj.notes.splice(noteObj.notes.indexOf(comment), 1, note);
+      Object.assign(comment, note);
     }
   },
 
   [types.UPDATE_DISCUSSION](state, noteData) {
     const note = noteData;
-    let index = 0;
-
-    state.discussions.forEach((n, i) => {
-      if (n.id === note.id) {
-        index = i;
-      }
-    });
-
+    const selectedDiscussion = state.discussions.find(n => n.id === note.id);
     note.expanded = true; // override expand flag to prevent collapse
-    state.discussions.splice(index, 1, note);
+    Object.assign(selectedDiscussion, note);
   },
 
   [types.CLOSE_ISSUE](state) {
@@ -215,12 +207,9 @@ export default {
 
   [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) {
     const discussion = utils.findNoteObjectById(state.discussions, discussionId);
-    const index = state.discussions.indexOf(discussion);
 
-    const discussionWithDiffLines = Object.assign({}, discussion, {
+    Object.assign(discussion, {
       truncated_diff_lines: diffLines,
     });
-
-    state.discussions.splice(index, 1, discussionWithDiffLines);
   },
 };

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

@@ -2,13 +2,11 @@ import AjaxCache from '~/lib/utils/ajax_cache';
 
 const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm;
 
-export const findNoteObjectById = (notes, id) =>
-  notes.filter(n => n.id === id)[0];
+export const findNoteObjectById = (notes, id) => notes.find(n => n.id === id);
 
 export const getQuickActionText = note => {
   let text = 'Applying command';
-  const quickActions =
-    AjaxCache.get(gl.GfmAutoComplete.dataSources.commands) || [];
+  const quickActions = AjaxCache.get(gl.GfmAutoComplete.dataSources.commands) || [];
 
   const executedCommands = quickActions.filter(command => {
     const commandRegex = new RegExp(`/${command.name}`);
@@ -29,5 +27,4 @@ export const getQuickActionText = note => {
 
 export const hasQuickActions = note => REGEX_QUICK_ACTIONS.test(note);
 
-export const stripQuickActions = note =>
-  note.replace(REGEX_QUICK_ACTIONS, '').trim();
+export const stripQuickActions = note => note.replace(REGEX_QUICK_ACTIONS, '').trim();

app/serializers/discussion_entity.rb

@@ -4,6 +4,7 @@ class DiscussionEntity < Grape::Entity
 
   expose :id, :reply_id
   expose :position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
+  expose :original_position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
   expose :line_code, if: -> (d, _) { d.diff_discussion? }
   expose :expanded?, as: :expanded
   expose :active?, as: :active, if: -> (d, _) { d.diff_discussion? }

changelogs/unreleased/48817-fix-mr-changes-discussion-navigation.yml

+---
+title: Fix navigation to First and Next discussion on MR Changes tab
+merge_request: 20434
+author:
+type: fixed

changelogs/unreleased/_acet-fix-expanding-context-lines.yml

+---
+title: Fix rendering of the context lines in MR diffs page
+merge_request: 20642
+author:
+type: fixed

changelogs/unreleased/_acet-fix-mr-autosave.yml

+---
+title: Fix autosave and ESC confirmation issues for MR discussions
+merge_request: 20569
+author:
+type: fixed

changelogs/unreleased/_acet-fix-outdated-discussions.yml

+---
+title: Fix showing outdated discussions on Changes tab
+merge_request: 20445
+author:
+type: fixed

changelogs/unreleased/tz-mr-refactor-memory-reduction.yml

+---
+title: Reduces the client side memory footprint on merge requests
+merge_request: 20744
+author:
+type: performance

locale/gitlab.pot

@@ -3190,6 +3190,9 @@ msgstr ""
 msgid "Note: Consider asking your GitLab administrator to configure %{github_integration_link}, which will allow login via GitHub and allow importing repositories without generating a Personal Access Token."
 msgstr ""
 
+msgid "Notes|Are you sure you want to cancel creating this comment?"
+msgstr ""
+
 msgid "Notification events"
 msgstr ""
 

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -342,8 +342,9 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
         end
       end
 
-      it 'shows jump to next discussion button' do
-        expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn'))
+      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)
       end
 
       it 'displays next discussion even if hidden' do

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();
       });
     });
   });

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);
 

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);

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);

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: [

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);
+    });
+  });
 });

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);
+    });
+  });
 });

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();

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);
       });
     });
   });

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,
+};

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();
+    });
+  });
 });