From 3612402bcd5db5638bf05a5d13f2c2dd35c68214 Mon Sep 17 00:00:00 2001
From: jboyson <jboyson@gitlab.com>
Date: Tue, 2 Jun 2020 16:41:00 -0500
Subject: [PATCH 1/7] Fix issues with MLC in parallel mode

Add support to side by side diffs for
multiline comments.

Most of this is around accounting for `left`
and `right` lines.
---
 .../batch_comments/components/draft_note.vue  |  2 +-
 .../components/parallel_draft_comment_row.vue |  8 +-
 .../diffs/components/diff_line_note_form.vue  | 31 ++++---
 .../notes/components/discussion_notes.vue     |  2 +
 .../components/multiline_comment_form.vue     | 32 ++++++--
 .../components/multiline_comment_utils.js     | 32 +++++---
 .../notes/components/noteable_note.vue        | 80 +++++++++++++------
 .../notes/mixins/diff_line_note_form.js       | 17 ++++
 app/controllers/concerns/notes_actions.rb     |  2 +-
 doc/api/discussions.md                        | 62 +++++++-------
 lib/api/discussions.rb                        | 16 +++-
 .../components/diff_line_note_form_spec.js    | 16 +++-
 .../multiline_comment_utils_spec.js           | 14 ++--
 .../notes/components/noteable_note_spec.js    | 44 +++++++++-
 14 files changed, 256 insertions(+), 102 deletions(-)

