From fd589be32f02c160812bbce0f5fb2178fb7142d3 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Tue, 20 Aug 2019 15:14:39 -0500 Subject: Add CE changelog for assignee style change --- changelogs/unreleased/ce-22058-improve-ux-multi-assignees-in-mr.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/ce-22058-improve-ux-multi-assignees-in-mr.yml diff --git a/changelogs/unreleased/ce-22058-improve-ux-multi-assignees-in-mr.yml b/changelogs/unreleased/ce-22058-improve-ux-multi-assignees-in-mr.yml new file mode 100644 index 00000000000..e7453a2b9bd --- /dev/null +++ b/changelogs/unreleased/ce-22058-improve-ux-multi-assignees-in-mr.yml @@ -0,0 +1,5 @@ +--- +title: Update assignee (cannot merge) style +merge_request: 31545 +author: +type: changed -- cgit v1.2.1 From f1f34baf6f4e0866feb3f4c7523e3dccf5784b0b Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Thu, 25 Jul 2019 19:30:50 -0700 Subject: Improve UX multi assigness in MR Add merge warning on avatar in: - open view assigness - collapsed view assigness - dropdown (search) view assigness Add can_merge option to MR sidebar entity --- .../components/assignees/assignee_avatar.vue | 48 +++++ .../components/assignees/assignee_avatar_link.vue | 74 ++++++++ .../sidebar/components/assignees/assignees.vue | 210 ++------------------- .../components/assignees/collapsed_assignee.vue | 27 +++ .../assignees/collapsed_assignee_list.vue | 121 ++++++++++++ .../assignees/uncollapsed_assignee_list.vue | 96 ++++++++++ app/assets/javascripts/users_select.js | 66 +++++-- app/assets/stylesheets/pages/issuable.scss | 28 +-- app/serializers/issuable_sidebar_basic_entity.rb | 1 + app/serializers/merge_request_serializer.rb | 2 +- .../merge_request_sidebar_basic_entity.rb | 6 + .../shared/issuable/_sidebar_assignees.html.haml | 2 + locale/gitlab.pot | 21 ++- .../assignees/assignee_avatar_link_spec.js | 84 +++++++++ .../components/assignees/assignee_avatar_spec.js | 78 ++++++++ .../assignees/collapsed_assignee_list_spec.js | 201 ++++++++++++++++++++ .../assignees/collapsed_assignee_spec.js | 34 ++++ .../assignees/uncollapsed_assignee_list_spec.js | 135 +++++++++++++ spec/frontend/sidebar/user_data_mock.js | 9 + spec/javascripts/sidebar/assignees_spec.js | 208 +++----------------- spec/javascripts/sidebar/mock_data.js | 10 + .../merge_request_sidebar_basic_entity_spec.rb | 22 +++ 22 files changed, 1077 insertions(+), 406 deletions(-) create mode 100644 app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue create mode 100644 app/assets/javascripts/sidebar/components/assignees/assignee_avatar_link.vue create mode 100644 app/assets/javascripts/sidebar/components/assignees/collapsed_assignee.vue create mode 100644 app/assets/javascripts/sidebar/components/assignees/collapsed_assignee_list.vue create mode 100644 app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue create mode 100644 app/serializers/merge_request_sidebar_basic_entity.rb create mode 100644 spec/frontend/sidebar/components/assignees/assignee_avatar_link_spec.js create mode 100644 spec/frontend/sidebar/components/assignees/assignee_avatar_spec.js create mode 100644 spec/frontend/sidebar/components/assignees/collapsed_assignee_list_spec.js create mode 100644 spec/frontend/sidebar/components/assignees/collapsed_assignee_spec.js create mode 100644 spec/frontend/sidebar/components/assignees/uncollapsed_assignee_list_spec.js create mode 100644 spec/frontend/sidebar/user_data_mock.js create mode 100644 spec/serializers/merge_request_sidebar_basic_entity_spec.rb diff --git a/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue new file mode 100644 index 00000000000..0bb4f757de8 --- /dev/null +++ b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue @@ -0,0 +1,48 @@ + + + diff --git a/app/assets/javascripts/sidebar/components/assignees/assignee_avatar_link.vue b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar_link.vue new file mode 100644 index 00000000000..6563d1d3410 --- /dev/null +++ b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar_link.vue @@ -0,0 +1,74 @@ + + + diff --git a/app/assets/javascripts/sidebar/components/assignees/assignees.vue b/app/assets/javascripts/sidebar/components/assignees/assignees.vue index 631e2e28d4d..ca4090eee96 100644 --- a/app/assets/javascripts/sidebar/components/assignees/assignees.vue +++ b/app/assets/javascripts/sidebar/components/assignees/assignees.vue @@ -1,13 +1,14 @@ - - + + diff --git a/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee.vue b/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee.vue new file mode 100644 index 00000000000..3f3626092a8 --- /dev/null +++ b/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee.vue @@ -0,0 +1,27 @@ + + + diff --git a/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee_list.vue b/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee_list.vue new file mode 100644 index 00000000000..6db198f1c1e --- /dev/null +++ b/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee_list.vue @@ -0,0 +1,121 @@ + + + diff --git a/app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue b/app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue new file mode 100644 index 00000000000..e84b323c7bd --- /dev/null +++ b/app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue @@ -0,0 +1,96 @@ + + + diff --git a/app/assets/javascripts/users_select.js b/app/assets/javascripts/users_select.js index 33cedf78331..12c939aa70f 100644 --- a/app/assets/javascripts/users_select.js +++ b/app/assets/javascripts/users_select.js @@ -62,6 +62,8 @@ function UsersSelect(currentUser, els, options = {}) { options.showCurrentUser = $dropdown.data('currentUser'); options.todoFilter = $dropdown.data('todoFilter'); options.todoStateFilter = $dropdown.data('todoStateFilter'); + options.iid = $dropdown.data('iid'); + options.issuableType = $dropdown.data('issuableType'); showNullUser = $dropdown.data('nullUser'); defaultNullUser = $dropdown.data('nullUserDefault'); showMenuAbove = $dropdown.data('showMenuAbove'); @@ -239,7 +241,7 @@ function UsersSelect(currentUser, els, options = {}) { '<% if( avatar ) { %> <% } else { %> <% } %>', ); assigneeTemplate = _.template( - `<% if (username) { %> <% if( avatar ) { %> <% } %> <%- name %> @<%- username %> <% } else { %> + `<% if (username) { %> <% if( avatar ) { %> <% } %> <%- name %> @<%- username %> <% } else { %> ${sprintf(s__('UsersSelect|No assignee - %{openingTag} assign yourself %{closingTag}'), { openingTag: '', closingTag: '', @@ -423,6 +425,8 @@ function UsersSelect(currentUser, els, options = {}) { const { $el, e, isMarking } = options; const user = options.selectedObj; + $el.tooltip('dispose'); + if ($dropdown.hasClass('js-multiselect')) { const isActive = $el.hasClass('is-active'); const previouslySelected = $dropdown @@ -570,20 +574,11 @@ function UsersSelect(currentUser, els, options = {}) { user.name, )}`; } else { - img = ""; + // 0 margin, because it's now handled by a wrapper + img = ""; } - return ` -
  • - - ${img} - - ${_.escape(user.name)} - - ${username ? `${username}` : ''} - -
    • - `; + return _this.renderRow(options.issuableType, user, selected, username, img); }, }); }; @@ -764,6 +759,11 @@ UsersSelect.prototype.users = function(query, options, callback) { author_id: options.authorId || null, skip_users: options.skipUsers || null, }; + + if (options.issuableType === 'merge_request') { + params.merge_request_iid = options.iid || null; + } + return axios.get(url, { params }).then(({ data }) => { callback(data); }); @@ -776,4 +776,44 @@ UsersSelect.prototype.buildUrl = function(url) { return url; }; +UsersSelect.prototype.renderRow = function(issuableType, user, selected, username, img) { + const tooltip = issuableType === 'merge_request' && !user.can_merge ? __('Cannot merge') : ''; + const tooltipClass = tooltip ? `has-tooltip` : ''; + const selectedClass = selected === true ? 'is-active' : ''; + const linkClasses = `${selectedClass} ${tooltipClass}`; + const tooltipAttributes = tooltip + ? `data-container="body" data-placement="left" data-title="${tooltip}"` + : ''; + + return ` +
  • + + ${this.renderRowAvatar(issuableType, user, img)} + + + ${_.escape(user.name)} + + ${username ? `${username}` : ''} + + +
    • + `; +}; + +UsersSelect.prototype.renderRowAvatar = function(issuableType, user, img) { + if (user.beforeDivider) { + return img; + } + + const mergeIcon = + issuableType === 'merge_request' && !user.can_merge + ? '' + : ''; + + return ` + ${img} + ${mergeIcon} + `; +}; + export default UsersSelect; diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index fa52ce6402d..0e844b0e4a5 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -126,6 +126,16 @@ } } +.assignee { + .merge-icon { + color: $orange-500; + position: absolute; + bottom: 0; + right: 0; + text-shadow: -1px -1px 0 $white-light, 1px -1px 0 $white-light, -1px 1px 0 $white-light, 1px 1px 0 $white-light; + } +} + .right-sidebar { position: fixed; top: $header-height; @@ -202,7 +212,6 @@ &.assignee { .author-link { display: block; - padding-left: 42px; position: relative; &:hover { @@ -210,12 +219,6 @@ text-decoration: underline; } } - - .avatar { - left: 0; - position: absolute; - top: 0; - } } } } @@ -354,13 +357,6 @@ margin-top: 0; } - .assignee .avatar { - float: left; - margin-right: 10px; - margin-bottom: 0; - margin-left: 0; - } - .assignee .user-list .avatar { margin: 0; } @@ -521,6 +517,10 @@ display: none; } + .merge-icon { + font-size: 10px; + } + .multiple-users { position: relative; height: 24px; diff --git a/app/serializers/issuable_sidebar_basic_entity.rb b/app/serializers/issuable_sidebar_basic_entity.rb index c02fd024345..058c707ef9d 100644 --- a/app/serializers/issuable_sidebar_basic_entity.rb +++ b/app/serializers/issuable_sidebar_basic_entity.rb @@ -4,6 +4,7 @@ class IssuableSidebarBasicEntity < Grape::Entity include RequestAwareEntity expose :id + expose :iid expose :type do |issuable| issuable.to_ability_name end diff --git a/app/serializers/merge_request_serializer.rb b/app/serializers/merge_request_serializer.rb index 8ad1df5dfe0..bd2e682a122 100644 --- a/app/serializers/merge_request_serializer.rb +++ b/app/serializers/merge_request_serializer.rb @@ -8,7 +8,7 @@ class MergeRequestSerializer < BaseSerializer entity ||= case opts[:serializer] when 'sidebar' - IssuableSidebarBasicEntity + MergeRequestSidebarBasicEntity when 'sidebar_extras' MergeRequestSidebarExtrasEntity when 'basic' diff --git a/app/serializers/merge_request_sidebar_basic_entity.rb b/app/serializers/merge_request_sidebar_basic_entity.rb new file mode 100644 index 00000000000..fdd0cdc50d2 --- /dev/null +++ b/app/serializers/merge_request_sidebar_basic_entity.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class MergeRequestSidebarBasicEntity < IssuableSidebarBasicEntity +end + +MergeRequestSidebarBasicEntity.prepend_if_ee('EE::MergeRequestSidebarBasicEntity') diff --git a/app/views/shared/issuable/_sidebar_assignees.html.haml b/app/views/shared/issuable/_sidebar_assignees.html.haml index ab01094ed6e..1dc538826dc 100644 --- a/app/views/shared/issuable/_sidebar_assignees.html.haml +++ b/app/views/shared/issuable/_sidebar_assignees.html.haml @@ -20,6 +20,8 @@ placeholder: _('Search users'), data: { first_user: issuable_sidebar.dig(:current_user, :username), current_user: true, + iid: issuable_sidebar[:iid], + issuable_type: issuable_type, project_id: issuable_sidebar[:project_id], author_id: issuable_sidebar[:author_id], field_name: "#{issuable_type}[assignee_ids][]", diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 26e6cb524bd..2b71d099a01 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -128,9 +128,6 @@ msgstr[1] "" msgid "%{actionText} & %{openOrClose} %{noteable}" msgstr "" -msgid "%{canMergeCount}/%{assigneesCount} can merge" -msgstr "" - msgid "%{commit_author_link} authored %{commit_timeago}" msgstr "" @@ -202,6 +199,9 @@ msgstr "" msgid "%{lock_path} is locked by GitLab User %{lock_user_id}" msgstr "" +msgid "%{mergeLength}/%{usersLength} can merge" +msgstr "" + msgid "%{mrText}, this issue will be closed automatically." msgstr "" @@ -279,6 +279,9 @@ msgstr "" msgid "%{usage_ping_link_start}Learn more%{usage_ping_link_end} about what information is shared with GitLab Inc." msgstr "" +msgid "%{userName} (cannot merge)" +msgstr "" + msgid "%{userName}'s avatar" msgstr "" @@ -306,6 +309,9 @@ msgstr "" msgid "(external source)" msgstr "" +msgid "+ %{amount} more" +msgstr "" + msgid "+ %{count} more" msgstr "" @@ -7439,9 +7445,6 @@ msgstr "" msgid "No milestones to show" msgstr "" -msgid "No one can merge" -msgstr "" - msgid "No other labels with such name or description" msgstr "" @@ -13452,6 +13455,9 @@ msgstr "" msgid "cannot include leading slash or directory traversal." msgstr "" +msgid "cannot merge" +msgstr "" + msgid "comment" msgstr "" @@ -13879,6 +13885,9 @@ msgstr "" msgid "no contributions" msgstr "" +msgid "no one can merge" +msgstr "" + msgid "none" msgstr "" diff --git a/spec/frontend/sidebar/components/assignees/assignee_avatar_link_spec.js b/spec/frontend/sidebar/components/assignees/assignee_avatar_link_spec.js new file mode 100644 index 00000000000..9e50eefc228 --- /dev/null +++ b/spec/frontend/sidebar/components/assignees/assignee_avatar_link_spec.js @@ -0,0 +1,84 @@ +import { shallowMount } from '@vue/test-utils'; +import { joinPaths } from '~/lib/utils/url_utility'; +import userDataMock from '../../user_data_mock'; +import AssigneeAvatarLink from '~/sidebar/components/assignees/assignee_avatar_link.vue'; + +const TOOLTIP_PLACEMENT = 'bottom'; +const { name: USER_NAME } = userDataMock(); + +describe('AssigneeAvatarLink component', () => { + let wrapper; + + function createComponent(props = {}) { + const propsData = { + user: userDataMock(), + showLess: true, + rootPath: 'http://localhost:3000/', + tooltipPlacement: TOOLTIP_PLACEMENT, + singleUser: false, + issuableType: 'merge_request', + ...props, + }; + + wrapper = shallowMount(AssigneeAvatarLink, { + propsData, + sync: false, + }); + } + + afterEach(() => { + wrapper.destroy(); + }); + + const findTooltipText = () => wrapper.attributes('data-original-title'); + + it('user who cannot merge has "cannot merge" in tooltip', () => { + createComponent({ + user: { + can_merge: false, + }, + }); + + expect(findTooltipText().includes('cannot merge')).toBe(true); + }); + + it('has the root url present in the assigneeUrl method', () => { + createComponent(); + const assigneeUrl = joinPaths( + `${wrapper.props('rootPath')}`, + `${wrapper.props('user').username}`, + ); + + expect(wrapper.attributes().href).toEqual(assigneeUrl); + }); + + describe.each` + issuableType | tooltipHasName | canMerge | expected + ${'merge_request'} | ${true} | ${true} | ${USER_NAME} + ${'merge_request'} | ${true} | ${false} | ${`${USER_NAME} (cannot merge)`} + ${'merge_request'} | ${false} | ${true} | ${''} + ${'merge_request'} | ${false} | ${false} | ${'Cannot merge'} + ${'issue'} | ${true} | ${true} | ${USER_NAME} + ${'issue'} | ${true} | ${false} | ${USER_NAME} + ${'issue'} | ${false} | ${true} | ${''} + ${'issue'} | ${false} | ${false} | ${''} + `( + 'with $issuableType and tooltipHasName=$tooltipHasName and canMerge=$canMerge', + ({ issuableType, tooltipHasName, canMerge, expected }) => { + beforeEach(() => { + createComponent({ + issuableType, + tooltipHasName, + user: { + ...userDataMock(), + can_merge: canMerge, + }, + }); + }); + + it('sets tooltip', () => { + expect(findTooltipText()).toBe(expected); + }); + }, + ); +}); diff --git a/spec/frontend/sidebar/components/assignees/assignee_avatar_spec.js b/spec/frontend/sidebar/components/assignees/assignee_avatar_spec.js new file mode 100644 index 00000000000..53e5dbea1df --- /dev/null +++ b/spec/frontend/sidebar/components/assignees/assignee_avatar_spec.js @@ -0,0 +1,78 @@ +import { shallowMount } from '@vue/test-utils'; +import AssigneeAvatar from '~/sidebar/components/assignees/assignee_avatar.vue'; +import { TEST_HOST } from 'helpers/test_constants'; +import userDataMock from '../../user_data_mock'; + +const TEST_AVATAR = `${TEST_HOST}/avatar.png`; +const TEST_DEFAULT_AVATAR_URL = `${TEST_HOST}/default/avatar/url.png`; + +describe('AssigneeAvatar', () => { + let origGon; + let wrapper; + + function createComponent(props = {}) { + const propsData = { + user: userDataMock(), + imgSize: 24, + issuableType: 'merge_request', + ...props, + }; + + wrapper = shallowMount(AssigneeAvatar, { + propsData, + sync: false, + }); + } + + beforeEach(() => { + origGon = window.gon; + window.gon = { default_avatar_url: TEST_DEFAULT_AVATAR_URL }; + }); + + afterEach(() => { + window.gon = origGon; + wrapper.destroy(); + }); + + const findImg = () => wrapper.find('img'); + + it('does not show warning icon if assignee can merge', () => { + createComponent(); + + expect(wrapper.element.querySelector('.merge-icon')).toBeNull(); + }); + + it('shows warning icon if assignee cannot merge', () => { + createComponent({ + user: { + can_merge: false, + }, + }); + + expect(wrapper.element.querySelector('.merge-icon')).not.toBeNull(); + }); + + it('does not show warning icon for issuableType = "issue"', () => { + createComponent({ + issuableType: 'issue', + }); + + expect(wrapper.element.querySelector('.merge-icon')).toBeNull(); + }); + + it.each` + avatar | avatar_url | expected | desc + ${TEST_AVATAR} | ${null} | ${TEST_AVATAR} | ${'with avatar'} + ${null} | ${TEST_AVATAR} | ${TEST_AVATAR} | ${'with avatar_url'} + ${null} | ${null} | ${TEST_DEFAULT_AVATAR_URL} | ${'with no avatar'} + `('$desc', ({ avatar, avatar_url, expected }) => { + createComponent({ + user: { + avatar, + avatar_url, + }, + }); + + expect(findImg().attributes('src')).toEqual(expected); + }); +}); diff --git a/spec/frontend/sidebar/components/assignees/collapsed_assignee_list_spec.js b/spec/frontend/sidebar/components/assignees/collapsed_assignee_list_spec.js new file mode 100644 index 00000000000..377c0e1d211 --- /dev/null +++ b/spec/frontend/sidebar/components/assignees/collapsed_assignee_list_spec.js @@ -0,0 +1,201 @@ +import { shallowMount } from '@vue/test-utils'; +import CollapsedAssigneeList from '~/sidebar/components/assignees/collapsed_assignee_list.vue'; +import CollapsedAssignee from '~/sidebar/components/assignees/collapsed_assignee.vue'; +import UsersMockHelper from 'helpers/user_mock_data_helper'; + +const DEFAULT_MAX_COUNTER = 99; + +describe('CollapsedAssigneeList component', () => { + let wrapper; + + function createComponent(props = {}) { + const propsData = { + users: [], + issuableType: 'merge_request', + ...props, + }; + + wrapper = shallowMount(CollapsedAssigneeList, { + propsData, + sync: false, + }); + } + + const findNoUsersIcon = () => wrapper.find('i[aria-label=None]'); + const findAvatarCounter = () => wrapper.find('.avatar-counter'); + const findAssignees = () => wrapper.findAll(CollapsedAssignee); + const getTooltipTitle = () => wrapper.attributes('data-original-title'); + + afterEach(() => { + wrapper.destroy(); + }); + + describe('No assignees/users', () => { + beforeEach(() => { + createComponent({ + users: [], + }); + }); + + it('has no users', () => { + expect(findNoUsersIcon().exists()).toBe(true); + }); + }); + + describe('One assignee/user', () => { + let users; + + beforeEach(() => { + users = UsersMockHelper.createNumberRandomUsers(1); + }); + + it('should not show no users icon', () => { + createComponent({ users }); + + expect(findNoUsersIcon().exists()).toBe(false); + }); + + it('has correct "cannot merge" tooltip when user cannot merge', () => { + users[0].can_merge = false; + + createComponent({ users }); + + expect(getTooltipTitle()).toContain('cannot merge'); + }); + + it('does not have "merge" word in tooltip if user can merge', () => { + users[0].can_merge = true; + + createComponent({ users }); + + expect(getTooltipTitle()).not.toContain('merge'); + }); + }); + + describe('More than one assignees/users', () => { + let users; + + beforeEach(() => { + users = UsersMockHelper.createNumberRandomUsers(2); + }); + + describe('default', () => { + beforeEach(() => { + createComponent({ users }); + }); + + it('has multiple-users class', () => { + expect(wrapper.classes('multiple-users')).toBe(true); + }); + + it('does not display an avatar count', () => { + expect(findAvatarCounter().exists()).toBe(false); + }); + + it('returns just two collapsed users', () => { + expect(findAssignees().length).toBe(2); + }); + }); + + it('has correct "cannot merge" tooltip when no user can merge', () => { + users[0].can_merge = false; + users[1].can_merge = false; + + createComponent({ + users, + }); + + expect(getTooltipTitle()).toEqual(`${users[0].name}, ${users[1].name} (no one can merge)`); + }); + + it('does not have "merge" word in tooltip if everyone can merge', () => { + users[0].can_merge = true; + users[1].can_merge = true; + + createComponent({ + users, + }); + + expect(getTooltipTitle()).toEqual(`${users[0].name}, ${users[1].name}`); + }); + }); + + describe('More than two assignees/users', () => { + let users; + + beforeEach(() => { + users = UsersMockHelper.createNumberRandomUsers(3); + }); + + describe('default', () => { + beforeEach(() => { + createComponent({ users }); + }); + + it('does display an avatar count', () => { + expect(findAvatarCounter().exists()).toBe(true); + expect(findAvatarCounter().text()).toEqual('+2'); + }); + + it('returns one collapsed users', () => { + expect(findAssignees().length).toBe(1); + }); + }); + + it('has correct "cannot merge" tooltip when one user can merge', () => { + users[0].can_merge = true; + users[1].can_merge = false; + users[2].can_merge = false; + + createComponent({ + users, + }); + + expect(getTooltipTitle()).toContain('1/3 can merge'); + }); + + it('has correct "cannot merge" tooltip when more than one user can merge', () => { + users[0].can_merge = false; + users[1].can_merge = true; + users[2].can_merge = true; + + createComponent({ + users, + }); + + expect(getTooltipTitle()).toContain('2/3 can merge'); + }); + + it('does not have "merge" in tooltip if everyone can merge', () => { + users[0].can_merge = true; + users[1].can_merge = true; + users[2].can_merge = true; + + createComponent({ + users, + }); + + expect(getTooltipTitle()).not.toContain('merge'); + }); + + it('displays the correct avatar count via a computed property if less than default max counter', () => { + users = UsersMockHelper.createNumberRandomUsers(5); + + createComponent({ + users, + }); + + expect(findAvatarCounter().text()).toEqual(`+${users.length - 1}`); + }); + + it('displays the correct avatar count via a computed property if more than default max counter', () => { + users = UsersMockHelper.createNumberRandomUsers(100); + + createComponent({ + users, + }); + + expect(findAvatarCounter().text()).toEqual(`${DEFAULT_MAX_COUNTER}+`); + }); + }); +}); diff --git a/spec/frontend/sidebar/components/assignees/collapsed_assignee_spec.js b/spec/frontend/sidebar/components/assignees/collapsed_assignee_spec.js new file mode 100644 index 00000000000..03b61daa2f8 --- /dev/null +++ b/spec/frontend/sidebar/components/assignees/collapsed_assignee_spec.js @@ -0,0 +1,34 @@ +import { shallowMount } from '@vue/test-utils'; +import CollapsedAssignee from '~/sidebar/components/assignees/collapsed_assignee.vue'; +import userDataMock from '../../user_data_mock'; + +describe('CollapsedAssignee assignee component', () => { + let wrapper; + + function createComponent(props = {}) { + const propsData = { + user: userDataMock(), + ...props, + }; + + wrapper = shallowMount(CollapsedAssignee, { + propsData, + sync: false, + }); + } + + afterEach(() => { + wrapper.destroy(); + }); + + it('has author name', () => { + createComponent(); + + expect( + wrapper + .find('.author') + .text() + .trim(), + ).toEqual(wrapper.vm.user.name); + }); +}); diff --git a/spec/frontend/sidebar/components/assignees/uncollapsed_assignee_list_spec.js b/spec/frontend/sidebar/components/assignees/uncollapsed_assignee_list_spec.js new file mode 100644 index 00000000000..64170a53a7f --- /dev/null +++ b/spec/frontend/sidebar/components/assignees/uncollapsed_assignee_list_spec.js @@ -0,0 +1,135 @@ +import Vue from 'vue'; +import { mount } from '@vue/test-utils'; +import UncollapsedAssigneeList from '~/sidebar/components/assignees/uncollapsed_assignee_list.vue'; +import AssigneeAvatarLink from '~/sidebar/components/assignees/assignee_avatar_link.vue'; +import { TEST_HOST } from 'helpers/test_constants'; +import userDataMock from '../../user_data_mock'; +import UsersMockHelper from '../../../helpers/user_mock_data_helper'; + +const DEFAULT_RENDER_COUNT = 5; + +describe('UncollapsedAssigneeList component', () => { + let wrapper; + + function createComponent(props = {}) { + const propsData = { + users: [], + rootPath: TEST_HOST, + ...props, + }; + + wrapper = mount(UncollapsedAssigneeList, { + sync: false, + propsData, + }); + } + + afterEach(() => { + wrapper.destroy(); + }); + + const findMoreButton = () => wrapper.find('.user-list-more button'); + + describe('One assignee/user', () => { + let user; + + beforeEach(() => { + user = userDataMock(); + + createComponent({ + users: [user], + }); + }); + + it('only has one user', () => { + expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(1); + }); + + it('calls the AssigneeAvatarLink with the proper props', () => { + expect(wrapper.find(AssigneeAvatarLink).exists()).toBe(true); + expect(wrapper.find(AssigneeAvatarLink).props().tooltipPlacement).toEqual('left'); + }); + + it('Shows one user with avatar, username and author name', () => { + expect(wrapper.text()).toContain(user.name); + expect(wrapper.text()).toContain(`@${user.username}`); + }); + }); + + describe('Two or more assignees/users', () => { + beforeEach(() => { + createComponent({ + users: UsersMockHelper.createNumberRandomUsers(3), + }); + }); + + it('more than one user', () => { + expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(3); + }); + + it('shows the "show-less" assignees label', done => { + const users = UsersMockHelper.createNumberRandomUsers(6); + + createComponent({ + users, + }); + + expect(wrapper.vm.$el.querySelectorAll('.user-item').length).toEqual(DEFAULT_RENDER_COUNT); + + expect(wrapper.vm.$el.querySelector('.user-list-more')).not.toBe(null); + const usersLabelExpectation = users.length - DEFAULT_RENDER_COUNT; + + expect(wrapper.vm.$el.querySelector('.user-list-more .btn-link').innerText.trim()).not.toBe( + `+${usersLabelExpectation} more`, + ); + wrapper.vm.toggleShowLess(); + Vue.nextTick(() => { + expect(wrapper.vm.$el.querySelector('.user-list-more .btn-link').innerText.trim()).toBe( + '- show less', + ); + done(); + }); + }); + + it('shows the "show-less" when "n+ more " label is clicked', done => { + createComponent({ + users: UsersMockHelper.createNumberRandomUsers(6), + }); + + wrapper.vm.$el.querySelector('.user-list-more .btn-link').click(); + Vue.nextTick(() => { + expect(wrapper.vm.$el.querySelector('.user-list-more .btn-link').innerText.trim()).toBe( + '- show less', + ); + done(); + }); + }); + + it('does not show n+ more label when less than render count', () => { + expect(findMoreButton().exists()).toBe(false); + }); + }); + + describe('n+ more label', () => { + beforeEach(() => { + createComponent({ + users: UsersMockHelper.createNumberRandomUsers(DEFAULT_RENDER_COUNT + 1), + }); + }); + + it('shows "+1 more" label', () => { + expect(findMoreButton().text()).toBe('+ 1 more'); + expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(DEFAULT_RENDER_COUNT); + }); + + it('shows "show less" label', done => { + findMoreButton().trigger('click'); + + Vue.nextTick(() => { + expect(findMoreButton().text()).toBe('- show less'); + expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(DEFAULT_RENDER_COUNT + 1); + done(); + }); + }); + }); +}); diff --git a/spec/frontend/sidebar/user_data_mock.js b/spec/frontend/sidebar/user_data_mock.js new file mode 100644 index 00000000000..8ad70bb3499 --- /dev/null +++ b/spec/frontend/sidebar/user_data_mock.js @@ -0,0 +1,9 @@ +export default () => ({ + avatar_url: 'mock_path', + id: 1, + name: 'Root', + state: 'active', + username: 'root', + web_url: '', + can_merge: true, +}); diff --git a/spec/javascripts/sidebar/assignees_spec.js b/spec/javascripts/sidebar/assignees_spec.js index 4ae2141d5f0..fc731e731ae 100644 --- a/spec/javascripts/sidebar/assignees_spec.js +++ b/spec/javascripts/sidebar/assignees_spec.js @@ -94,115 +94,9 @@ describe('Assignee component', () => { expect(assignee.querySelector('.author').innerText.trim()).toEqual(UsersMock.user.name); }); - - it('Shows one user with avatar, username and author name', () => { - component = new AssigneeComponent({ - propsData: { - rootPath: 'http://localhost:3000/', - users: [UsersMock.user], - editable: true, - }, - }).$mount(); - - expect(component.$el.querySelector('.author-link')).not.toBeNull(); - // The image - expect(component.$el.querySelector('.author-link img').getAttribute('src')).toEqual( - UsersMock.user.avatar, - ); - // Author name - expect(component.$el.querySelector('.author-link .author').innerText.trim()).toEqual( - UsersMock.user.name, - ); - // Username - expect(component.$el.querySelector('.author-link .username').innerText.trim()).toEqual( - `@${UsersMock.user.username}`, - ); - }); - - it('has the root url present in the assigneeUrl method', () => { - component = new AssigneeComponent({ - propsData: { - rootPath: 'http://localhost:3000/', - users: [UsersMock.user], - editable: true, - }, - }).$mount(); - - expect(component.assigneeUrl(UsersMock.user).indexOf('http://localhost:3000/')).not.toEqual( - -1, - ); - }); - - it('has correct "cannot merge" tooltip when user cannot merge', () => { - const user = Object.assign({}, UsersMock.user, { can_merge: false }); - - component = new AssigneeComponent({ - propsData: { - rootPath: 'http://localhost:3000/', - users: [user], - editable: true, - issuableType: 'merge_request', - }, - }).$mount(); - - expect(component.mergeNotAllowedTooltipMessage).toEqual('Cannot merge'); - }); }); describe('Two or more assignees/users', () => { - it('has correct "cannot merge" tooltip when one user can merge', () => { - const users = UsersMockHelper.createNumberRandomUsers(3); - users[0].can_merge = true; - users[1].can_merge = false; - users[2].can_merge = false; - - component = new AssigneeComponent({ - propsData: { - rootPath: 'http://localhost:3000/', - users, - editable: true, - issuableType: 'merge_request', - }, - }).$mount(); - - expect(component.mergeNotAllowedTooltipMessage).toEqual('1/3 can merge'); - }); - - it('has correct "cannot merge" tooltip when no user can merge', () => { - const users = UsersMockHelper.createNumberRandomUsers(2); - users[0].can_merge = false; - users[1].can_merge = false; - - component = new AssigneeComponent({ - propsData: { - rootPath: 'http://localhost:3000/', - users, - editable: true, - issuableType: 'merge_request', - }, - }).$mount(); - - expect(component.mergeNotAllowedTooltipMessage).toEqual('No one can merge'); - }); - - it('has correct "cannot merge" tooltip when more than one user can merge', () => { - const users = UsersMockHelper.createNumberRandomUsers(3); - users[0].can_merge = false; - users[1].can_merge = true; - users[2].can_merge = true; - - component = new AssigneeComponent({ - propsData: { - rootPath: 'http://localhost:3000/', - users, - editable: true, - issuableType: 'merge_request', - }, - }).$mount(); - - expect(component.mergeNotAllowedTooltipMessage).toEqual('2/3 can merge'); - }); - it('has no "cannot merge" tooltip when every user can merge', () => { const users = UsersMockHelper.createNumberRandomUsers(2); users[0].can_merge = true; @@ -217,7 +111,7 @@ describe('Assignee component', () => { }, }).$mount(); - expect(component.mergeNotAllowedTooltipMessage).toEqual(null); + expect(component.collapsedTooltipTitle).not.toContain('cannot merge'); }); it('displays two assignee icons when collapsed', () => { @@ -295,8 +189,12 @@ describe('Assignee component', () => { expect(component.$el.querySelector('.user-list-more')).toBe(null); }); - it('sets tooltip container to body', () => { - const users = UsersMockHelper.createNumberRandomUsers(2); + it('shows sorted assignee where "can merge" users are sorted first', () => { + const users = UsersMockHelper.createNumberRandomUsers(3); + users[0].can_merge = false; + users[1].can_merge = false; + users[2].can_merge = true; + component = new AssigneeComponent({ propsData: { rootPath: 'http://localhost:3000', @@ -305,98 +203,48 @@ describe('Assignee component', () => { }, }).$mount(); - expect(component.$el.querySelector('.user-link').getAttribute('data-container')).toBe('body'); + expect(component.sortedAssigness[0].can_merge).toBe(true); }); - it('Shows the "show-less" assignees label', done => { - const users = UsersMockHelper.createNumberRandomUsers(6); + it('passes the sorted assignees to the uncollapsed-assignee-list', () => { + const users = UsersMockHelper.createNumberRandomUsers(3); + users[0].can_merge = false; + users[1].can_merge = false; + users[2].can_merge = true; + component = new AssigneeComponent({ propsData: { rootPath: 'http://localhost:3000', users, - editable: true, + editable: false, }, }).$mount(); - expect(component.$el.querySelectorAll('.user-item').length).toEqual( - component.defaultRenderCount, - ); - - expect(component.$el.querySelector('.user-list-more')).not.toBe(null); - const usersLabelExpectation = users.length - component.defaultRenderCount; + const userItems = component.$el.querySelectorAll('.user-list .user-item a'); - expect(component.$el.querySelector('.user-list-more .btn-link').innerText.trim()).not.toBe( - `+${usersLabelExpectation} more`, - ); - component.toggleShowLess(); - Vue.nextTick(() => { - expect(component.$el.querySelector('.user-list-more .btn-link').innerText.trim()).toBe( - '- show less', - ); - done(); - }); + expect(userItems.length).toBe(3); + expect(userItems[0].dataset.originalTitle).toBe(users[2].name); + expect(userItems[0].dataset.originalTitle).not.toBe(users[0].name); }); - it('Shows the "show-less" when "n+ more " label is clicked', done => { - const users = UsersMockHelper.createNumberRandomUsers(6); - component = new AssigneeComponent({ - propsData: { - rootPath: 'http://localhost:3000', - users, - editable: true, - }, - }).$mount(); - - component.$el.querySelector('.user-list-more .btn-link').click(); - Vue.nextTick(() => { - expect(component.$el.querySelector('.user-list-more .btn-link').innerText.trim()).toBe( - '- show less', - ); - done(); - }); - }); + it('passes the sorted assignees to the collapsed-assignee-list', () => { + const users = UsersMockHelper.createNumberRandomUsers(3); + users[0].can_merge = false; + users[1].can_merge = false; + users[2].can_merge = true; - it('gets the count of avatar via a computed property ', () => { - const users = UsersMockHelper.createNumberRandomUsers(6); component = new AssigneeComponent({ propsData: { rootPath: 'http://localhost:3000', users, - editable: true, + editable: false, }, }).$mount(); - expect(component.sidebarAvatarCounter).toEqual(`+${users.length - 1}`); - }); + const collapsedButton = component.$el.querySelector('.sidebar-collapsed-user button'); - describe('n+ more label', () => { - beforeEach(() => { - const users = UsersMockHelper.createNumberRandomUsers(6); - component = new AssigneeComponent({ - propsData: { - rootPath: 'http://localhost:3000', - users, - editable: true, - }, - }).$mount(); - }); - - it('shows "+1 more" label', () => { - expect(component.$el.querySelector('.user-list-more .btn-link').innerText.trim()).toBe( - '+ 1 more', - ); - }); - - it('shows "show less" label', done => { - component.toggleShowLess(); - - Vue.nextTick(() => { - expect(component.$el.querySelector('.user-list-more .btn-link').innerText.trim()).toBe( - '- show less', - ); - done(); - }); - }); + expect(collapsedButton.innerText.trim()).toBe(users[2].name); + expect(collapsedButton.innerText.trim()).not.toBe(users[0].name); }); }); }); diff --git a/spec/javascripts/sidebar/mock_data.js b/spec/javascripts/sidebar/mock_data.js index 7f20b0da991..107acead3c6 100644 --- a/spec/javascripts/sidebar/mock_data.js +++ b/spec/javascripts/sidebar/mock_data.js @@ -1,3 +1,13 @@ +export const userDataMock = { + avatar_url: 'mock_path', + id: 1, + name: 'Root', + state: 'active', + username: 'root', + web_url: '', + can_merge: true, +}; + const RESPONSE_MAP = { GET: { '/gitlab-org/gitlab-shell/issues/5.json': { diff --git a/spec/serializers/merge_request_sidebar_basic_entity_spec.rb b/spec/serializers/merge_request_sidebar_basic_entity_spec.rb new file mode 100644 index 00000000000..b364b1a3306 --- /dev/null +++ b/spec/serializers/merge_request_sidebar_basic_entity_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequestSidebarBasicEntity do + let(:project) { create :project, :repository } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:user) { create(:user) } + + let(:request) { double('request', current_user: user, project: project) } + + let(:entity) { described_class.new(merge_request, request: request).as_json } + + describe '#current_user' do + it 'contains attributes related to the current user' do + expect(entity[:current_user].keys).to contain_exactly( + :id, :name, :username, :state, :avatar_url, :web_url, :todo, + :can_edit, :can_move, :can_admin_label, :can_merge + ) + end + end +end -- cgit v1.2.1 From 4e3c7a0bab0dc42cb46dd06059820eea538adfc9 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Sat, 17 Aug 2019 00:50:04 -0500 Subject: Fix `require` typo to `required` --- app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue | 2 +- app/assets/javascripts/sidebar/components/assignees/assignees.vue | 2 +- .../javascripts/sidebar/components/assignees/collapsed_assignee.vue | 2 +- .../sidebar/components/assignees/collapsed_assignee_list.vue | 2 +- .../javascripts/sidebar/components/assignees/sidebar_assignees.vue | 2 +- .../sidebar/components/assignees/uncollapsed_assignee_list.vue | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue index 0bb4f757de8..71a1fc31315 100644 --- a/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue +++ b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar.vue @@ -13,7 +13,7 @@ export default { }, issuableType: { type: String, - require: true, + required: false, default: 'issue', }, }, diff --git a/app/assets/javascripts/sidebar/components/assignees/assignees.vue b/app/assets/javascripts/sidebar/components/assignees/assignees.vue index ca4090eee96..d9739e8d197 100644 --- a/app/assets/javascripts/sidebar/components/assignees/assignees.vue +++ b/app/assets/javascripts/sidebar/components/assignees/assignees.vue @@ -25,7 +25,7 @@ export default { }, issuableType: { type: String, - require: true, + required: false, default: 'issue', }, }, diff --git a/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee.vue b/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee.vue index 3f3626092a8..2f654409561 100644 --- a/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee.vue +++ b/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee.vue @@ -12,7 +12,7 @@ export default { }, issuableType: { type: String, - require: true, + required: false, default: 'issue', }, }, diff --git a/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee_list.vue b/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee_list.vue index 6db198f1c1e..5b4a43399ca 100644 --- a/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee_list.vue +++ b/app/assets/javascripts/sidebar/components/assignees/collapsed_assignee_list.vue @@ -20,7 +20,7 @@ export default { }, issuableType: { type: String, - require: true, + required: false, default: 'issue', }, }, diff --git a/app/assets/javascripts/sidebar/components/assignees/sidebar_assignees.vue b/app/assets/javascripts/sidebar/components/assignees/sidebar_assignees.vue index be1e4811856..c6cc04a139f 100644 --- a/app/assets/javascripts/sidebar/components/assignees/sidebar_assignees.vue +++ b/app/assets/javascripts/sidebar/components/assignees/sidebar_assignees.vue @@ -29,7 +29,7 @@ export default { }, issuableType: { type: String, - require: true, + required: false, default: 'issue', }, }, diff --git a/app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue b/app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue index e84b323c7bd..3a4623121f4 100644 --- a/app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue +++ b/app/assets/javascripts/sidebar/components/assignees/uncollapsed_assignee_list.vue @@ -19,7 +19,7 @@ export default { }, issuableType: { type: String, - require: true, + required: false, default: 'issue', }, }, -- cgit v1.2.1 From 96b994bd05db072655bcd28204770013feeaa779 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Mon, 19 Aug 2019 11:34:19 +0300 Subject: Fix comments related to BE part - Edit commit messge body to fix "danger-review" --- app/serializers/merge_request_sidebar_basic_entity.rb | 7 +++++-- spec/fixtures/api/schemas/entities/merge_request_sidebar.json | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/serializers/merge_request_sidebar_basic_entity.rb b/app/serializers/merge_request_sidebar_basic_entity.rb index fdd0cdc50d2..3c911bbe4c8 100644 --- a/app/serializers/merge_request_sidebar_basic_entity.rb +++ b/app/serializers/merge_request_sidebar_basic_entity.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class MergeRequestSidebarBasicEntity < IssuableSidebarBasicEntity + expose :current_user, if: lambda { |_issuable| current_user } do + expose :can_merge do |merge_request| + merge_request.can_be_merged_by?(current_user) + end + end end - -MergeRequestSidebarBasicEntity.prepend_if_ee('EE::MergeRequestSidebarBasicEntity') diff --git a/spec/fixtures/api/schemas/entities/merge_request_sidebar.json b/spec/fixtures/api/schemas/entities/merge_request_sidebar.json index 214b67a9a0f..9945de8a856 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_sidebar.json +++ b/spec/fixtures/api/schemas/entities/merge_request_sidebar.json @@ -2,6 +2,7 @@ "type": "object", "properties" : { "id": { "type": "integer" }, + "iid": { "type": "integer" }, "type": { "type": "string" }, "author_id": { "type": "integer" }, "project_id": { "type": "integer" }, -- cgit v1.2.1 From 1c7cbc25b9a877e492643d8749fa2862d86fd93f Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Mon, 19 Aug 2019 21:29:47 -0700 Subject: Apply patch and update spec per MR comments - Use wrapper instead of global Vue object - Use vue utils helper method - Remove redundant spec tests --- .../assignees/assignee_avatar_link_spec.js | 35 +++++---- .../components/assignees/assignee_avatar_spec.js | 6 +- .../assignees/collapsed_assignee_list_spec.js | 64 +++++++-------- .../assignees/collapsed_assignee_spec.js | 17 +++- .../assignees/uncollapsed_assignee_list_spec.js | 90 +++++++--------------- spec/javascripts/sidebar/assignees_spec.js | 2 - spec/javascripts/sidebar/mock_data.js | 10 --- 7 files changed, 92 insertions(+), 132 deletions(-) diff --git a/spec/frontend/sidebar/components/assignees/assignee_avatar_link_spec.js b/spec/frontend/sidebar/components/assignees/assignee_avatar_link_spec.js index 9e50eefc228..452d4cd07cc 100644 --- a/spec/frontend/sidebar/components/assignees/assignee_avatar_link_spec.js +++ b/spec/frontend/sidebar/components/assignees/assignee_avatar_link_spec.js @@ -1,10 +1,13 @@ import { shallowMount } from '@vue/test-utils'; import { joinPaths } from '~/lib/utils/url_utility'; -import userDataMock from '../../user_data_mock'; +import { TEST_HOST } from 'helpers/test_constants'; import AssigneeAvatarLink from '~/sidebar/components/assignees/assignee_avatar_link.vue'; +import AssigneeAvatar from '~/sidebar/components/assignees/assignee_avatar.vue'; +import userDataMock from '../../user_data_mock'; const TOOLTIP_PLACEMENT = 'bottom'; -const { name: USER_NAME } = userDataMock(); +const { name: USER_NAME, username: USER_USERNAME } = userDataMock(); +const TEST_ISSUABLE_TYPE = 'merge_request'; describe('AssigneeAvatarLink component', () => { let wrapper; @@ -13,10 +16,10 @@ describe('AssigneeAvatarLink component', () => { const propsData = { user: userDataMock(), showLess: true, - rootPath: 'http://localhost:3000/', + rootPath: TEST_HOST, tooltipPlacement: TOOLTIP_PLACEMENT, singleUser: false, - issuableType: 'merge_request', + issuableType: TEST_ISSUABLE_TYPE, ...props, }; @@ -32,24 +35,22 @@ describe('AssigneeAvatarLink component', () => { const findTooltipText = () => wrapper.attributes('data-original-title'); - it('user who cannot merge has "cannot merge" in tooltip', () => { - createComponent({ - user: { - can_merge: false, - }, - }); + it('has the root url present in the assigneeUrl method', () => { + createComponent(); + const assigneeUrl = joinPaths(TEST_HOST, USER_USERNAME); - expect(findTooltipText().includes('cannot merge')).toBe(true); + expect(wrapper.attributes().href).toEqual(assigneeUrl); }); - it('has the root url present in the assigneeUrl method', () => { + it('renders assignee avatar', () => { createComponent(); - const assigneeUrl = joinPaths( - `${wrapper.props('rootPath')}`, - `${wrapper.props('user').username}`, - ); - expect(wrapper.attributes().href).toEqual(assigneeUrl); + expect(wrapper.find(AssigneeAvatar).props()).toEqual( + expect.objectContaining({ + issuableType: TEST_ISSUABLE_TYPE, + user: userDataMock(), + }), + ); }); describe.each` diff --git a/spec/frontend/sidebar/components/assignees/assignee_avatar_spec.js b/spec/frontend/sidebar/components/assignees/assignee_avatar_spec.js index 53e5dbea1df..d60ae17733b 100644 --- a/spec/frontend/sidebar/components/assignees/assignee_avatar_spec.js +++ b/spec/frontend/sidebar/components/assignees/assignee_avatar_spec.js @@ -39,7 +39,7 @@ describe('AssigneeAvatar', () => { it('does not show warning icon if assignee can merge', () => { createComponent(); - expect(wrapper.element.querySelector('.merge-icon')).toBeNull(); + expect(wrapper.find('.merge-icon').exists()).toBe(false); }); it('shows warning icon if assignee cannot merge', () => { @@ -49,7 +49,7 @@ describe('AssigneeAvatar', () => { }, }); - expect(wrapper.element.querySelector('.merge-icon')).not.toBeNull(); + expect(wrapper.find('.merge-icon').exists()).toBe(true); }); it('does not show warning icon for issuableType = "issue"', () => { @@ -57,7 +57,7 @@ describe('AssigneeAvatar', () => { issuableType: 'issue', }); - expect(wrapper.element.querySelector('.merge-icon')).toBeNull(); + expect(wrapper.find('.merge-icon').exists()).toBe(false); }); it.each` diff --git a/spec/frontend/sidebar/components/assignees/collapsed_assignee_list_spec.js b/spec/frontend/sidebar/components/assignees/collapsed_assignee_list_spec.js index 377c0e1d211..ff0c8d181b5 100644 --- a/spec/frontend/sidebar/components/assignees/collapsed_assignee_list_spec.js +++ b/spec/frontend/sidebar/components/assignees/collapsed_assignee_list_spec.js @@ -77,54 +77,30 @@ describe('CollapsedAssigneeList component', () => { beforeEach(() => { users = UsersMockHelper.createNumberRandomUsers(2); - }); - - describe('default', () => { - beforeEach(() => { - createComponent({ users }); - }); - - it('has multiple-users class', () => { - expect(wrapper.classes('multiple-users')).toBe(true); - }); - - it('does not display an avatar count', () => { - expect(findAvatarCounter().exists()).toBe(false); - }); - it('returns just two collapsed users', () => { - expect(findAssignees().length).toBe(2); - }); + createComponent({ users }); }); - it('has correct "cannot merge" tooltip when no user can merge', () => { - users[0].can_merge = false; - users[1].can_merge = false; - - createComponent({ - users, - }); - - expect(getTooltipTitle()).toEqual(`${users[0].name}, ${users[1].name} (no one can merge)`); + it('has multiple-users class', () => { + expect(wrapper.classes('multiple-users')).toBe(true); }); - it('does not have "merge" word in tooltip if everyone can merge', () => { - users[0].can_merge = true; - users[1].can_merge = true; - - createComponent({ - users, - }); + it('does not display an avatar count', () => { + expect(findAvatarCounter().exists()).toBe(false); + }); - expect(getTooltipTitle()).toEqual(`${users[0].name}, ${users[1].name}`); + it('returns just two collapsed users', () => { + expect(findAssignees().length).toBe(2); }); }); describe('More than two assignees/users', () => { let users; + let userNames; beforeEach(() => { users = UsersMockHelper.createNumberRandomUsers(3); + userNames = users.map(x => x.name).join(', '); }); describe('default', () => { @@ -142,6 +118,18 @@ describe('CollapsedAssigneeList component', () => { }); }); + it('has corrent "no one can merge" tooltip when no one can merge', () => { + users[0].can_merge = false; + users[1].can_merge = false; + users[2].can_merge = false; + + createComponent({ + users, + }); + + expect(getTooltipTitle()).toEqual(`${userNames} (no one can merge)`); + }); + it('has correct "cannot merge" tooltip when one user can merge', () => { users[0].can_merge = true; users[1].can_merge = false; @@ -151,7 +139,7 @@ describe('CollapsedAssigneeList component', () => { users, }); - expect(getTooltipTitle()).toContain('1/3 can merge'); + expect(getTooltipTitle()).toEqual(`${userNames} (1/3 can merge)`); }); it('has correct "cannot merge" tooltip when more than one user can merge', () => { @@ -163,7 +151,7 @@ describe('CollapsedAssigneeList component', () => { users, }); - expect(getTooltipTitle()).toContain('2/3 can merge'); + expect(getTooltipTitle()).toEqual(`${userNames} (2/3 can merge)`); }); it('does not have "merge" in tooltip if everyone can merge', () => { @@ -175,10 +163,10 @@ describe('CollapsedAssigneeList component', () => { users, }); - expect(getTooltipTitle()).not.toContain('merge'); + expect(getTooltipTitle()).toEqual(userNames); }); - it('displays the correct avatar count via a computed property if less than default max counter', () => { + it('displays the correct avatar count', () => { users = UsersMockHelper.createNumberRandomUsers(5); createComponent({ diff --git a/spec/frontend/sidebar/components/assignees/collapsed_assignee_spec.js b/spec/frontend/sidebar/components/assignees/collapsed_assignee_spec.js index 03b61daa2f8..f9ca7bc1ecb 100644 --- a/spec/frontend/sidebar/components/assignees/collapsed_assignee_spec.js +++ b/spec/frontend/sidebar/components/assignees/collapsed_assignee_spec.js @@ -1,13 +1,18 @@ import { shallowMount } from '@vue/test-utils'; import CollapsedAssignee from '~/sidebar/components/assignees/collapsed_assignee.vue'; +import AssigneeAvatar from '~/sidebar/components/assignees/assignee_avatar.vue'; import userDataMock from '../../user_data_mock'; +const TEST_USER = userDataMock(); +const TEST_ISSUABLE_TYPE = 'merge_request'; + describe('CollapsedAssignee assignee component', () => { let wrapper; function createComponent(props = {}) { const propsData = { user: userDataMock(), + issuableType: TEST_ISSUABLE_TYPE, ...props, }; @@ -29,6 +34,16 @@ describe('CollapsedAssignee assignee component', () => { .find('.author') .text() .trim(), - ).toEqual(wrapper.vm.user.name); + ).toEqual(TEST_USER.name); + }); + + it('has assignee avatar', () => { + createComponent(); + + expect(wrapper.find(AssigneeAvatar).props()).toEqual({ + imgSize: 24, + user: TEST_USER, + issuableType: TEST_ISSUABLE_TYPE, + }); }); }); diff --git a/spec/frontend/sidebar/components/assignees/uncollapsed_assignee_list_spec.js b/spec/frontend/sidebar/components/assignees/uncollapsed_assignee_list_spec.js index 64170a53a7f..6398351834c 100644 --- a/spec/frontend/sidebar/components/assignees/uncollapsed_assignee_list_spec.js +++ b/spec/frontend/sidebar/components/assignees/uncollapsed_assignee_list_spec.js @@ -1,4 +1,3 @@ -import Vue from 'vue'; import { mount } from '@vue/test-utils'; import UncollapsedAssigneeList from '~/sidebar/components/assignees/uncollapsed_assignee_list.vue'; import AssigneeAvatarLink from '~/sidebar/components/assignees/assignee_avatar_link.vue'; @@ -56,79 +55,48 @@ describe('UncollapsedAssigneeList component', () => { }); }); - describe('Two or more assignees/users', () => { - beforeEach(() => { - createComponent({ - users: UsersMockHelper.createNumberRandomUsers(3), - }); - }); - - it('more than one user', () => { - expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(3); - }); - - it('shows the "show-less" assignees label', done => { - const users = UsersMockHelper.createNumberRandomUsers(6); - - createComponent({ - users, + describe('n+ more label', () => { + describe('when users count is rendered users', () => { + beforeEach(() => { + createComponent({ + users: UsersMockHelper.createNumberRandomUsers(DEFAULT_RENDER_COUNT), + }); }); - expect(wrapper.vm.$el.querySelectorAll('.user-item').length).toEqual(DEFAULT_RENDER_COUNT); - - expect(wrapper.vm.$el.querySelector('.user-list-more')).not.toBe(null); - const usersLabelExpectation = users.length - DEFAULT_RENDER_COUNT; - - expect(wrapper.vm.$el.querySelector('.user-list-more .btn-link').innerText.trim()).not.toBe( - `+${usersLabelExpectation} more`, - ); - wrapper.vm.toggleShowLess(); - Vue.nextTick(() => { - expect(wrapper.vm.$el.querySelector('.user-list-more .btn-link').innerText.trim()).toBe( - '- show less', - ); - done(); + it('does not show more label', () => { + expect(findMoreButton().exists()).toBe(false); }); }); - it('shows the "show-less" when "n+ more " label is clicked', done => { - createComponent({ - users: UsersMockHelper.createNumberRandomUsers(6), + describe('when more than rendered users', () => { + beforeEach(() => { + createComponent({ + users: UsersMockHelper.createNumberRandomUsers(DEFAULT_RENDER_COUNT + 1), + }); }); - wrapper.vm.$el.querySelector('.user-list-more .btn-link').click(); - Vue.nextTick(() => { - expect(wrapper.vm.$el.querySelector('.user-list-more .btn-link').innerText.trim()).toBe( - '- show less', - ); - done(); + it('shows "+1 more" label', () => { + expect(findMoreButton().text()).toBe('+ 1 more'); }); - }); - - it('does not show n+ more label when less than render count', () => { - expect(findMoreButton().exists()).toBe(false); - }); - }); - describe('n+ more label', () => { - beforeEach(() => { - createComponent({ - users: UsersMockHelper.createNumberRandomUsers(DEFAULT_RENDER_COUNT + 1), + it('shows truncated users', () => { + expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(DEFAULT_RENDER_COUNT); }); - }); - it('shows "+1 more" label', () => { - expect(findMoreButton().text()).toBe('+ 1 more'); - expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(DEFAULT_RENDER_COUNT); - }); + describe('when more button is clicked', () => { + beforeEach(() => { + findMoreButton().trigger('click'); + + return wrapper.vm.$nextTick(); + }); - it('shows "show less" label', done => { - findMoreButton().trigger('click'); + it('shows "show less" label', () => { + expect(findMoreButton().text()).toBe('- show less'); + }); - Vue.nextTick(() => { - expect(findMoreButton().text()).toBe('- show less'); - expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(DEFAULT_RENDER_COUNT + 1); - done(); + it('shows all users', () => { + expect(wrapper.findAll(AssigneeAvatarLink).length).toBe(DEFAULT_RENDER_COUNT + 1); + }); }); }); }); diff --git a/spec/javascripts/sidebar/assignees_spec.js b/spec/javascripts/sidebar/assignees_spec.js index fc731e731ae..a1df5389a38 100644 --- a/spec/javascripts/sidebar/assignees_spec.js +++ b/spec/javascripts/sidebar/assignees_spec.js @@ -224,7 +224,6 @@ describe('Assignee component', () => { expect(userItems.length).toBe(3); expect(userItems[0].dataset.originalTitle).toBe(users[2].name); - expect(userItems[0].dataset.originalTitle).not.toBe(users[0].name); }); it('passes the sorted assignees to the collapsed-assignee-list', () => { @@ -244,7 +243,6 @@ describe('Assignee component', () => { const collapsedButton = component.$el.querySelector('.sidebar-collapsed-user button'); expect(collapsedButton.innerText.trim()).toBe(users[2].name); - expect(collapsedButton.innerText.trim()).not.toBe(users[0].name); }); }); }); diff --git a/spec/javascripts/sidebar/mock_data.js b/spec/javascripts/sidebar/mock_data.js index 107acead3c6..7f20b0da991 100644 --- a/spec/javascripts/sidebar/mock_data.js +++ b/spec/javascripts/sidebar/mock_data.js @@ -1,13 +1,3 @@ -export const userDataMock = { - avatar_url: 'mock_path', - id: 1, - name: 'Root', - state: 'active', - username: 'root', - web_url: '', - can_merge: true, -}; - const RESPONSE_MAP = { GET: { '/gitlab-org/gitlab-shell/issues/5.json': { -- cgit v1.2.1 From e4c44b089b58d010fe8e05dbc76acbc68720ae43 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Tue, 20 Aug 2019 13:34:36 -0500 Subject: Fix tooltip alignment issue caused by flex-basis **Context:** https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14851#note_205954645 --- .../sidebar/components/assignees/assignee_avatar_link.vue | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/sidebar/components/assignees/assignee_avatar_link.vue b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar_link.vue index 6563d1d3410..6633a63d046 100644 --- a/app/assets/javascripts/sidebar/components/assignees/assignee_avatar_link.vue +++ b/app/assets/javascripts/sidebar/components/assignees/assignee_avatar_link.vue @@ -67,8 +67,17 @@ export default { -- cgit v1.2.1