Merge branch '49854-recover-mr-regression-fixes-safe' into 'master'

Resolve "Recover reverted fixes to Merge Request refactor regressions"

Closes #49242, #49343, #48876, #48877, #48817, #48602, and #49843

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

12 files changed, 197 insertions, 77 deletions

diff --git a/app/assets/javascripts/autosave.js b/app/assets/javascripts/autosave.js
index fa00a3cf386..e8c59fab609 100644
--- a/app/assets/javascripts/autosave.js
+++ b/app/assets/javascripts/autosave.js
@@ -53,4 +53,8 @@ export default class Autosave {
 
     return window.localStorage.removeItem(this.key);
   }
+
+  dispose() {
+    this.field.off('input');
+  }
 }
diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue
index 20483161033..e64d5511d78 100644
--- a/app/assets/javascripts/diffs/components/diff_discussions.vue
+++ b/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>
diff --git a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
index ad838a32518..a73f898e10b 100644
--- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
@@ -189,7 +189,6 @@ export default {
       </button>
       <a
         v-if="lineNumber"
-        v-once
         :data-linenumber="lineNumber"
         :href="lineHref"
       >
diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
index 32f9516d332..cbe4551d06b 100644
--- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
@@ -1,17 +1,17 @@
 <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);
diff --git a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue
index 0197a510ef1..0e306f39a9f 100644
--- a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue
+++ b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue
@@ -101,7 +101,6 @@ export default {
       class="diff-line-num new_line"
     />
     <td
-      v-once
       :class="line.type"
       class="line_content"
       v-html="line.richText"
diff --git a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue
index ee5bb4d8d05..0031cedc68f 100644
--- a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue
+++ b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue
@@ -119,7 +119,6 @@ export default {
       class="diff-line-num old_line"
     />
     <td
-      v-once
       :id="line.left.lineCode"
       :class="parallelViewLeftLineType"
       class="line_content parallel left-side"
@@ -140,7 +139,6 @@ export default {
       class="diff-line-num new_line"
     />
     <td
-      v-once
       :id="line.right.lineCode"
       :class="line.right.type"
       class="line_content parallel right-side"
diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue
index 6385b75e557..ad6e7cf501d 100644
--- a/app/assets/javascripts/notes/components/discussion_counter.vue
+++ b/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);
     },
   },
 };
diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue
index 26482a02e00..abcd4422d7c 100644
--- a/app/assets/javascripts/notes/components/note_form.vue
+++ b/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,
diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue
index bee635398b3..0fe1c16854a 100644
--- a/app/assets/javascripts/notes/components/noteable_discussion.vue
+++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
@@ -1,11 +1,11 @@
 <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>
diff --git a/app/assets/javascripts/notes/mixins/autosave.js b/app/assets/javascripts/notes/mixins/autosave.js
index 36cc8d5d056..4f45f912479 100644
--- a/app/assets/javascripts/notes/mixins/autosave.js
+++ b/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();
+    },
   },
 };
diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js
new file mode 100644
index 00000000000..f7c4deee1f8
--- /dev/null
+++ b/app/assets/javascripts/notes/mixins/discussion_navigation.js
@@ -0,0 +1,29 @@
+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;
+    },
+  },
+};
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index 5c65e1c3bb5..5b3b9f8776f 100644
--- a/app/assets/javascripts/notes/stores/getters.js
+++ b/app/assets/javascripts/notes/stores/getters.js
@@ -82,6 +82,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 +101,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 +162,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 () => {};