Merge branch '27033-extract-diff-discussion-header' into 'master'

Resolve "Extract `DiffDiscussionHeader` component from `NoteableDiscussion`"

See merge request gitlab-org/gitlab!19107

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

+<script>
+import { mapActions } from 'vuex';
+import _ from 'underscore';
+
+import { s__, __, sprintf } from '~/locale';
+import { truncateSha } from '~/lib/utils/text_utility';
+
+import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue';
+import noteEditedText from './note_edited_text.vue';
+import noteHeader from './note_header.vue';
+
+export default {
+  name: 'DiffDiscussionHeader',
+  components: {
+    userAvatarLink,
+    noteEditedText,
+    noteHeader,
+  },
+  props: {
+    discussion: {
+      type: Object,
+      required: true,
+    },
+  },
+  computed: {
+    notes() {
+      return this.discussion.notes;
+    },
+    firstNote() {
+      return this.notes[0];
+    },
+    lastNote() {
+      return this.notes[this.notes.length - 1];
+    },
+    author() {
+      return this.firstNote.author;
+    },
+    resolvedText() {
+      return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved');
+    },
+    lastUpdatedBy() {
+      return this.notes.length > 1 ? this.lastNote.author : null;
+    },
+    lastUpdatedAt() {
+      return this.notes.length > 1 ? this.lastNote.created_at : null;
+    },
+    headerText() {
+      const linkStart = `<a href="${_.escape(this.discussion.discussion_path)}">`;
+      const linkEnd = '</a>';
+
+      const { commit_id: commitId } = this.discussion;
+      let commitDisplay = commitId;
+
+      if (commitId) {
+        commitDisplay = `<span class="commit-sha">${truncateSha(commitId)}</span>`;
+      }
+
+      const {
+        for_commit: isForCommit,
+        diff_discussion: isDiffDiscussion,
+        active: isActive,
+      } = this.discussion;
+
+      let text = s__('MergeRequests|started a thread');
+      if (isForCommit) {
+        text = s__(
+          'MergeRequests|started a thread on commit %{linkStart}%{commitDisplay}%{linkEnd}',
+        );
+      } else if (isDiffDiscussion && commitId) {
+        text = isActive
+          ? s__('MergeRequests|started a thread on commit %{linkStart}%{commitDisplay}%{linkEnd}')
+          : s__(
+              'MergeRequests|started a thread on an outdated change in commit %{linkStart}%{commitDisplay}%{linkEnd}',
+            );
+      } else if (isDiffDiscussion) {
+        text = isActive
+          ? s__('MergeRequests|started a thread on %{linkStart}the diff%{linkEnd}')
+          : s__(
+              'MergeRequests|started a thread on %{linkStart}an old version of the diff%{linkEnd}',
+            );
+      }
+
+      return sprintf(text, { commitDisplay, linkStart, linkEnd }, false);
+    },
+  },
+  methods: {
+    ...mapActions(['toggleDiscussion']),
+    toggleDiscussionHandler() {
+      this.toggleDiscussion({ discussionId: this.discussion.id });
+    },
+  },
+};
+</script>
+
+<template>
+  <div class="discussion-header note-wrapper">
+    <div v-once class="timeline-icon align-self-start flex-shrink-0">
+      <user-avatar-link
+        v-if="author"
+        :link-href="author.path"
+        :img-src="author.avatar_url"
+        :img-alt="author.name"
+        :img-size="40"
+      />
+    </div>
+    <div class="timeline-content w-100">
+      <note-header
+        :author="author"
+        :created-at="firstNote.created_at"
+        :note-id="firstNote.id"
+        :include-toggle="true"
+        :expanded="discussion.expanded"
+        @toggleHandler="toggleDiscussionHandler"
+      >
+        <span v-html="headerText"></span>
+      </note-header>
+      <note-edited-text
+        v-if="discussion.resolved"
+        :edited-at="discussion.resolved_at"
+        :edited-by="discussion.resolved_by"
+        :action-text="resolvedText"
+        class-name="discussion-headline-light js-discussion-headline"
+      />
+      <note-edited-text
+        v-else-if="lastUpdatedAt"
+        :edited-at="lastUpdatedAt"
+        :edited-by="lastUpdatedBy"
+        :action-text="__('Last updated')"
+        class-name="discussion-headline-light js-discussion-headline"
+      />
+    </div>
+  </div>
+</template>

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

 <script>
