From 98a2471279a0ab203685a66f4c8c2fdaa18eca9b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 7 Oct 2024 12:03:33 -0600 Subject: [PATCH 01/15] Convert HAML "reviewers" MR List region to Vue list component --- .../issuable/components/issue_reviewers.vue | 108 ++++++++++++++++++ .../queries/merge_request.fragment.graphql | 5 + .../list/components/issuable_item.vue | 20 +++- .../javascripts/work_items/constants.js | 1 + app/assets/javascripts/work_items/utils.js | 3 + 5 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 app/assets/javascripts/issuable/components/issue_reviewers.vue diff --git a/app/assets/javascripts/issuable/components/issue_reviewers.vue b/app/assets/javascripts/issuable/components/issue_reviewers.vue new file mode 100644 index 00000000000000..172a98af47f6b4 --- /dev/null +++ b/app/assets/javascripts/issuable/components/issue_reviewers.vue @@ -0,0 +1,108 @@ + + diff --git a/app/assets/javascripts/merge_requests/list/queries/merge_request.fragment.graphql b/app/assets/javascripts/merge_requests/list/queries/merge_request.fragment.graphql index d6ac8bd0fdc60e..6c4375f1e0abb9 100644 --- a/app/assets/javascripts/merge_requests/list/queries/merge_request.fragment.graphql +++ b/app/assets/javascripts/merge_requests/list/queries/merge_request.fragment.graphql @@ -17,6 +17,11 @@ fragment MergeRequestFragment on MergeRequest { ...User } } + reviewers @skip(if: $hideUsers) { + nodes { + ...User + } + } author @skip(if: $hideUsers) { ...User } diff --git a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue index 32807ebdf8aa50..676c1224e926cd 100644 --- a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue +++ b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue @@ -15,12 +15,13 @@ import { isScopedLabel } from '~/lib/utils/common_utils'; import { isExternal, setUrlFragment, visitUrl } from '~/lib/utils/url_utility'; import { __, n__, sprintf } from '~/locale'; import IssuableAssignees from '~/issuable/components/issue_assignees.vue'; +import IssuableReviewers from '~/issuable/components/issue_reviewers.vue'; import timeagoMixin from '~/vue_shared/mixins/timeago'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import WorkItemTypeIcon from '~/work_items/components/work_item_type_icon.vue'; import WorkItemPrefetch from '~/work_items/components/work_item_prefetch.vue'; import { STATE_OPEN, STATE_CLOSED } from '~/work_items/constants'; -import { isAssigneesWidget, isLabelsWidget } from '~/work_items/utils'; +import { isAssigneesWidget, isReviewersWidget, isLabelsWidget } from '~/work_items/utils'; export default { components: { @@ -31,6 +32,7 @@ export default { GlFormCheckbox, GlSprintf, IssuableAssignees, + IssuableReviewers, WorkItemTypeIcon, WorkItemPrefetch, }, @@ -197,6 +199,14 @@ export default { notesCount() { return this.issuable.userDiscussionsCount ?? this.issuable.userNotesCount; }, + reviewers() { + return ( + this.issuable.reviewers?.nodes || + this.issuable.reviewers || + this.issuable.widgets?.find(isReviewersWidget)?.reviewers.nodes || + [] + ); + }, showDiscussions() { return typeof this.notesCount === 'number'; }, @@ -479,6 +489,14 @@ export default { +
  • + +
  • widget.type === WIDGET_TYPE_ASSIGNEES; +export const isReviewersWidget = (widget) => widget.type === WIDGET_TYPE_REVIEWERS; + export const isHealthStatusWidget = (widget) => widget.type === WIDGET_TYPE_HEALTH_STATUS; export const isLabelsWidget = (widget) => widget.type === WIDGET_TYPE_LABELS; -- GitLab From 19c28e5166752fdde2c2623f8e6570eb678c594d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 7 Oct 2024 12:18:34 -0600 Subject: [PATCH 02/15] Issues can never have reviewers, so make them MR-specific --- .../{issue_reviewers.vue => merge_request_reviewers.vue} | 0 .../vue_shared/issuable/list/components/issuable_item.vue | 6 +++--- 2 files changed, 3 insertions(+), 3 deletions(-) rename app/assets/javascripts/issuable/components/{issue_reviewers.vue => merge_request_reviewers.vue} (100%) diff --git a/app/assets/javascripts/issuable/components/issue_reviewers.vue b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue similarity index 100% rename from app/assets/javascripts/issuable/components/issue_reviewers.vue rename to app/assets/javascripts/issuable/components/merge_request_reviewers.vue diff --git a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue index 676c1224e926cd..cd6a493e69e27d 100644 --- a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue +++ b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue @@ -15,7 +15,7 @@ import { isScopedLabel } from '~/lib/utils/common_utils'; import { isExternal, setUrlFragment, visitUrl } from '~/lib/utils/url_utility'; import { __, n__, sprintf } from '~/locale'; import IssuableAssignees from '~/issuable/components/issue_assignees.vue'; -import IssuableReviewers from '~/issuable/components/issue_reviewers.vue'; +import MergeRequestReviewers from '~/issuable/components/merge_request_reviewers.vue'; import timeagoMixin from '~/vue_shared/mixins/timeago'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import WorkItemTypeIcon from '~/work_items/components/work_item_type_icon.vue'; @@ -32,7 +32,7 @@ export default { GlFormCheckbox, GlSprintf, IssuableAssignees, - IssuableReviewers, + MergeRequestReviewers, WorkItemTypeIcon, WorkItemPrefetch, }, @@ -490,7 +490,7 @@ export default {
  • - Date: Mon, 7 Oct 2024 18:48:07 -0600 Subject: [PATCH 03/15] Update localizations --- locale/gitlab.pot | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index be8f71954fc43b..3b90b87afabee7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -737,6 +737,9 @@ msgid_plural "%{count} more releases" msgstr[0] "" msgstr[1] "" +msgid "%{count} more reviewers" +msgstr "" + msgid "%{count} of %{required} approvals from %{name}" msgstr "" @@ -46436,6 +46439,9 @@ msgstr "" msgid "Review changes" msgstr "" +msgid "Review requested from %{reviewerName}" +msgstr "" + msgid "Review requests" msgstr "" -- GitLab From e10eeefa23c574f9cfa7d8e4c5487ada552c8688 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 7 Oct 2024 19:08:31 -0600 Subject: [PATCH 04/15] Correct missed "Assignee" label --- .../issuable/components/merge_request_reviewers.vue | 2 +- locale/gitlab.pot | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue index 172a98af47f6b4..cba5576833a201 100644 --- a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue +++ b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue @@ -92,7 +92,7 @@ export default { data-testid="reviewer-link" > - {{ s__('Label|Assignee') }} {{ reviewer.name }} + {{ s__('Label|Reviewer') }} {{ reviewer.name }} @{{ reviewer.username }} diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3b90b87afabee7..e0909aa2016b33 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -31767,6 +31767,9 @@ msgstr "" msgid "Label|Assignee" msgstr "" +msgid "Label|Reviewer" +msgstr "" + msgid "Lacking permissions to the blocking merge request" msgstr "" -- GitLab From 1454c7621f0baa17c2e5ee64c17c5a32e03d6a73 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 7 Oct 2024 19:44:47 -0600 Subject: [PATCH 05/15] Add tests for reviewers area of issuable list item for MR list page --- .../merge_request_reviewers_spec.js | 149 ++++++++++++++++++ .../frontend/merge_requests/list/mock_data.js | 13 ++ .../list/components/issuable_item_spec.js | 19 +++ 3 files changed, 181 insertions(+) create mode 100644 spec/frontend/issuable/components/merge_request_reviewers_spec.js diff --git a/spec/frontend/issuable/components/merge_request_reviewers_spec.js b/spec/frontend/issuable/components/merge_request_reviewers_spec.js new file mode 100644 index 00000000000000..69a764372418bb --- /dev/null +++ b/spec/frontend/issuable/components/merge_request_reviewers_spec.js @@ -0,0 +1,149 @@ +import { shallowMount } from '@vue/test-utils'; +import { mockAssigneesList as mockReviewersList } from 'jest/boards/mock_data'; +import MergeRequestReviewers from '~/issuable/components/merge_request_reviewers.vue'; +import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link.vue'; + +const TEST_CSS_CLASSES = 'test-classes'; +const TEST_MAX_VISIBLE = 4; +const TEST_ICON_SIZE = 16; + +describe('MergeRequestReviewersComponent', () => { + let wrapper; + let vm; + + const factory = (props) => { + wrapper = shallowMount(MergeRequestReviewers, { + propsData: { + reviewers: mockReviewersList, + ...props, + }, + }); + vm = wrapper.vm; + }; + + const findTooltipText = () => wrapper.find('.js-reviewer-tooltip').text(); + const findAvatars = () => wrapper.findAllComponents(UserAvatarLink); + const findOverflowCounter = () => wrapper.find('.avatar-counter'); + + it('returns default data props', () => { + factory({ reviewers: mockReviewersList }); + expect(vm.iconSize).toBe(24); + expect(vm.maxVisible).toBe(3); + expect(vm.maxReviewers).toBe(99); + }); + + describe.each` + numReviewers | maxVisible | expectedShown | expectedHidden + ${0} | ${3} | ${0} | ${''} + ${1} | ${3} | ${1} | ${''} + ${2} | ${3} | ${2} | ${''} + ${3} | ${3} | ${3} | ${''} + ${4} | ${3} | ${2} | ${'+2'} + ${5} | ${2} | ${1} | ${'+4'} + ${1000} | ${5} | ${4} | ${'99+'} + `( + 'with reviewers ($numReviewers) and maxVisible ($maxVisible)', + ({ numReviewers, maxVisible, expectedShown, expectedHidden }) => { + beforeEach(() => { + factory({ reviewers: Array(numReviewers).fill({}), maxVisible }); + }); + + if (expectedShown) { + it('shows reviewer avatars', () => { + expect(findAvatars().length).toEqual(expectedShown); + }); + } else { + it('does not show reviewer avatars', () => { + expect(findAvatars().length).toEqual(0); + }); + } + + if (expectedHidden) { + it('shows overflow counter', () => { + const hiddenCount = numReviewers - expectedShown; + + expect(findOverflowCounter().exists()).toBe(true); + expect(findOverflowCounter().text()).toEqual(expectedHidden.toString()); + expect(findOverflowCounter().attributes('title')).toEqual( + `${hiddenCount} more reviewers`, + ); + }); + } else { + it('does not show overflow counter', () => { + expect(findOverflowCounter().exists()).toBe(false); + }); + } + }, + ); + + describe('when mounted', () => { + beforeEach(() => { + factory({ + imgCssClasses: TEST_CSS_CLASSES, + maxVisible: TEST_MAX_VISIBLE, + iconSize: TEST_ICON_SIZE, + }); + }); + + it('computes alt text for reviewer avatar', () => { + expect(vm.avatarUrlTitle(mockReviewersList[0])).toBe('Review requested from Terrell Graham'); + }); + + it('renders reviewer', () => { + const data = findAvatars().wrappers.map((x) => ({ + ...x.props(), + })); + + const expected = mockReviewersList.slice(0, TEST_MAX_VISIBLE - 1).map((x) => + expect.objectContaining({ + imgAlt: `Review requested from ${x.name}`, + imgCssClasses: TEST_CSS_CLASSES, + imgSrc: x.avatar_url, + imgSize: TEST_ICON_SIZE, + }), + ); + + expect(data).toEqual(expected); + }); + + describe('reviewer tooltips', () => { + it('renders "Reviewer" header', () => { + expect(findTooltipText()).toContain('Reviewer'); + }); + + it('renders reviewer name', () => { + expect(findTooltipText()).toContain('Terrell Graham'); + }); + + it('renders reviewer @username', () => { + expect(findTooltipText()).toContain('@monserrate.gleichner'); + }); + + it('does not render `@` when username not available', () => { + const userName = 'User without username'; + factory({ + reviewers: [ + { + name: userName, + }, + ], + }); + + const tooltipText = findTooltipText(); + + expect(tooltipText).toContain(userName); + expect(tooltipText).not.toContain('@'); + }); + }); + describe('Author Link', () => { + it('properly sets href on each reviewer', () => { + const template = findAvatars().wrappers.map((x) => x.props('linkHref')); + const expected = mockReviewersList + .slice(0, TEST_MAX_VISIBLE - 1) + .map((x) => `/${x.username}`); + + expect(template).toEqual(expected); + }); + }); + }); +}); diff --git a/spec/frontend/merge_requests/list/mock_data.js b/spec/frontend/merge_requests/list/mock_data.js index ac6184c4758b69..9cd4fa7437bb65 100644 --- a/spec/frontend/merge_requests/list/mock_data.js +++ b/spec/frontend/merge_requests/list/mock_data.js @@ -37,6 +37,19 @@ export const getQueryResponse = { }, ], }, + reviewers: { + nodes: [ + { + __typename: 'UserCore', + id: 'gid://gitlab/User/789', + avatarUrl: 'avatar/url', + name: 'Bart Simpson', + username: 'bsimpson', + webUrl: 'url/bsimpson', + webPath: '/bsimpson', + }, + ], + }, author: { __typename: 'UserCore', id: 'gid://gitlab/User/456', diff --git a/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js b/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js index 42c09388aa5906..e7c4679780cd0e 100644 --- a/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js +++ b/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js @@ -6,6 +6,7 @@ import { shallowMountExtended as shallowMount } from 'helpers/vue_test_utils_hel import IssuableItem from '~/vue_shared/issuable/list/components/issuable_item.vue'; import WorkItemTypeIcon from '~/work_items/components/work_item_type_icon.vue'; import IssuableAssignees from '~/issuable/components/issue_assignees.vue'; +import MergeRequestReviewers from '~/issuable/components/merge_request_reviewers.vue'; import { localeDateFormat } from '~/lib/utils/datetime/locale_dateformat'; import { mockIssuable, mockRegularLabel } from '../mock_data'; @@ -576,6 +577,24 @@ describe('IssuableItem', () => { }); }); + it('renders merge-request-reviewers component', () => { + const mr = { + ...mockIssuable, + reviewers: Array(5).fill(mockAuthor), + }; + + wrapper = createComponent({ issuable: mr }); + + const reviewersEl = wrapper.findComponent(MergeRequestReviewers); + + expect(reviewersEl.exists()).toBe(true); + expect(reviewersEl.props()).toMatchObject({ + reviewers: mr.reviewers, + iconSize: 16, + maxVisible: 4, + }); + }); + it('renders issuable updatedAt info', () => { wrapper = createComponent(); -- GitLab From e8c64bbedf65cecbec3617e135979147b51ac262 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Oct 2024 09:37:08 -0600 Subject: [PATCH 06/15] Move reviewers to be after assignees --- .../issuable/list/components/issuable_item.vue | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue index cd6a493e69e27d..7516cfe919badb 100644 --- a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue +++ b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue @@ -489,17 +489,17 @@ export default {
    • -
  • - +
    • -
  • - + Date: Wed, 9 Oct 2024 10:58:42 -0600 Subject: [PATCH 07/15] Remove work item / issuable setup from reviewers --- .../issuable/components/merge_request_reviewers.vue | 2 +- .../issuable/list/components/issuable_item.vue | 9 ++------- app/assets/javascripts/work_items/constants.js | 1 - app/assets/javascripts/work_items/utils.js | 3 --- 4 files changed, 3 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue index cba5576833a201..32897758c082e5 100644 --- a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue +++ b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue @@ -87,7 +87,7 @@ export default { img-css-wrapper-classes="gl-inline-flex" :img-src="avatarUrl(reviewer)" :img-size="iconSize" - class="js-no-trigger author-link" + class="author-link" tooltip-placement="bottom" data-testid="reviewer-link" > diff --git a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue index 7516cfe919badb..0bc1086e13df8a 100644 --- a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue +++ b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue @@ -21,7 +21,7 @@ import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import WorkItemTypeIcon from '~/work_items/components/work_item_type_icon.vue'; import WorkItemPrefetch from '~/work_items/components/work_item_prefetch.vue'; import { STATE_OPEN, STATE_CLOSED } from '~/work_items/constants'; -import { isAssigneesWidget, isReviewersWidget, isLabelsWidget } from '~/work_items/utils'; +import { isAssigneesWidget, isLabelsWidget } from '~/work_items/utils'; export default { components: { @@ -200,12 +200,7 @@ export default { return this.issuable.userDiscussionsCount ?? this.issuable.userNotesCount; }, reviewers() { - return ( - this.issuable.reviewers?.nodes || - this.issuable.reviewers || - this.issuable.widgets?.find(isReviewersWidget)?.reviewers.nodes || - [] - ); + return this.issuable.reviewers?.nodes || []; }, showDiscussions() { return typeof this.notesCount === 'number'; diff --git a/app/assets/javascripts/work_items/constants.js b/app/assets/javascripts/work_items/constants.js index 4e788c9adeb650..e33260728ae425 100644 --- a/app/assets/javascripts/work_items/constants.js +++ b/app/assets/javascripts/work_items/constants.js @@ -10,7 +10,6 @@ export const STATE_EVENT_CLOSE = 'CLOSE'; export const TRACKING_CATEGORY_SHOW = 'workItems:show'; export const WIDGET_TYPE_ASSIGNEES = 'ASSIGNEES'; -export const WIDGET_TYPE_REVIEWERS = 'REVIEWERS'; export const WIDGET_TYPE_DESCRIPTION = 'DESCRIPTION'; export const WIDGET_TYPE_AWARD_EMOJI = 'AWARD_EMOJI'; export const WIDGET_TYPE_NOTIFICATIONS = 'NOTIFICATIONS'; diff --git a/app/assets/javascripts/work_items/utils.js b/app/assets/javascripts/work_items/utils.js index bb2549d176b530..dc85419251b55e 100644 --- a/app/assets/javascripts/work_items/utils.js +++ b/app/assets/javascripts/work_items/utils.js @@ -13,7 +13,6 @@ import { WIDGET_TYPE_LABELS, WIDGET_TYPE_MILESTONE, WIDGET_TYPE_NOTES, - WIDGET_TYPE_REVIEWERS, WIDGET_TYPE_START_AND_DUE_DATE, WIDGET_TYPE_WEIGHT, WIDGET_TYPE_AWARD_EMOJI, @@ -34,8 +33,6 @@ import { export const isAssigneesWidget = (widget) => widget.type === WIDGET_TYPE_ASSIGNEES; -export const isReviewersWidget = (widget) => widget.type === WIDGET_TYPE_REVIEWERS; - export const isHealthStatusWidget = (widget) => widget.type === WIDGET_TYPE_HEALTH_STATUS; export const isLabelsWidget = (widget) => widget.type === WIDGET_TYPE_LABELS; -- GitLab From 424d7f844f190af4195cf173fe9b571ba141144b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Oct 2024 11:28:15 -0600 Subject: [PATCH 08/15] Remove unused QA id --- .../javascripts/issuable/components/merge_request_reviewers.vue | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue index 32897758c082e5..e3424f732b30cc 100644 --- a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue +++ b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue @@ -89,7 +89,6 @@ export default { :img-size="iconSize" class="author-link" tooltip-placement="bottom" - data-testid="reviewer-link" > {{ s__('Label|Reviewer') }} {{ reviewer.name }} -- GitLab From 34b46ff2db7f537c8d895365f647e76d04c4b5be Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Oct 2024 14:39:00 -0600 Subject: [PATCH 09/15] Remove class making username unreadable in Dark mode --- .../javascripts/issuable/components/merge_request_reviewers.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue index e3424f732b30cc..1e009a28bbe7ee 100644 --- a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue +++ b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue @@ -92,7 +92,7 @@ export default { > {{ s__('Label|Reviewer') }} {{ reviewer.name }} - @{{ reviewer.username }} + @{{ reviewer.username }} Date: Thu, 10 Oct 2024 11:30:40 -0600 Subject: [PATCH 10/15] Make the test data match the structure of the live data --- .../issuable/list/components/issuable_item_spec.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js b/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js index e7c4679780cd0e..5a6cfa419ab202 100644 --- a/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js +++ b/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js @@ -580,7 +580,9 @@ describe('IssuableItem', () => { it('renders merge-request-reviewers component', () => { const mr = { ...mockIssuable, - reviewers: Array(5).fill(mockAuthor), + reviewers: { + nodes: Array(5).fill(mockAuthor), + }, }; wrapper = createComponent({ issuable: mr }); @@ -589,7 +591,7 @@ describe('IssuableItem', () => { expect(reviewersEl.exists()).toBe(true); expect(reviewersEl.props()).toMatchObject({ - reviewers: mr.reviewers, + reviewers: mr.reviewers.nodes, iconSize: 16, maxVisible: 4, }); -- GitLab From af05258325c14e03f49b5ab4e85e006a18029fb2 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Oct 2024 10:16:07 -0600 Subject: [PATCH 11/15] Remove unnecessary wrapper methods --- .../issuable/components/merge_request_reviewers.vue | 13 ++----------- .../components/merge_request_reviewers_spec.js | 13 ++++++++++++- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue index 1e009a28bbe7ee..4a28f1ff024827 100644 --- a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue +++ b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue @@ -64,15 +64,6 @@ export default { reviewerName: reviewer.name, }); }, - // This method is for backward compat - // since Graph query would return camelCase - // props while Rails would return snake_case - reviewerHref(reviewer) { - return reviewer.web_path || reviewer.webPath; - }, - avatarUrl(reviewer) { - return reviewer.avatar_url || reviewer.avatarUrl; - }, }, }; @@ -81,11 +72,11 @@ export default { { + let normalizedReviewer = { ...reviewer }; + + normalizedReviewer.avatarUrl = reviewer.avatar_url; + delete normalizedReviewer.avatar_url; + + return normalizedReviewer; + }); +} + describe('MergeRequestReviewersComponent', () => { let wrapper; let vm; @@ -14,7 +25,7 @@ describe('MergeRequestReviewersComponent', () => { const factory = (props) => { wrapper = shallowMount(MergeRequestReviewers, { propsData: { - reviewers: mockReviewersList, + reviewers: normalizeMockReviewers(mockReviewersList), ...props, }, }); -- GitLab From ef6caeb3530d02945e88d5088e675ec1b0122ccf Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Oct 2024 10:48:41 -0600 Subject: [PATCH 12/15] Switch to using a data-testid instead of a CSS class --- .../javascripts/issuable/components/merge_request_reviewers.vue | 2 +- .../issuable/components/merge_request_reviewers_spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue index 4a28f1ff024827..c6b6b38694afd5 100644 --- a/app/assets/javascripts/issuable/components/merge_request_reviewers.vue +++ b/app/assets/javascripts/issuable/components/merge_request_reviewers.vue @@ -81,7 +81,7 @@ export default { class="author-link" tooltip-placement="bottom" > - + {{ s__('Label|Reviewer') }} {{ reviewer.name }} @{{ reviewer.username }} diff --git a/spec/frontend/issuable/components/merge_request_reviewers_spec.js b/spec/frontend/issuable/components/merge_request_reviewers_spec.js index cfbd06abd9e193..3bd557a49d4cdc 100644 --- a/spec/frontend/issuable/components/merge_request_reviewers_spec.js +++ b/spec/frontend/issuable/components/merge_request_reviewers_spec.js @@ -32,7 +32,7 @@ describe('MergeRequestReviewersComponent', () => { vm = wrapper.vm; }; - const findTooltipText = () => wrapper.find('.js-reviewer-tooltip').text(); + const findTooltipText = () => wrapper.find('[data-testid=js-reviewer-tooltip]').text(); const findAvatars = () => wrapper.findAllComponents(UserAvatarLink); const findOverflowCounter = () => wrapper.find('.avatar-counter'); -- GitLab From 013537e54a2eac5fde3ce486c59cd5cf883afb3f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Oct 2024 20:45:09 -0600 Subject: [PATCH 13/15] Switch to passing reviewers into a slot --- .../list/components/merge_requests_list_app.vue | 16 ++++++++++++++++ .../issuable/list/components/issuable_item.vue | 15 ++------------- .../list/components/issuable_list_root.vue | 3 +++ 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 3a7238b799ee50..905b2c4f431d03 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -70,6 +70,7 @@ import { urlSortParams, } from '~/issues/list/constants'; import CiIcon from '~/vue_shared/components/ci_icon/ci_icon.vue'; +import MergeRequestReviewers from '~/issuable/components/merge_request_reviewers.vue'; import setSortPreferenceMutation from '~/issues/list/queries/set_sort_preference.mutation.graphql'; import issuableEventHub from '~/issues/list/eventhub'; import { AutocompleteCache } from '../../utils/autocomplete_cache'; @@ -110,6 +111,7 @@ export default { CiIcon, MergeRequestStatistics, MergeRequestMoreActionsDropdown, + MergeRequestReviewers, ApprovalCount, EmptyState, IssuableMilestone, @@ -571,6 +573,9 @@ export default { } return undefined; }, + getReviewers(issuable) { + return issuable.reviewers?.nodes || []; + }, handleClickTab(state) { if (this.state === state) { return; @@ -799,6 +804,17 @@ export default {
    • + + diff --git a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue index 0bc1086e13df8a..d47e97b82e430f 100644 --- a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue +++ b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue @@ -15,7 +15,7 @@ import { isScopedLabel } from '~/lib/utils/common_utils'; import { isExternal, setUrlFragment, visitUrl } from '~/lib/utils/url_utility'; import { __, n__, sprintf } from '~/locale'; import IssuableAssignees from '~/issuable/components/issue_assignees.vue'; -import MergeRequestReviewers from '~/issuable/components/merge_request_reviewers.vue'; + import timeagoMixin from '~/vue_shared/mixins/timeago'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import WorkItemTypeIcon from '~/work_items/components/work_item_type_icon.vue'; @@ -32,7 +32,6 @@ export default { GlFormCheckbox, GlSprintf, IssuableAssignees, - MergeRequestReviewers, WorkItemTypeIcon, WorkItemPrefetch, }, @@ -199,9 +198,6 @@ export default { notesCount() { return this.issuable.userDiscussionsCount ?? this.issuable.userNotesCount; }, - reviewers() { - return this.issuable.reviewers?.nodes || []; - }, showDiscussions() { return typeof this.notesCount === 'number'; }, @@ -492,14 +488,7 @@ export default { class="gl-flex gl-items-center" /> -
  • - -
    • +
  • + -- GitLab From 8cef75b8e527bb511e4de46fed0693049d2311cd Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Oct 2024 22:40:21 -0600 Subject: [PATCH 14/15] Use const instead of let for a mutated value --- .../issuable/components/merge_request_reviewers_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/frontend/issuable/components/merge_request_reviewers_spec.js b/spec/frontend/issuable/components/merge_request_reviewers_spec.js index 3bd557a49d4cdc..dd6db6aa8a73f1 100644 --- a/spec/frontend/issuable/components/merge_request_reviewers_spec.js +++ b/spec/frontend/issuable/components/merge_request_reviewers_spec.js @@ -9,7 +9,7 @@ const TEST_ICON_SIZE = 16; function normalizeMockReviewers(data) { return data.map((reviewer) => { - let normalizedReviewer = { ...reviewer }; + const normalizedReviewer = { ...reviewer }; normalizedReviewer.avatarUrl = reviewer.avatar_url; delete normalizedReviewer.avatar_url; -- GitLab From c3273838c16c68173d87b26423a76d1bfae8e9af Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Oct 2024 23:55:54 -0600 Subject: [PATCH 15/15] Move the test for the MR reviewers component into the list app spec --- .../merge_requests_list_app_spec.js | 16 ++++++++++++++ .../list/components/issuable_item_spec.js | 21 ------------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index 1ce17e781372d8..cb81bf78ee85e2 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -43,6 +43,7 @@ import { BRANCH_LIST_REFRESH_INTERVAL } from '~/merge_requests/list/constants'; import getMergeRequestsQuery from '~/merge_requests/list/queries/get_merge_requests.query.graphql'; import getMergeRequestsCountQuery from '~/merge_requests/list/queries/get_merge_requests_counts.query.graphql'; import IssuableList from '~/vue_shared/issuable/list/components/issuable_list_root.vue'; +import MergeRequestReviewers from '~/issuable/components/merge_request_reviewers.vue'; import issuableEventHub from '~/issues/list/eventhub'; Vue.use(VueApollo); @@ -505,6 +506,21 @@ describe('Merge requests list app', () => { ); }); + it('renders merge-request-reviewers component', async () => { + createComponent({ mountFn: mountExtended }); + + await waitForPromises(); + + const reviewersEl = wrapper.findComponent(MergeRequestReviewers); + + expect(reviewersEl.exists()).toBe(true); + expect(reviewersEl.props()).toMatchObject({ + reviewers: getQueryResponse.data.project.mergeRequests.nodes[0].reviewers.nodes, + iconSize: 16, + maxVisible: 4, + }); + }); + describe('bulk edit', () => { it('renders when user has permissions', () => { createComponent({ provide: { canBulkUpdate: true }, mountFn: mountExtended }); diff --git a/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js b/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js index 5a6cfa419ab202..42c09388aa5906 100644 --- a/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js +++ b/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js @@ -6,7 +6,6 @@ import { shallowMountExtended as shallowMount } from 'helpers/vue_test_utils_hel import IssuableItem from '~/vue_shared/issuable/list/components/issuable_item.vue'; import WorkItemTypeIcon from '~/work_items/components/work_item_type_icon.vue'; import IssuableAssignees from '~/issuable/components/issue_assignees.vue'; -import MergeRequestReviewers from '~/issuable/components/merge_request_reviewers.vue'; import { localeDateFormat } from '~/lib/utils/datetime/locale_dateformat'; import { mockIssuable, mockRegularLabel } from '../mock_data'; @@ -577,26 +576,6 @@ describe('IssuableItem', () => { }); }); - it('renders merge-request-reviewers component', () => { - const mr = { - ...mockIssuable, - reviewers: { - nodes: Array(5).fill(mockAuthor), - }, - }; - - wrapper = createComponent({ issuable: mr }); - - const reviewersEl = wrapper.findComponent(MergeRequestReviewers); - - expect(reviewersEl.exists()).toBe(true); - expect(reviewersEl.props()).toMatchObject({ - reviewers: mr.reviewers.nodes, - iconSize: 16, - maxVisible: 4, - }); - }); - it('renders issuable updatedAt info', () => { wrapper = createComponent(); -- GitLab