diff --git a/app/assets/javascripts/batch_comments/components/draft_note.vue b/app/assets/javascripts/batch_comments/components/draft_note.vue
index 9abfd43d85cebc..db5516b7b93316 100644
--- a/app/assets/javascripts/batch_comments/components/draft_note.vue
+++ b/app/assets/javascripts/batch_comments/components/draft_note.vue
@@ -71,8 +71,8 @@ export default {
     <ul class="notes draft-notes">
       <noteable-note
         :note="draft"
-        :diff-lines="diffFile.highlighted_diff_lines"
         :line="line"
+        :discussion-root="true"
         class="draft-note"
         @handleEdit="handleEditing"
         @cancelForm="handleNotEditing"
diff --git a/app/assets/javascripts/batch_comments/components/parallel_draft_comment_row.vue b/app/assets/javascripts/batch_comments/components/parallel_draft_comment_row.vue
index 68fd20e56bc950..b0916623cd2198 100644
--- a/app/assets/javascripts/batch_comments/components/parallel_draft_comment_row.vue
+++ b/app/assets/javascripts/batch_comments/components/parallel_draft_comment_row.vue
@@ -35,11 +35,15 @@ export default {
   <tr :class="className" class="notes_holder">
     <td class="notes_line old"></td>
     <td class="notes-content parallel old" colspan="2">
-      <div v-if="leftDraft.isDraft" class="content"><draft-note :draft="leftDraft" /></div>
+      <div v-if="leftDraft.isDraft" class="content">
+        <draft-note :draft="leftDraft" :line="line.left" />
+      </div>
     </td>
     <td class="notes_line new"></td>
     <td class="notes-content parallel new" colspan="2">
-      <div v-if="rightDraft.isDraft" class="content"><draft-note :draft="rightDraft" /></div>
+      <div v-if="rightDraft.isDraft" class="content">
+        <draft-note :draft="rightDraft" :line="line.right" />
+      </div>
     </td>
   </tr>
 </template>
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 74305ee69bc4f2..f4c2f8b0e6ddf5 100644
--- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
@@ -44,8 +44,10 @@ export default {
   data() {
     return {
       commentLineStart: {
-        lineCode: this.line.line_code,
+        line_code: this.line.line_code,
         type: this.line.type,
+        old_line: this.line.old_line,
+        new_line: this.line.new_line,
       },
     };
   },
@@ -75,10 +77,18 @@ export default {
         diffFile: this.diffFile,
         linePosition: this.linePosition,
         lineRange: {
-          start_line_code: this.commentLineStart.lineCode,
-          start_line_type: this.commentLineStart.type,
-          end_line_code: this.line.line_code,
-          end_line_type: this.line.type,
+          start: {
+            line_code: this.commentLineStart.line_code,
+            type: this.commentLineStart.type,
+            old_line: this.commentLineStart.old_line,
+            new_line: this.commentLineStart.new_line,
+          },
+          end: {
+            line_code: this.line.line_code,
+            type: this.line.type,
+            old_line: this.line.old_line,
+            new_line: this.line.new_line,
+          },
         },
       };
     },
@@ -86,7 +96,11 @@ export default {
       return this.getDiffFileByHash(this.diffFileHash);
     },
     commentLineOptions() {
-      return commentLineOptions(this.diffFile.highlighted_diff_lines, this.line.line_code);
+      const side = this.line.type === 'new' ? 'right' : 'left';
+      const lines = this.diffFile.highlighted_diff_lines.length
+        ? this.diffFile.highlighted_diff_lines
+        : this.diffFile.parallel_diff_lines.filter(l => Boolean(l[side])).map(l => l[side]);
+      return commentLineOptions(lines, this.line, this.line.line_code, side);
     },
   },
   mounted() {
@@ -136,10 +150,7 @@ export default {
 
 <template>
   <div class="content discussion-form discussion-form-container discussion-notes">
-    <div
-      v-if="glFeatures.multilineComments"
-      class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3"
-    >
+    <div v-if="glFeatures.multilineComments" class="gl-mb-3 gl-text-gray-700">
       <multiline-comment-form
         v-model="commentLineStart"
         :line="line"
diff --git a/app/assets/javascripts/notes/components/discussion_notes.vue b/app/assets/javascripts/notes/components/discussion_notes.vue
index 0b136549c1408a..b5e5daa88093cf 100644
--- a/app/assets/javascripts/notes/components/discussion_notes.vue
+++ b/app/assets/javascripts/notes/components/discussion_notes.vue
@@ -108,6 +108,7 @@ export default {
           :commit="commit"
           :help-page-path="helpPagePath"
           :show-reply-button="userCanReply"
+          :discussion-root="true"
           @handleDeleteNote="$emit('deleteNote')"
           @startReplying="$emit('startReplying')"
         >
@@ -151,6 +152,7 @@ export default {
           :note="componentData(note)"
           :help-page-path="helpPagePath"
           :line="diffLine"
+          :discussion-root="index === 0"
           @handleDeleteNote="$emit('deleteNote')"
         >
           <slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"></slot>
diff --git a/app/assets/javascripts/notes/components/multiline_comment_form.vue b/app/assets/javascripts/notes/components/multiline_comment_form.vue
index 5fba011a1535a9..69d978120993a8 100644
--- a/app/assets/javascripts/notes/components/multiline_comment_form.vue
+++ b/app/assets/javascripts/notes/components/multiline_comment_form.vue
@@ -21,10 +21,26 @@ export default {
   },
   data() {
     return {
-      commentLineStart: {
-        lineCode: this.lineRange ? this.lineRange.start_line_code : this.line.line_code,
-        type: this.lineRange ? this.lineRange.start_line_type : this.line.type,
-      },
+      commentLineStart: {},
+      commentLineEndType: this.lineRange?.end?.line_type || this.line.type,
+    };
+  },
+  computed: {
+    lineNumber() {
+      return (
+        this.getSymbol(this.line) +
+        (this.commentLineEndType === 'new' ? this.line.new_line : this.line.old_line)
+      );
+    },
+  },
+  created() {
+    const line = this.lineRange?.start || this.line;
+
+    this.commentLineStart = {
+      line_code: line.line_code,
+      type: line.type,
+      old_line: line.old_line,
+      new_line: line.new_line,
     };
   },
   methods: {
@@ -34,6 +50,10 @@ export default {
     getLineClasses(line) {
       return getLineClasses(line);
     },
+    updateCommentLineStart(value) {
+      this.commentLineStart = value;
+      this.$emit('input', value);
+    },
   },
 };
 </script>
@@ -55,12 +75,12 @@ export default {
           :options="commentLineOptions"
           size="sm"
           class="gl-w-auto gl-vertical-align-baseline"
-          @change="$emit('input', $event)"
+          @change="updateCommentLineStart"
         />
       </template>
       <template #end>
         <span :class="getLineClasses(line)">
-          {{ getSymbol(line) + (line.new_line || line.old_line) }}
+          {{ lineNumber }}
         </span>
       </template>
     </gl-sprintf>
diff --git a/app/assets/javascripts/notes/components/multiline_comment_utils.js b/app/assets/javascripts/notes/components/multiline_comment_utils.js
index dc9c55e9b30f12..59805f384ed1f7 100644
--- a/app/assets/javascripts/notes/components/multiline_comment_utils.js
+++ b/app/assets/javascripts/notes/components/multiline_comment_utils.js
@@ -8,8 +8,8 @@ export function getSymbol(type) {
 
 function getLineNumber(lineRange, key) {
   if (!lineRange || !key) return '';
-  const lineCode = lineRange[`${key}_line_code`] || '';
-  const lineType = lineRange[`${key}_line_type`] || '';
+  const lineCode = lineRange[key].line_code || '';
+  const lineType = lineRange[key].type || '';
   const lines = lineCode.split('_') || [];
   const lineNumber = lineType === 'old' ? lines[1] : lines[2];
   return (lineNumber && getSymbol(lineType) + lineNumber) || '';
@@ -37,21 +37,35 @@ export function getLineClasses(line) {
   ];
 }
 
-export function commentLineOptions(diffLines, lineCode) {
-  const selectedIndex = diffLines.findIndex(line => line.line_code === lineCode);
+export function commentLineOptions(diffLines, startingLine, lineCode, side = 'left') {
+  const preferredSide = side === 'left' ? 'old_line' : 'new_line';
+  const fallbackSide = preferredSide === 'new_line' ? 'old_line' : 'new_line';
   const notMatchType = l => l.type !== 'match';
+  const linesCopy = [...diffLines]; // don't mutate the argument
+  const startingLineCode = startingLine.line_code;
+
+  const currentIndex = linesCopy.findIndex(line => line.line_code === lineCode);
 
   // We're limiting adding comments to only lines above the current line
   // to make rendering simpler. Future interations will use a more
   // intuitive dragging interface that will make this unnecessary
-  const upToSelected = diffLines.slice(0, selectedIndex + 1);
+  const upToSelected = linesCopy.slice(0, currentIndex + 1);
 
   // Only include the lines up to the first "Show unchanged lines" block
   // i.e. not a "match" type
   const lines = takeRightWhile(upToSelected, notMatchType);
 
-  return lines.map(l => ({
-    value: { lineCode: l.line_code, type: l.type },
-    text: `${getSymbol(l.type)}${l.new_line || l.old_line}`,
-  }));
+  // If the selected line is "hidden" in an unchanged line block
+  // or "above" the current group of lines add it to the array so
+  // that the drop down is not defaulted to empty
+  const selectedIndex = lines.findIndex(line => line.line_code === startingLineCode);
+  if (selectedIndex < 0) lines.unshift(startingLine);
+
+  return lines.map(l => {
+    const { line_code, type, old_line, new_line } = l;
+    return {
+      value: { line_code, type, old_line, new_line },
+      text: `${getSymbol(type)}${l[preferredSide] || l[fallbackSide]}`,
+    };
+  });
 }
diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue
index 0e4dd1b9c842d0..57f51c90037890 100644
--- a/app/assets/javascripts/notes/components/noteable_note.vue
+++ b/app/assets/javascripts/notes/components/noteable_note.vue
@@ -62,10 +62,15 @@ export default {
       default: false,
     },
     diffLines: {
-      type: Object,
+      type: Array,
       required: false,
       default: null,
     },
+    discussionRoot: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -73,10 +78,7 @@ export default {
       isDeleting: false,
       isRequesting: false,
       isResolving: false,
-      commentLineStart: {
-        line_code: this.line?.line_code,
-        type: this.line?.type,
-      },
+      commentLineStart: {},
     };
   },
   computed: {
@@ -144,25 +146,42 @@ export default {
       return getEndLineNumber(this.lineRange);
     },
     showMultiLineComment() {
-      return (
-        this.glFeatures.multilineComments &&
-        this.startLineNumber &&
-        this.endLineNumber &&
-        (this.startLineNumber !== this.endLineNumber || this.isEditing)
-      );
+      if (!this.glFeatures.multilineComments) return false;
+      if (this.isEditing) return true;
+
+      return this.line && this.discussionRoot && this.startLineNumber !== this.endLineNumber;
     },
     commentLineOptions() {
-      if (this.diffLines) {
-        return commentLineOptions(this.diffLines, this.line.line_code);
+      if (!this.diffFile || !this.line) return [];
+
+      const sideA = this.line.type === 'new' ? 'right' : 'left';
+      const sideB = sideA === 'left' ? 'right' : 'left';
+      const lines = this.diffFile.highlighted_diff_lines.length
+        ? this.diffFile.highlighted_diff_lines
+        : this.diffFile.parallel_diff_lines.map(l => l[sideA] || l[sideB]);
+      return commentLineOptions(lines, this.commentLineStart, this.line.line_code, sideA);
+    },
+    diffFile() {
+      if (this.commentLineStart.line_code) {
+        const lineCode = this.commentLineStart.line_code.split('_')[0];
+        return this.getDiffFileByHash(lineCode);
       }
 
-      const diffFile = this.diffFile || this.getDiffFileByHash(this.targetNoteHash);
-      if (!diffFile) return null;
-      return commentLineOptions(diffFile.highlighted_diff_lines, this.line.line_code);
+      return null;
     },
   },
-
   created() {
+    const line = this.note.position?.line_range?.start || this.line;
+
+    this.commentLineStart = line
+      ? {
+          line_code: line.line_code,
+          type: line.type,
+          old_line: line.old_line,
+          new_line: line.new_line,
+        }
+      : {};
+
     eventHub.$on('enterEditMode', ({ noteId }) => {
       if (noteId === this.note.id) {
         this.isEditing = true;
@@ -225,10 +244,18 @@ export default {
       const position = {
         ...this.note.position,
         line_range: {
-          start_line_code: this.commentLineStart?.lineCode,
-          start_line_type: this.commentLineStart?.type,
-          end_line_code: this.line?.line_code,
-          end_line_type: this.line?.type,
+          start: {
+            line_code: this.commentLineStart?.line_code,
+            type: this.commentLineStart?.type,
+            old_line: this.commentLineStart?.old_line,
+            new_line: this.commentLineStart?.new_line,
+          },
+          end: {
+            line_code: this.line?.line_code,
+            type: this.line?.type,
+            old_line: this.line?.old_line,
+            new_line: this.line?.new_line,
+          },
         },
       };
       this.$emit('handleUpdateNote', {
@@ -246,7 +273,7 @@ export default {
         note: {
           target_type: this.getNoteableData.targetType,
           target_id: this.note.noteable_id,
-          note: { note: noteText },
+          note: { note: noteText, position: JSON.stringify(position) },
         },
       };
       this.isRequesting = true;
@@ -317,14 +344,17 @@ export default {
   >
     <div v-if="showMultiLineComment" data-testid="multiline-comment">
       <multiline-comment-form
-        v-if="isEditing && commentLineOptions && line"
+        v-if="isEditing && note.position"
         v-model="commentLineStart"
         :line="line"
         :comment-line-options="commentLineOptions"
         :line-range="note.position.line_range"
-        class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3"
+        class="gl-mb-3 gl-text-gray-700"
       />
-      <div v-else class="gl-mb-3 gl-text-gray-700">
+      <div
+        v-else
+        class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3"
+      >
         <gl-sprintf :message="__('Comment on lines %{startLine} to %{endLine}')">
           <template #startLine>
             <span :class="getLineClasses(startLineNumber)">{{ startLineNumber }}</span>
diff --git a/app/assets/javascripts/notes/mixins/diff_line_note_form.js b/app/assets/javascripts/notes/mixins/diff_line_note_form.js
index 5930b5f332157d..eddd09deda8a76 100644
--- a/app/assets/javascripts/notes/mixins/diff_line_note_form.js
+++ b/app/assets/javascripts/notes/mixins/diff_line_note_form.js
@@ -45,6 +45,22 @@ export default {
         });
     },
     addToReview(note) {
+      const lineRange = this.line
+        ? {
+            start: {
+              line_code: this.commentLineStart?.line_code,
+              type: this.commentLineStart?.type,
+              old_line: this.commentLineStart?.old_line,
+              new_line: this.commentLineStart?.new_line,
+            },
+            end: {
+              line_code: this.line.line_code,
+              type: this.line.type,
+              old_line: this.line.old_line,
+              new_line: this.line.new_line,
+            },
+          }
+        : {};
       const positionType = this.diffFileCommentForm
         ? IMAGE_DIFF_POSITION_TYPE
         : TEXT_DIFF_POSITION_TYPE;
@@ -60,6 +76,7 @@ export default {
         linePosition: this.position,
         positionType,
         ...this.diffFileCommentForm,
+        lineRange,
       });
 
       const diffFileHeadSha = this.commit && this?.diffFile?.diff_refs?.head_sha;
diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb
index d3dfb1813e4fef..eff125a1957797 100644
--- a/app/controllers/concerns/notes_actions.rb
+++ b/app/controllers/concerns/notes_actions.rb
@@ -226,7 +226,7 @@ def create_note_params
   end
 
   def update_note_params
-    params.require(:note).permit(:note)
+    params.require(:note).permit(:note, :position)
   end
 
   def set_polling_interval_header
diff --git a/doc/api/discussions.md b/doc/api/discussions.md
index 1f509a7aadca7d..aa1a691a8f8e94 100644
--- a/doc/api/discussions.md
+++ b/doc/api/discussions.md
@@ -782,10 +782,14 @@ Diff comments also contain position:
           "old_line": 27,
           "new_line": 27,
           "line_range": {
-            "start_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10",
-            "start_line_type": "new",
-            "end_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11",
-            "end_line_type": "old"
+            "start": {
+              "line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10",
+              "type": "new",
+            },
+            "end": {
+              "line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11",
+              "type": "old"
+            },
           }
         },
         "resolved": false,
@@ -832,30 +836,32 @@ POST /projects/:id/merge_requests/:merge_request_iid/discussions
 
 Parameters:
 
-| Attribute                               | Type           | Required | Description |
-| --------------------------------------- | -------------- | -------- | ----------- |
-| `id`                                    | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
-| `merge_request_iid`                     | integer        | yes      | The IID of a merge request |
-| `body`                                  | string         | yes      | The content of the thread |
-| `created_at`                            | string         | no       | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) |
-| `position`                              | hash           | no       | Position when creating a diff note |
-| `position[base_sha]`                    | string         | yes      | Base commit SHA in the source branch |
-| `position[start_sha]`                   | string         | yes      | SHA referencing commit in target branch |
-| `position[head_sha]`                    | string         | yes      | SHA referencing HEAD of this merge request |
-| `position[position_type]`               | string         | yes      | Type of the position reference', allowed values: 'text' or 'image' |
-| `position[new_path]`                    | string         | no       | File path after change |
-| `position[new_line]`                    | integer        | no       | Line number after change (for 'text' diff notes) |
-| `position[old_path]`                    | string         | no       | File path before change |
-| `position[old_line]`                    | integer        | no       | Line number before change (for 'text' diff notes) |
-| `position[line_range]`                  | hash           | no       | Line range for a multi-line diff note |
-| `position[line_range][start_line_code]` | string         | yes      | Line code for the start line |
-| `position[line_range][end_line_code]`   | string         | yes      | Line code for the end line |
-| `position[line_range][start_line_type]` | string         | yes      | Line type for the start line |
-| `position[line_range][end_line_type]`   | string         | yes      | Line type for the end line |
-| `position[width]`                       | integer        | no       | Width of the image (for 'image' diff notes) |
-| `position[height]`                      | integer        | no       | Height of the image (for 'image' diff notes) |
-| `position[x]`                           | integer        | no       | X coordinate (for 'image' diff notes) |
-| `position[y]`                           | integer        | no       | Y coordinate (for 'image' diff notes) |
+| Attribute                                | Type           | Required | Description |
+| ---------------------------------------- | -------------- | -------- | ----------- |
+| `id`                                     | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
+| `merge_request_iid`                      | integer        | yes      | The IID of a merge request |
+| `body`                                   | string         | yes      | The content of the thread |
+| `created_at`                             | string         | no       | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) |
+| `position`                               | hash           | no       | Position when creating a diff note |
+| `position[base_sha]`                     | string         | yes      | Base commit SHA in the source branch |
+| `position[start_sha]`                    | string         | yes      | SHA referencing commit in target branch |
+| `position[head_sha]`                     | string         | yes      | SHA referencing HEAD of this merge request |
+| `position[position_type]`                | string         | yes      | Type of the position reference', allowed values: 'text' or 'image' |
+| `position[new_path]`                     | string         | no       | File path after change |
+| `position[new_line]`                     | integer        | no       | Line number after change (for 'text' diff notes) |
+| `position[old_path]`                     | string         | no       | File path before change |
+| `position[old_line]`                     | integer        | no       | Line number before change (for 'text' diff notes) |
+| `position[line_range]`                   | hash           | no       | Line range for a multi-line diff note |
+| `position[line_range][start]`            | hash           | no       | Multiline note starting line |
+| `position[line_range][start][line_code]` | string         | yes      | Line code for the start line |
+| `position[line_range][start][type]`      | string         | yes      | Line type for the start line |
+| `position[line_range][end]`              | hash           | no       | Multiline note ending line |
+| `position[line_range][end][line_code]`   | string         | yes      | Line code for the end line |
+| `position[line_range][end][type]`        | string         | yes      | Line type for the end line |
+| `position[width]`                        | integer        | no       | Width of the image (for 'image' diff notes) |
+| `position[height]`                       | integer        | no       | Height of the image (for 'image' diff notes) |
+| `position[x]`                            | integer        | no       | X coordinate (for 'image' diff notes) |
+| `position[y]`                            | integer        | no       | Y coordinate (for 'image' diff notes) |
 
 ```shell
 curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions?body=comment"
diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb
index 29a13e4420dc48..c431ec8e1e4ef3 100644
--- a/lib/api/discussions.rb
+++ b/lib/api/discussions.rb
@@ -76,10 +76,18 @@ class Discussions < Grape::API::Instance
             optional :y, type: Integer, desc: 'Y coordinate in the image'
 
             optional :line_range, type: Hash, desc: 'Multi-line start and end' do
-              requires :start_line_code, type: String, desc: 'Start line code for multi-line note'
-              requires :end_line_code, type: String, desc: 'End line code for multi-line note'
-              requires :start_line_type, type: String, desc: 'Start line type for multi-line note'
-              requires :end_line_type, type: String, desc: 'End line type for multi-line note'
+              optional :start, type: Hash do
+                optional :line_code, type: String, desc: 'Start line code for multi-line note'
+                optional :type, type: String, desc: 'Start line type for multi-line note'
+                optional :old_line, type: String, desc: 'Start old_line line number'
+                optional :new_line, type: String, desc: 'Start new_line line number'
+              end
+              optional :end, type: Hash do
+                optional :line_code, type: String, desc: 'End line code for multi-line note'
+                optional :type, type: String, desc: 'End line type for multi-line note'
+                optional :old_line, type: String, desc: 'End old_line line number'
+                optional :new_line, type: String, desc: 'End new_line line number'
+              end
             end
           end
         end
diff --git a/spec/frontend/diffs/components/diff_line_note_form_spec.js b/spec/frontend/diffs/components/diff_line_note_form_spec.js
index 623df8bd55e010..75ec5c202af2ae 100644
--- a/spec/frontend/diffs/components/diff_line_note_form_spec.js
+++ b/spec/frontend/diffs/components/diff_line_note_form_spec.js
@@ -78,10 +78,18 @@ describe('DiffLineNoteForm', () => {
           .mockReturnValue(Promise.resolve());
 
         const lineRange = {
-          start_line_code: wrapper.vm.commentLineStart.lineCode,
-          start_line_type: wrapper.vm.commentLineStart.type,
-          end_line_code: wrapper.vm.line.line_code,
-          end_line_type: wrapper.vm.line.type,
+          start: {
+            line_code: wrapper.vm.commentLineStart.line_code,
+            type: wrapper.vm.commentLineStart.type,
+            new_line: 1,
+            old_line: null,
+          },
+          end: {
+            line_code: wrapper.vm.line.line_code,
+            type: wrapper.vm.line.type,
+            new_line: 1,
+            old_line: null,
+          },
         };
 
         const formData = {
diff --git a/spec/frontend/notes/components/multiline_comment_utils_spec.js b/spec/frontend/notes/components/multiline_comment_utils_spec.js
index 261bfb106e7811..cd5507322c5d73 100644
--- a/spec/frontend/notes/components/multiline_comment_utils_spec.js
+++ b/spec/frontend/notes/components/multiline_comment_utils_spec.js
@@ -7,30 +7,28 @@ import {
 describe('Multiline comment utilities', () => {
   describe('getStartLineNumber', () => {
     it.each`
-      lineCode        | type     | result
+      line_code       | type     | result
       ${'abcdef_1_1'} | ${'old'} | ${'-1'}
       ${'abcdef_1_1'} | ${'new'} | ${'+1'}
       ${'abcdef_1_1'} | ${null}  | ${'1'}
       ${'abcdef'}     | ${'new'} | ${''}
       ${'abcdef'}     | ${'old'} | ${''}
       ${'abcdef'}     | ${null}  | ${''}
-    `('returns line number', ({ lineCode, type, result }) => {
-      expect(getStartLineNumber({ start_line_code: lineCode, start_line_type: type })).toEqual(
-        result,
-      );
+    `('returns line number', ({ line_code, type, result }) => {
+      expect(getStartLineNumber({ start: { line_code, type } })).toEqual(result);
     });
   });
   describe('getEndLineNumber', () => {
     it.each`
-      lineCode        | type     | result
+      line_code       | type     | result
       ${'abcdef_1_1'} | ${'old'} | ${'-1'}
       ${'abcdef_1_1'} | ${'new'} | ${'+1'}
       ${'abcdef_1_1'} | ${null}  | ${'1'}
       ${'abcdef'}     | ${'new'} | ${''}
       ${'abcdef'}     | ${'old'} | ${''}
       ${'abcdef'}     | ${null}  | ${''}
-    `('returns line number', ({ lineCode, type, result }) => {
-      expect(getEndLineNumber({ end_line_code: lineCode, end_line_type: type })).toEqual(result);
+    `('returns line number', ({ line_code, type, result }) => {
+      expect(getEndLineNumber({ end: { line_code, type } })).toEqual(result);
     });
   });
   describe('getSymbol', () => {
diff --git a/spec/frontend/notes/components/noteable_note_spec.js b/spec/frontend/notes/components/noteable_note_spec.js
index aa3eaa97e200c3..3cb6cfe9787c93 100644
--- a/spec/frontend/notes/components/noteable_note_spec.js
+++ b/spec/frontend/notes/components/noteable_note_spec.js
@@ -46,12 +46,30 @@ describe('issue_note', () => {
     it('should render if has multiline comment', () => {
       const position = {
         line_range: {
-          start_line_code: 'abc_1_1',
-          end_line_code: 'abc_2_2',
+          start: {
+            line_code: 'abc_1_1',
+            type: null,
+            old_line: '1',
+            new_line: '1',
+          },
+          end: {
+            line_code: 'abc_2_2',
+            type: null,
+            old_line: '2',
+            new_line: '2',
+          },
         },
       };
+      const line = {
+        line_code: 'abc_1_1',
+        type: null,
+        old_line: '1',
+        new_line: '1',
+      };
       wrapper.setProps({
         note: { ...note, position },
+        discussionRoot: true,
+        line,
       });
 
       return wrapper.vm.$nextTick().then(() => {
@@ -62,12 +80,30 @@ describe('issue_note', () => {
     it('should not render if has single line comment', () => {
       const position = {
         line_range: {
-          start_line_code: 'abc_1_1',
-          end_line_code: 'abc_1_1',
+          start: {
+            line_code: 'abc_1_1',
+            type: null,
+            old_line: '1',
+            new_line: '1',
+          },
+          end: {
+            line_code: 'abc_1_1',
+            type: null,
+            old_line: '1',
+            new_line: '1',
+          },
         },
       };
+      const line = {
+        line_code: 'abc_1_1',
+        type: null,
+        old_line: '1',
+        new_line: '1',
+      };
       wrapper.setProps({
         note: { ...note, position },
+        discussionRoot: true,
+        line,
       });
 
       return wrapper.vm.$nextTick().then(() => {
-- 
GitLab


From d7583272b96e56179ce58c221d27cb090fa9ebbd Mon Sep 17 00:00:00 2001
From: jboyson <jboyson@gitlab.com>
Date: Mon, 15 Jun 2020 13:59:28 -0500
Subject: [PATCH 2/7] Code review changes

---
 .../diffs/components/diff_line_note_form.vue  | 25 +++++++------------
 .../components/multiline_comment_utils.js     | 13 ++++++++++
 .../notes/components/noteable_note.vue        | 19 ++++----------
 .../notes/mixins/diff_line_note_form.js       | 20 +++------------
 4 files changed, 31 insertions(+), 46 deletions(-)

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 f4c2f8b0e6ddf5..08f21c5f9091b7 100644
--- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
@@ -8,7 +8,10 @@ import MultilineCommentForm from '../../notes/components/multiline_comment_form.
 import autosave from '../../notes/mixins/autosave';
 import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue';
 import { DIFF_NOTE_TYPE } from '../constants';
-import { commentLineOptions } from '../../notes/components/multiline_comment_utils';
+import {
+  commentLineOptions,
+  formatLineRange,
+} from '../../notes/components/multiline_comment_utils';
 
 export default {
   components: {
@@ -76,20 +79,7 @@ export default {
         diffViewType: this.diffViewType,
         diffFile: this.diffFile,
         linePosition: this.linePosition,
-        lineRange: {
-          start: {
-            line_code: this.commentLineStart.line_code,
-            type: this.commentLineStart.type,
-            old_line: this.commentLineStart.old_line,
-            new_line: this.commentLineStart.new_line,
-          },
-          end: {
-            line_code: this.line.line_code,
-            type: this.line.type,
-            old_line: this.line.old_line,
-            new_line: this.line.new_line,
-          },
-        },
+        lineRange: formatLineRange(this.commentLineStart, this.line),
       };
     },
     diffFile() {
@@ -99,7 +89,10 @@ export default {
       const side = this.line.type === 'new' ? 'right' : 'left';
       const lines = this.diffFile.highlighted_diff_lines.length
         ? this.diffFile.highlighted_diff_lines
-        : this.diffFile.parallel_diff_lines.filter(l => Boolean(l[side])).map(l => l[side]);
+        : this.diffFile.parallel_diff_lines.reduce(
+            (acc, l) => (l[side] ? [...acc, l[side]] : acc),
+            [],
+          );
       return commentLineOptions(lines, this.line, this.line.line_code, side);
     },
   },
diff --git a/app/assets/javascripts/notes/components/multiline_comment_utils.js b/app/assets/javascripts/notes/components/multiline_comment_utils.js
index 59805f384ed1f7..349200d77c5bbf 100644
--- a/app/assets/javascripts/notes/components/multiline_comment_utils.js
+++ b/app/assets/javascripts/notes/components/multiline_comment_utils.js
@@ -69,3 +69,16 @@ export function commentLineOptions(diffLines, startingLine, lineCode, side = 'le
     };
   });
 }
+
+export function formatLineRange(start, end) {
+  const extractProps = ({ line_code, type, old_line, new_line }) => ({
+    line_code,
+    type,
+    old_line,
+    new_line,
+  });
+  return {
+    start: extractProps(start),
+    end: extractProps(end),
+  };
+}
diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue
index 57f51c90037890..611bd19e628e2e 100644
--- a/app/assets/javascripts/notes/components/noteable_note.vue
+++ b/app/assets/javascripts/notes/components/noteable_note.vue
@@ -21,6 +21,7 @@ import {
   getEndLineNumber,
   getLineClasses,
   commentLineOptions,
+  formatLineRange,
 } from './multiline_comment_utils';
 import MultilineCommentForm from './multiline_comment_form.vue';
 
@@ -243,21 +244,11 @@ export default {
     formUpdateHandler(noteText, parentElement, callback, resolveDiscussion) {
       const position = {
         ...this.note.position,
-        line_range: {
-          start: {
-            line_code: this.commentLineStart?.line_code,
-            type: this.commentLineStart?.type,
-            old_line: this.commentLineStart?.old_line,
-            new_line: this.commentLineStart?.new_line,
-          },
-          end: {
-            line_code: this.line?.line_code,
-            type: this.line?.type,
-            old_line: this.line?.old_line,
-            new_line: this.line?.new_line,
-          },
-        },
       };
+
+      if (this.commentLineStart && this.line)
+        position.line_range = formatLineRange(this.commentLineStart, this.line);
+
       this.$emit('handleUpdateNote', {
         note: this.note,
         noteText,
diff --git a/app/assets/javascripts/notes/mixins/diff_line_note_form.js b/app/assets/javascripts/notes/mixins/diff_line_note_form.js
index eddd09deda8a76..9a2e86aeed2937 100644
--- a/app/assets/javascripts/notes/mixins/diff_line_note_form.js
+++ b/app/assets/javascripts/notes/mixins/diff_line_note_form.js
@@ -4,6 +4,7 @@ import { TEXT_DIFF_POSITION_TYPE, IMAGE_DIFF_POSITION_TYPE } from '~/diffs/const
 import createFlash from '~/flash';
 import { s__ } from '~/locale';
 import { clearDraft } from '~/lib/utils/autosave';
+import { formatLineRange } from '~/notes/components/multiline_comment_utils';
 
 export default {
   computed: {
@@ -45,22 +46,9 @@ export default {
         });
     },
     addToReview(note) {
-      const lineRange = this.line
-        ? {
-            start: {
-              line_code: this.commentLineStart?.line_code,
-              type: this.commentLineStart?.type,
-              old_line: this.commentLineStart?.old_line,
-              new_line: this.commentLineStart?.new_line,
-            },
-            end: {
-              line_code: this.line.line_code,
-              type: this.line.type,
-              old_line: this.line.old_line,
-              new_line: this.line.new_line,
-            },
-          }
-        : {};
+      const lineRange =
+        (this.line && this.commentLineStart && formatLineRange(this.commentLineStart, this.line)) ||
+        {};
       const positionType = this.diffFileCommentForm
         ? IMAGE_DIFF_POSITION_TYPE
         : TEXT_DIFF_POSITION_TYPE;
-- 
GitLab


From eb1064a69db907a9c829ed487a00182f6002a18a Mon Sep 17 00:00:00 2001
From: jboyson <jboyson@gitlab.com>
Date: Wed, 24 Jun 2020 17:32:49 -0500
Subject: [PATCH 3/7] Get rspecs working

---
 .../diffs/components/diff_line_note_form.vue  | 13 ++-
 .../components/multiline_comment_utils.js     | 18 ++--
 .../user_comments_on_diff_spec.rb             | 32 +------
 ..._on_merge_request_files_shared_examples.rb | 84 +++++++++++++++++++
 4 files changed, 107 insertions(+), 40 deletions(-)
 create mode 100644 spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb

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 08f21c5f9091b7..a737d95659930f 100644
--- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
@@ -86,13 +86,18 @@ export default {
       return this.getDiffFileByHash(this.diffFileHash);
     },
     commentLineOptions() {
+      const combineSides = (acc, { left, right }) => {
+        // ignore null values match lines
+        if (left && left.type !== 'match') acc.push(left);
+        // if the line_codes are identically, return to avoid duplicates
+        if (left?.line_code === right?.line_code) return acc;
+        if (right && right.type !== 'match') acc.push(right);
+        return acc;
+      };
       const side = this.line.type === 'new' ? 'right' : 'left';
       const lines = this.diffFile.highlighted_diff_lines.length
         ? this.diffFile.highlighted_diff_lines
-        : this.diffFile.parallel_diff_lines.reduce(
-            (acc, l) => (l[side] ? [...acc, l[side]] : acc),
-            [],
-          );
+        : this.diffFile.parallel_diff_lines.reduce(combineSides, []);
       return commentLineOptions(lines, this.line, this.line.line_code, side);
     },
   },
diff --git a/app/assets/javascripts/notes/components/multiline_comment_utils.js b/app/assets/javascripts/notes/components/multiline_comment_utils.js
index 349200d77c5bbf..77d23c807b37c2 100644
--- a/app/assets/javascripts/notes/components/multiline_comment_utils.js
+++ b/app/assets/javascripts/notes/components/multiline_comment_utils.js
@@ -7,11 +7,19 @@ export function getSymbol(type) {
 }
 
 function getLineNumber(lineRange, key) {
-  if (!lineRange || !key) return '';
-  const lineCode = lineRange[key].line_code || '';
-  const lineType = lineRange[key].type || '';
-  const lines = lineCode.split('_') || [];
-  const lineNumber = lineType === 'old' ? lines[1] : lines[2];
+  if (!lineRange || !key || !lineRange[key]) return '';
+  const { new_line: newLine, old_line: oldLine, type } = lineRange[key];
+  const otherKey = key === 'start' ? 'end' : 'start';
+
+  // By default we want to see the "old" or "left side" line number
+  // The exception is if the "end" line is on the "right" side
+  // `otherLineType` is only used if `type` is null to make sure the line
+  // number relfects the "right" side number, if that is the side
+  // the comment form is located on
+  const otherLineType = !type ? lineRange[otherKey].type : null;
+  const lineType = type || '';
+  let lineNumber = oldLine;
+  if (lineType === 'new' || otherLineType === 'new') lineNumber = newLine;
   return (lineNumber && getSymbol(lineType) + lineNumber) || '';
 }
 
diff --git a/spec/features/merge_request/user_comments_on_diff_spec.rb b/spec/features/merge_request/user_comments_on_diff_spec.rb
index 9cd3c7eaf76677..eccd3f0bf4f6fd 100644
--- a/spec/features/merge_request/user_comments_on_diff_spec.rb
+++ b/spec/features/merge_request/user_comments_on_diff_spec.rb
@@ -115,37 +115,7 @@
   end
 
   context 'when adding multiline comments' do
-    it 'saves a multiline comment' do
-      click_diff_line(find("[id='#{sample_commit.line_code}']"))
-
-      page.within('.discussion-form') do
-        find('#comment-line-start option', text: '-13').select_option
-      end
-
-      page.within('.js-discussion-note-form') do
-        fill_in(:note_note, with: 'Line is wrong')
-        click_button('Add comment now')
-      end
-
-      wait_for_requests
-
-      page.within('.notes_holder') do
-        expect(page).to have_content('Line is wrong')
-        expect(page).to have_content('Comment on lines -13 to +14')
-      end
-
-      visit(merge_request_path(merge_request))
-
-      page.within('.notes .discussion') do
-        expect(page).to have_content("#{user.name} #{user.to_reference} started a thread")
-        expect(page).to have_content(sample_commit.line_code_path)
-        expect(page).to have_content('Line is wrong')
-      end
-
-      page.within('.notes-tab .badge') do
-        expect(page).to have_content('1')
-      end
-    end
+    include_examples 'multiline comment on merge request file'
   end
 
   context 'when editing comments' do
diff --git a/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb b/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb
new file mode 100644
index 00000000000000..53611a0c707ec0
--- /dev/null
+++ b/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb
@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+
+RSpec.shared_context 'multiline comment on merge request file' do
+  it 'saves a multiline comment' do
+    click_diff_line(find("[id='#{sample_commit.line_code}']"))
+    add_comment('-13', '+14')
+  end
+
+  context 'when in side-by-side view' do
+    before do
+      visit(diffs_project_merge_request_path(project, merge_request, view: 'parallel'))
+    end
+
+    # In `files/ruby/popen.rb`
+    it 'allows comments for changes involving both sides' do
+      # click +15, select -13 add and verify comment
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="15"]').find(:xpath, '../../..'), 'right')
+      add_comment('-13', '+15')
+    end
+
+    it 'allows comments to start above hidden lines and end below' do
+      # click +28, select 21 add and verify comment
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="28"]').find(:xpath, '../../..'), 'right')
+      add_comment('21', '+28')
+    end
+
+    it 'allows comments on previously hidden lines at the top of a file' do
+      # Click -9, expand up, select 1 add and verify comment
+      page.within('[data-path="files/ruby/popen.rb"]') do
+        all('.js-unfold-all')[0].click
+      end
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="9"]').find(:xpath, '../../..'), 'left')
+      add_comment('1', '-9')
+    end
+
+    it 'allows comments on previously hidden lines the middle of a file' do
+      # Click 27, expand up, select 18, add and verify comment
+      page.within('[data-path="files/ruby/popen.rb"]') do
+        all('.js-unfold-all')[1].click
+      end
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="21"]').find(:xpath, '../../..'), 'left')
+      add_comment('18', '21')
+    end
+
+    it 'allows comments on previously hidden lines at the bottom of a file' do
+      # Click +28, expand down, select 37 add and verify comment
+      page.within('[data-path="files/ruby/popen.rb"]') do
+        all('.js-unfold-down')[1].click
+      end
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="30"]').find(:xpath, '../../..'), 'left')
+      add_comment('+28', '37')
+    end
+  end
+
+  def add_comment(start_line, end_line)
+    page.within('.discussion-form') do
+      find('#comment-line-start option', exact_text: start_line).select_option
+    end
+
+    page.within('.js-discussion-note-form') do
+      fill_in(:note_note, with: 'Line is wrong')
+      click_button('Add comment now')
+    end
+
+    wait_for_requests
+
+    page.within('.notes_holder') do
+      expect(page).to have_content('Line is wrong')
+      expect(page).to have_content("Comment on lines #{start_line} to #{end_line}")
+    end
+
+    visit(merge_request_path(merge_request))
+
+    page.within('.notes .discussion') do
+      expect(page).to have_content("#{user.name} #{user.to_reference} started a thread")
+      expect(page).to have_content(sample_commit.line_code_path)
+      expect(page).to have_content('Line is wrong')
+    end
+
+    page.within('.notes-tab .badge') do
+      expect(page).to have_content('1')
+    end
+  end
+end
-- 
GitLab


From 8f7bd94625a6a544535531c56c8054391b2d83d4 Mon Sep 17 00:00:00 2001
From: jboyson <jboyson@gitlab.com>
Date: Fri, 26 Jun 2020 16:26:42 -0500
Subject: [PATCH 4/7] Update tests to match new parallel data

---
 .../components/multiline_comment_utils.js     |  2 +-
 .../multiline_comment_utils_spec.js           | 35 +++++++------------
 ..._on_merge_request_files_shared_examples.rb | 10 +++---
 3 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/app/assets/javascripts/notes/components/multiline_comment_utils.js b/app/assets/javascripts/notes/components/multiline_comment_utils.js
index 77d23c807b37c2..e8a834df7138c1 100644
--- a/app/assets/javascripts/notes/components/multiline_comment_utils.js
+++ b/app/assets/javascripts/notes/components/multiline_comment_utils.js
@@ -16,7 +16,7 @@ function getLineNumber(lineRange, key) {
   // `otherLineType` is only used if `type` is null to make sure the line
   // number relfects the "right" side number, if that is the side
   // the comment form is located on
-  const otherLineType = !type ? lineRange[otherKey].type : null;
+  const otherLineType = !type ? lineRange[otherKey]?.type : null;
   const lineType = type || '';
   let lineNumber = oldLine;
   if (lineType === 'new' || otherLineType === 'new') lineNumber = newLine;
diff --git a/spec/frontend/notes/components/multiline_comment_utils_spec.js b/spec/frontend/notes/components/multiline_comment_utils_spec.js
index cd5507322c5d73..4afb8d976712a0 100644
--- a/spec/frontend/notes/components/multiline_comment_utils_spec.js
+++ b/spec/frontend/notes/components/multiline_comment_utils_spec.js
@@ -5,30 +5,19 @@ import {
 } from '~/notes/components/multiline_comment_utils';
 
 describe('Multiline comment utilities', () => {
-  describe('getStartLineNumber', () => {
+  describe('get start & end line numbers', () => {
+    const lineRanges = ['old', 'new', null].map(type => ({
+      start: { new_line: 1, old_line: 1, type },
+      end: { new_line: 2, old_line: 2, type },
+    }));
     it.each`
-      line_code       | type     | result
-      ${'abcdef_1_1'} | ${'old'} | ${'-1'}
-      ${'abcdef_1_1'} | ${'new'} | ${'+1'}
-      ${'abcdef_1_1'} | ${null}  | ${'1'}
-      ${'abcdef'}     | ${'new'} | ${''}
-      ${'abcdef'}     | ${'old'} | ${''}
-      ${'abcdef'}     | ${null}  | ${''}
-    `('returns line number', ({ line_code, type, result }) => {
-      expect(getStartLineNumber({ start: { line_code, type } })).toEqual(result);
-    });
-  });
-  describe('getEndLineNumber', () => {
-    it.each`
-      line_code       | type     | result
-      ${'abcdef_1_1'} | ${'old'} | ${'-1'}
-      ${'abcdef_1_1'} | ${'new'} | ${'+1'}
-      ${'abcdef_1_1'} | ${null}  | ${'1'}
-      ${'abcdef'}     | ${'new'} | ${''}
-      ${'abcdef'}     | ${'old'} | ${''}
-      ${'abcdef'}     | ${null}  | ${''}
-    `('returns line number', ({ line_code, type, result }) => {
-      expect(getEndLineNumber({ end: { line_code, type } })).toEqual(result);
+      lineRange        | start   | end
+      ${lineRanges[0]} | ${'-1'} | ${'-2'}
+      ${lineRanges[1]} | ${'+1'} | ${'+2'}
+      ${lineRanges[2]} | ${'1'}  | ${'2'}
+    `('returns line numbers `$start` & `$end`', ({ lineRange, start, end }) => {
+      expect(getStartLineNumber(lineRange)).toEqual(start);
+      expect(getEndLineNumber(lineRange)).toEqual(end);
     });
   });
   describe('getSymbol', () => {
diff --git a/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb b/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb
index 53611a0c707ec0..f54e6fa99af7d2 100644
--- a/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb
+++ b/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb
@@ -14,13 +14,13 @@
     # In `files/ruby/popen.rb`
     it 'allows comments for changes involving both sides' do
       # click +15, select -13 add and verify comment
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="15"]').find(:xpath, '../../..'), 'right')
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="15"]').find(:xpath, '../..'), 'right')
       add_comment('-13', '+15')
     end
 
     it 'allows comments to start above hidden lines and end below' do
       # click +28, select 21 add and verify comment
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="28"]').find(:xpath, '../../..'), 'right')
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="28"]').find(:xpath, '../..'), 'right')
       add_comment('21', '+28')
     end
 
@@ -29,7 +29,7 @@
       page.within('[data-path="files/ruby/popen.rb"]') do
         all('.js-unfold-all')[0].click
       end
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="9"]').find(:xpath, '../../..'), 'left')
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="9"]').find(:xpath, '../..'), 'left')
       add_comment('1', '-9')
     end
 
@@ -38,7 +38,7 @@
       page.within('[data-path="files/ruby/popen.rb"]') do
         all('.js-unfold-all')[1].click
       end
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="21"]').find(:xpath, '../../..'), 'left')
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="21"]').find(:xpath, '../..'), 'left')
       add_comment('18', '21')
     end
 
@@ -47,7 +47,7 @@
       page.within('[data-path="files/ruby/popen.rb"]') do
         all('.js-unfold-down')[1].click
       end
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="30"]').find(:xpath, '../../..'), 'left')
+      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="30"]').find(:xpath, '../..'), 'left')
       add_comment('+28', '37')
     end
   end
-- 
GitLab


From 9f8c86f2bcc393e2485abbc94a9b17058d7f8531 Mon Sep 17 00:00:00 2001
From: jboyson <jboyson@gitlab.com>
Date: Tue, 30 Jun 2020 11:57:45 -0500
Subject: [PATCH 5/7] Move multiline tests back inline with user comments

---
 .../user_comments_on_diff_spec.rb             | 81 +++++++++++++++++-
 ..._on_merge_request_files_shared_examples.rb | 84 -------------------
 2 files changed, 80 insertions(+), 85 deletions(-)
 delete mode 100644 spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb

diff --git a/spec/features/merge_request/user_comments_on_diff_spec.rb b/spec/features/merge_request/user_comments_on_diff_spec.rb
index eccd3f0bf4f6fd..30bf82e3665964 100644
--- a/spec/features/merge_request/user_comments_on_diff_spec.rb
+++ b/spec/features/merge_request/user_comments_on_diff_spec.rb
@@ -115,7 +115,86 @@
   end
 
   context 'when adding multiline comments' do
-    include_examples 'multiline comment on merge request file'
+    it 'saves a multiline comment' do
+      click_diff_line(find("[id='#{sample_commit.line_code}']"))
+      add_comment('-13', '+14')
+    end
+
+    context 'when in side-by-side view' do
+      before do
+        visit(diffs_project_merge_request_path(project, merge_request, view: 'parallel'))
+      end
+
+      # In `files/ruby/popen.rb`
+      it 'allows comments for changes involving both sides' do
+        # click +15, select -13 add and verify comment
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="15"]').find(:xpath, '../..'), 'right')
+        add_comment('-13', '+15')
+      end
+
+      it 'allows comments to start above hidden lines and end below' do
+        # click +28, select 21 add and verify comment
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="28"]').find(:xpath, '../..'), 'right')
+        add_comment('21', '+28')
+      end
+
+      it 'allows comments on previously hidden lines at the top of a file' do
+        # Click -9, expand up, select 1 add and verify comment
+        page.within('[data-path="files/ruby/popen.rb"]') do
+          all('.js-unfold-all')[0].click
+        end
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="9"]').find(:xpath, '../..'), 'left')
+        add_comment('1', '-9')
+      end
+
+      it 'allows comments on previously hidden lines the middle of a file' do
+        # Click 27, expand up, select 18, add and verify comment
+        page.within('[data-path="files/ruby/popen.rb"]') do
+          all('.js-unfold-all')[1].click
+        end
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="21"]').find(:xpath, '../..'), 'left')
+        add_comment('18', '21')
+      end
+
+      it 'allows comments on previously hidden lines at the bottom of a file' do
+        # Click +28, expand down, select 37 add and verify comment
+        page.within('[data-path="files/ruby/popen.rb"]') do
+          all('.js-unfold-down')[1].click
+        end
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="30"]').find(:xpath, '../..'), 'left')
+        add_comment('+28', '37')
+      end
+    end
+
+    def add_comment(start_line, end_line)
+      page.within('.discussion-form') do
+        find('#comment-line-start option', exact_text: start_line).select_option
+      end
+
+      page.within('.js-discussion-note-form') do
+        fill_in(:note_note, with: 'Line is wrong')
+        click_button('Add comment now')
+      end
+
+      wait_for_requests
+
+      page.within('.notes_holder') do
+        expect(page).to have_content('Line is wrong')
+        expect(page).to have_content("Comment on lines #{start_line} to #{end_line}")
+      end
+
+      visit(merge_request_path(merge_request))
+
+      page.within('.notes .discussion') do
+        expect(page).to have_content("#{user.name} #{user.to_reference} started a thread")
+        expect(page).to have_content(sample_commit.line_code_path)
+        expect(page).to have_content('Line is wrong')
+      end
+
+      page.within('.notes-tab .badge') do
+        expect(page).to have_content('1')
+      end
+    end
   end
 
   context 'when editing comments' do