-import _ from 'underscore';
 import { mapActions, mapGetters } from 'vuex';
 import { GlTooltipDirective } from '@gitlab/ui';
-import { truncateSha } from '~/lib/utils/text_utility';
-import { s__, __, sprintf } from '~/locale';
+import { s__, __ } from '~/locale';
 import { clearDraft, getDiscussionReplyKey } from '~/lib/utils/autosave';
 import icon from '~/vue_shared/components/icon.vue';
 import diffLineNoteFormMixin from 'ee_else_ce/notes/mixins/diff_line_note_form';
 import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue';
 import Flash from '../../flash';
 import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue';
-import noteHeader from './note_header.vue';
+import diffDiscussionHeader from './diff_discussion_header.vue';
 import noteSignedOutWidget from './note_signed_out_widget.vue';
-import noteEditedText from './note_edited_text.vue';
 import noteForm from './note_form.vue';
 import diffWithNote from './diff_with_note.vue';
 import noteable from '../mixins/noteable';
@@ -27,9 +24,8 @@ export default {
   components: {
     icon,
     userAvatarLink,
-    noteHeader,
+    diffDiscussionHeader,
     noteSignedOutWidget,
-    noteEditedText,
     noteForm,
     DraftNote: () => import('ee_component/batch_comments/components/draft_note.vue'),
     TimelineEntryItem,
@@ -92,9 +88,6 @@ export default {
     currentUser() {
       return this.getUserData;
     },
-    author() {
-      return this.firstNote.author;
-    },
     autosaveKey() {
       return getDiscussionReplyKey(this.firstNote.noteable_type, this.discussion.id);
     },
@@ -104,27 +97,6 @@ export default {
     firstNote() {
       return this.discussion.notes.slice(0, 1)[0];
     },
-    lastUpdatedBy() {
-      const { notes } = this.discussion;
-
-      if (notes.length > 1) {
-        return notes[notes.length - 1].author;
-      }
-
-      return null;
-    },
-    lastUpdatedAt() {
-      const { notes } = this.discussion;
-
-      if (notes.length > 1) {
-        return notes[notes.length - 1].created_at;
-      }
-
-      return null;
-    },
-    resolvedText() {
-      return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved');
-    },
     shouldShowJumpToNextDiscussion() {
       return this.showJumpToNextDiscussion(this.discussionsByDiffOrder ? 'diff' : 'discussion');
     },
@@ -150,40 +122,6 @@ export default {
     shouldHideDiscussionBody() {
       return this.shouldRenderDiffs && !this.isExpanded;
     },
-    actionText() {
-      const linkStart = `<a href="${_.escape(this.discussion.discussion_path)}">`;
-      const linkEnd = '</a>';
-
-      let { commit_id: commitId } = this.discussion;
-      if (commitId) {
-        commitId = `<span class="commit-sha">${truncateSha(commitId)}</span>`;
-      }
-
-      const {
-        for_commit: isForCommit,
-        diff_discussion: isDiffDiscussion,
-        active: isActive,
-      } = this.discussion;
-
-      let text = s__('MergeRequests|started a thread');
-      if (isForCommit) {
-        text = s__('MergeRequests|started a thread on commit %{linkStart}%{commitId}%{linkEnd}');
-      } else if (isDiffDiscussion && commitId) {
-        text = isActive
-          ? s__('MergeRequests|started a thread on commit %{linkStart}%{commitId}%{linkEnd}')
-          : s__(
-              'MergeRequests|started a thread on an outdated change in commit %{linkStart}%{commitId}%{linkEnd}',
-            );
-      } else if (isDiffDiscussion) {
-        text = isActive
-          ? s__('MergeRequests|started a thread on %{linkStart}the diff%{linkEnd}')
-          : s__(
-              'MergeRequests|started a thread on %{linkStart}an old version of the diff%{linkEnd}',
-            );
-      }
-
-      return sprintf(text, { commitId, linkStart, linkEnd }, false);
-    },
     diffLine() {
       if (this.line) {
         return this.line;
@@ -208,16 +146,11 @@ export default {
   methods: {
     ...mapActions([
       'saveNote',
-      'toggleDiscussion',
       'removePlaceholderNotes',
       'toggleResolveNote',
       'expandDiscussion',
       'removeConvertedDiscussion',
     ]),
-    truncateSha,
-    toggleDiscussionHandler() {
-      this.toggleDiscussion({ discussionId: this.discussion.id });
-    },
     showReplyForm() {
       this.isReplying = true;
     },
@@ -311,43 +244,7 @@ export default {
         class="discussion js-discussion-container"
         data-qa-selector="discussion_content"
       >
-        <div v-if="shouldRenderDiffs" class="discussion-header note-wrapper">
-          <div v-once class="timeline-icon align-self-start flex-shrink-0">
-            <user-avatar-link
-              v-if="author"
-              :link-href="author.path"
-              :img-src="author.avatar_url"
-              :img-alt="author.name"
-              :img-size="40"
-            />
-          </div>
-          <div class="timeline-content w-100">
-            <note-header
-              :author="author"
-              :created-at="firstNote.created_at"
-              :note-id="firstNote.id"
-              :include-toggle="true"
-              :expanded="discussion.expanded"
-              @toggleHandler="toggleDiscussionHandler"
-            >
-              <span v-html="actionText"></span>
-            </note-header>
-            <note-edited-text
-              v-if="discussion.resolved"
-              :edited-at="discussion.resolved_at"
-              :edited-by="discussion.resolved_by"
-              :action-text="resolvedText"
-              class-name="discussion-headline-light js-discussion-headline"
-            />
-            <note-edited-text
-              v-else-if="lastUpdatedAt"
-              :edited-at="lastUpdatedAt"
-              :edited-by="lastUpdatedBy"
-              action-text="Last updated"
-              class-name="discussion-headline-light js-discussion-headline"
-            />
-          </div>
-        </div>
+        <diff-discussion-header v-if="shouldRenderDiffs" :discussion="discussion" />
         <div v-if="!shouldHideDiscussionBody" class="discussion-body">
           <component
             :is="wrapperComponent"

locale/gitlab.pot

@@ -10660,10 +10660,10 @@ msgstr ""
 msgid "MergeRequests|started a thread on %{linkStart}the diff%{linkEnd}"
 msgstr ""
 
-msgid "MergeRequests|started a thread on an outdated change in commit %{linkStart}%{commitId}%{linkEnd}"
+msgid "MergeRequests|started a thread on an outdated change in commit %{linkStart}%{commitDisplay}%{linkEnd}"
 msgstr ""
 
-msgid "MergeRequests|started a thread on commit %{linkStart}%{commitId}%{linkEnd}"
+msgid "MergeRequests|started a thread on commit %{linkStart}%{commitDisplay}%{linkEnd}"
 msgstr ""
 
 msgid "MergeRequest| %{paragraphStart}changed the description %{descriptionChangedTimes} times %{timeDifferenceMinutes}%{paragraphEnd}"

spec/frontend/notes/components/diff_discussion_header_spec.js

+import { mount, createLocalVue } from '@vue/test-utils';
+
+import createStore from '~/notes/stores';
+import diffDiscussionHeader from '~/notes/components/diff_discussion_header.vue';
+
+import { discussionMock } from '../../../javascripts/notes/mock_data';
+import mockDiffFile from '../../diffs/mock_data/diff_discussions';
+
+const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json';
+
+describe('diff_discussion_header component', () => {
+  let store;
+  let wrapper;
+
+  preloadFixtures(discussionWithTwoUnresolvedNotes);
+
+  beforeEach(() => {
+    window.mrTabs = {};
+    store = createStore();
+
+    const localVue = createLocalVue();
+    wrapper = mount(diffDiscussionHeader, {
+      store,
+      propsData: { discussion: discussionMock },
+      localVue,
+      sync: false,
+    });
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  it('should render user avatar', () => {
+    const discussion = { ...discussionMock };
+    discussion.diff_file = mockDiffFile;
+    discussion.diff_discussion = true;
+
+    wrapper.setProps({ discussion });
+
+    expect(wrapper.find('.user-avatar-link').exists()).toBe(true);
+  });
+
+  describe('action text', () => {
+    const commitId = 'razupaltuff';
+    const truncatedCommitId = commitId.substr(0, 8);
+    let commitElement;
+
+    beforeEach(done => {
+      store.state.diffs = {
+        projectPath: 'something',
+      };
+
+      wrapper.setProps({
+        discussion: {
+          ...discussionMock,
+          for_commit: true,
+          commit_id: commitId,
+          diff_discussion: true,
+          diff_file: {
+            ...mockDiffFile,
+          },
+        },
+      });
+
+      wrapper.vm
+        .$nextTick()
+        .then(() => {
+          commitElement = wrapper.find('.commit-sha');
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    describe('for diff threads without a commit id', () => {
+      it('should show started a thread on the diff text', done => {
+        Object.assign(wrapper.vm.discussion, {
+          for_commit: false,
+          commit_id: null,
+        });
+
+        wrapper.vm.$nextTick(() => {
+          expect(wrapper.text()).toContain('started a thread on the diff');
+
+          done();
+        });
+      });
+
+      it('should show thread on older version text', done => {
+        Object.assign(wrapper.vm.discussion, {
+          for_commit: false,
+          commit_id: null,
+          active: false,
+        });
+
+        wrapper.vm.$nextTick(() => {
+          expect(wrapper.text()).toContain('started a thread on an old version of the diff');
+
+          done();
+        });
+      });
+    });
+
+    describe('for commit threads', () => {
+      it('should display a monospace started a thread on commit', () => {
+        expect(wrapper.text()).toContain(`started a thread on commit ${truncatedCommitId}`);
+        expect(commitElement.exists()).toBe(true);
+        expect(commitElement.text()).toContain(truncatedCommitId);
+      });
+    });
+
+    describe('for diff thread with a commit id', () => {
+      it('should display started thread on commit header', done => {
+        wrapper.vm.discussion.for_commit = false;
+
+        wrapper.vm.$nextTick(() => {
+          expect(wrapper.text()).toContain(`started a thread on commit ${truncatedCommitId}`);
+
+          expect(commitElement).not.toBe(null);
+
+          done();
+        });
+      });
+
+      it('should display outdated change on commit header', done => {
+        wrapper.vm.discussion.for_commit = false;
+        wrapper.vm.discussion.active = false;
+
+        wrapper.vm.$nextTick(() => {
+          expect(wrapper.text()).toContain(
+            `started a thread on an outdated change in commit ${truncatedCommitId}`,
+          );
+
+          expect(commitElement).not.toBe(null);
+
+          done();
+        });
+      });
+    });
+  });
+});

spec/javascripts/notes/components/noteable_discussion_spec.js

-import { shallowMount, createLocalVue } from '@vue/test-utils';
+import { mount, createLocalVue } from '@vue/test-utils';
 import createStore from '~/notes/stores';
 import noteableDiscussion from '~/notes/components/noteable_discussion.vue';
 import ReplyPlaceholder from '~/notes/components/discussion_reply_placeholder.vue';
@@ -23,7 +23,7 @@ describe('noteable_discussion component', () => {
     store.dispatch('setNotesData', notesDataMock);
 
     const localVue = createLocalVue();
-    wrapper = shallowMount(noteableDiscussion, {
+    wrapper = mount(noteableDiscussion, {
       store,
       propsData: { discussion: discussionMock },
       localVue,
@@ -35,16 +35,6 @@ describe('noteable_discussion component', () => {
     wrapper.destroy();
   });
 
-  it('should render user avatar', () => {
-    const discussion = { ...discussionMock };
-    discussion.diff_file = mockDiffFile;
-    discussion.diff_discussion = true;
-
-    wrapper.setProps({ discussion, renderDiffFile: true });
-
-    expect(wrapper.find('.user-avatar-link').exists()).toBe(true);
-  });
-
   it('should not render thread header for non diff threads', () => {
     expect(wrapper.find('.discussion-header').exists()).toBe(false);
   });
@@ -134,105 +124,6 @@ describe('noteable_discussion component', () => {
     });
   });
 
-  describe('action text', () => {
-    const commitId = 'razupaltuff';
-    const truncatedCommitId = commitId.substr(0, 8);
-    let commitElement;
-
-    beforeEach(done => {
-      store.state.diffs = {
-        projectPath: 'something',
-      };
-
-      wrapper.setProps({
-        discussion: {
-          ...discussionMock,
-          for_commit: true,
-          commit_id: commitId,
-          diff_discussion: true,
-          diff_file: {
-            ...mockDiffFile,
-          },
-        },
-        renderDiffFile: true,
-      });
-
-      wrapper.vm
-        .$nextTick()
-        .then(() => {
-          commitElement = wrapper.find('.commit-sha');
-        })
-        .then(done)
-        .catch(done.fail);
-    });
-
-    describe('for commit threads', () => {
-      it('should display a monospace started a thread on commit', () => {
-        expect(wrapper.text()).toContain(`started a thread on commit ${truncatedCommitId}`);
-        expect(commitElement.exists()).toBe(true);
-        expect(commitElement.text()).toContain(truncatedCommitId);
-      });
-    });
-
-    describe('for diff thread with a commit id', () => {
-      it('should display started thread on commit header', done => {
-        wrapper.vm.discussion.for_commit = false;
-
-        wrapper.vm.$nextTick(() => {
-          expect(wrapper.text()).toContain(`started a thread on commit ${truncatedCommitId}`);
-
-          expect(commitElement).not.toBe(null);
-
-          done();
-        });
-      });
-
-      it('should display outdated change on commit header', done => {
-        wrapper.vm.discussion.for_commit = false;
-        wrapper.vm.discussion.active = false;
-
-        wrapper.vm.$nextTick(() => {
-          expect(wrapper.text()).toContain(
-            `started a thread on an outdated change in commit ${truncatedCommitId}`,
-          );
-
-          expect(commitElement).not.toBe(null);
-
-          done();
-        });
-      });
-    });
-
-    describe('for diff threads without a commit id', () => {
-      it('should show started a thread on the diff text', done => {
-        Object.assign(wrapper.vm.discussion, {
-          for_commit: false,
-          commit_id: null,
-        });
-
-        wrapper.vm.$nextTick(() => {
-          expect(wrapper.text()).toContain('started a thread on the diff');
-
-          done();
-        });
-      });
-
-      it('should show thread on older version text', done => {
-        Object.assign(wrapper.vm.discussion, {
-          for_commit: false,
-          commit_id: null,
-          active: false,
-        });
-
-        wrapper.vm.$nextTick(() => {
-          expect(wrapper.text()).toContain('started a thread on an old version of the diff');
-
-          done();
-        });
-      });
-    });
-  });
-
   describe('for resolved thread', () => {
     beforeEach(() => {
       const discussion = getJSONFixture(discussionWithTwoUnresolvedNotes)[0];
@@ -262,6 +153,7 @@ describe('noteable_discussion component', () => {
       }));
 
       wrapper.setProps({ discussion });
+
       wrapper.vm
         .$nextTick()
         .then(done)