Merge branch 'jdb/refactor-inline-diff-table-row' into 'master'

Jdb/refactor inline diff table row

See merge request gitlab-org/gitlab!40906

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

 <script>
 /* eslint-disable vue/no-v-html */
 import { mapActions, mapGetters, mapState } from 'vuex';
-import { GlTooltipDirective } from '@gitlab/ui';
-import DiffTableCell from './diff_table_cell.vue';
+import { GlTooltipDirective, GlIcon } from '@gitlab/ui';
 import {
   MATCH_LINE_TYPE,
   NEW_LINE_TYPE,
@@ -11,11 +10,19 @@ import {
   CONTEXT_LINE_CLASS_NAME,
   LINE_POSITION_LEFT,
   LINE_POSITION_RIGHT,
+  LINE_HOVER_CLASS_NAME,
+  OLD_NO_NEW_LINE_TYPE,
+  NEW_NO_NEW_LINE_TYPE,
+  EMPTY_CELL_TYPE,
 } from '../constants';
+import { __ } from '~/locale';
+import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
+import DiffGutterAvatars from './diff_gutter_avatars.vue';
 
 export default {
   components: {
-    DiffTableCell,
+    DiffGutterAvatars,
+    GlIcon,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
@@ -50,6 +57,7 @@ export default {
     };
   },
   computed: {
+    ...mapGetters(['isLoggedIn']),
     ...mapGetters('diffs', ['fileLineCoverage']),
     ...mapState({
       isHighlighted(state) {
@@ -79,6 +87,70 @@ export default {
     coverageState() {
       return this.fileLineCoverage(this.filePath, this.line.new_line);
     },
+    isMetaLine() {
+      const { type } = this.line;
+
+      return (
+        type === OLD_NO_NEW_LINE_TYPE || type === NEW_NO_NEW_LINE_TYPE || type === EMPTY_CELL_TYPE
+      );
+    },
+    classNameMapCell() {
+      const { type } = this.line;
+
+      return [
+        type,
+        {
+          hll: this.isHighlighted,
+          [LINE_HOVER_CLASS_NAME]:
+            this.isLoggedIn && this.isHover && !this.isContextLine && !this.isMetaLine,
+        },
+      ];
+    },
+    addCommentTooltip() {
+      const brokenSymlinks = this.line.commentsDisabled;
+      let tooltip = __('Add a comment to this line');
+
+      if (brokenSymlinks) {
+        if (brokenSymlinks.wasSymbolic || brokenSymlinks.isSymbolic) {
+          tooltip = __(
+            'Commenting on symbolic links that replace or are replaced by files is currently not supported.',
+          );
+        } else if (brokenSymlinks.wasReal || brokenSymlinks.isReal) {
+          tooltip = __(
+            'Commenting on files that replace or are replaced by symbolic links is currently not supported.',
+          );
+        }
+      }
+
+      return tooltip;
+    },
+    shouldRenderCommentButton() {
+      if (this.isLoggedIn) {
+        const isDiffHead = parseBoolean(getParameterByName('diff_head'));
+        return !isDiffHead || gon.features?.mergeRefHeadComments;
+      }
+
+      return false;
+    },
+    shouldShowCommentButton() {
+      return this.isHover && !this.isContextLine && !this.isMetaLine && !this.hasDiscussions;
+    },
+    hasDiscussions() {
+      return this.line.discussions && this.line.discussions.length > 0;
+    },
+    lineHref() {
+      return `#${this.line.line_code || ''}`;
+    },
+    lineCode() {
+      return (
+        this.line.line_code ||
+        (this.line.left && this.line.left.line_code) ||
+        (this.line.right && this.line.right.line_code)
+      );
+    },
+    shouldShowAvatarsOnGutter() {
+      return this.hasDiscussions;
+    },
   },
   created() {
     this.newLineType = NEW_LINE_TYPE;
@@ -90,12 +162,20 @@ export default {
     this.scrollToLineIfNeededInline(this.line);
   },
   methods: {
-    ...mapActions('diffs', ['scrollToLineIfNeededInline']),
+    ...mapActions('diffs', [
+      'scrollToLineIfNeededInline',
+      'showCommentForm',
+      'setHighlightedRow',
+      'toggleLineDiscussions',
+    ]),
     handleMouseMove(e) {
       // To show the comment icon on the gutter we need to know if we hover the line.
       // Current table structure doesn't allow us to do this with CSS in both of the diff view types
       this.isHover = e.type === 'mouseover';
     },
+    handleCommentButton() {
+      this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash });
+    },
   },
 };
 </script>
@@ -109,25 +189,52 @@ export default {
     @mouseover="handleMouseMove"
     @mouseout="handleMouseMove"
   >
-    <diff-table-cell
-      :file-hash="fileHash"
-      :line="line"
-      :line-type="oldLineType"
-      :is-bottom="isBottom"
-      :is-hover="isHover"
-      :show-comment-button="true"
-      :is-highlighted="isHighlighted"
-      class="diff-line-num old_line"
-    />
-    <diff-table-cell
-      :file-hash="fileHash"
-      :line="line"
-      :line-type="newLineType"
-      :is-bottom="isBottom"
-      :is-hover="isHover"
-      :is-highlighted="isHighlighted"
-      class="diff-line-num new_line qa-new-diff-line"
-    />
+    <td ref="oldTd" class="diff-line-num old_line" :class="classNameMapCell">
+      <span
+        v-if="shouldRenderCommentButton"
+        ref="addNoteTooltip"
+        v-gl-tooltip
+        class="add-diff-note tooltip-wrapper"
+        :title="addCommentTooltip"
+      >
+        <button
+          v-show="shouldShowCommentButton"
+          ref="addDiffNoteButton"
+          type="button"
+          class="add-diff-note note-button js-add-diff-note-button qa-diff-comment"
+          :disabled="line.commentsDisabled"
+          @click="handleCommentButton"
+        >
+          <gl-icon :size="12" name="comment" />
+        </button>
+      </span>
+      <a
+        v-if="line.old_line"
+        ref="lineNumberRefOld"
+        :data-linenumber="line.old_line"
+        :href="lineHref"
+        @click="setHighlightedRow(lineCode)"
+      >
+      </a>
+      <diff-gutter-avatars
+        v-if="shouldShowAvatarsOnGutter"
+        :discussions="line.discussions"
+        :discussions-expanded="line.discussionsExpanded"
+        @toggleLineDiscussions="
+          toggleLineDiscussions({ lineCode, fileHash, expanded: !line.discussionsExpanded })
+        "
+      />
+    </td>
+    <td ref="newTd" class="diff-line-num new_line qa-new-diff-line" :class="classNameMapCell">
+      <a
+        v-if="line.new_line"
+        ref="lineNumberRefNew"
+        :data-linenumber="line.new_line"
+        :href="lineHref"
+        @click="setHighlightedRow(lineCode)"
+      >
+      </a>
+    </td>
     <td
       v-gl-tooltip.hover
       :title="coverageState.text"

changelogs/unreleased/jdb-refactor-inline-diff-table-row.yml

+---
+title: Jdb/refactor inline diff table row
+merge_request: 40906
+author:
+type: performance

spec/frontend/diffs/components/inline_diff_table_row_spec.js

 import { shallowMount } from '@vue/test-utils';
+import { TEST_HOST } from 'helpers/test_constants';
 import { createStore } from '~/mr_notes/stores';
 import InlineDiffTableRow from '~/diffs/components/inline_diff_table_row.vue';
+import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
 import diffFileMockData from '../mock_data/diff_file';
+import discussionsMockData from '../mock_data/diff_discussions';
+
+const TEST_USER_ID = 'abc123';
+const TEST_USER = { id: TEST_USER_ID };
 
 describe('InlineDiffTableRow', () => {
   let wrapper;
-  let vm;
+  let store;
   const thisLine = diffFileMockData.highlighted_diff_lines[0];
 
-  beforeEach(() => {
+  const createComponent = (props = {}, propsStore = store) => {
     wrapper = shallowMount(InlineDiffTableRow, {
-      store: createStore(),
+      store: propsStore,
       propsData: {
         line: thisLine,
         fileHash: diffFileMockData.file_hash,
         filePath: diffFileMockData.file_path,
         contextLinesPath: 'contextLinesPath',
         isHighlighted: false,
+        ...props,
       },
     });
-    vm = wrapper.vm;
+  };
+
+  const setWindowLocation = value => {
+    Object.defineProperty(window, 'location', {
+      writable: true,
+      value,
+    });
+  };
+
+  beforeEach(() => {
+    store = createStore();
+    store.state.notes.userData = TEST_USER;
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
   });
 
-  it('does not add hll class to line content when line does not match highlighted row', done => {
-    vm.$nextTick()
-      .then(() => {
-        expect(wrapper.find('.line_content').classes('hll')).toBe(false);
-      })
-      .then(done)
-      .catch(done.fail);
+  it('does not add hll class to line content when line does not match highlighted row', () => {
+    createComponent();
+    expect(wrapper.find('.line_content').classes('hll')).toBe(false);
   });
 
-  it('adds hll class to lineContent when line is the highlighted row', done => {
-    vm.$nextTick()
-      .then(() => {
-        vm.$store.state.diffs.highlightedRow = thisLine.line_code;
-
-        return vm.$nextTick();
-      })
-      .then(() => {
-        expect(wrapper.find('.line_content').classes('hll')).toBe(true);
-      })
-      .then(done)
-      .catch(done.fail);
+  it('adds hll class to lineContent when line is the highlighted row', () => {
+    store.state.diffs.highlightedRow = thisLine.line_code;
+    createComponent({}, store);
+    expect(wrapper.find('.line_content').classes('hll')).toBe(true);
   });
 
   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);
-    });
+    createComponent({ isCommented: true });
+    expect(wrapper.find('.line_content').classes('hll')).toBe(true);
   });
 
   describe('sets coverage title and class', () => {
-    it('for lines with coverage', done => {
-      vm.$nextTick()
-        .then(() => {
-          const name = diffFileMockData.file_path;
-          const line = thisLine.new_line;
-
-          vm.$store.state.diffs.coverageFiles = { files: { [name]: { [line]: 5 } } };
-
-          return vm.$nextTick();
-        })
-        .then(() => {
-          const coverage = wrapper.find('.line-coverage');
-
-          expect(coverage.attributes('title')).toContain('Test coverage: 5 hits');
-          expect(coverage.classes('coverage')).toBe(true);
-        })
-        .then(done)
-        .catch(done.fail);
+    it('for lines with coverage', () => {
+      const name = diffFileMockData.file_path;
+      const line = thisLine.new_line;
+
+      store.state.diffs.coverageFiles = { files: { [name]: { [line]: 5 } } };
+      createComponent({}, store);
+      const coverage = wrapper.find('.line-coverage');
+
+      expect(coverage.attributes('title')).toContain('Test coverage: 5 hits');
+      expect(coverage.classes('coverage')).toBe(true);
+    });
+
+    it('for lines without coverage', () => {
+      const name = diffFileMockData.file_path;
+      const line = thisLine.new_line;
+
+      store.state.diffs.coverageFiles = { files: { [name]: { [line]: 0 } } };
+      createComponent({}, store);
+      const coverage = wrapper.find('.line-coverage');
+
+      expect(coverage.attributes('title')).toContain('No test coverage');
+      expect(coverage.classes('no-coverage')).toBe(true);
+    });
+
+    it('for unknown lines', () => {
+      store.state.diffs.coverageFiles = {};
+      createComponent({}, store);
+
+      const coverage = wrapper.find('.line-coverage');
+
+      expect(coverage.attributes('title')).toBeUndefined();
+      expect(coverage.classes('coverage')).toBe(false);
+      expect(coverage.classes('no-coverage')).toBe(false);
+    });
+  });
+
+  describe('Table Cells', () => {
+    const findNewTd = () => wrapper.find({ ref: 'newTd' });
+    const findOldTd = () => wrapper.find({ ref: 'oldTd' });
+
+    describe('td', () => {
+      it('highlights when isHighlighted true', () => {
+        store.state.diffs.highlightedRow = thisLine.line_code;
+        createComponent({}, store);
+
+        expect(findNewTd().classes()).toContain('hll');
+        expect(findOldTd().classes()).toContain('hll');
+      });
+
+      it('does not highlight when isHighlighted false', () => {
+        createComponent();
+
+        expect(findNewTd().classes()).not.toContain('hll');
+        expect(findOldTd().classes()).not.toContain('hll');
+      });
+    });
+
+    describe('comment button', () => {
+      const findNoteButton = () => wrapper.find({ ref: 'addDiffNoteButton' });
+
+      it.each`
+        userData     | query                | mergeRefHeadComments | expectation
+        ${TEST_USER} | ${'diff_head=false'} | ${false}             | ${true}
+        ${TEST_USER} | ${'diff_head=true'}  | ${true}              | ${true}
+        ${TEST_USER} | ${'diff_head=true'}  | ${false}             | ${false}
+        ${null}      | ${''}                | ${true}              | ${false}
+      `(
+        'exists is $expectation - with userData ($userData) query ($query)',
+        ({ userData, query, mergeRefHeadComments, expectation }) => {
+          store.state.notes.userData = userData;
+          gon.features = { mergeRefHeadComments };
+          setWindowLocation({ href: `${TEST_HOST}?${query}` });
+          createComponent({}, store);
+
+          expect(findNoteButton().exists()).toBe(expectation);
+        },
+      );
+
+      it.each`
+        isHover  | line                                                       | expectation
+        ${true}  | ${{ ...thisLine, discussions: [] }}                        | ${true}
+        ${false} | ${{ ...thisLine, discussions: [] }}                        | ${false}
+        ${true}  | ${{ ...thisLine, type: 'context', discussions: [] }}       | ${false}
+        ${true}  | ${{ ...thisLine, type: 'old-nonewline', discussions: [] }} | ${false}
+        ${true}  | ${{ ...thisLine, discussions: [{}] }}                      | ${false}
+      `('visible is $expectation - line ($line)', ({ isHover, line, expectation }) => {
+        createComponent({ line });
+        wrapper.setData({ isHover });
+
+        return wrapper.vm.$nextTick().then(() => {
+          expect(findNoteButton().isVisible()).toBe(expectation);
+        });
+      });
+
+      it.each`
+        disabled      | commentsDisabled
+        ${'disabled'} | ${true}
+        ${undefined}  | ${false}
+      `(
+        'has attribute disabled=$disabled when the outer component has prop commentsDisabled=$commentsDisabled',
+        ({ disabled, commentsDisabled }) => {
+          createComponent({
+            line: { ...thisLine, commentsDisabled },
+          });
+
+          wrapper.setData({ isHover: true });
+
+          return wrapper.vm.$nextTick().then(() => {
+            expect(findNoteButton().attributes('disabled')).toBe(disabled);
+          });
+        },
+      );
+
+      const symlinkishFileTooltip =
+        'Commenting on symbolic links that replace or are replaced by files is currently not supported.';
+      const realishFileTooltip =
+        'Commenting on files that replace or are replaced by symbolic links is currently not supported.';
+      const otherFileTooltip = 'Add a comment to this line';
+      const findTooltip = () => wrapper.find({ ref: 'addNoteTooltip' });
+
+      it.each`
+        tooltip                  | commentsDisabled
+        ${symlinkishFileTooltip} | ${{ wasSymbolic: true }}
+        ${symlinkishFileTooltip} | ${{ isSymbolic: true }}
+        ${realishFileTooltip}    | ${{ wasReal: true }}
+        ${realishFileTooltip}    | ${{ isReal: true }}
+        ${otherFileTooltip}      | ${false}
+      `(
+        'has the correct tooltip when commentsDisabled=$commentsDisabled',
+        ({ tooltip, commentsDisabled }) => {
+          createComponent({
+            line: { ...thisLine, commentsDisabled },
+          });
+
+          wrapper.setData({ isHover: true });
+
+          return wrapper.vm.$nextTick().then(() => {
+            expect(findTooltip().attributes('title')).toBe(tooltip);
+          });
+        },
+      );
     });
 
-    it('for lines without coverage', done => {
-      vm.$nextTick()
-        .then(() => {
-          const name = diffFileMockData.file_path;
-          const line = thisLine.new_line;
+    describe('line number', () => {
+      const findLineNumberOld = () => wrapper.find({ ref: 'lineNumberRefOld' });
+      const findLineNumberNew = () => wrapper.find({ ref: 'lineNumberRefNew' });
+
+      it('renders line numbers in correct cells', () => {
+        createComponent();
+
+        expect(findLineNumberOld().exists()).toBe(false);
+        expect(findLineNumberNew().exists()).toBe(true);
+      });
 
-          vm.$store.state.diffs.coverageFiles = { files: { [name]: { [line]: 0 } } };
+      describe('with lineNumber prop', () => {
+        const TEST_LINE_CODE = 'LC_42';
+        const TEST_LINE_NUMBER = 1;
 
-          return vm.$nextTick();
-        })
-        .then(() => {
-          const coverage = wrapper.find('.line-coverage');
+        describe.each`
+          lineProps                                                                                     | findLineNumber       | expectedHref            | expectedClickArg
+          ${{ line_code: TEST_LINE_CODE, old_line: TEST_LINE_NUMBER }}                                  | ${findLineNumberOld} | ${`#${TEST_LINE_CODE}`} | ${TEST_LINE_CODE}
+          ${{ line_code: undefined, old_line: TEST_LINE_NUMBER }}                                       | ${findLineNumberOld} | ${'#'}                  | ${undefined}
+          ${{ line_code: undefined, left: { line_code: TEST_LINE_CODE }, old_line: TEST_LINE_NUMBER }}  | ${findLineNumberOld} | ${'#'}                  | ${TEST_LINE_CODE}
+          ${{ line_code: undefined, right: { line_code: TEST_LINE_CODE }, new_line: TEST_LINE_NUMBER }} | ${findLineNumberNew} | ${'#'}                  | ${TEST_LINE_CODE}
+        `(
+          'with line ($lineProps)',
+          ({ lineProps, findLineNumber, expectedHref, expectedClickArg }) => {
+            beforeEach(() => {
+              jest.spyOn(store, 'dispatch').mockImplementation();
+              createComponent({
+                line: { ...thisLine, ...lineProps },
+              });
+            });
 
-          expect(coverage.attributes('title')).toContain('No test coverage');
-          expect(coverage.classes('no-coverage')).toBe(true);
-        })
-        .then(done)
-        .catch(done.fail);
+            it('renders', () => {
+              expect(findLineNumber().exists()).toBe(true);
+              expect(findLineNumber().attributes()).toEqual({
+                href: expectedHref,
+                'data-linenumber': TEST_LINE_NUMBER.toString(),
+              });
+            });
+
+            it('on click, dispatches setHighlightedRow', () => {
+              expect(store.dispatch).toHaveBeenCalledTimes(1);
+
+              findLineNumber().trigger('click');
+
+              expect(store.dispatch).toHaveBeenCalledWith(
+                'diffs/setHighlightedRow',
+                expectedClickArg,
+              );
+              expect(store.dispatch).toHaveBeenCalledTimes(2);
+            });
+          },
+        );
+      });
     });
 
-    it('for unknown lines', done => {
-      vm.$nextTick()
-        .then(() => {
-          vm.$store.state.diffs.coverageFiles = {};
-
-          return vm.$nextTick();
-        })
-        .then(() => {
-          const coverage = wrapper.find('.line-coverage');
-
-          expect(coverage.attributes('title')).toBeUndefined();
-          expect(coverage.classes('coverage')).toBe(false);
-          expect(coverage.classes('no-coverage')).toBe(false);
-        })
-        .then(done)
-        .catch(done.fail);
+    describe('diff-gutter-avatars', () => {
+      const TEST_LINE_CODE = 'LC_42';
+      const TEST_FILE_HASH = diffFileMockData.file_hash;
+      const findAvatars = () => wrapper.find(DiffGutterAvatars);
+      let line;
+
+      beforeEach(() => {
+        jest.spyOn(store, 'dispatch').mockImplementation();
+
+        line = {
+          line_code: TEST_LINE_CODE,
+          type: 'new',
+          old_line: null,
+          new_line: 1,
+          discussions: [{ ...discussionsMockData }],
+          discussionsExpanded: true,
+          text: '+<span id="LC1" class="line" lang="plaintext">  - Bad dates</span>\n',
+          rich_text: '+<span id="LC1" class="line" lang="plaintext">  - Bad dates</span>\n',
+          meta_data: null,
+        };
+      });
+
+      describe('with showCommentButton', () => {
+        it('renders if line has discussions', () => {
+          createComponent({ line });
+
+          expect(findAvatars().props()).toEqual({
+            discussions: line.discussions,
+            discussionsExpanded: line.discussionsExpanded,
+          });
+        });
+
+        it('does notrender if line has no discussions', () => {
+          line.discussions = [];
+          createComponent({ line });
+
+          expect(findAvatars().exists()).toEqual(false);
+        });
+
+        it('toggles line discussion', () => {
+          createComponent({ line });
+
+          expect(store.dispatch).toHaveBeenCalledTimes(1);
+
+          findAvatars().vm.$emit('toggleLineDiscussions');
+
+          expect(store.dispatch).toHaveBeenCalledWith('diffs/toggleLineDiscussions', {
+            lineCode: TEST_LINE_CODE,
+            fileHash: TEST_FILE_HASH,
+            expanded: !line.discussionsExpanded,
+          });
+        });
+      });
     });
   });
 });