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

Jdb/refactor parallel diff table row

See merge request gitlab-org/gitlab!41606

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

 <script>
 import { mapActions, mapGetters, mapState } from 'vuex';
 import $ from 'jquery';
-import { GlTooltipDirective, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui';
-import DiffTableCell from './diff_table_cell.vue';
+import { GlTooltipDirective, GlIcon, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui';
 import {
   MATCH_LINE_TYPE,
   NEW_LINE_TYPE,
@@ -13,11 +12,16 @@ import {
   PARALLEL_DIFF_VIEW_TYPE,
   NEW_NO_NEW_LINE_TYPE,
   EMPTY_CELL_TYPE,
+  LINE_HOVER_CLASS_NAME,
 } from '../constants';
+import { __ } from '~/locale';
+import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
+import DiffGutterAvatars from './diff_gutter_avatars.vue';
 
 export default {
   components: {
-    DiffTableCell,
+    GlIcon,
+    DiffGutterAvatars,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
@@ -51,10 +55,12 @@ export default {
     return {
       isLeftHover: false,
       isRightHover: false,
+      isCommentButtonRendered: false,
     };
   },
   computed: {
     ...mapGetters('diffs', ['fileLineCoverage']),
+    ...mapGetters(['isLoggedIn']),
     ...mapState({
       isHighlighted(state) {
         if (this.isCommented) return true;
@@ -66,12 +72,15 @@ export default {
         return lineCode ? lineCode === state.diffs.highlightedRow : false;
       },
     }),
-    isContextLine() {
+    isContextLineLeft() {
       return this.line.left && this.line.left.type === CONTEXT_LINE_TYPE;
     },
+    isContextLineRight() {
+      return this.line.right && this.line.right.type === CONTEXT_LINE_TYPE;
+    },
     classNameMap() {
       return {
-        [CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
+        [CONTEXT_LINE_CLASS_NAME]: this.isContextLineLeft,
         [PARALLEL_DIFF_VIEW_TYPE]: true,
       };
     },
@@ -98,6 +107,129 @@ export default {
     coverageState() {
       return this.fileLineCoverage(this.filePath, this.line.right.new_line);
     },
+    classNameMapCellLeft() {
+      const { type } = this.line.left;
+
+      return [
+        type,
+        {
+          hll: this.isHighlighted,
+          [LINE_HOVER_CLASS_NAME]:
+            this.isLoggedIn && this.isLeftHover && !this.isContextLineLeft && !this.isMetaLineLeft,
+        },
+      ];
+    },
+    classNameMapCellRight() {
+      const { type } = this.line.right;
+
+      return [
+        type,
+        {
+          hll: this.isHighlighted,
+          [LINE_HOVER_CLASS_NAME]:
+            this.isLoggedIn &&
+            this.isRightHover &&
+            !this.isContextLineRight &&
+            !this.isMetaLineRight,
+        },
+      ];
+    },
+    addCommentTooltipLeft() {
+      const brokenSymlinks = this.line.left.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;
+    },
+    addCommentTooltipRight() {
+      const brokenSymlinks = this.line.right.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.isCommentButtonRendered) {
+        return false;
+      }
+
+      if (this.isLoggedIn) {
+        const isDiffHead = parseBoolean(getParameterByName('diff_head'));
+        return !isDiffHead || gon.features?.mergeRefHeadComments;
+      }
+
+      return false;
+    },
+    shouldShowCommentButtonLeft() {
+      return (
+        this.isLeftHover &&
+        !this.isContextLineLeft &&
+        !this.isMetaLineLeft &&
+        !this.hasDiscussionsLeft
+      );
+    },
+    shouldShowCommentButtonRight() {
+      return (
+        this.isRightHover &&
+        !this.isContextLineRight &&
+        !this.isMetaLineRight &&
+        !this.hasDiscussionsRight
+      );
+    },
+    hasDiscussionsLeft() {
+      return this.line.left.discussions && this.line.left.discussions.length > 0;
+    },
+    hasDiscussionsRight() {
+      return this.line.right.discussions && this.line.right.discussions.length > 0;
+    },
+    lineHrefOld() {
+      return `#${this.line.left.line_code || ''}`;
+    },
+    lineHrefNew() {
+      return `#${this.line.right.line_code || ''}`;
+    },
+    lineCode() {
+      return (
+        (this.line.left && this.line.left.line_code) ||
+        (this.line.right && this.line.right.line_code)
+      );
+    },
+    isMetaLineLeft() {
+      const { type } = this.line.left;
+
+      return (
+        type === OLD_NO_NEW_LINE_TYPE || type === NEW_NO_NEW_LINE_TYPE || type === EMPTY_CELL_TYPE
+      );
+    },
+    isMetaLineRight() {
+      const { type } = this.line.right;
+
+      return (
+        type === OLD_NO_NEW_LINE_TYPE || type === NEW_NO_NEW_LINE_TYPE || type === EMPTY_CELL_TYPE
+      );
+    },
   },
   created() {
     this.newLineType = NEW_LINE_TYPE;
@@ -106,9 +238,26 @@ export default {
   },
   mounted() {
     this.scrollToLineIfNeededParallel(this.line);
+    this.unwatchShouldShowCommentButton = this.$watch(
+      vm => [vm.shouldShowCommentButtonLeft, vm.shouldShowCommentButtonRight].join(),
+      newVal => {
+        if (newVal) {
+          this.isCommentButtonRendered = true;
+          this.unwatchShouldShowCommentButton();
+        }
+      },
+    );
+  },
+  beforeDestroy() {
+    this.unwatchShouldShowCommentButton();
   },
   methods: {
-    ...mapActions('diffs', ['scrollToLineIfNeededParallel']),
+    ...mapActions('diffs', [
+      'scrollToLineIfNeededParallel',
+      'showCommentForm',
+      'setHighlightedRow',
+      'toggleLineDiscussions',
+    ]),
     handleMouseMove(e) {
       const isHover = e.type === 'mouseover';
       const hoveringCell = e.target.closest('td');
@@ -134,6 +283,9 @@ export default {
         table.addClass(`${lineClass}-selected`);
       }
     },
+    handleCommentButton(line) {
+      this.showCommentForm({ lineCode: line.line_code, fileHash: this.fileHash });
+    },
   },
 };
 </script>
@@ -146,18 +298,46 @@ export default {
     @mouseout="handleMouseMove"
   >
     <template v-if="line.left && !isMatchLineLeft">
-      <diff-table-cell
-        :file-hash="fileHash"
-        :line="line.left"
-        :line-type="oldLineType"
-        :is-bottom="isBottom"
-        :is-hover="isLeftHover"
-        :is-highlighted="isHighlighted"
-        :show-comment-button="true"
-        :diff-view-type="parallelDiffViewType"
-        line-position="left"
-        class="diff-line-num old_line"
-      />
+      <td ref="oldTd" :class="classNameMapCellLeft" class="diff-line-num old_line">
+        <span
+          v-if="shouldRenderCommentButton"
+          ref="addNoteTooltipLeft"
+          v-gl-tooltip
+          class="add-diff-note tooltip-wrapper"
+          :title="addCommentTooltipLeft"
+        >
+          <button
+            v-show="shouldShowCommentButtonLeft"
+            ref="addDiffNoteButtonLeft"
+            type="button"
+            class="add-diff-note note-button js-add-diff-note-button qa-diff-comment"
+            :disabled="line.left.commentsDisabled"
+            @click="handleCommentButton(line.left)"
+          >
+            <gl-icon :size="12" name="comment" />
+          </button>
+        </span>
+        <a
+          v-if="line.left.old_line"
+          ref="lineNumberRefOld"
+          :data-linenumber="line.left.old_line"
+          :href="lineHrefOld"
+          @click="setHighlightedRow(lineCode)"
+        >
+        </a>
+        <diff-gutter-avatars
+          v-if="hasDiscussionsLeft"
+          :discussions="line.left.discussions"
+          :discussions-expanded="line.left.discussionsExpanded"
+          @toggleLineDiscussions="
+            toggleLineDiscussions({
+              lineCode: line.left.line_code,
+              fileHash,
+              expanded: !line.left.discussionsExpanded,
+            })
+          "
+        />
+      </td>
       <td :class="parallelViewLeftLineType" class="line-coverage left-side"></td>
       <td
         :id="line.left.line_code"
@@ -173,18 +353,46 @@ export default {
       <td class="line_content with-coverage parallel left-side empty-cell"></td>
     </template>
     <template v-if="line.right && !isMatchLineRight">
-      <diff-table-cell
-        :file-hash="fileHash"
-        :line="line.right"
-        :line-type="newLineType"
-        :is-bottom="isBottom"
-        :is-hover="isRightHover"
-        :is-highlighted="isHighlighted"
-        :show-comment-button="true"
-        :diff-view-type="parallelDiffViewType"
-        line-position="right"
-        class="diff-line-num new_line"
-      />
+      <td ref="newTd" :class="classNameMapCellRight" class="diff-line-num new_line">
+        <span
+          v-if="shouldRenderCommentButton"
+          ref="addNoteTooltipRight"
+          v-gl-tooltip
+          class="add-diff-note tooltip-wrapper"
+          :title="addCommentTooltipRight"
+        >
+          <button
+            v-show="shouldShowCommentButtonRight"
+            ref="addDiffNoteButtonRight"
+            type="button"
+            class="add-diff-note note-button js-add-diff-note-button qa-diff-comment"
+            :disabled="line.right.commentsDisabled"
+            @click="handleCommentButton(line.right)"
+          >
+            <gl-icon :size="12" name="comment" />
+          </button>
+        </span>
+        <a
+          v-if="line.right.new_line"
+          ref="lineNumberRefNew"
+          :data-linenumber="line.right.new_line"
+          :href="lineHrefNew"
+          @click="setHighlightedRow(lineCode)"
+        >
+        </a>
+        <diff-gutter-avatars
+          v-if="hasDiscussionsRight"
+          :discussions="line.right.discussions"
+          :discussions-expanded="line.right.discussionsExpanded"
+          @toggleLineDiscussions="
+            toggleLineDiscussions({
+              lineCode: line.right.line_code,
+              fileHash,
+              expanded: !line.right.discussionsExpanded,
+            })
+          "
+        />
+      </td>
       <td
         v-gl-tooltip.hover
         :title="coverageState.text"

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

+---
+title: Jdb/refactor parallel diff table row
+merge_request: 41606
+author:
+type: performance

qa/qa/page/merge_request/show.rb

@@ -53,10 +53,6 @@ module QA
           element :diffs_tab
         end
 
-        view 'app/assets/javascripts/diffs/components/diff_table_cell.vue' do
-          element :diff_comment
-        end
-
         view 'app/assets/javascripts/diffs/components/inline_diff_table_row.vue' do
           element :new_diff_line
         end

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 { TEST_HOST } from 'helpers/test_constants';
 import { createStore } from '~/mr_notes/stores';
 import ParallelDiffTableRow from '~/diffs/components/parallel_diff_table_row.vue';
 import diffFileMockData from '../mock_data/diff_file';
+import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
+import discussionsMockData from '../mock_data/diff_discussions';
 
 describe('ParallelDiffTableRow', () => {
   describe('when one side is empty', () => {
@@ -158,4 +161,259 @@ describe('ParallelDiffTableRow', () => {
       });
     });
   });
+
+  describe('Table Cells', () => {
+    let wrapper;
+    let store;
+    let thisLine;
+    const TEST_USER_ID = 'abc123';
+    const TEST_USER = { id: TEST_USER_ID };
+
+    const createComponent = (props = {}, propsStore = store, data = {}) => {
+      wrapper = shallowMount(ParallelDiffTableRow, {
+        store: propsStore,
+        propsData: {
+          line: thisLine,
+          fileHash: diffFileMockData.file_hash,
+          filePath: diffFileMockData.file_path,
+          contextLinesPath: 'contextLinesPath',
+          isHighlighted: false,
+          ...props,
+        },
+        data() {
+          return data;
+        },
+      });
+    };
+
+    const setWindowLocation = value => {
+      Object.defineProperty(window, 'location', {
+        writable: true,
+        value,
+      });
+    };
+
+    beforeEach(() => {
+      // eslint-disable-next-line prefer-destructuring
+      thisLine = diffFileMockData.parallel_diff_lines[2];
+      store = createStore();
+      store.state.notes.userData = TEST_USER;
+    });
+
+    afterEach(() => {
+      wrapper.destroy();
+    });
+
+    const findNewTd = () => wrapper.find({ ref: 'newTd' });
+    const findOldTd = () => wrapper.find({ ref: 'oldTd' });
+
+    describe('td', () => {
+      it('highlights when isHighlighted true', () => {
+        store.state.diffs.highlightedRow = thisLine.left.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: 'addDiffNoteButtonLeft' });
+
+      it.each`
+        hover    | userData     | query                | mergeRefHeadComments | expectation
+        ${true}  | ${TEST_USER} | ${'diff_head=false'} | ${false}             | ${true}
+        ${true}  | ${TEST_USER} | ${'diff_head=true'}  | ${true}              | ${true}
+        ${true}  | ${TEST_USER} | ${'diff_head=true'}  | ${false}             | ${false}
+        ${true}  | ${null}      | ${''}                | ${true}              | ${false}
+        ${false} | ${TEST_USER} | ${'diff_head=false'} | ${false}             | ${false}
+      `(
+        'exists is $expectation - with userData ($userData) query ($query)',
+        async ({ hover, userData, query, mergeRefHeadComments, expectation }) => {
+          store.state.notes.userData = userData;
+          gon.features = { mergeRefHeadComments };
+          setWindowLocation({ href: `${TEST_HOST}?${query}` });
+          createComponent({}, store);
+          if (hover) await wrapper.find('.line_holder').trigger('mouseover');
+
+          expect(findNoteButton().exists()).toBe(expectation);
+        },
+      );
+
+      it.each`
+        line                                                                 | expectation
+        ${{ ...thisLine, left: { discussions: [] } }}                        | ${true}
+        ${{ ...thisLine, left: { type: 'context', discussions: [] } }}       | ${false}
+        ${{ ...thisLine, left: { type: 'old-nonewline', discussions: [] } }} | ${false}
+        ${{ ...thisLine, left: { discussions: [{}] } }}                      | ${false}
+      `('visible is $expectation - line ($line)', async ({ line, expectation }) => {
+        createComponent({ line }, store, { isLeftHover: true, isCommentButtonRendered: true });
+
+        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 }) => {
+          thisLine.left.commentsDisabled = commentsDisabled;
+          createComponent({ line: { ...thisLine } }, store, {
+            isLeftHover: true,
+            isCommentButtonRendered: true,
+          });
+
+          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: 'addNoteTooltipLeft' });
+
+      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 }) => {
+          thisLine.left.commentsDisabled = commentsDisabled;
+          createComponent({ line: { ...thisLine } }, store, {
+            isLeftHover: true,
+            isCommentButtonRendered: true,
+          });
+
+          expect(findTooltip().attributes('title')).toBe(tooltip);
+        },
+      );
+    });
+
+    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(true);
+        expect(findLineNumberNew().exists()).toBe(true);
+      });
+
+      describe('with lineNumber prop', () => {
+        const TEST_LINE_CODE = 'LC_42';
+        const TEST_LINE_NUMBER = 1;
+
+        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}
+        `(
+          'with line ($lineProps)',
+          ({ lineProps, findLineNumber, expectedHref, expectedClickArg }) => {
+            beforeEach(() => {
+              jest.spyOn(store, 'dispatch').mockImplementation();
+              Object.assign(thisLine.left, lineProps);
+              Object.assign(thisLine.right, lineProps);
+              createComponent({
+                line: { ...thisLine },
+              });
+            });
+
+            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);
+            });
+          },
+        );
+      });
+    });
+
+    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 = {
+          left: {
+            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.left.discussions,
+            discussionsExpanded: line.left.discussionsExpanded,
+          });
+        });
+
+        it('does notrender if line has no discussions', () => {
+          line.left.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.left.discussionsExpanded,
+          });
+        });
+      });
+    });
+  });
 });