diff --git a/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb b/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb
deleted file mode 100644
index f54e6fa99af7d2..00000000000000
--- a/spec/support/shared_examples/features/multiline_comments_on_merge_request_files_shared_examples.rb
+++ /dev/null
@@ -1,84 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.shared_context 'multiline comment on merge request file' do
-  it 'saves a multiline comment' do
-    click_diff_line(find("[id='#{sample_commit.line_code}']"))
-    add_comment('-13', '+14')
-  end
-
-  context 'when in side-by-side view' do
-    before do
-      visit(diffs_project_merge_request_path(project, merge_request, view: 'parallel'))
-    end
-
-    # In `files/ruby/popen.rb`
-    it 'allows comments for changes involving both sides' do
-      # click +15, select -13 add and verify comment
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="15"]').find(:xpath, '../..'), 'right')
-      add_comment('-13', '+15')
-    end
-
-    it 'allows comments to start above hidden lines and end below' do
-      # click +28, select 21 add and verify comment
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="28"]').find(:xpath, '../..'), 'right')
-      add_comment('21', '+28')
-    end
-
-    it 'allows comments on previously hidden lines at the top of a file' do
-      # Click -9, expand up, select 1 add and verify comment
-      page.within('[data-path="files/ruby/popen.rb"]') do
-        all('.js-unfold-all')[0].click
-      end
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="9"]').find(:xpath, '../..'), 'left')
-      add_comment('1', '-9')
-    end
-
-    it 'allows comments on previously hidden lines the middle of a file' do
-      # Click 27, expand up, select 18, add and verify comment
-      page.within('[data-path="files/ruby/popen.rb"]') do
-        all('.js-unfold-all')[1].click
-      end
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="21"]').find(:xpath, '../..'), 'left')
-      add_comment('18', '21')
-    end
-
-    it 'allows comments on previously hidden lines at the bottom of a file' do
-      # Click +28, expand down, select 37 add and verify comment
-      page.within('[data-path="files/ruby/popen.rb"]') do
-        all('.js-unfold-down')[1].click
-      end
-      click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="30"]').find(:xpath, '../..'), 'left')
-      add_comment('+28', '37')
-    end
-  end
-
-  def add_comment(start_line, end_line)
-    page.within('.discussion-form') do
-      find('#comment-line-start option', exact_text: start_line).select_option
-    end
-
-    page.within('.js-discussion-note-form') do
-      fill_in(:note_note, with: 'Line is wrong')
-      click_button('Add comment now')
-    end
-
-    wait_for_requests
-
-    page.within('.notes_holder') do
-      expect(page).to have_content('Line is wrong')
-      expect(page).to have_content("Comment on lines #{start_line} to #{end_line}")
-    end
-
-    visit(merge_request_path(merge_request))
-
-    page.within('.notes .discussion') do
-      expect(page).to have_content("#{user.name} #{user.to_reference} started a thread")
-      expect(page).to have_content(sample_commit.line_code_path)
-      expect(page).to have_content('Line is wrong')
-    end
-
-    page.within('.notes-tab .badge') do
-      expect(page).to have_content('1')
-    end
-  end
-end
-- 
GitLab


