Merge branch '58294-discussion-notes-component' into 'master'

Extract discussion notes into new component

Closes #58294

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

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

+<script>
+import { mapGetters } from 'vuex';
+import { SYSTEM_NOTE } from '../constants';
+import { __ } from '~/locale';
+import NoteableNote from './noteable_note.vue';
+import PlaceholderNote from '../../vue_shared/components/notes/placeholder_note.vue';
+import PlaceholderSystemNote from '../../vue_shared/components/notes/placeholder_system_note.vue';
+import SystemNote from '~/vue_shared/components/notes/system_note.vue';
+import ToggleRepliesWidget from './toggle_replies_widget.vue';
+import NoteEditedText from './note_edited_text.vue';
+
+export default {
+  name: 'DiscussionNotes',
+  components: {
+    ToggleRepliesWidget,
+    NoteEditedText,
+  },
+  props: {
+    discussion: {
+      type: Object,
+      required: true,
+    },
+    isExpanded: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+    diffLine: {
+      type: Object,
+      required: false,
+      default: null,
+    },
+    line: {
+      type: Object,
+      required: false,
+      default: null,
+    },
+    shouldGroupReplies: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+    helpPagePath: {
+      type: String,
+      required: false,
+      default: '',
+    },
+  },
+  computed: {
+    ...mapGetters(['userCanReply']),
+    hasReplies() {
+      return !!this.replies.length;
+    },
+    replies() {
+      return this.discussion.notes.slice(1);
+    },
+    firstNote() {
+      return this.discussion.notes.slice(0, 1)[0];
+    },
+    resolvedText() {
+      return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved');
+    },
+    commit() {
+      if (!this.discussion.for_commit) {
+        return null;
+      }
+
+      return {
+        id: this.discussion.commit_id,
+        url: this.discussion.discussion_path,
+      };
+    },
+  },
+  methods: {
+    componentName(note) {
+      if (note.isPlaceholderNote) {
+        if (note.placeholderType === SYSTEM_NOTE) {
+          return PlaceholderSystemNote;
+        }
+
+        return PlaceholderNote;
+      }
+
+      if (note.system) {
+        return SystemNote;
+      }
+
+      return NoteableNote;
+    },
+    componentData(note) {
+      return note.isPlaceholderNote ? note.notes[0] : note;
+    },
+  },
+};
+</script>
+
+<template>
+  <div class="discussion-notes">
+    <ul class="notes">
+      <template v-if="shouldGroupReplies">
+        <component
+          :is="componentName(firstNote)"
+          :note="componentData(firstNote)"
+          :line="line"
+          :commit="commit"
+          :help-page-path="helpPagePath"
+          :show-reply-button="userCanReply"
+          @handle-delete-note="$emit('deleteNote')"
+          @start-replying="$emit('startReplying')"
+        >
+          <note-edited-text
+            v-if="discussion.resolved"
+            slot="discussion-resolved-text"
+            :edited-at="discussion.resolved_at"
+            :edited-by="discussion.resolved_by"
+            :action-text="resolvedText"
+            class-name="discussion-headline-light js-discussion-headline discussion-resolved-text"
+          />
+          <slot slot="avatar-badge" name="avatar-badge"></slot>
+        </component>
+        <toggle-replies-widget
+          v-if="hasReplies"
+          :collapsed="!isExpanded"
+          :replies="replies"
+          @toggle="$emit('toggleDiscussion')"
+        />
+        <template v-if="isExpanded">
+          <component
+            :is="componentName(note)"
+            v-for="note in replies"
+            :key="note.id"
+            :note="componentData(note)"
+            :help-page-path="helpPagePath"
+            :line="line"
+            @handle-delete-note="$emit('deleteNote')"
+          />
+        </template>
+      </template>
+      <template v-else>
+        <component
+          :is="componentName(note)"
+          v-for="(note, index) in discussion.notes"
+          :key="note.id"
+          :note="componentData(note)"
+          :help-page-path="helpPagePath"
+          :line="diffLine"
+          @handle-delete-note="$emit('deleteNote')"
+        >
+          <slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"></slot>
+        </component>
+      </template>
+    </ul>
+    <slot :show-replies="isExpanded || !hasReplies" name="footer"></slot>
+  </div>
+</template>

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

