Merge branch 'jdb/highlight-commented-rows' into 'master'

Highlight commented rows

See merge request gitlab-org/gitlab!34432

app/assets/javascripts/batch_comments/components/draft_note.vue

@@ -51,6 +51,7 @@ export default {
       'scrollToDraft',
       'toggleResolveDiscussion',
     ]),
+    ...mapActions(['setSelectedCommentPositionHover']),
     update(data) {
       this.updateDraft(data);
     },
@@ -67,7 +68,11 @@ export default {
 };
 </script>
 <template>
-  <article class="draft-note-component note-wrapper">
+  <article
+    class="draft-note-component note-wrapper"
+    @mouseenter="setSelectedCommentPositionHover(draft.position.line_range)"
+    @mouseleave="setSelectedCommentPositionHover()"
+  >
     <ul class="notes draft-notes">
       <noteable-note
         :note="draft"

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

@@ -148,10 +148,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-100 gl-border-b-solid gl-border-b-1 gl-pb-3"
-    >
+    <div v-if="glFeatures.multilineComments" class="gl-mb-3 gl-text-gray-700 gl-pb-3">
       <multiline-comment-form
         v-model="commentLineStart"
         :line="line"

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

@@ -37,6 +37,11 @@ export default {
       required: false,
       default: false,
     },