From adf02a64a294237a686d1f205585f1d6fd0631e0 Mon Sep 17 00:00:00 2001
From: jboyson <jboyson@gitlab.com>
Date: Tue, 30 Jun 2020 12:56:24 -0500
Subject: [PATCH 6/7] Use commentLineOptions for lineNUmber

---
 .../javascripts/notes/components/multiline_comment_form.vue  | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/assets/javascripts/notes/components/multiline_comment_form.vue b/app/assets/javascripts/notes/components/multiline_comment_form.vue
index 69d978120993a8..6aa9a9aa450974 100644
--- a/app/assets/javascripts/notes/components/multiline_comment_form.vue
+++ b/app/assets/javascripts/notes/components/multiline_comment_form.vue
@@ -27,10 +27,7 @@ export default {
   },
   computed: {
     lineNumber() {
-      return (
-        this.getSymbol(this.line) +
-        (this.commentLineEndType === 'new' ? this.line.new_line : this.line.old_line)
-      );
+      return this.commentLineOptions[this.commentLineOptions.length - 1].text;
     },
   },
   created() {
-- 
GitLab


From b58f80d73ae8078ab3156de70ee604d4d84a2c8e Mon Sep 17 00:00:00 2001
From: jboyson <jboyson@gitlab.com>
Date: Tue, 7 Jul 2020 12:46:45 -0500
Subject: [PATCH 7/7] Fix saving position in drafts

---
 .../batch_comments/components/preview_item.vue    | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/app/assets/javascripts/batch_comments/components/preview_item.vue b/app/assets/javascripts/batch_comments/components/preview_item.vue
index 22495eb4d7d052..3162a83f09939e 100644
--- a/app/assets/javascripts/batch_comments/components/preview_item.vue
+++ b/app/assets/javascripts/batch_comments/components/preview_item.vue
@@ -52,14 +52,12 @@ export default {
       });
     },
     linePosition() {
-      if (this.draft.position && this.draft.position.position_type === IMAGE_DIFF_POSITION_TYPE) {
+      if (this.position?.position_type === IMAGE_DIFF_POSITION_TYPE) {
         // eslint-disable-next-line @gitlab/require-i18n-strings
-        return `${this.draft.position.x}x ${this.draft.position.y}y`;
+        return `${this.position.x}x ${this.position.y}y`;
       }
 
-      const position = this.discussion ? this.discussion.position : this.draft.position;
-
-      return position?.new_line || position?.old_line;
+      return this.position?.new_line || this.position?.old_line;
     },
     content() {
       const el = document.createElement('div');
@@ -70,11 +68,14 @@ export default {
     showLinePosition() {
       return this.draft.file_hash || this.isDiffDiscussion;
     },
+    position() {
+      return this.draft.position || this.discussion.position;
+    },
     startLineNumber() {
-      return getStartLineNumber(this.draft.position?.line_range);
+      return getStartLineNumber(this.position?.line_range);
     },
     endLineNumber() {
-      return getEndLineNumber(this.draft.position?.line_range);
+      return getEndLineNumber(this.position?.line_range);
     },
   },
   methods: {
-- 
GitLab