Merge branch 'dm-trim-discussion-truncated-line-first-chars' into 'master'

Fix bug that caused Suggestion Markdown toolbar button to insert snippet with leading +/-/<space>

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

8 files changed, 40 insertions, 24 deletions

diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js
index ada93b570b0..09afacc24df 100644
--- a/app/assets/javascripts/diffs/store/utils.js
+++ b/app/assets/javascripts/diffs/store/utils.js
@@ -181,8 +181,6 @@ export function addContextLines(options) {
 export function trimFirstCharOfLineContent(line = {}) {
   // eslint-disable-next-line no-param-reassign
   delete line.text;
-  // eslint-disable-next-line no-param-reassign
-  line.discussions = [];
 
   const parsedLine = Object.assign({}, line);
 
@@ -222,10 +220,12 @@ export function prepareDiffData(diffData) {
         line.line_code = getLineCode(line, u);
         if (line.left) {
           line.left = trimFirstCharOfLineContent(line.left);
+          line.left.discussions = [];
           line.left.hasForm = false;
         }
         if (line.right) {
           line.right = trimFirstCharOfLineContent(line.right);
+          line.right.discussions = [];
           line.right.hasForm = false;
         }
       }
@@ -235,7 +235,11 @@ export function prepareDiffData(diffData) {
       const linesLength = file.highlighted_diff_lines.length;
       for (let u = 0; u < linesLength; u += 1) {
         const line = file.highlighted_diff_lines[u];
-        Object.assign(line, { ...trimFirstCharOfLineContent(line), hasForm: false });
+        Object.assign(line, {
+          ...trimFirstCharOfLineContent(line),
+          discussions: [],
+          hasForm: false,
+        });
       }
       showingLines += file.parallel_diff_lines.length;
     }
diff --git a/app/assets/javascripts/notes/components/diff_with_note.vue b/app/assets/javascripts/notes/components/diff_with_note.vue
index af821df0fd2..376d4114efd 100644
--- a/app/assets/javascripts/notes/components/diff_with_note.vue
+++ b/app/assets/javascripts/notes/components/diff_with_note.vue
@@ -6,8 +6,6 @@ import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue';
 import { GlSkeletonLoading } from '@gitlab/ui';
 import { getDiffMode } from '~/diffs/store/utils';
 
-const FIRST_CHAR_REGEX = /^(\+|-| )/;
-
 export default {
   components: {
     DiffFileHeader,
@@ -54,9 +52,6 @@ export default {
           this.error = true;
         });
     },
-    trimChar(line) {
-      return line.replace(FIRST_CHAR_REGEX, '');
-    },
   },
   userColorSchemeClass: window.gon.user_color_scheme,
 };
