Merge branch '64134-edit-comment' into 'master'

Resolve "Clicking edit button in a thread reply doesn’t work"

Closes #64134

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

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

@@ -8,12 +8,14 @@ import SystemNote from '~/vue_shared/components/notes/system_note.vue';
 import NoteableNote from './noteable_note.vue';
 import ToggleRepliesWidget from './toggle_replies_widget.vue';
 import NoteEditedText from './note_edited_text.vue';
+import DiscussionNotesRepliesWrapper from './discussion_notes_replies_wrapper.vue';
 
 export default {
   name: 'DiscussionNotes',
   components: {
     ToggleRepliesWidget,
     NoteEditedText,
+    DiscussionNotesRepliesWrapper,
   },
   props: {
     discussion: {
@@ -119,9 +121,7 @@ export default {
           />
           <slot slot="avatar-badge" name="avatar-badge"></slot>
         </component>
-        <div
-          :class="discussion.diff_discussion ? 'discussion-collapsible bordered-box clearfix' : ''"
-        >
+        <discussion-notes-replies-wrapper :is-diff-discussion="discussion.diff_discussion">
           <toggle-replies-widget
             v-if="hasReplies"
             :collapsed="!isExpanded"
@@ -141,7 +141,7 @@ export default {
             />
           </template>
           <slot :show-replies="isExpanded || !hasReplies" name="footer"></slot>
-        </div>
+        </discussion-notes-replies-wrapper>
       </template>
       <template v-else>
         <component

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

+<script>
+/**
+ * Wrapper for discussion notes replies section.
+ *
+ * This is a functional component using the render method because in some cases
+ * the wrapper is not needed and we want to simply render along the children.
+ */
+export default {
+  functional: true,
+  props: {
+    isDiffDiscussion: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
+  render(h, { props, children }) {
+    if (props.isDiffDiscussion) {
+      return h('li', { class: 'discussion-collapsible bordered-box clearfix' }, [
+        h('ul', { class: 'notes' }, children),
+      ]);
+    }
+
+    return children;
+  },
+};
+</script>

app/assets/stylesheets/pages/diff.scss

@@ -1095,6 +1095,10 @@ table.code {
 
 .discussion-collapsible {
   margin: 0 $gl-padding $gl-padding 71px;
+
+  .notes {
+    border-radius: $border-radius-default;
+  }
 }
 
 .parallel {

app/assets/stylesheets/pages/notes.scss

@@ -139,7 +139,6 @@ $note-form-margin-left: 72px;
     border-radius: 4px 4px 0 0;
 
     &.collapsed {
-      border: 0;
       border-radius: 4px;
     }
   }

spec/frontend/notes/components/discussion_notes_replies_wrapper_spec.js

+import { mount } from '@vue/test-utils';
+import DiscussionNotesRepliesWrapper from '~/notes/components/discussion_notes_replies_wrapper.vue';
+
+const TEST_CHILDREN = '<li>Hello!</li><li>World!</li>';
+
+// We have to wrap our SUT with a TestComponent because multiple roots are possible
+// because it's a functional component.
+const TestComponent = {
+  components: { DiscussionNotesRepliesWrapper },
+  template: `<ul><discussion-notes-replies-wrapper v-bind="$attrs">${TEST_CHILDREN}</discussion-notes-replies-wrapper></ul>`,
+};
+
+describe('DiscussionNotesRepliesWrapper', () => {
+  let wrapper;
+
+  const createComponent = (props = {}) => {
+    wrapper = mount(TestComponent, {
+      propsData: props,
+      sync: false,
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  describe('when normal discussion', () => {
+    beforeEach(() => {
+      createComponent();
+    });
+
+    it('renders children directly', () => {
+      expect(wrapper.html()).toEqual(`<ul>${TEST_CHILDREN}</ul>`);
+    });
+  });
+
+  describe('when diff discussion', () => {
+    beforeEach(() => {
+      createComponent({
+        isDiffDiscussion: true,
+      });
+    });
+
+    it('wraps children with notes', () => {
+      const notes = wrapper.find('li.discussion-collapsible ul.notes');
+
+      expect(notes.exists()).toBe(true);
+      expect(notes.html()).toEqual(`<ul class="notes">${TEST_CHILDREN}</ul>`);
+    });
+  });
+});

spec/javascripts/diffs/components/inline_diff_view_spec.js

@@ -10,6 +10,7 @@ describe('InlineDiffView', () => {
   let component;
   const getDiffFileMock = () => Object.assign({}, diffFileMockData);
   const getDiscussionsMockData = () => [Object.assign({}, discussionsMockData)];
+  const notesLength = getDiscussionsMockData()[0].notes.length;
 
   beforeEach(done => {
     const diffFile = getDiffFileMock();
@@ -40,7 +41,7 @@ describe('InlineDiffView', () => {
 
       Vue.nextTick(() => {
         expect(el.querySelectorAll('.notes_holder').length).toEqual(1);
-        expect(el.querySelectorAll('.notes_holder .note-discussion li').length).toEqual(6);
+        expect(el.querySelectorAll('.notes_holder .note').length).toEqual(notesLength + 1);
         expect(el.innerText.indexOf('comment 5')).toBeGreaterThan(-1);
         component.$store.dispatch('setInitialNotes', []);