+    isCommented: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -47,7 +52,10 @@ export default {
     ...mapGetters('diffs', ['fileLineCoverage']),
     ...mapState({
       isHighlighted(state) {
-        return this.line.line_code !== null && this.line.line_code === state.diffs.highlightedRow;
+        if (this.isCommented) return true;
+
+        const lineCode = this.line.line_code;
+        return lineCode ? lineCode === state.diffs.highlightedRow : false;
       },
     }),
     isContextLine() {

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

 <script>
-import { mapGetters } from 'vuex';
+import { mapGetters, mapState } from 'vuex';
 import draftCommentsMixin from '~/diffs/mixins/draft_comments';
 import InlineDraftCommentRow from '~/batch_comments/components/inline_draft_comment_row.vue';
 import inlineDiffTableRow from './inline_diff_table_row.vue';
 import inlineDiffCommentRow from './inline_diff_comment_row.vue';
 import inlineDiffExpansionRow from './inline_diff_expansion_row.vue';
+import { getCommentedLines } from '~/notes/components/multiline_comment_utils';
 
 export default {
   components: {
@@ -31,9 +32,19 @@ export default {
   },
   computed: {
     ...mapGetters('diffs', ['commitId']),
+    ...mapState({
+      selectedCommentPosition: ({ notes }) => notes.selectedCommentPosition,
+      selectedCommentPositionHover: ({ notes }) => notes.selectedCommentPositionHover,
+    }),
     diffLinesLength() {
       return this.diffLines.length;
     },
+    commentedLines() {
+      return getCommentedLines(
+        this.selectedCommentPosition || this.selectedCommentPositionHover,
+        this.diffLines,
+      );
+    },
   },
   userColorScheme: window.gon.user_color_scheme,
 };
@@ -67,6 +78,7 @@ export default {
           :file-path="diffFile.file_path"
           :line="line"
           :is-bottom="index + 1 === diffLinesLength"
+          :is-commented="index >= commentedLines.startLine && index <= commentedLines.endLine"
         />
         <inline-diff-comment-row
           :key="`icr-${line.line_code || index}`"

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

@@ -40,6 +40,11 @@ export default {
       required: false,
       default: false,
     },
+    isCommented: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -51,6 +56,8 @@ export default {
     ...mapGetters('diffs', ['fileLineCoverage']),
     ...mapState({
       isHighlighted(state) {
+        if (this.isCommented) return true;
+
         const lineCode =
           (this.line.left && this.line.left.line_code) ||
           (this.line.right && this.line.right.line_code);

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

 <script>
-import { mapGetters } from 'vuex';
+import { mapGetters, mapState } from 'vuex';
 import draftCommentsMixin from '~/diffs/mixins/draft_comments';
 import ParallelDraftCommentRow from '~/batch_comments/components/parallel_draft_comment_row.vue';
 import parallelDiffTableRow from './parallel_diff_table_row.vue';
 import parallelDiffCommentRow from './parallel_diff_comment_row.vue';
 import parallelDiffExpansionRow from './parallel_diff_expansion_row.vue';
+import { getCommentedLines } from '~/notes/components/multiline_comment_utils';
 
 export default {
   components: {
@@ -31,9 +32,19 @@ export default {
   },
   computed: {
     ...mapGetters('diffs', ['commitId']),
+    ...mapState({
+      selectedCommentPosition: ({ notes }) => notes.selectedCommentPosition,
+      selectedCommentPositionHover: ({ notes }) => notes.selectedCommentPositionHover,
+    }),
     diffLinesLength() {
       return this.diffLines.length;
     },
+    commentedLines() {
+      return getCommentedLines(
+        this.selectedCommentPosition || this.selectedCommentPositionHover,
+        this.diffLines,
+      );
+    },
   },
   userColorScheme: window.gon.user_color_scheme,
 };
@@ -69,6 +80,7 @@ export default {
           :file-path="diffFile.file_path"
           :line="line"
           :is-bottom="index + 1 === diffLinesLength"
+          :is-commented="index >= commentedLines.startLine && index <= commentedLines.endLine"
         />
         <parallel-diff-comment-row
           :key="`dcr-${line.line_code || index}`"

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

@@ -74,7 +74,7 @@ export default {
     },
   },
   methods: {
-    ...mapActions(['toggleDiscussion']),
+    ...mapActions(['toggleDiscussion', 'setSelectedCommentPositionHover']),
     componentName(note) {
       if (note.isPlaceholderNote) {
         if (note.placeholderType === SYSTEM_NOTE) {
@@ -99,7 +99,11 @@ export default {
 
 <template>
   <div class="discussion-notes">
-    <ul class="notes">
+    <ul
+      class="notes"
+      @mouseenter="setSelectedCommentPositionHover(discussion.position.line_range)"
+      @mouseleave="setSelectedCommentPositionHover()"
+    >
       <template v-if="shouldGroupReplies">
         <component
           :is="componentName(firstNote)"

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

 <script>
+import { mapActions } from 'vuex';
 import { GlFormSelect, GlSprintf } from '@gitlab/ui';
 import { getSymbol, getLineClasses } from './multiline_comment_utils';
 
@@ -39,8 +40,13 @@ export default {
       old_line: line.old_line,
       new_line: line.new_line,
     };
+    this.highlightSelection();
+  },
+  destroyed() {
+    this.setSelectedCommentPosition();
   },
   methods: {
+    ...mapActions(['setSelectedCommentPosition']),
     getSymbol({ type }) {
       return getSymbol(type);
     },
@@ -50,6 +56,16 @@ export default {
     updateCommentLineStart(value) {
       this.commentLineStart = value;
       this.$emit('input', value);
+      this.highlightSelection();
+    },
+    highlightSelection() {
+      const { line_code, new_line, old_line, type } = this.line;
+      const updatedLineRange = {
+        start: { ...this.commentLineStart },
+        end: { line_code, new_line, old_line, type },
+      };
+
+      this.setSelectedCommentPosition(updatedLineRange);
     },
   },
 };

app/assets/javascripts/notes/components/multiline_comment_utils.js

@@ -90,3 +90,22 @@ export function formatLineRange(start, end) {
     end: extractProps(end),
   };
 }
+
+export function getCommentedLines(selectedCommentPosition, diffLines) {
+  if (!selectedCommentPosition) {
+    // This structure simplifies the logic that consumes this result
+    // by keeping the returned shape the same and adjusting the bounds
+    // to something unreachable. This way our component logic stays:
+    // "if index between start and end"
+    return {
+      startLine: diffLines.length + 1,
+      endLine: diffLines.length + 1,
+    };
+  }
+
+  const { start, end } = selectedCommentPosition;
+  const startLine = diffLines.findIndex(l => l.line_code === start.line_code);
+  const endLine = diffLines.findIndex(l => l.line_code === end.line_code);
+
+  return { startLine, endLine };
+}

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

@@ -186,6 +186,7 @@ export default {
     eventHub.$on('enterEditMode', ({ noteId }) => {
       if (noteId === this.note.id) {
         this.isEditing = true;
+        this.setSelectedCommentPositionHover();
         this.scrollToNoteIfNeeded($(this.$el));
       }
     });
@@ -205,9 +206,11 @@ export default {
       'toggleResolveNote',
       'scrollToNoteIfNeeded',
       'updateAssignees',
+      'setSelectedCommentPositionHover',
     ]),
     editHandler() {
       this.isEditing = true;
+      this.setSelectedCommentPositionHover();
       this.$emit('handleEdit');
     },
     deleteHandler() {
@@ -284,6 +287,7 @@ export default {
           } else {
             this.isRequesting = false;
             this.isEditing = true;
+            this.setSelectedCommentPositionHover();
             this.$nextTick(() => {
               const msg = __('Something went wrong while editing your comment. Please try again.');
               Flash(msg, 'alert', this.$el);
@@ -340,7 +344,7 @@ export default {
         :line="line"
         :comment-line-options="commentLineOptions"
         :line-range="note.position.line_range"
-        class="gl-mb-3 gl-text-gray-700 gl-border-gray-100 gl-border-b-solid gl-border-b-1 gl-pb-3"
+        class="gl-mb-3 gl-text-gray-700 gl-pb-3"
       />
       <div
         v-else

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

@@ -99,6 +99,14 @@ export const setDiscussionSortDirection = ({ commit }, direction) => {
   commit(types.SET_DISCUSSIONS_SORT, direction);
 };
 
+export const setSelectedCommentPosition = ({ commit }, position) => {
+  commit(types.SET_SELECTED_COMMENT_POSITION, position);
+};
+
+export const setSelectedCommentPositionHover = ({ commit }, position) => {
+  commit(types.SET_SELECTED_COMMENT_POSITION_HOVER, position);
+};
+
 export const removeNote = ({ commit, dispatch, state }, note) => {
   const discussion = state.discussions.find(({ id }) => id === note.discussion_id);
 

app/assets/javascripts/notes/stores/modules/index.js

@@ -12,6 +12,15 @@ export default () => ({
     lastFetchedAt: null,
     currentDiscussionId: null,
     batchSuggestionsInfo: [],
+    /**
+     * selectedCommentPosition & selectedCommentPosition structures are the same as `position.line_range`:
+     * {
+     *  start: { line_code: string, new_line: number, old_line:number, type: string },
+     *  end: { line_code: string, new_line: number, old_line:number, type: string },
+     * }
+     */
+    selectedCommentPosition: null,
+    selectedCommentPositionHover: null,
 
     // View layer
     isToggleStateButtonLoading: false,

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

@@ -33,6 +33,8 @@ export const SET_EXPAND_DISCUSSIONS = 'SET_EXPAND_DISCUSSIONS';
 export const UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS = 'UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS';
 export const SET_CURRENT_DISCUSSION_ID = 'SET_CURRENT_DISCUSSION_ID';
 export const SET_DISCUSSIONS_SORT = 'SET_DISCUSSIONS_SORT';
+export const SET_SELECTED_COMMENT_POSITION = 'SET_SELECTED_COMMENT_POSITION';
+export const SET_SELECTED_COMMENT_POSITION_HOVER = 'SET_SELECTED_COMMENT_POSITION_HOVER';
 
 // Issue
 export const CLOSE_ISSUE = 'CLOSE_ISSUE';

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

@@ -308,6 +308,14 @@ export default {
     state.discussionSortOrder = sort;
   },
 
+  [types.SET_SELECTED_COMMENT_POSITION](state, position) {
+    state.selectedCommentPosition = position;
+  },
+
+  [types.SET_SELECTED_COMMENT_POSITION_HOVER](state, position) {
+    state.selectedCommentPositionHover = position;
+  },
+
   [types.DISABLE_COMMENTS](state, value) {
     state.commentsDisabled = value;
   },

changelogs/unreleased/jdb-highlight-commented-rows.yml

+---
+title: Highlight commented rows
+merge_request: 34432
+author:
+type: added

spec/frontend/diffs/components/inline_diff_table_row_spec.js

-import Vue from 'vue';
-import { createComponentWithStore } from 'helpers/vue_mount_component_helper';
+import { shallowMount } from '@vue/test-utils';
 import { createStore } from '~/mr_notes/stores';
 import InlineDiffTableRow from '~/diffs/components/inline_diff_table_row.vue';
 import diffFileMockData from '../mock_data/diff_file';
 
 describe('InlineDiffTableRow', () => {
+  let wrapper;
   let vm;
   const thisLine = diffFileMockData.highlighted_diff_lines[0];
 
   beforeEach(() => {
-    vm = createComponentWithStore(Vue.extend(InlineDiffTableRow), createStore(), {
-      line: thisLine,
-      fileHash: diffFileMockData.file_hash,
-      filePath: diffFileMockData.file_path,
-      contextLinesPath: 'contextLinesPath',
-      isHighlighted: false,
-    }).$mount();
+    wrapper = shallowMount(InlineDiffTableRow, {
+      store: createStore(),
+      propsData: {
+        line: thisLine,
+        fileHash: diffFileMockData.file_hash,
+        filePath: diffFileMockData.file_path,
+        contextLinesPath: 'contextLinesPath',
+        isHighlighted: false,
+      },
+    });
+    vm = wrapper.vm;
   });
 
   it('does not add hll class to line content when line does not match highlighted row', done => {
     vm.$nextTick()
       .then(() => {
-        expect(vm.$el.querySelector('.line_content').classList).not.toContain('hll');
+        expect(wrapper.find('.line_content').classes('hll')).toBe(false);
       })
       .then(done)
       .catch(done.fail);
@@ -35,12 +39,19 @@ describe('InlineDiffTableRow', () => {
         return vm.$nextTick();
       })
       .then(() => {
-        expect(vm.$el.querySelector('.line_content').classList).toContain('hll');
+        expect(wrapper.find('.line_content').classes('hll')).toBe(true);
       })
       .then(done)
       .catch(done.fail);
   });
 
+  it('adds hll class to lineContent when line is part of a multiline comment', () => {
+    wrapper.setProps({ isCommented: true });
+    return vm.$nextTick().then(() => {
+      expect(wrapper.find('.line_content').classes('hll')).toBe(true);
+    });
+  });
+
   describe('sets coverage title and class', () => {
     it('for lines with coverage', done => {
       vm.$nextTick()
@@ -53,10 +64,10 @@ describe('InlineDiffTableRow', () => {
           return vm.$nextTick();
         })
         .then(() => {
-          const coverage = vm.$el.querySelector('.line-coverage');
+          const coverage = wrapper.find('.line-coverage');
 
-          expect(coverage.title).toContain('Test coverage: 5 hits');
-          expect(coverage.classList).toContain('coverage');
+          expect(coverage.attributes('title')).toContain('Test coverage: 5 hits');
+          expect(coverage.classes('coverage')).toBe(true);
         })
         .then(done)
         .catch(done.fail);
@@ -73,10 +84,10 @@ describe('InlineDiffTableRow', () => {
           return vm.$nextTick();
         })
         .then(() => {
-          const coverage = vm.$el.querySelector('.line-coverage');
+          const coverage = wrapper.find('.line-coverage');
 
-          expect(coverage.title).toContain('No test coverage');
-          expect(coverage.classList).toContain('no-coverage');
+          expect(coverage.attributes('title')).toContain('No test coverage');
+          expect(coverage.classes('no-coverage')).toBe(true);
         })
         .then(done)
         .catch(done.fail);
@@ -90,11 +101,11 @@ describe('InlineDiffTableRow', () => {
           return vm.$nextTick();
         })
         .then(() => {
-          const coverage = vm.$el.querySelector('.line-coverage');
+          const coverage = wrapper.find('.line-coverage');
 
-          expect(coverage.title).not.toContain('Coverage');
-          expect(coverage.classList).not.toContain('coverage');
-          expect(coverage.classList).not.toContain('no-coverage');
+          expect(coverage.attributes('title')).toBeUndefined();
+          expect(coverage.classes('coverage')).toBe(false);
+          expect(coverage.classes('no-coverage')).toBe(false);
         })
         .then(done)
         .catch(done.fail);

spec/frontend/diffs/components/parallel_diff_table_row_spec.js

 import Vue from 'vue';
+import { shallowMount } from '@vue/test-utils';
 import { createComponentWithStore } from 'helpers/vue_mount_component_helper';
 import { createStore } from '~/mr_notes/stores';
 import ParallelDiffTableRow from '~/diffs/components/parallel_diff_table_row.vue';
@@ -6,18 +7,24 @@ import diffFileMockData from '../mock_data/diff_file';
 
 describe('ParallelDiffTableRow', () => {
   describe('when one side is empty', () => {
+    let wrapper;
     let vm;
     const thisLine = diffFileMockData.parallel_diff_lines[0];
     const rightLine = diffFileMockData.parallel_diff_lines[0].right;
 
     beforeEach(() => {
-      vm = createComponentWithStore(Vue.extend(ParallelDiffTableRow), createStore(), {
-        line: thisLine,
-        fileHash: diffFileMockData.file_hash,
-        filePath: diffFileMockData.file_path,
-        contextLinesPath: 'contextLinesPath',
-        isHighlighted: false,
-      }).$mount();
+      wrapper = shallowMount(ParallelDiffTableRow, {
+        store: createStore(),
+        propsData: {
+          line: thisLine,
+          fileHash: diffFileMockData.file_hash,
+          filePath: diffFileMockData.file_path,
+          contextLinesPath: 'contextLinesPath',
+          isHighlighted: false,
+        },
+      });
+
+      vm = wrapper.vm;
     });
 
     it('does not highlight non empty line content when line does not match highlighted row', done => {
@@ -42,6 +49,13 @@ describe('ParallelDiffTableRow', () => {
         .then(done)
         .catch(done.fail);
     });
+
+    it('highlights nonempty line content when line is part of a multiline comment', () => {
+      wrapper.setProps({ isCommented: true });
+      return vm.$nextTick().then(() => {
+        expect(vm.$el.querySelector('.line_content.right-side').classList).toContain('hll');
+      });
+    });
   });
 
   describe('when both sides have content', () => {

spec/frontend/notes/components/multiline_comment_utils_spec.js

@@ -2,6 +2,7 @@ import {
   getSymbol,
   getStartLineNumber,
   getEndLineNumber,
+  getCommentedLines,
 } from '~/notes/components/multiline_comment_utils';
 
 describe('Multiline comment utilities', () => {
@@ -33,4 +34,30 @@ describe('Multiline comment utilities', () => {
       expect(getSymbol(type)).toEqual(result);
     });
   });
+  describe('getCommentedLines', () => {
+    const diffLines = [{ line_code: '1' }, { line_code: '2' }, { line_code: '3' }];
+    it('returns a default object when `selectedCommentPosition` is not provided', () => {
+      expect(getCommentedLines(undefined, diffLines)).toEqual({ startLine: 4, endLine: 4 });
+    });
+    it('returns an object with startLine and endLine equal to 0', () => {
+      const selectedCommentPosition = {
+        start: { line_code: '1' },
+        end: { line_code: '1' },
+      };
+      expect(getCommentedLines(selectedCommentPosition, diffLines)).toEqual({
+        startLine: 0,
+        endLine: 0,
+      });
+    });
+    it('returns an object with startLine and endLine equal to 0 and 1', () => {
+      const selectedCommentPosition = {
+        start: { line_code: '1' },
+        end: { line_code: '2' },
+      };
+      expect(getCommentedLines(selectedCommentPosition, diffLines)).toEqual({
+        startLine: 0,
+        endLine: 1,
+      });
+    });
+  });
 });

spec/frontend/notes/components/noteable_note_spec.js

@@ -175,6 +175,7 @@ describe('issue_note', () => {
     store.hotUpdate({
       actions: {
         updateNote() {},
+        setSelectedCommentPositionHover() {},
       },
     });
     const noteBodyComponent = wrapper.find(NoteBody);

spec/frontend/notes/stores/actions_spec.js

@@ -1127,6 +1127,19 @@ describe('Actions Notes Store', () => {
     });
   });
 
+  describe('setSelectedCommentPosition', () => {
+    it('calls the correct mutation with the correct args', done => {
+      testAction(
+        actions.setSelectedCommentPosition,
+        {},
+        {},
+        [{ type: mutationTypes.SET_SELECTED_COMMENT_POSITION, payload: {} }],
+        [],
+        done,
+      );
+    });
+  });
+
   describe('softDeleteDescriptionVersion', () => {
     const endpoint = '/path/to/diff/1';
     const payload = {

spec/frontend/notes/stores/mutation_spec.js

@@ -524,6 +524,26 @@ describe('Notes Store mutations', () => {
     });
   });
 
+  describe('SET_SELECTED_COMMENT_POSITION', () => {
+    it('should set comment position state', () => {
+      const state = {};
+
+      mutations.SET_SELECTED_COMMENT_POSITION(state, {});
+
+      expect(state.selectedCommentPosition).toEqual({});
+    });
+  });
+
+  describe('SET_SELECTED_COMMENT_POSITION_HOVER', () => {
+    it('should set comment hover position state', () => {
+      const state = {};
+
+      mutations.SET_SELECTED_COMMENT_POSITION_HOVER(state, {});
+
+      expect(state.selectedCommentPositionHover).toEqual({});
+    });
+  });
+
   describe('DISABLE_COMMENTS', () => {
     it('should set comments disabled state', () => {
       const state = {};