@@ -67,6 +67,7 @@ export default {
       'isLoading',
       'commentsDisabled',
       'getNoteableData',
+      'userCanReply',
     ]),
     noteableType() {
       return this.noteableData.noteableType;
@@ -83,7 +84,7 @@ export default {
       return this.discussions;
     },
     canReply() {
-      return this.getNoteableData.current_user.can_create_note && !this.commentsDisabled;
+      return this.userCanReply && !this.commentsDisabled;
     },
   },
   watch: {

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

@@ -20,6 +20,8 @@ export const getNoteableData = state => state.noteableData;
 
 export const getNoteableDataByProp = state => prop => state.noteableData[prop];
 
+export const userCanReply = state => !!state.noteableData.current_user.can_create_note;
+
 export const openState = state => state.noteableData.state;
 
 export const getUserData = state => state.userData || {};

changelogs/unreleased/58294-discussion-notes-component.yml

+---
+title: Extract DiscussionNotes component from NoteableDiscussion
+merge_request: 27066
+author:
+type: other

jest.config.js

@@ -24,6 +24,7 @@ module.exports = {
     '^helpers(/.*)$': '<rootDir>/spec/frontend/helpers$1',
     '^vendor(/.*)$': '<rootDir>/vendor/assets/javascripts$1',
     '\\.(jpg|jpeg|png|svg)$': '<rootDir>/spec/frontend/__mocks__/file_mock.js',
+    'emojis(/.*).json': '<rootDir>/fixtures/emojis$1.json',
   },
   collectCoverageFrom: ['<rootDir>/app/assets/javascripts/**/*.{js,vue}'],
   coverageDirectory: '<rootDir>/coverage-frontend/',

spec/frontend/notes/components/discussion_notes_spec.js

+import { mount, createLocalVue } from '@vue/test-utils';
+import '~/behaviors/markdown/render_gfm';
+import { SYSTEM_NOTE } from '~/notes/constants';
+import DiscussionNotes from '~/notes/components/discussion_notes.vue';
+import NoteableNote from '~/notes/components/noteable_note.vue';
+import PlaceholderNote from '~/vue_shared/components/notes/placeholder_note.vue';
+import PlaceholderSystemNote from '~/vue_shared/components/notes/placeholder_system_note.vue';
+import SystemNote from '~/vue_shared/components/notes/system_note.vue';
+import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue';
+import createStore from '~/notes/stores';
+import {
+  noteableDataMock,
+  discussionMock,
+  notesDataMock,
+} from '../../../javascripts/notes/mock_data';
+
+const localVue = createLocalVue();
+
+describe('DiscussionNotes', () => {
+  let wrapper;
+
+  const createComponent = props => {
+    const store = createStore();
+    store.dispatch('setNoteableData', noteableDataMock);
+    store.dispatch('setNotesData', notesDataMock);
+
+    wrapper = mount(DiscussionNotes, {
+      localVue,
+      store,
+      propsData: {
+        discussion: discussionMock,
+        isExpanded: false,
+        shouldGroupReplies: false,
+        ...props,
+      },
+      scopedSlots: {
+        footer: '<p slot-scope="{ showReplies }">showReplies:{{showReplies}}</p>',
+      },
+      slots: {
+        'avatar-badge': '<span class="avatar-badge-slot-content" />',
+      },
+      sync: false,
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  describe('rendering', () => {
+    it('renders an element for each note in the discussion', () => {
+      createComponent();
+      const notesCount = discussionMock.notes.length;
+      const els = wrapper.findAll(TimelineEntryItem);
+      expect(els.length).toBe(notesCount);
+    });
+
+    it('renders one element if replies groupping is enabled', () => {
+      createComponent({ shouldGroupReplies: true });
+      const els = wrapper.findAll(TimelineEntryItem);
+      expect(els.length).toBe(1);
+    });
+
+    it('uses proper component to render each note type', () => {
+      const discussion = { ...discussionMock };
+      const notesData = [
+        // PlaceholderSystemNote
+        {
+          id: 1,
+          isPlaceholderNote: true,
+          placeholderType: SYSTEM_NOTE,
+          notes: [{ body: 'PlaceholderSystemNote' }],
+        },
+        // PlaceholderNote
+        {
+          id: 2,
+          isPlaceholderNote: true,
+          notes: [{ body: 'PlaceholderNote' }],
+        },
+        // SystemNote
+        {
+          id: 3,
+          system: true,
+          note: 'SystemNote',
+        },
+        // NoteableNote
+        discussion.notes[0],
+      ];
+      discussion.notes = notesData;
+      createComponent({ discussion });
+      const notes = wrapper.findAll('.notes > li');
+
+      expect(notes.at(0).is(PlaceholderSystemNote)).toBe(true);
+      expect(notes.at(1).is(PlaceholderNote)).toBe(true);
+      expect(notes.at(2).is(SystemNote)).toBe(true);
+      expect(notes.at(3).is(NoteableNote)).toBe(true);
+    });
+
+    it('renders footer scoped slot with showReplies === true when expanded', () => {
+      createComponent({ isExpanded: true });
+      expect(wrapper.text()).toMatch('showReplies:true');
+    });
+
+    it('renders footer scoped slot with showReplies === false when collapsed', () => {
+      createComponent({ isExpanded: false });
+      expect(wrapper.text()).toMatch('showReplies:false');
+    });
+
+    it('passes down avatar-badge slot content', () => {
+      createComponent();
+      expect(wrapper.find('.avatar-badge-slot-content').exists()).toBe(true);
+    });
+  });
+
+  describe('componentData', () => {
+    beforeEach(() => {
+      createComponent();
+    });
+
+    it('should return first note object for placeholder note', () => {
+      const data = {
+        isPlaceholderNote: true,
+        notes: [{ body: 'hello world!' }],
+      };
+      const note = wrapper.vm.componentData(data);
+
+      expect(note).toEqual(data.notes[0]);
+    });
+
+    it('should return given note for nonplaceholder notes', () => {
+      const data = {
+        notes: [{ id: 12 }],
+      };
+      const note = wrapper.vm.componentData(data);
+
+      expect(note).toEqual(data);
+    });
+  });
+});

spec/javascripts/diffs/components/diff_discussions_spec.js

-import Vue from 'vue';
+import { mount, createLocalVue } from '@vue/test-utils';
 import DiffDiscussions from '~/diffs/components/diff_discussions.vue';
+import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue';
+import NoteableDiscussion from '~/notes/components/noteable_discussion.vue';
+import DiscussionNotes from '~/notes/components/discussion_notes.vue';
+import Icon from '~/vue_shared/components/icon.vue';
 import { createStore } from '~/mr_notes/stores';
-import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
 import '~/behaviors/markdown/render_gfm';
 import discussionsMockData from '../mock_data/diff_discussions';
 
+const localVue = createLocalVue();
+
 describe('DiffDiscussions', () => {
-  let vm;
+  let store;
+  let wrapper;
   const getDiscussionsMockData = () => [Object.assign({}, discussionsMockData)];
 
-  function createComponent(props = {}) {
-    const store = createStore();
-
-    vm = createComponentWithStore(Vue.extend(DiffDiscussions), store, {
-      discussions: getDiscussionsMockData(),
-      ...props,
-    }).$mount();
-  }
+  const createComponent = props => {
+    store = createStore();
+    wrapper = mount(localVue.extend(DiffDiscussions), {
+      store,
+      propsData: {
+        discussions: getDiscussionsMockData(),
+        ...props,
+      },
+      localVue,
+      sync: false,
+    });
+  };
 
   afterEach(() => {
-    vm.$destroy();
+    wrapper.destroy();
   });
 
   describe('template', () => {
     it('should have notes list', () => {
       createComponent();
 
-      expect(vm.$el.querySelectorAll('.discussion .note.timeline-entry').length).toEqual(5);
+      expect(wrapper.find(NoteableDiscussion).exists()).toBe(true);
+      expect(wrapper.find(DiscussionNotes).exists()).toBe(true);
+      expect(wrapper.find(DiscussionNotes).findAll(TimelineEntryItem).length).toBe(
+        discussionsMockData.notes.length,
+      );
     });
   });
 
   describe('image commenting', () => {
+    const findDiffNotesToggle = () => wrapper.find('.js-diff-notes-toggle');
+
     it('renders collapsible discussion button', () => {
       createComponent({ shouldCollapseDiscussions: true });
+      const diffNotesToggle = findDiffNotesToggle();
 
-      expect(vm.$el.querySelector('.js-diff-notes-toggle')).not.toBe(null);
-      expect(vm.$el.querySelector('.js-diff-notes-toggle svg')).not.toBe(null);
-      expect(vm.$el.querySelector('.js-diff-notes-toggle').classList).toContain(
-        'diff-notes-collapse',
-      );
+      expect(diffNotesToggle.exists()).toBe(true);
+      expect(diffNotesToggle.find(Icon).exists()).toBe(true);
+      expect(diffNotesToggle.classes('diff-notes-collapse')).toBe(true);
     });
 
     it('dispatches toggleDiscussion when clicking collapse button', () => {
       createComponent({ shouldCollapseDiscussions: true });
+      spyOn(wrapper.vm.$store, 'dispatch').and.stub();
+      const diffNotesToggle = findDiffNotesToggle();
+      diffNotesToggle.trigger('click');
 
-      spyOn(vm.$store, 'dispatch').and.stub();
-
-      vm.$el.querySelector('.js-diff-notes-toggle').click();
-
-      expect(vm.$store.dispatch).toHaveBeenCalledWith('toggleDiscussion', {
-        discussionId: vm.discussions[0].id,
+      expect(wrapper.vm.$store.dispatch).toHaveBeenCalledWith('toggleDiscussion', {
+        discussionId: discussionsMockData.id,
       });
     });
 
-    it('renders expand button when discussion is collapsed', done => {
-      createComponent({ shouldCollapseDiscussions: true });
-
-      vm.discussions[0].expanded = false;
-
-      vm.$nextTick(() => {
-        expect(vm.$el.querySelector('.js-diff-notes-toggle').textContent.trim()).toBe('1');
-        expect(vm.$el.querySelector('.js-diff-notes-toggle').className).toContain(
-          'btn-transparent badge badge-pill',
-        );
+    it('renders expand button when discussion is collapsed', () => {
+      const discussions = getDiscussionsMockData();
+      discussions[0].expanded = false;
+      createComponent({ discussions, shouldCollapseDiscussions: true });
+      const diffNotesToggle = findDiffNotesToggle();
 
-        done();
-      });
+      expect(diffNotesToggle.text().trim()).toBe('1');
+      expect(diffNotesToggle.classes()).toEqual(
+        jasmine.arrayContaining(['btn-transparent', 'badge', 'badge-pill']),
+      );
     });
 
-    it('hides discussion when collapsed', done => {
-      createComponent({ shouldCollapseDiscussions: true });
+    it('hides discussion when collapsed', () => {
+      const discussions = getDiscussionsMockData();
+      discussions[0].expanded = false;
+      createComponent({ discussions, shouldCollapseDiscussions: true });
 
-      vm.discussions[0].expanded = false;
-
-      vm.$nextTick(() => {
-        expect(vm.$el.querySelector('.note-discussion').style.display).toBe('none');
-
-        done();
-      });
+      expect(wrapper.find(NoteableDiscussion).isVisible()).toBe(false);
     });
 
     it('renders badge on avatar', () => {
-      createComponent({ renderAvatarBadge: true, discussions: [{ ...discussionsMockData }] });
-
-      expect(vm.$el.querySelector('.user-avatar-link .badge-pill')).not.toBe(null);
-      expect(vm.$el.querySelector('.user-avatar-link .badge-pill').textContent.trim()).toBe('1');
+      createComponent({ renderAvatarBadge: true });
+      const noteableDiscussion = wrapper.find(NoteableDiscussion);
+
+      expect(noteableDiscussion.find('.badge-pill').exists()).toBe(true);
+      expect(
+        noteableDiscussion
+          .find('.badge-pill')
+          .text()
+          .trim(),
+      ).toBe('1');
     });
   });
 });

spec/javascripts/notes/components/noteable_discussion_spec.js

@@ -130,29 +130,6 @@ describe('noteable_discussion component', () => {
     });
   });
 
-  describe('componentData', () => {
-    it('should return first note object for placeholder note', () => {
-      const data = {
-        isPlaceholderNote: true,
-        notes: [{ body: 'hello world!' }],
-      };
-
-      const note = wrapper.vm.componentData(data);
-
-      expect(note).toEqual(data.notes[0]);
-    });
-
-    it('should return given note for nonplaceholder notes', () => {
-      const data = {
-        notes: [{ id: 12 }],
-      };
-
-      const note = wrapper.vm.componentData(data);
-
-      expect(note).toEqual(data);
-    });
-  });
-
   describe('action text', () => {
     const commitId = 'razupaltuff';
     const truncatedCommitId = commitId.substr(0, 8);