@@ -85,7 +80,7 @@ export default {
           >
             <td class="diff-line-num old_line">{{ line.old_line }}</td>
             <td class="diff-line-num new_line">{{ line.new_line }}</td>
-            <td :class="line.type" class="line_content" v-html="trimChar(line.rich_text)"></td>
+            <td :class="line.type" class="line_content" v-html="line.rich_text"></td>
           </tr>
         </template>
         <tr v-if="!hasTruncatedDiffLines" class="line_holder line-holder-placeholder">
diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue
index 4480ec74182..1a116161e3c 100644
--- a/app/assets/javascripts/notes/components/noteable_discussion.vue
+++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
@@ -206,11 +206,15 @@ export default {
       return sprintf(text, { commitId, linkStart, linkEnd }, false);
     },
     diffLine() {
+      if (this.line) {
+        return this.line;
+      }
+
       if (this.discussion.diff_discussion && this.discussion.truncated_diff_lines) {
         return this.discussion.truncated_diff_lines.slice(-1)[0];
       }
 
-      return this.line;
+      return null;
     },
   },
   watch: {
diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js
index 8992454be2e..33d39ad2ec9 100644
--- a/app/assets/javascripts/notes/stores/mutations.js
+++ b/app/assets/javascripts/notes/stores/mutations.js
@@ -105,7 +105,10 @@ export default {
 
       if (discussion.diff_file) {
         diffData.file_hash = discussion.diff_file.file_hash;
-        diffData.truncated_diff_lines = discussion.truncated_diff_lines || [];
+
+        diffData.truncated_diff_lines = utils.prepareDiffLines(
+          discussion.truncated_diff_lines || [],
+        );
       }
 
       // To support legacy notes, should be very rare case.
@@ -243,7 +246,7 @@ export default {
   [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) {
     const discussion = utils.findNoteObjectById(state.discussions, discussionId);
 
-    discussion.truncated_diff_lines = diffLines;
+    discussion.truncated_diff_lines = utils.prepareDiffLines(diffLines);
   },
 
   [types.DISABLE_COMMENTS](state, value) {
diff --git a/app/assets/javascripts/notes/stores/utils.js b/app/assets/javascripts/notes/stores/utils.js
index dd57539e4d8..4b0feb0f94d 100644
--- a/app/assets/javascripts/notes/stores/utils.js
+++ b/app/assets/javascripts/notes/stores/utils.js
@@ -1,4 +1,5 @@
 import AjaxCache from '~/lib/utils/ajax_cache';
+import { trimFirstCharOfLineContent } from '~/diffs/store/utils';
 
 const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm;
 
@@ -28,3 +29,6 @@ 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 prepareDiffLines = diffLines =>
+  diffLines.map(line => ({ ...trimFirstCharOfLineContent(line) }));
diff --git a/changelogs/unreleased/dm-trim-discussion-truncated-line-first-chars.yml b/changelogs/unreleased/dm-trim-discussion-truncated-line-first-chars.yml
new file mode 100644
index 00000000000..1e1fa8295c3
--- /dev/null
+++ b/changelogs/unreleased/dm-trim-discussion-truncated-line-first-chars.yml
@@ -0,0 +1,5 @@
+---
+title: Fix bug that caused Suggestion Markdown toolbar button to insert snippet with leading +/-/<space>
+merge_request:
+author:
+type: fixed
diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js
index ea86844ddca..3641946518b 100644
--- a/spec/javascripts/diffs/store/utils_spec.js
+++ b/spec/javascripts/diffs/store/utils_spec.js
@@ -251,45 +251,40 @@ describe('DiffsStoreUtils', () => {
   describe('trimFirstCharOfLineContent', () => {
     it('trims the line when it starts with a space', () => {
       expect(utils.trimFirstCharOfLineContent({ rich_text: ' diff' })).toEqual({
-        discussions: [],
         rich_text: 'diff',
       });
     });
 
     it('trims the line when it starts with a +', () => {
       expect(utils.trimFirstCharOfLineContent({ rich_text: '+diff' })).toEqual({
-        discussions: [],
         rich_text: 'diff',
       });
     });
 
     it('trims the line when it starts with a -', () => {
       expect(utils.trimFirstCharOfLineContent({ rich_text: '-diff' })).toEqual({
-        discussions: [],
         rich_text: 'diff',
       });
     });
 
     it('does not trims the line when it starts with a letter', () => {
       expect(utils.trimFirstCharOfLineContent({ rich_text: 'diff' })).toEqual({
-        discussions: [],
         rich_text: 'diff',
       });
     });
 
     it('does not modify the provided object', () => {
       const lineObj = {
-        discussions: [],
         rich_text: ' diff',
       };
 
       utils.trimFirstCharOfLineContent(lineObj);
 
-      expect(lineObj).toEqual({ discussions: [], rich_text: ' diff' });
+      expect(lineObj).toEqual({ rich_text: ' diff' });
     });
 
     it('handles a undefined or null parameter', () => {
-      expect(utils.trimFirstCharOfLineContent()).toEqual({ discussions: [] });
+      expect(utils.trimFirstCharOfLineContent()).toEqual({});
     });
   });
 
diff --git a/spec/javascripts/notes/stores/mutation_spec.js b/spec/javascripts/notes/stores/mutation_spec.js
index 3fbae82f16c..b6b2c7d60a5 100644
--- a/spec/javascripts/notes/stores/mutation_spec.js
+++ b/spec/javascripts/notes/stores/mutation_spec.js
@@ -179,11 +179,11 @@ describe('Notes Store mutations', () => {
           diff_file: {
             file_hash: 'a',
           },
-          truncated_diff_lines: ['a'],
+          truncated_diff_lines: [{ text: '+a', rich_text: '+<span>a</span>' }],
         },
       ]);
 
-      expect(state.discussions[0].truncated_diff_lines).toEqual(['a']);
+      expect(state.discussions[0].truncated_diff_lines).toEqual([{ rich_text: '<span>a</span>' }]);
     });
 
     it('adds empty truncated_diff_lines when not in discussion', () => {
@@ -420,9 +420,12 @@ describe('Notes Store mutations', () => {
         ],
       };
 
-      mutations.SET_DISCUSSION_DIFF_LINES(state, { discussionId: 1, diffLines: ['test'] });
+      mutations.SET_DISCUSSION_DIFF_LINES(state, {
+        discussionId: 1,
+        diffLines: [{ text: '+a', rich_text: '+<span>a</span>' }],
+      });
 
-      expect(state.discussions[0].truncated_diff_lines).toEqual(['test']);
+      expect(state.discussions[0].truncated_diff_lines).toEqual([{ rich_text: '<span>a</span>' }]);
     });
 
     it('keeps reactivity of discussion', () => {
@@ -435,7 +438,10 @@ describe('Notes Store mutations', () => {
       ]);
       const discussion = state.discussions[0];
 
-      mutations.SET_DISCUSSION_DIFF_LINES(state, { discussionId: 1, diffLines: ['test'] });
+      mutations.SET_DISCUSSION_DIFF_LINES(state, {
+        discussionId: 1,
+        diffLines: [{ rich_text: '<span>a</span>' }],
+      });
 
       discussion.expanded = true;