Merge branch '223076-disable-commenting-on-symlinks-in-an-mr' into 'master'

Disable commenting on symlinks in symlink-related files

See merge request gitlab-org/gitlab!35371

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

@@ -147,7 +147,7 @@ export default {
           slot="image-overlay"
           :discussions="imageDiscussions"
           :file-hash="diffFileHash"
-          :can-comment="getNoteableData.current_user.can_create_note"
+          :can-comment="getNoteableData.current_user.can_create_note && !diffFile.brokenSymlink"
         />
         <div v-if="showNotesContainer" class="note-container">
           <user-avatar-link

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

@@ -167,6 +167,7 @@ export default {
     :id="file.file_hash"
     :class="{
       'is-active': currentDiffFileId === file.file_hash,
+      'comments-disabled': Boolean(file.brokenSymlink),
     }"
     :data-path="file.new_path"
     class="diff-file file-holder"

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

 <script>
 import { mapGetters, mapActions } from 'vuex';
-import { GlIcon } from '@gitlab/ui';
+import { GlIcon, GlTooltipDirective } from '@gitlab/ui';
 import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
 import DiffGutterAvatars from './diff_gutter_avatars.vue';
+import { __ } from '~/locale';
 import {
   CONTEXT_LINE_TYPE,
   LINE_POSITION_RIGHT,
@@ -18,6 +19,9 @@ export default {
     DiffGutterAvatars,
     GlIcon,
   },
+  directives: {
+    GlTooltip: GlTooltipDirective,
+  },
   props: {
     line: {
       type: Object,
@@ -123,6 +127,24 @@ export default {
     lineNumber() {
       return this.lineType === OLD_LINE_TYPE ? this.line.old_line : this.line.new_line;
     },
+    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;
+    },
   },
   mounted() {
     this.unwatchShouldShowCommentButton = this.$watch('shouldShowCommentButton', newVal => {
@@ -146,17 +168,24 @@ export default {
 
 <template>
   <td ref="td" :class="classNameMap">
-    <button
-      v-if="shouldRenderCommentButton"
-      v-show="shouldShowCommentButton"
-      ref="addDiffNoteButton"
-      type="button"
-      class="add-diff-note js-add-diff-note-button qa-diff-comment"
-      title="Add a comment to this line"
-      @click="handleCommentButton"
+    <span
+      ref="addNoteTooltip"
+      v-gl-tooltip
+      class="add-diff-note tooltip-wrapper"
+      :title="addCommentTooltip"
     >
-      <gl-icon :size="12" name="comment" />
-    </button>
+      <button
+        v-if="shouldRenderCommentButton"
+        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="lineNumber"
       ref="lineNumberRef"

app/assets/javascripts/diffs/store/utils.js

@@ -480,6 +480,10 @@ export function getDiffPositionByLineCode(diffFiles, useSingleDiffStyle) {
 // This method will check whether the discussion is still applicable
 // to the diff line in question regarding different versions of the MR
 export function isDiscussionApplicableToLine({ discussion, diffPosition, latestDiff }) {
+  if (!diffPosition) {
+    return false;
+  }
+
   const { line_code, ...dp } = diffPosition;
   // Removing `line_range` from diffPosition because the backend does not
   // yet consistently return this property. This check can be removed,

app/assets/stylesheets/pages/notes.scss

@@ -820,9 +820,7 @@ $note-form-margin-left: 72px;
   }
 }
 
-.add-diff-note {
-  @include btn-comment-icon;
-  opacity: 0;
+.tooltip-wrapper.add-diff-note {
   margin-left: -52px;
   position: absolute;
   top: 50%;
@@ -830,6 +828,18 @@ $note-form-margin-left: 72px;
   z-index: 10;
 }
 
+.note-button.add-diff-note {
+  @include btn-comment-icon;
+  opacity: 0;
+
+  &[disabled] {
+    background: $white;
+    border-color: $gray-200;
+    color: $gl-gray-400;
+    cursor: not-allowed;
+  }
+}
+
 .disabled-comment {
   background-color: $gray-light;
   border-radius: $border-radius-base;

changelogs/unreleased/symlink-comments-disable-symlink-comments.yml

+---
+title: Disable commenting on lines in files that were or are symlinks or replace or
+  are replaced by symlinks
+merge_request: 35371
+author:
+type: fixed

locale/gitlab.pot

@@ -6161,6 +6161,12 @@ msgstr ""
 msgid "Comment/Reply (quoting selected text)"
 msgstr ""
 
+msgid "Commenting on files that replace or are replaced by symbolic links is currently not supported."
+msgstr ""
+
+msgid "Commenting on symbolic links that replace or are replaced by files is currently not supported."
+msgstr ""
+
 msgid "Comments"
 msgstr ""
 

spec/frontend/diffs/components/diff_table_cell_spec.js

@@ -18,6 +18,12 @@ const TEST_LINE_CODE = 'LC_42';
 const TEST_FILE_HASH = diffFileMockData.file_hash;
 
 describe('DiffTableCell', () => {
+  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';
+
   let wrapper;
   let line;
   let store;
@@ -67,6 +73,7 @@ describe('DiffTableCell', () => {
   const findTd = () => wrapper.find({ ref: 'td' });
   const findNoteButton = () => wrapper.find({ ref: 'addDiffNoteButton' });
   const findLineNumber = () => wrapper.find({ ref: 'lineNumberRef' });
+  const findTooltip = () => wrapper.find({ ref: 'addNoteTooltip' });
   const findAvatars = () => wrapper.find(DiffGutterAvatars);
 
   describe('td', () => {
@@ -134,6 +141,53 @@ describe('DiffTableCell', () => {
         });
       },
     );
+
+    it.each`
+      disabled      | commentsDisabled
+      ${'disabled'} | ${true}
+      ${undefined}  | ${false}
+    `(
+      'has attribute disabled=$disabled when the outer component has prop commentsDisabled=$commentsDisabled',
+      ({ disabled, commentsDisabled }) => {
+        line.commentsDisabled = commentsDisabled;
+
+        createComponent({
+          showCommentButton: true,
+          isHover: true,
+        });
+
+        wrapper.setData({ isCommentButtonRendered: true });
+
+        return wrapper.vm.$nextTick().then(() => {
+          expect(findNoteButton().attributes('disabled')).toBe(disabled);
+        });
+      },
+    );
+
+    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 }) => {
+        line.commentsDisabled = commentsDisabled;
+
+        createComponent({
+          showCommentButton: true,
+          isHover: true,
+        });
+
+        wrapper.setData({ isCommentButtonRendered: true });
+
+        return wrapper.vm.$nextTick().then(() => {
+          expect(findTooltip().attributes('title')).toBe(tooltip);
+        });
+      },
+    );
   });
 
   describe('line number', () => {