From 52a4868359f195fb1eaac9be8d8deb8bc200253f Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 2 Apr 2025 10:10:29 +0100 Subject: [PATCH 01/29] Restrict reassign options to enterprise users Only list enterprise users when reassigning contributions, if domain verification is in use. Changelog: added EE: true --- app/assets/javascripts/members/index.js | 2 + .../components/placeholder_actions.vue | 94 +++++++++++++++--- ee/app/assets/javascripts/api/groups_api.js | 16 +++ .../helpers/ee/groups/group_members_helper.rb | 4 +- .../ee/groups/group_members_helper_spec.rb | 39 ++++++++ .../placeholders/components/app_spec.js | 1 + .../components/placeholder_actions_spec.js | 99 ++++++++++++++++++- .../components/placeholders_table_spec.js | 1 + .../members/placeholders/mock_data.js | 28 ++++++ 9 files changed, 269 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/members/index.js b/app/assets/javascripts/members/index.js index d707aa543f1b0a..5b5a0773e41b8f 100644 --- a/app/assets/javascripts/members/index.js +++ b/app/assets/javascripts/members/index.js @@ -39,6 +39,7 @@ export const initMembersApp = (el, context, options) => { namespaceUserLimit, availableRoles, reassignmentCsvPath, + restrictReassignmentToEnterprise, ...vuexStoreAttributes } = parseDataAttributes(el); @@ -84,6 +85,7 @@ export const initMembersApp = (el, context, options) => { availableRoles, context, reassignmentCsvPath, + restrictReassignmentToEnterprise, group: { id: isGroup ? sourceId : null, name: groupName, diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index 268ac0590c5943..6916f639ad27dc 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -4,14 +4,18 @@ import { debounce, isEmpty, isNull } from 'lodash'; import { GlAvatarLabeled, GlButton, GlCollapsibleListbox } from '@gitlab/ui'; import { __, s__ } from '~/locale'; import { createAlert } from '~/alert'; -import { getFirstPropertyValue } from '~/lib/utils/common_utils'; - +import { + getFirstPropertyValue, + normalizeHeaders, + parseIntPagination, +} from '~/lib/utils/common_utils'; import searchUsersQuery from '~/graphql_shared/queries/users_search_all_paginated.query.graphql'; import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; import { PLACEHOLDER_STATUS_AWAITING_APPROVAL, PLACEHOLDER_STATUS_REASSIGNING, } from '~/import_entities/import_groups/constants'; +import { fetchGroupEnterpriseUsers } from 'ee_component/api/groups_api'; import importSourceUsersQuery from '../graphql/queries/import_source_users.query.graphql'; import importSourceUserReassignMutation from '../graphql/mutations/reassign.mutation.graphql'; import importSourceUserKeepAsPlaceholderMutation from '../graphql/mutations/keep_as_placeholder.mutation.graphql'; @@ -33,6 +37,7 @@ export default { GlButton, GlCollapsibleListbox, }, + inject: ['group', 'restrictReassignmentToEnterprise'], props: { sourceUser: { type: Object, @@ -50,6 +55,8 @@ export default { isLoadingMore: false, isValidated: false, search: '', + enterpriseUsers: [], + enterpriseUsersNextPage: 1, selectedUserToReassign: null, }; }, @@ -66,6 +73,9 @@ export default { result() { this.isLoadingInitial = false; }, + skip() { + return this.restrictReassignmentToEnterprise; + }, error() { this.onError(); }, @@ -83,6 +93,10 @@ export default { }, hasNextPage() { + if (this.restrictReassignmentToEnterprise) { + return Boolean(this.enterpriseUsersNextPage); + } + return this.users?.pageInfo?.hasNextPage; }, @@ -95,6 +109,10 @@ export default { }, userItems() { + if (this.restrictReassignmentToEnterprise) { + return this.enterpriseUsers?.map((user) => this.createUserObjectFromEnterprise(user)); + } + return this.users?.nodes?.map((user) => createUserObject(user)); }, @@ -138,6 +156,12 @@ export default { this.debouncedSetSearch = debounce(this.setSearch, DEFAULT_DEBOUNCE_AND_THROTTLE_MS); }, + mounted() { + if (this.restrictReassignmentToEnterprise) { + this.getGroupEnterpriseUsers(); + } + }, + methods: { async loadMoreUsers() { if (!this.hasNextPage) return; @@ -145,17 +169,21 @@ export default { this.isLoadingMore = true; try { - await this.$apollo.queries.users.fetchMore({ - variables: { - ...this.queryVariables, - after: this.users.pageInfo?.endCursor, - }, - updateQuery: (previousResult, { fetchMoreResult }) => { - return produce(fetchMoreResult, (draftData) => { - draftData.users.nodes = [...previousResult.users.nodes, ...draftData.users.nodes]; - }); - }, - }); + if (this.restrictReassignmentToEnterprise) { + await this.getGroupEnterpriseUsers(); + } else { + await this.$apollo.queries.users.fetchMore({ + variables: { + ...this.queryVariables, + after: this.users.pageInfo?.endCursor, + }, + updateQuery: (previousResult, { fetchMoreResult }) => { + return produce(fetchMoreResult, (draftData) => { + draftData.users.nodes = [...previousResult.users.nodes, ...draftData.users.nodes]; + }); + }, + }); + } } catch (error) { this.onError(); } finally { @@ -163,6 +191,27 @@ export default { } }, + async getGroupEnterpriseUsers() { + try { + const { data, headers } = await fetchGroupEnterpriseUsers(this.group.id, { + page: this.enterpriseUsersNextPage, + per_page: USERS_PER_PAGE, + search: this.search, + }); + + this.isLoadingInitial = false; + this.enterpriseUsersNextPage = parseIntPagination(normalizeHeaders(headers)).nextPage; + this.enterpriseUsers.push(...data); + } catch (error) { + this.onError(); + } + }, + + resetEnterpriseSearch() { + this.enterpriseUsers = []; + this.enterpriseUsersNextPage = 1; + }, + onError() { createAlert({ message: __('There was a problem fetching users.'), @@ -171,6 +220,25 @@ export default { setSearch(searchTerm) { this.search = searchTerm; + + if (this.restrictReassignmentToEnterprise) { + this.resetEnterpriseSearch(); + this.getGroupEnterpriseUsers(); + } + }, + + createUserObjectFromEnterprise(user) { + const gid = `gid://gitlab/User/${user.id}`; + + return { + username: user.username, + webUrl: user.web_url, + webPath: user.web_path, + avatarUrl: user.avatar_url, + id: gid, + text: user.name, + value: gid, + }; }, onSelect(value) { diff --git a/ee/app/assets/javascripts/api/groups_api.js b/ee/app/assets/javascripts/api/groups_api.js index 6cfacfc990466e..51d7dbb9c2d3ad 100644 --- a/ee/app/assets/javascripts/api/groups_api.js +++ b/ee/app/assets/javascripts/api/groups_api.js @@ -10,6 +10,7 @@ const GROUPS_BILLABLE_MEMBERS_SINGLE_MEMBERSHIPS_PATH = '/api/:version/groups/:group_id/billable_members/:member_id/memberships'; const GROUPS_BILLABLE_MEMBERS_SINGLE_INDIRECT_MEMBERSHIPS_PATH = '/api/:version/groups/:group_id/billable_members/:member_id/indirect'; +const GROUPS_ENTERPRISE_USERS_PATH = '/api/:version/groups/:id/enterprise_users'; export const fetchBillableGroupMembersList = (namespaceId, options = {}) => { const url = buildApiUrl(GROUPS_BILLABLE_MEMBERS_PATH).replace(':id', namespaceId); @@ -50,6 +51,21 @@ export const removeBillableMemberFromGroup = (groupId, memberId) => { return axios.delete(url); }; +export const fetchGroupEnterpriseUsers = (groupId, options) => { + const url = buildApiUrl(GROUPS_ENTERPRISE_USERS_PATH).replace(':id', groupId); + const defaults = { + per_page: DEFAULT_PER_PAGE, + page: 1, + }; + + return axios.get(url, { + params: { + ...defaults, + ...options, + }, + }); +}; + export const updateGroupSettings = (id, settings) => { const url = buildApiUrl(GROUP_PATH).replace(':id', id); diff --git a/ee/app/helpers/ee/groups/group_members_helper.rb b/ee/app/helpers/ee/groups/group_members_helper.rb index a8e69018ea2ec3..7ca99a904bc5e2 100644 --- a/ee/app/helpers/ee/groups/group_members_helper.rb +++ b/ee/app/helpers/ee/groups/group_members_helper.rb @@ -32,7 +32,9 @@ def group_members_app_data( manage_member_roles_path: manage_member_roles_path(group), promotion_request: { enabled: member_promotion_management_enabled?, total_items: pending_members_count }, can_approve_access_requests: !::Namespaces::FreeUserCap::Enforcement.new(group.root_ancestor).reached_limit?, - namespace_user_limit: ::Namespaces::FreeUserCap.dashboard_limit + namespace_user_limit: ::Namespaces::FreeUserCap.dashboard_limit, + restrict_reassignment_to_enterprise: + group.domain_verification_available? && group.all_projects_pages_domains(only_verified: true).any? }) end # rubocop:enable Metrics/ParameterLists diff --git a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb index 86e8fd7af7ff97..ea4227f115fff7 100644 --- a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb +++ b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb @@ -55,6 +55,45 @@ expect(subject[:manage_member_roles_path]).to eq(admin_application_settings_roles_and_permissions_path) end + context 'when domain_verification_available? is true' do + let_it_be(:group, reload: true) { create(:group, path: 'root-path') } + let_it_be(:project) { create(:project, group:) } + + before do + stub_licensed_features(domain_verification: true) + allow(::Gitlab).to receive(:com?).and_return(true) + end + + context 'when there are verified domains' do + let_it_be(:pages_domain) { create(:pages_domain, project:) } + + it 'sets the restrict_reassignment_to_enterprise flag to true' do + expect(subject[:restrict_reassignment_to_enterprise]).to be(true) + end + end + + context 'when there are no verified domains' do + it 'sets the restrict_reassignment_to_enterprise flag to false' do + expect(subject[:restrict_reassignment_to_enterprise]).to be(false) + end + end + end + + context 'when domain_verification_available? is false' do + let_it_be(:group, reload: true) { create(:group, path: 'root-path') } + let_it_be(:project) { create(:project, group:) } + let_it_be(:pages_domain) { create(:pages_domain, project:) } + + before do + stub_licensed_features(domain_verification: false) + allow(::Gitlab).to receive(:com?).and_return(true) + end + + it 'sets the restrict_reassignment_to_enterprise flag to false' do + expect(subject[:restrict_reassignment_to_enterprise]).to be(false) + end + end + context 'adds `can_approve_access_requests`' do before do stub_ee_application_setting(dashboard_limit_enabled: true) diff --git a/spec/frontend/members/placeholders/components/app_spec.js b/spec/frontend/members/placeholders/components/app_spec.js index d3d9fe6a0cdef6..db0c721c388331 100644 --- a/spec/frontend/members/placeholders/components/app_spec.js +++ b/spec/frontend/members/placeholders/components/app_spec.js @@ -72,6 +72,7 @@ describe('PlaceholdersTabApp', () => { provide: { group: mockGroup, reassignmentCsvPath: 'foo/bar', + restrictReassignmentToEnterprise: false, ...provide, }, mocks: { $toast }, diff --git a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index 0e35acb78c7a8d..93a18a87a76c93 100644 --- a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -6,6 +6,7 @@ import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; +import * as GroupsApi from 'ee/api/groups_api'; import PlaceholderActions from '~/members/placeholders/components/placeholder_actions.vue'; import searchUsersQuery from '~/graphql_shared/queries/users_search_all_paginated.query.graphql'; import importSourceUsersQuery from '~/members/placeholders/graphql/queries/import_source_users.query.graphql'; @@ -25,18 +26,35 @@ import { mockUser2, mockUsersQueryResponse, mockUsersWithPaginationQueryResponse, + mockEnterpriseUser1, + mockEnterpriseUser2, + mockEnterpriseUsersQueryResponse, + mockEnterpriseUsersWithPaginationQueryResponse, } from '../mock_data'; Vue.use(VueApollo); jest.mock('~/alert'); +jest.mock('ee_component/api/groups_api'); describe('PlaceholderActions', () => { let wrapper; let mockApollo; + const group = { + path: 'imported-group', + name: 'Imported group', + }; + + const restrictReassignmentToEnterprise = false; + const defaultProps = { sourceUser: mockSourceUsers[0], }; + const defaultProvide = { + group, + restrictReassignmentToEnterprise, + }; + const usersQueryHandler = jest.fn().mockResolvedValue(mockUsersQueryResponse); const sourceUsersQueryHandler = jest.fn().mockResolvedValue(mockSourceUsersQueryResponse()); const reassignMutationHandler = jest.fn().mockResolvedValue(mockReassignMutationResponse); @@ -53,7 +71,11 @@ describe('PlaceholderActions', () => { show: jest.fn(), }; - const createComponent = ({ seachUsersQueryHandler = usersQueryHandler, props = {} } = {}) => { + const createComponent = ({ + seachUsersQueryHandler = usersQueryHandler, + props = {}, + provide = {}, + } = {}) => { mockApollo = createMockApollo([ [searchUsersQuery, seachUsersQueryHandler], [importSourceUsersQuery, sourceUsersQueryHandler], @@ -79,6 +101,10 @@ describe('PlaceholderActions', () => { ...defaultProps, ...props, }, + provide: { + ...defaultProvide, + ...provide, + }, mocks: { $toast }, }); }; @@ -230,6 +256,77 @@ describe('PlaceholderActions', () => { }); }); + describe('when restrictReassignmentToEnterprise is true', () => { + describe('when users query is loading', () => { + beforeEach(() => { + GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); + }); + + it('renders listbox as loading', () => { + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + + expect(findListbox().props('loading')).toBe(true); + }); + }); + + describe('when users query succeeds and has pagination', () => { + beforeEach(async () => { + GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce( + mockEnterpriseUsersWithPaginationQueryResponse, + ); + GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse); + + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + await waitForPromises(); + }); + + describe('when "bottom-reached" event is emitted', () => { + beforeEach(() => { + findListbox().vm.$emit('bottom-reached'); + }); + + it('calls getGroupEnterpriseUsers again to get next page', () => { + expect(findListbox().props('infiniteScrollLoading')).toBe(true); + + expect(GroupsApi.fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); + }); + + it('appends query results to "items"', async () => { + const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; + + await waitForPromises(); + + expect(findListbox().props('infiniteScrollLoading')).toBe(false); + + const dropdownItems = findListbox().props('items'); + expect(dropdownItems).toHaveLength(allUsers.length); + dropdownItems.forEach((user, index) => { + expect(user).toMatchObject({ + id: `gid://gitlab/User/${allUsers[index].id}`, + username: allUsers[index].username, + value: `gid://gitlab/User/${allUsers[index].id}`, + }); + }); + }); + }); + }); + + describe('when users query fails', () => { + beforeEach(async () => { + GroupsApi.fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); + + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + await waitForPromises(); + }); + + it('creates an alert', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: 'There was a problem fetching users.', + }); + }); + }); + }); + describe('when users query succeeds and has pagination', () => { const usersPaginatedQueryHandler = jest.fn(); diff --git a/spec/frontend/members/placeholders/components/placeholders_table_spec.js b/spec/frontend/members/placeholders/components/placeholders_table_spec.js index eb502f8f1f94f6..d12c7f6e0a20da 100644 --- a/spec/frontend/members/placeholders/components/placeholders_table_spec.js +++ b/spec/frontend/members/placeholders/components/placeholders_table_spec.js @@ -84,6 +84,7 @@ describe('PlaceholdersTable', () => { }, provide: { group: mockGroup, + restrictReassignmentToEnterprise: false, }, mocks: { $toast }, directives: { diff --git a/spec/frontend/members/placeholders/mock_data.js b/spec/frontend/members/placeholders/mock_data.js index 77f85857194f88..bfec0bb3a871fb 100644 --- a/spec/frontend/members/placeholders/mock_data.js +++ b/spec/frontend/members/placeholders/mock_data.js @@ -206,6 +206,34 @@ export const mockUsersWithPaginationQueryResponse = { }, }; +export const mockEnterpriseUser1 = { + username: 'Administrator', + web_url: '/root', + web_path: '/root', + avatar_url: '/avatar1', + id: 1, + name: 'Admin', +}; + +export const mockEnterpriseUser2 = { + username: 'Rookie', + web_url: '/rookie', + web_path: '/rookie', + avatar_url: '/avatar2', + id: 2, + name: 'Rookie', +}; + +export const mockEnterpriseUsersQueryResponse = { + data: [mockEnterpriseUser1], + headers: { 'X-Next-Page': null }, +}; + +export const mockEnterpriseUsersWithPaginationQueryResponse = { + data: [mockEnterpriseUser2], + headers: { 'X-Next-Page': 2 }, +}; + export const pagination = { awaitingReassignmentItems: 5, reassignedItems: 2, -- GitLab From d92267a86ca589a9263f5ae561f82fcf61a8f215 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Fri, 11 Apr 2025 12:14:02 +0100 Subject: [PATCH 02/29] Add to docs about reassigning to enterprise users --- doc/user/group/import/direct_transfer_migrations.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/user/group/import/direct_transfer_migrations.md b/doc/user/group/import/direct_transfer_migrations.md index f65c2264be0dd4..1f8acf6e10fa7a 100644 --- a/doc/user/group/import/direct_transfer_migrations.md +++ b/doc/user/group/import/direct_transfer_migrations.md @@ -122,6 +122,9 @@ has an existing membership in the destination namespace with a [higher role](../ the one being mapped, the membership is mapped as a direct membership instead. This ensures the member does not get elevated permissions. +If the root namespace has domain verification enabled with at least 1 verified domain, then users can only be +reassigned to [Enterprise Users](../../enterprise_user/_index.md). + {{< alert type="note" >}} There is a [known issue](_index.md#known-issues) affecting the mapping of shared memberships. -- GitLab From eb65df124646f169bb00917d7e150a7bb78c1371 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Fri, 11 Apr 2025 12:54:35 +0100 Subject: [PATCH 03/29] Move imports --- .../members/placeholders/components/placeholder_actions.vue | 2 +- .../members/placeholders/components/placeholder_actions_spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index 6916f639ad27dc..a7b0f64fae4dc4 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -2,6 +2,7 @@ import produce from 'immer'; import { debounce, isEmpty, isNull } from 'lodash'; import { GlAvatarLabeled, GlButton, GlCollapsibleListbox } from '@gitlab/ui'; +import { fetchGroupEnterpriseUsers } from 'ee_component/api/groups_api'; import { __, s__ } from '~/locale'; import { createAlert } from '~/alert'; import { @@ -15,7 +16,6 @@ import { PLACEHOLDER_STATUS_AWAITING_APPROVAL, PLACEHOLDER_STATUS_REASSIGNING, } from '~/import_entities/import_groups/constants'; -import { fetchGroupEnterpriseUsers } from 'ee_component/api/groups_api'; import importSourceUsersQuery from '../graphql/queries/import_source_users.query.graphql'; import importSourceUserReassignMutation from '../graphql/mutations/reassign.mutation.graphql'; import importSourceUserKeepAsPlaceholderMutation from '../graphql/mutations/keep_as_placeholder.mutation.graphql'; diff --git a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index 93a18a87a76c93..5aea79a12e9d40 100644 --- a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -1,12 +1,12 @@ import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import { GlCollapsibleListbox } from '@gitlab/ui'; +import * as GroupsApi from 'ee/api/groups_api'; import { createAlert } from '~/alert'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; -import * as GroupsApi from 'ee/api/groups_api'; import PlaceholderActions from '~/members/placeholders/components/placeholder_actions.vue'; import searchUsersQuery from '~/graphql_shared/queries/users_search_all_paginated.query.graphql'; import importSourceUsersQuery from '~/members/placeholders/graphql/queries/import_source_users.query.graphql'; -- GitLab From 78d1d099e590bab9b44ba4c449ee2dc3b98f2a8f Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Fri, 11 Apr 2025 14:27:15 +0100 Subject: [PATCH 04/29] Use ee_component instead of ee This fixes the FOSS pipeline --- .../members/placeholders/components/placeholder_actions_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index 5aea79a12e9d40..b100d559b078fc 100644 --- a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -1,7 +1,7 @@ import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import { GlCollapsibleListbox } from '@gitlab/ui'; -import * as GroupsApi from 'ee/api/groups_api'; +import * as GroupsApi from 'ee_component/api/groups_api'; import { createAlert } from '~/alert'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; -- GitLab From df1a8335de0625de0a4e858896bb7cff2e89a7dd Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Fri, 11 Apr 2025 15:22:07 +0100 Subject: [PATCH 05/29] Move EE specific specs to ee dir --- .../components/placeholder_actions_spec.js | 133 ++++++++++++++++++ .../members/placeholders/mock_data.js | 78 ++++++++++ .../components/placeholder_actions_spec.js | 90 +----------- .../members/placeholders/mock_data.js | 28 ---- 4 files changed, 214 insertions(+), 115 deletions(-) create mode 100644 ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js create mode 100644 ee/spec/frontend/members/placeholders/mock_data.js diff --git a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js new file mode 100644 index 00000000000000..d35143c4a42ece --- /dev/null +++ b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -0,0 +1,133 @@ +import Vue from 'vue'; +import VueApollo from 'vue-apollo'; +import { GlCollapsibleListbox } from '@gitlab/ui'; +import * as GroupsApi from 'ee_component/api/groups_api'; +import { createAlert } from '~/alert'; +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; +import createMockApollo from 'helpers/mock_apollo_helper'; +import waitForPromises from 'helpers/wait_for_promises'; + +import PlaceholderActions from '~/members/placeholders/components/placeholder_actions.vue'; + +import { + mockSourceUsers, + mockEnterpriseUser1, + mockEnterpriseUser2, + mockEnterpriseUsersQueryResponse, + mockEnterpriseUsersWithPaginationQueryResponse, +} from '../mock_data'; + +Vue.use(VueApollo); +jest.mock('~/alert'); +jest.mock('ee_component/api/groups_api'); + +describe('PlaceholderActions', () => { + let wrapper; + + const group = { + path: 'imported-group', + name: 'Imported group', + }; + + const restrictReassignmentToEnterprise = false; + + const defaultProps = { + sourceUser: mockSourceUsers[0], + }; + const defaultProvide = { + group, + restrictReassignmentToEnterprise, + }; + + const $toast = { + show: jest.fn(), + }; + + const createComponent = ({ props = {}, provide = {} } = {}) => { + wrapper = shallowMountExtended(PlaceholderActions, { + apolloProvider: createMockApollo(), + propsData: { + ...defaultProps, + ...props, + }, + provide: { + ...defaultProvide, + ...provide, + }, + mocks: { $toast }, + }); + }; + + const findListbox = () => wrapper.findComponent(GlCollapsibleListbox); + + describe('when restrictReassignmentToEnterprise is true', () => { + describe('when users query is loading', () => { + beforeEach(() => { + GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); + }); + + it('renders listbox as loading', () => { + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + + expect(findListbox().props('loading')).toBe(true); + }); + }); + + describe('when users query succeeds and has pagination', () => { + beforeEach(async () => { + GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce( + mockEnterpriseUsersWithPaginationQueryResponse, + ); + GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse); + + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + await waitForPromises(); + }); + + describe('when "bottom-reached" event is emitted', () => { + beforeEach(() => { + findListbox().vm.$emit('bottom-reached'); + }); + + it('calls getGroupEnterpriseUsers again to get next page', () => { + expect(findListbox().props('infiniteScrollLoading')).toBe(true); + + expect(GroupsApi.fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); + }); + + it('appends query results to "items"', async () => { + const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; + + await waitForPromises(); + + expect(findListbox().props('infiniteScrollLoading')).toBe(false); + + const dropdownItems = findListbox().props('items'); + expect(dropdownItems).toHaveLength(allUsers.length); + dropdownItems.forEach((user, index) => { + expect(user).toMatchObject({ + id: `gid://gitlab/User/${allUsers[index].id}`, + username: allUsers[index].username, + value: `gid://gitlab/User/${allUsers[index].id}`, + }); + }); + }); + }); + }); + + describe('when users query fails', () => { + beforeEach(async () => { + GroupsApi.fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); + + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + await waitForPromises(); + }); + + it('creates an alert', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: 'There was a problem fetching users.', + }); + }); + }); + }); +}); diff --git a/ee/spec/frontend/members/placeholders/mock_data.js b/ee/spec/frontend/members/placeholders/mock_data.js new file mode 100644 index 00000000000000..8e5cddd104daf6 --- /dev/null +++ b/ee/spec/frontend/members/placeholders/mock_data.js @@ -0,0 +1,78 @@ +const createMockPlaceholderUser = (index) => { + return { + __typename: 'UserCore', + id: `gid://gitlab/User/382${index}`, + avatarUrl: '/avatar1', + name: `Placeholder ${index}`, + username: `placeholder_${index}`, + webUrl: '/', + webPath: '/', + }; +}; + +const createMockReassignUser = (index) => { + return { + __typename: 'UserCore', + id: `gid://gitlab/User/741${index}`, + avatarUrl: '/avatar2', + name: `Reassigned ${index}`, + username: `reassigned_${index}`, + webUrl: '/', + webPath: '/', + }; +}; + +export const importSourceUserCreatedAt = '2024-09-17T00:41:28.000Z'; + +const createMockSourceUser = ( + index, + { status, placeholderUser = true, reassignToUser = false, sourceUserExists = true } = {}, +) => { + return { + __typename: 'ImportSourceUser', + createdAt: importSourceUserCreatedAt, + id: `gid://gitlab/Import::SourceUser/${index}`, + sourceHostname: 'https://gitlab.com', + sourceName: sourceUserExists ? `Old User ${index}` : null, + sourceUsername: sourceUserExists ? `old_user_${index}` : null, + sourceUserIdentifier: index, + status, + reassignmentError: null, + placeholderUser: placeholderUser ? createMockPlaceholderUser(index) : null, + reassignToUser: reassignToUser ? createMockReassignUser(index) : null, + }; +}; + +export const mockSourceUsers = [ + createMockSourceUser(1, { + status: 'PENDING_REASSIGNMENT', + }), +]; + +export const mockEnterpriseUser1 = { + username: 'Administrator', + web_url: '/root', + web_path: '/root', + avatar_url: '/avatar1', + id: 1, + name: 'Admin', +}; + +export const mockEnterpriseUser2 = { + username: 'Rookie', + web_url: '/rookie', + web_path: '/rookie', + avatar_url: '/avatar2', + id: 2, + name: 'Rookie', +}; + +export const mockEnterpriseUsersQueryResponse = { + data: [mockEnterpriseUser1], + headers: { 'X-Next-Page': null }, +}; + +export const mockEnterpriseUsersWithPaginationQueryResponse = { + data: [mockEnterpriseUser2], + headers: { 'X-Next-Page': 2 }, +}; diff --git a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index b100d559b078fc..5801e1f9b995e8 100644 --- a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -1,7 +1,6 @@ import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import { GlCollapsibleListbox } from '@gitlab/ui'; -import * as GroupsApi from 'ee_component/api/groups_api'; import { createAlert } from '~/alert'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; @@ -26,15 +25,10 @@ import { mockUser2, mockUsersQueryResponse, mockUsersWithPaginationQueryResponse, - mockEnterpriseUser1, - mockEnterpriseUser2, - mockEnterpriseUsersQueryResponse, - mockEnterpriseUsersWithPaginationQueryResponse, } from '../mock_data'; Vue.use(VueApollo); jest.mock('~/alert'); -jest.mock('ee_component/api/groups_api'); describe('PlaceholderActions', () => { let wrapper; @@ -50,7 +44,7 @@ describe('PlaceholderActions', () => { const defaultProps = { sourceUser: mockSourceUsers[0], }; - const defaultProvide = { + const provide = { group, restrictReassignmentToEnterprise, }; @@ -71,11 +65,7 @@ describe('PlaceholderActions', () => { show: jest.fn(), }; - const createComponent = ({ - seachUsersQueryHandler = usersQueryHandler, - props = {}, - provide = {}, - } = {}) => { + const createComponent = ({ seachUsersQueryHandler = usersQueryHandler, props = {} } = {}) => { mockApollo = createMockApollo([ [searchUsersQuery, seachUsersQueryHandler], [importSourceUsersQuery, sourceUsersQueryHandler], @@ -101,10 +91,7 @@ describe('PlaceholderActions', () => { ...defaultProps, ...props, }, - provide: { - ...defaultProvide, - ...provide, - }, + provide, mocks: { $toast }, }); }; @@ -256,77 +243,6 @@ describe('PlaceholderActions', () => { }); }); - describe('when restrictReassignmentToEnterprise is true', () => { - describe('when users query is loading', () => { - beforeEach(() => { - GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); - }); - - it('renders listbox as loading', () => { - createComponent({ provide: { restrictReassignmentToEnterprise: true } }); - - expect(findListbox().props('loading')).toBe(true); - }); - }); - - describe('when users query succeeds and has pagination', () => { - beforeEach(async () => { - GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce( - mockEnterpriseUsersWithPaginationQueryResponse, - ); - GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse); - - createComponent({ provide: { restrictReassignmentToEnterprise: true } }); - await waitForPromises(); - }); - - describe('when "bottom-reached" event is emitted', () => { - beforeEach(() => { - findListbox().vm.$emit('bottom-reached'); - }); - - it('calls getGroupEnterpriseUsers again to get next page', () => { - expect(findListbox().props('infiniteScrollLoading')).toBe(true); - - expect(GroupsApi.fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); - }); - - it('appends query results to "items"', async () => { - const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; - - await waitForPromises(); - - expect(findListbox().props('infiniteScrollLoading')).toBe(false); - - const dropdownItems = findListbox().props('items'); - expect(dropdownItems).toHaveLength(allUsers.length); - dropdownItems.forEach((user, index) => { - expect(user).toMatchObject({ - id: `gid://gitlab/User/${allUsers[index].id}`, - username: allUsers[index].username, - value: `gid://gitlab/User/${allUsers[index].id}`, - }); - }); - }); - }); - }); - - describe('when users query fails', () => { - beforeEach(async () => { - GroupsApi.fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); - - createComponent({ provide: { restrictReassignmentToEnterprise: true } }); - await waitForPromises(); - }); - - it('creates an alert', () => { - expect(createAlert).toHaveBeenCalledWith({ - message: 'There was a problem fetching users.', - }); - }); - }); - }); - describe('when users query succeeds and has pagination', () => { const usersPaginatedQueryHandler = jest.fn(); diff --git a/spec/frontend/members/placeholders/mock_data.js b/spec/frontend/members/placeholders/mock_data.js index bfec0bb3a871fb..77f85857194f88 100644 --- a/spec/frontend/members/placeholders/mock_data.js +++ b/spec/frontend/members/placeholders/mock_data.js @@ -206,34 +206,6 @@ export const mockUsersWithPaginationQueryResponse = { }, }; -export const mockEnterpriseUser1 = { - username: 'Administrator', - web_url: '/root', - web_path: '/root', - avatar_url: '/avatar1', - id: 1, - name: 'Admin', -}; - -export const mockEnterpriseUser2 = { - username: 'Rookie', - web_url: '/rookie', - web_path: '/rookie', - avatar_url: '/avatar2', - id: 2, - name: 'Rookie', -}; - -export const mockEnterpriseUsersQueryResponse = { - data: [mockEnterpriseUser1], - headers: { 'X-Next-Page': null }, -}; - -export const mockEnterpriseUsersWithPaginationQueryResponse = { - data: [mockEnterpriseUser2], - headers: { 'X-Next-Page': 2 }, -}; - export const pagination = { awaitingReassignmentItems: 5, reassignedItems: 2, -- GitLab From 40a9bcc5aa62702801f44aff947acbccf3983ca7 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Mon, 14 Apr 2025 12:45:05 +0100 Subject: [PATCH 06/29] Replace ee_component with ee_else_ce --- .../members/placeholders/components/placeholder_actions.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index a7b0f64fae4dc4..ab427183eb2840 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -2,7 +2,7 @@ import produce from 'immer'; import { debounce, isEmpty, isNull } from 'lodash'; import { GlAvatarLabeled, GlButton, GlCollapsibleListbox } from '@gitlab/ui'; -import { fetchGroupEnterpriseUsers } from 'ee_component/api/groups_api'; +import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; import { __, s__ } from '~/locale'; import { createAlert } from '~/alert'; import { -- GitLab From b1cb5267ed44049fdff2ff1dc3acfe5f7480be61 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Mon, 14 Apr 2025 12:48:33 +0100 Subject: [PATCH 07/29] Remove props option from createComponent --- .../placeholders/components/placeholder_actions_spec.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index d35143c4a42ece..af125765f9dd95 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -31,7 +31,7 @@ describe('PlaceholderActions', () => { const restrictReassignmentToEnterprise = false; - const defaultProps = { + const propsData = { sourceUser: mockSourceUsers[0], }; const defaultProvide = { @@ -43,13 +43,10 @@ describe('PlaceholderActions', () => { show: jest.fn(), }; - const createComponent = ({ props = {}, provide = {} } = {}) => { + const createComponent = ({ provide = {} } = {}) => { wrapper = shallowMountExtended(PlaceholderActions, { apolloProvider: createMockApollo(), - propsData: { - ...defaultProps, - ...props, - }, + propsData, provide: { ...defaultProvide, ...provide, -- GitLab From 0aa0660c77af7f891e1a08e52d15ca020ddcef03 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Mon, 14 Apr 2025 12:53:27 +0100 Subject: [PATCH 08/29] Change 1 to one in docs --- doc/user/group/import/direct_transfer_migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/group/import/direct_transfer_migrations.md b/doc/user/group/import/direct_transfer_migrations.md index 1f8acf6e10fa7a..54b83b1371eb63 100644 --- a/doc/user/group/import/direct_transfer_migrations.md +++ b/doc/user/group/import/direct_transfer_migrations.md @@ -122,7 +122,7 @@ has an existing membership in the destination namespace with a [higher role](../ the one being mapped, the membership is mapped as a direct membership instead. This ensures the member does not get elevated permissions. -If the root namespace has domain verification enabled with at least 1 verified domain, then users can only be +If the root namespace has domain verification enabled with at least one verified domain, then users can only be reassigned to [Enterprise Users](../../enterprise_user/_index.md). {{< alert type="note" >}} -- GitLab From 78f29523d45cb657b341ee3810ef6262c4b66bfa Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Mon, 14 Apr 2025 14:36:59 +0100 Subject: [PATCH 09/29] Update imports to work in foss --- .../members/placeholders/components/placeholder_actions.vue | 4 ++-- .../placeholders/components/placeholder_actions_spec.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index ab427183eb2840..06b37fe91f3208 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -2,7 +2,7 @@ import produce from 'immer'; import { debounce, isEmpty, isNull } from 'lodash'; import { GlAvatarLabeled, GlButton, GlCollapsibleListbox } from '@gitlab/ui'; -import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; +import * as GroupsApi from 'ee_else_ce/api/groups_api'; import { __, s__ } from '~/locale'; import { createAlert } from '~/alert'; import { @@ -193,7 +193,7 @@ export default { async getGroupEnterpriseUsers() { try { - const { data, headers } = await fetchGroupEnterpriseUsers(this.group.id, { + const { data, headers } = await GroupsApi.fetchGroupEnterpriseUsers(this.group.id, { page: this.enterpriseUsersNextPage, per_page: USERS_PER_PAGE, search: this.search, diff --git a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index af125765f9dd95..6eff83d54c422f 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -1,7 +1,7 @@ import Vue from 'vue'; import VueApollo from 'vue-apollo'; import { GlCollapsibleListbox } from '@gitlab/ui'; -import * as GroupsApi from 'ee_component/api/groups_api'; +import * as GroupsApi from 'ee/api/groups_api'; import { createAlert } from '~/alert'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; -- GitLab From baae8a29cabbb9cefef3952de5c2bd220b27cda0 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Mon, 14 Apr 2025 14:42:12 +0100 Subject: [PATCH 10/29] Use destructuring assignment --- .../components/placeholder_actions.vue | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index 06b37fe91f3208..1965c91a43d75f 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -227,16 +227,23 @@ export default { } }, - createUserObjectFromEnterprise(user) { - const gid = `gid://gitlab/User/${user.id}`; + createUserObjectFromEnterprise({ + id, + username, + web_url: webUrl, + web_path: webPath, + avatar_url: avatarUrl, + name, + }) { + const gid = `gid://gitlab/User/${id}`; return { - username: user.username, - webUrl: user.web_url, - webPath: user.web_path, - avatarUrl: user.avatar_url, + username, + webUrl, + webPath, + avatarUrl, id: gid, - text: user.name, + text: name, value: gid, }; }, -- GitLab From 31eb9a5f3ed6a65017dd6d28495e79318dcf3ee0 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Tue, 15 Apr 2025 10:23:21 +0100 Subject: [PATCH 11/29] Ensure users are enterprise of the root namespace Check that the users are enterprise users of the root namespace, when reassigning contributions. This check only applies when enterprise users are enabled --- .../helpers/ee/groups/group_members_helper.rb | 3 +- ee/app/models/ee/namespace.rb | 5 +++ .../import/source_users/reassign_service.rb | 25 +++++++++++- ee/spec/models/ee/namespace_spec.rb | 38 +++++++++++++++++++ .../source_users/reassign_service_spec.rb | 35 +++++++++++++++++ locale/gitlab.pot | 3 ++ 6 files changed, 106 insertions(+), 3 deletions(-) diff --git a/ee/app/helpers/ee/groups/group_members_helper.rb b/ee/app/helpers/ee/groups/group_members_helper.rb index 7ca99a904bc5e2..0430e514e6659f 100644 --- a/ee/app/helpers/ee/groups/group_members_helper.rb +++ b/ee/app/helpers/ee/groups/group_members_helper.rb @@ -33,8 +33,7 @@ def group_members_app_data( promotion_request: { enabled: member_promotion_management_enabled?, total_items: pending_members_count }, can_approve_access_requests: !::Namespaces::FreeUserCap::Enforcement.new(group.root_ancestor).reached_limit?, namespace_user_limit: ::Namespaces::FreeUserCap.dashboard_limit, - restrict_reassignment_to_enterprise: - group.domain_verification_available? && group.all_projects_pages_domains(only_verified: true).any? + restrict_reassignment_to_enterprise: group.domain_verification_in_use? }) end # rubocop:enable Metrics/ParameterLists diff --git a/ee/app/models/ee/namespace.rb b/ee/app/models/ee/namespace.rb index 3a0e4a9f7e8431..cc039d197b2b4c 100644 --- a/ee/app/models/ee/namespace.rb +++ b/ee/app/models/ee/namespace.rb @@ -611,6 +611,11 @@ def domain_verification_available? ::Gitlab.com? && root? && licensed_feature_available?(:domain_verification) end + def domain_verification_in_use? + domain_verification_available? && + all_projects_pages_domains(only_verified: true).any? + end + def enforce_ssh_certificates? root? && namespace_settings&.enforce_ssh_certificates? end diff --git a/ee/app/services/ee/import/source_users/reassign_service.rb b/ee/app/services/ee/import/source_users/reassign_service.rb index 83f9cbae0a5d6b..5004bd0355be50 100644 --- a/ee/app/services/ee/import/source_users/reassign_service.rb +++ b/ee/app/services/ee/import/source_users/reassign_service.rb @@ -14,7 +14,11 @@ def run_validations return validation_error if validation_error - error_invalid_assignee_due_to_sso_enforcement unless valid_assignee_if_sso_enforcement_is_applicable? + return error_invalid_assignee_due_to_sso_enforcement unless valid_assignee_if_sso_enforcement_is_applicable? + + return if valid_assignee_if_domain_verification_is_applicable? + + error_invalid_assignee_due_to_domain_verification end def valid_assignee_if_sso_enforcement_is_applicable? @@ -29,6 +33,25 @@ def error_invalid_assignee_due_to_sso_enforcement ) end + def valid_assignee_if_domain_verification_is_applicable? + return true unless root_namespace.domain_verification_in_use? + + assignee_user.enterprise_group == root_namespace + end + + def error_invalid_assignee_due_to_domain_verification + ServiceResponse.error( + message: invalid_assignee_due_to_domain_verification_message, + reason: :invalid_assignee, + payload: import_source_user + ) + end + + def invalid_assignee_due_to_domain_verification_message + s_('UserMapping|You can assign only to Enterprise Users of the root namespace. ' \ + 'Ensure the user is an Enterprise member of the top level namespace you are importing to.') + end + def invalid_assignee_due_to_sso_enforcement_message s_('UserMapping|You can assign only users with linked SAML and SCIM identities. ' \ 'Ensure the user has signed into GitLab through your SAML SSO provider and has an ' \ diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb index e3b8cd3432ee7e..66763773094edc 100644 --- a/ee/spec/models/ee/namespace_spec.rb +++ b/ee/spec/models/ee/namespace_spec.rb @@ -2292,6 +2292,44 @@ end end + describe '#domain_verification_in_use?' do + let_it_be(:group, reload: true) { create(:group, path: 'root-path') } + let_it_be(:project) { create(:project, group:) } + + let!(:pages_domain) { create(:pages_domain, project:) } + + subject { group.domain_verification_in_use? } + + before do + allow(::Gitlab).to receive(:com?).and_return(true) + stub_licensed_features(domain_verification: true) + end + + it { is_expected.to be(true) } + + context 'when not on Gitlab.com' do + before do + allow(::Gitlab).to receive(:com?).and_return(false) + end + + it { is_expected.to be(false) } + end + + context 'when domain_verification feature is not available' do + before do + stub_licensed_features(domain_verification: false) + end + + it { is_expected.to be(false) } + end + + context 'when there is no pages domain' do + let!(:pages_domain) { nil } + + it { is_expected.to be(false) } + end + end + describe '#enforce_ssh_certificates?' do let(:namespace) { create(:group) } diff --git a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb index 7f6cc099b7c5fd..4ba0464016383e 100644 --- a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb +++ b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb @@ -88,5 +88,40 @@ end end end + + context 'for domain verification requirements' do + let_it_be(:project) { create(:project, group: subgroup) } + + before do + stub_licensed_features(domain_verification: true) + allow(::Gitlab).to receive(:com?).and_return(true) + end + + context 'when there are verified domains' do + let_it_be(:pages_domain) { create(:pages_domain, project:) } + + context 'when the user is part of the enterprise group' do + before do + assignee_user.update!(enterprise_group: group) + end + + it_behaves_like 'success response' + end + + context 'when the user is not part of the enterprise group' do + before do + assignee_user.update!(enterprise_group: nil) + end + + it_behaves_like 'an error response', + error: 'You can assign only to Enterprise Users of the root namespace. ' \ + 'Ensure the user is an Enterprise member of the top level namespace you are importing to.' + end + end + + context 'when there are no verified domains' do + it_behaves_like 'success response' + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b26f215cc85c9d..4a471e51ad4a49 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -65521,6 +65521,9 @@ msgstr "" msgid "UserMapping|You can assign only active users with regular or auditor access. To assign users with administrator access, ask your GitLab administrator to enable the \"Allow contribution mapping to administrators\" setting." msgstr "" +msgid "UserMapping|You can assign only to Enterprise Users of the root namespace. Ensure the user is an Enterprise member of the top level namespace you are importing to." +msgstr "" + msgid "UserMapping|You can assign only users with linked SAML and SCIM identities. Ensure the user has signed into GitLab through your SAML SSO provider and has an active SCIM identity for this group." msgstr "" -- GitLab From 3b5e32ce269cceb62fff5a3b6c2ded5063eac230 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 16 Apr 2025 12:41:21 +0100 Subject: [PATCH 12/29] Check users if enterprise users exist Only check if a user is an enterprise user if other enterprise users exist. This is instead of checking if domain verification is in use. --- .../helpers/ee/groups/group_members_helper.rb | 2 +- ee/app/models/ee/namespace.rb | 5 ++- .../import/source_users/reassign_service.rb | 14 ++++----- .../ee/groups/group_members_helper_spec.rb | 28 +++++++++++------ ee/spec/models/ee/namespace_spec.rb | 31 ++++++------------- .../source_users/reassign_service_spec.rb | 27 ++++++++-------- 6 files changed, 52 insertions(+), 55 deletions(-) diff --git a/ee/app/helpers/ee/groups/group_members_helper.rb b/ee/app/helpers/ee/groups/group_members_helper.rb index 0430e514e6659f..2ae7d8bef4609d 100644 --- a/ee/app/helpers/ee/groups/group_members_helper.rb +++ b/ee/app/helpers/ee/groups/group_members_helper.rb @@ -33,7 +33,7 @@ def group_members_app_data( promotion_request: { enabled: member_promotion_management_enabled?, total_items: pending_members_count }, can_approve_access_requests: !::Namespaces::FreeUserCap::Enforcement.new(group.root_ancestor).reached_limit?, namespace_user_limit: ::Namespaces::FreeUserCap.dashboard_limit, - restrict_reassignment_to_enterprise: group.domain_verification_in_use? + restrict_reassignment_to_enterprise: ::Gitlab.com? && group.any_enterprise_users? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks -- We only want this on .com }) end # rubocop:enable Metrics/ParameterLists diff --git a/ee/app/models/ee/namespace.rb b/ee/app/models/ee/namespace.rb index cc039d197b2b4c..2959d93fad1957 100644 --- a/ee/app/models/ee/namespace.rb +++ b/ee/app/models/ee/namespace.rb @@ -611,9 +611,8 @@ def domain_verification_available? ::Gitlab.com? && root? && licensed_feature_available?(:domain_verification) end - def domain_verification_in_use? - domain_verification_available? && - all_projects_pages_domains(only_verified: true).any? + def any_enterprise_users? + enterprise_users.any? end def enforce_ssh_certificates? diff --git a/ee/app/services/ee/import/source_users/reassign_service.rb b/ee/app/services/ee/import/source_users/reassign_service.rb index 5004bd0355be50..c8228005845408 100644 --- a/ee/app/services/ee/import/source_users/reassign_service.rb +++ b/ee/app/services/ee/import/source_users/reassign_service.rb @@ -16,9 +16,9 @@ def run_validations return error_invalid_assignee_due_to_sso_enforcement unless valid_assignee_if_sso_enforcement_is_applicable? - return if valid_assignee_if_domain_verification_is_applicable? + return if valid_assignee_if_should_check_enterprise_users? - error_invalid_assignee_due_to_domain_verification + error_invalid_assignee_due_to_enterprise_users_check end def valid_assignee_if_sso_enforcement_is_applicable? @@ -33,21 +33,21 @@ def error_invalid_assignee_due_to_sso_enforcement ) end - def valid_assignee_if_domain_verification_is_applicable? - return true unless root_namespace.domain_verification_in_use? + def valid_assignee_if_should_check_enterprise_users? + return true unless ::Gitlab.com? && root_namespace.any_enterprise_users? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks -- We only want this on .com assignee_user.enterprise_group == root_namespace end - def error_invalid_assignee_due_to_domain_verification + def error_invalid_assignee_due_to_enterprise_users_check ServiceResponse.error( - message: invalid_assignee_due_to_domain_verification_message, + message: invalid_assignee_due_to_enterprise_users_check_message, reason: :invalid_assignee, payload: import_source_user ) end - def invalid_assignee_due_to_domain_verification_message + def invalid_assignee_due_to_enterprise_users_check_message s_('UserMapping|You can assign only to Enterprise Users of the root namespace. ' \ 'Ensure the user is an Enterprise member of the top level namespace you are importing to.') end diff --git a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb index ea4227f115fff7..ece8840c3a6494 100644 --- a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb +++ b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb @@ -55,30 +55,38 @@ expect(subject[:manage_member_roles_path]).to eq(admin_application_settings_roles_and_permissions_path) end - context 'when domain_verification_available? is true' do - let_it_be(:group, reload: true) { create(:group, path: 'root-path') } - let_it_be(:project) { create(:project, group:) } + context 'when enterprise users exist for the group' do + let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } before do - stub_licensed_features(domain_verification: true) allow(::Gitlab).to receive(:com?).and_return(true) end - context 'when there are verified domains' do - let_it_be(:pages_domain) { create(:pages_domain, project:) } + it 'sets the restrict_reassignment_to_enterprise flag to true' do + expect(subject[:restrict_reassignment_to_enterprise]).to be(true) + end - it 'sets the restrict_reassignment_to_enterprise flag to true' do - expect(subject[:restrict_reassignment_to_enterprise]).to be(true) + context 'when not on .com' do + before do + allow(::Gitlab).to receive(:com?).and_return(false) end - end - context 'when there are no verified domains' do it 'sets the restrict_reassignment_to_enterprise flag to false' do expect(subject[:restrict_reassignment_to_enterprise]).to be(false) end end end + context 'when enterprise users do not exist for the group' do + before do + allow(::Gitlab).to receive(:com?).and_return(true) + end + + it 'sets the restrict_reassignment_to_enterprise flag to true' do + expect(subject[:restrict_reassignment_to_enterprise]).to be(false) + end + end + context 'when domain_verification_available? is false' do let_it_be(:group, reload: true) { create(:group, path: 'root-path') } let_it_be(:project) { create(:project, group:) } diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb index 66763773094edc..b730d59a24a79d 100644 --- a/ee/spec/models/ee/namespace_spec.rb +++ b/ee/spec/models/ee/namespace_spec.rb @@ -2292,40 +2292,29 @@ end end - describe '#domain_verification_in_use?' do - let_it_be(:group, reload: true) { create(:group, path: 'root-path') } - let_it_be(:project) { create(:project, group:) } + describe '#any_enterprise_users?' do + let_it_be(:group) { create(:group) } - let!(:pages_domain) { create(:pages_domain, project:) } - - subject { group.domain_verification_in_use? } + subject { group.any_enterprise_users? } before do allow(::Gitlab).to receive(:com?).and_return(true) - stub_licensed_features(domain_verification: true) end - it { is_expected.to be(true) } - - context 'when not on Gitlab.com' do - before do - allow(::Gitlab).to receive(:com?).and_return(false) - end + context 'when there are enterprise users for the group' do + let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } - it { is_expected.to be(false) } + it { is_expected.to be(true) } end - context 'when domain_verification feature is not available' do - before do - stub_licensed_features(domain_verification: false) - end + context 'when there are enterprise users for another group' do + let_it_be(:other_group) { create(:group) } + let_it_be(:enterprise_user) { create(:user, enterprise_group: other_group) } it { is_expected.to be(false) } end - context 'when there is no pages domain' do - let!(:pages_domain) { nil } - + context 'when there are no enterprise users' do it { is_expected.to be(false) } end end diff --git a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb index 4ba0464016383e..df617ec10e0f15 100644 --- a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb +++ b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb @@ -89,37 +89,38 @@ end end - context 'for domain verification requirements' do - let_it_be(:project) { create(:project, group: subgroup) } - + context 'for enterprise users check' do before do - stub_licensed_features(domain_verification: true) allow(::Gitlab).to receive(:com?).and_return(true) end - context 'when there are verified domains' do - let_it_be(:pages_domain) { create(:pages_domain, project:) } + context 'when there are enterprise users' do + let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } context 'when the user is part of the enterprise group' do - before do - assignee_user.update!(enterprise_group: group) - end + let_it_be(:assignee_user) { create(:user, enterprise_group: group) } it_behaves_like 'success response' end context 'when the user is not part of the enterprise group' do - before do - assignee_user.update!(enterprise_group: nil) - end + let_it_be(:assignee_user) { create(:user, enterprise_group: nil) } it_behaves_like 'an error response', error: 'You can assign only to Enterprise Users of the root namespace. ' \ 'Ensure the user is an Enterprise member of the top level namespace you are importing to.' + + context 'when not on .com' do + before do + allow(::Gitlab).to receive(:com?).and_return(false) + end + + it_behaves_like 'success response' + end end end - context 'when there are no verified domains' do + context 'when there are no enterprise users' do it_behaves_like 'success response' end end -- GitLab From 1eb2e9ae0c3da27f9a2858a56687748468efccad Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 16 Apr 2025 15:41:43 +0100 Subject: [PATCH 13/29] Add CE no-op for fetchGroupEnterpriseUsers --- app/assets/javascripts/api/groups_api.js | 3 +++ .../placeholders/components/placeholder_actions.vue | 4 ++-- .../components/placeholder_actions_spec.js | 12 ++++++------ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/api/groups_api.js b/app/assets/javascripts/api/groups_api.js index e6da406dadfbe2..7facf4b0bd76e3 100644 --- a/app/assets/javascripts/api/groups_api.js +++ b/app/assets/javascripts/api/groups_api.js @@ -83,3 +83,6 @@ export const getSharedGroups = (groupId, params = {}) => { return axios.get(url, { params: { ...defaultParams, ...params } }); }; + +// no-op: See EE code for implementation +export const fetchGroupEnterpriseUsers = () => {}; diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index 1965c91a43d75f..e65739895c521e 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -2,7 +2,7 @@ import produce from 'immer'; import { debounce, isEmpty, isNull } from 'lodash'; import { GlAvatarLabeled, GlButton, GlCollapsibleListbox } from '@gitlab/ui'; -import * as GroupsApi from 'ee_else_ce/api/groups_api'; +import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; import { __, s__ } from '~/locale'; import { createAlert } from '~/alert'; import { @@ -193,7 +193,7 @@ export default { async getGroupEnterpriseUsers() { try { - const { data, headers } = await GroupsApi.fetchGroupEnterpriseUsers(this.group.id, { + const { data, headers } = await fetchGroupEnterpriseUsers(this.group.id, { page: this.enterpriseUsersNextPage, per_page: USERS_PER_PAGE, search: this.search, diff --git a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index 6eff83d54c422f..ef7454f5984eb2 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -1,7 +1,7 @@ import Vue from 'vue'; import VueApollo from 'vue-apollo'; import { GlCollapsibleListbox } from '@gitlab/ui'; -import * as GroupsApi from 'ee/api/groups_api'; +import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; import { createAlert } from '~/alert'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; @@ -60,7 +60,7 @@ describe('PlaceholderActions', () => { describe('when restrictReassignmentToEnterprise is true', () => { describe('when users query is loading', () => { beforeEach(() => { - GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); + fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); }); it('renders listbox as loading', () => { @@ -72,10 +72,10 @@ describe('PlaceholderActions', () => { describe('when users query succeeds and has pagination', () => { beforeEach(async () => { - GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce( + fetchGroupEnterpriseUsers.mockResolvedValueOnce( mockEnterpriseUsersWithPaginationQueryResponse, ); - GroupsApi.fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse); + fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse); createComponent({ provide: { restrictReassignmentToEnterprise: true } }); await waitForPromises(); @@ -89,7 +89,7 @@ describe('PlaceholderActions', () => { it('calls getGroupEnterpriseUsers again to get next page', () => { expect(findListbox().props('infiniteScrollLoading')).toBe(true); - expect(GroupsApi.fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); + expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); }); it('appends query results to "items"', async () => { @@ -114,7 +114,7 @@ describe('PlaceholderActions', () => { describe('when users query fails', () => { beforeEach(async () => { - GroupsApi.fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); + fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); createComponent({ provide: { restrictReassignmentToEnterprise: true } }); await waitForPromises(); -- GitLab From 4e9aa8be6534843d94d5351a8e69798c583cec69 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Thu, 17 Apr 2025 16:03:37 +0100 Subject: [PATCH 14/29] Move enterprise user loading to table Having it in the placeholder actions, was causing duplicate queries --- .../components/placeholder_actions.vue | 117 ++++++------- .../components/placeholders_table.vue | 63 ++++++- .../components/placeholder_actions_spec.js | 94 +++++------ .../components/placeholders_table_spec.js | 158 ++++++++++++++++++ .../members/placeholders/mock_data.js | 21 +++ 5 files changed, 343 insertions(+), 110 deletions(-) create mode 100644 ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index e65739895c521e..db456c5972330f 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -2,14 +2,9 @@ import produce from 'immer'; import { debounce, isEmpty, isNull } from 'lodash'; import { GlAvatarLabeled, GlButton, GlCollapsibleListbox } from '@gitlab/ui'; -import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; +import { getFirstPropertyValue } from '~/lib/utils/common_utils'; import { __, s__ } from '~/locale'; import { createAlert } from '~/alert'; -import { - getFirstPropertyValue, - normalizeHeaders, - parseIntPagination, -} from '~/lib/utils/common_utils'; import searchUsersQuery from '~/graphql_shared/queries/users_search_all_paginated.query.graphql'; import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; import { @@ -39,11 +34,31 @@ export default { }, inject: ['group', 'restrictReassignmentToEnterprise'], props: { + enterpriseUsersIsLoadingInitial: { + type: Boolean, + required: false, + default: true, + }, + enterpriseUsersIsLoadingMore: { + type: Boolean, + required: false, + default: true, + }, sourceUser: { type: Object, required: false, default: () => ({}), }, + enterpriseUsers: { + type: Array, + required: false, + default: () => [], + }, + enterpriseUsersNextPage: { + type: Number, + required: false, + default: null, + }, }, data() { @@ -51,12 +66,10 @@ export default { isConfirmLoading: false, isCancelLoading: false, isNotifyLoading: false, - isLoadingInitial: true, - isLoadingMore: false, + apolloIsLoadingInitial: true, + apolloIsLoadingMore: false, isValidated: false, search: '', - enterpriseUsers: [], - enterpriseUsersNextPage: 1, selectedUserToReassign: null, }; }, @@ -71,7 +84,7 @@ export default { }; }, result() { - this.isLoadingInitial = false; + this.apolloIsLoadingInitial = false; }, skip() { return this.restrictReassignmentToEnterprise; @@ -100,8 +113,25 @@ export default { return this.users?.pageInfo?.hasNextPage; }, + // TODO: Make sure this works with Enterprise isLoading() { - return this.$apollo.queries.users.loading && !this.isLoadingMore; + return this.$apollo.queries.users.loading && !this.apolloIsLoadingMore; + }, + + isLoadingMore() { + if (this.restrictReassignmentToEnterprise) { + return this.enterpriseUsersIsLoadingMore; + } + + return this.apolloIsLoadingMore; + }, + + isLoadingInitial() { + if (this.restrictReassignmentToEnterprise) { + return this.enterpriseUsersIsLoadingInitial; + } + + return this.apolloIsLoadingInitial; }, userSelectInvalid() { @@ -156,62 +186,36 @@ export default { this.debouncedSetSearch = debounce(this.setSearch, DEFAULT_DEBOUNCE_AND_THROTTLE_MS); }, - mounted() { - if (this.restrictReassignmentToEnterprise) { - this.getGroupEnterpriseUsers(); - } - }, - methods: { async loadMoreUsers() { if (!this.hasNextPage) return; - this.isLoadingMore = true; - - try { - if (this.restrictReassignmentToEnterprise) { - await this.getGroupEnterpriseUsers(); - } else { - await this.$apollo.queries.users.fetchMore({ - variables: { - ...this.queryVariables, - after: this.users.pageInfo?.endCursor, - }, - updateQuery: (previousResult, { fetchMoreResult }) => { - return produce(fetchMoreResult, (draftData) => { - draftData.users.nodes = [...previousResult.users.nodes, ...draftData.users.nodes]; - }); - }, - }); - } - } catch (error) { - this.onError(); - } finally { - this.isLoadingMore = false; + if (this.restrictReassignmentToEnterprise) { + this.$emit('load-more-users'); + return; } - }, - async getGroupEnterpriseUsers() { + this.apolloIsLoadingMore = true; + try { - const { data, headers } = await fetchGroupEnterpriseUsers(this.group.id, { - page: this.enterpriseUsersNextPage, - per_page: USERS_PER_PAGE, - search: this.search, + await this.$apollo.queries.users.fetchMore({ + variables: { + ...this.queryVariables, + after: this.users.pageInfo?.endCursor, + }, + updateQuery: (previousResult, { fetchMoreResult }) => { + return produce(fetchMoreResult, (draftData) => { + draftData.users.nodes = [...previousResult.users.nodes, ...draftData.users.nodes]; + }); + }, }); - - this.isLoadingInitial = false; - this.enterpriseUsersNextPage = parseIntPagination(normalizeHeaders(headers)).nextPage; - this.enterpriseUsers.push(...data); } catch (error) { this.onError(); + } finally { + this.apolloIsLoadingMore = false; } }, - resetEnterpriseSearch() { - this.enterpriseUsers = []; - this.enterpriseUsersNextPage = 1; - }, - onError() { createAlert({ message: __('There was a problem fetching users.'), @@ -222,8 +226,7 @@ export default { this.search = searchTerm; if (this.restrictReassignmentToEnterprise) { - this.resetEnterpriseSearch(); - this.getGroupEnterpriseUsers(); + this.$emit('reset-enterprise-search', this.search); } }, diff --git a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue index 69a0fa6c6e6c96..c30127dc13decd 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue @@ -11,6 +11,8 @@ import { GlSprintf, GlLink, } from '@gitlab/ui'; +import { normalizeHeaders, parseIntPagination } from '~/lib/utils/common_utils'; +import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; import { createAlert } from '~/alert'; import { __, s__ } from '~/locale'; import { fetchPolicies } from '~/lib/graphql'; @@ -28,6 +30,7 @@ import importSourceUsersQuery from '../graphql/queries/import_source_users.query import PlaceholderActions from './placeholder_actions.vue'; const MINIMUM_QUERY_LENGTH = 3; +const USERS_PER_PAGE = 20; export default { name: 'PlaceholdersTable', @@ -47,7 +50,7 @@ export default { directives: { GlTooltip: GlTooltipDirective, }, - inject: ['group'], + inject: ['group', 'restrictReassignmentToEnterprise'], props: { queryStatuses: { type: Array, @@ -75,6 +78,11 @@ export default { before: null, after: null, }, + enterpriseUsersSearch: null, + enterpriseUsers: [], + enterpriseUsersNextPage: 1, + enterpriseUsersIsLoadingInitial: true, + enterpriseUsersIsLoadingMore: true, }; }, apollo: { @@ -157,6 +165,12 @@ export default { }, }, + mounted() { + if (this.restrictReassignmentToEnterprise) { + this.getGroupEnterpriseUsers(); + } + }, + methods: { statusBadge(item) { return placeholderUserBadges[item.status]; @@ -202,6 +216,40 @@ export default { formatDate(dateString) { return localeDateFormat.asDateTime.format(new Date(dateString)); }, + + async getGroupEnterpriseUsers() { + this.enterpriseUsersIsLoadingMore = true; + + try { + const { data, headers } = await fetchGroupEnterpriseUsers(this.group.id, { + page: this.enterpriseUsersNextPage, + per_page: USERS_PER_PAGE, + search: this.enterpriseUsersSearch, + }); + + this.enterpriseUsersIsLoadingInitial = false; + this.enterpriseUsersNextPage = parseIntPagination(normalizeHeaders(headers)).nextPage; + this.enterpriseUsers.push(...data); + } catch (error) { + this.onError(); + } finally { + this.enterpriseUsersIsLoadingMore = false; + } + }, + + resetEnterpriseSearch(searchTerm) { + this.enterpriseUsersSearch = searchTerm; + this.enterpriseUsers = []; + this.enterpriseUsersNextPage = 1; + + this.getGroupEnterpriseUsers(); + }, + + onError() { + createAlert({ + message: __('There was a problem fetching users.'), + }); + }, }, placeholderUsersHelpPath: helpPagePath('user/project/import/_index', { anchor: 'placeholder-users', @@ -304,7 +352,18 @@ export default { data-testid="placeholder-reassigned" /> - + diff --git a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index ef7454f5984eb2..7708b747b5d284 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -1,21 +1,13 @@ import Vue from 'vue'; import VueApollo from 'vue-apollo'; import { GlCollapsibleListbox } from '@gitlab/ui'; -import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; -import { createAlert } from '~/alert'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; import PlaceholderActions from '~/members/placeholders/components/placeholder_actions.vue'; -import { - mockSourceUsers, - mockEnterpriseUser1, - mockEnterpriseUser2, - mockEnterpriseUsersQueryResponse, - mockEnterpriseUsersWithPaginationQueryResponse, -} from '../mock_data'; +import { mockSourceUsers } from '../mock_data'; Vue.use(VueApollo); jest.mock('~/alert'); @@ -29,24 +21,26 @@ describe('PlaceholderActions', () => { name: 'Imported group', }; - const restrictReassignmentToEnterprise = false; - - const propsData = { + const defaultProps = { sourceUser: mockSourceUsers[0], + enterpriseUsersNextPage: 1, }; const defaultProvide = { group, - restrictReassignmentToEnterprise, + restrictReassignmentToEnterprise: false, }; const $toast = { show: jest.fn(), }; - const createComponent = ({ provide = {} } = {}) => { + const createComponent = ({ provide = {}, props = {} } = {}) => { wrapper = shallowMountExtended(PlaceholderActions, { apolloProvider: createMockApollo(), - propsData, + propsData: { + ...defaultProps, + ...props, + }, provide: { ...defaultProvide, ...provide, @@ -59,70 +53,68 @@ describe('PlaceholderActions', () => { describe('when restrictReassignmentToEnterprise is true', () => { describe('when users query is loading', () => { - beforeEach(() => { - fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); + beforeEach(async () => { + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + await waitForPromises(); }); it('renders listbox as loading', () => { - createComponent({ provide: { restrictReassignmentToEnterprise: true } }); - expect(findListbox().props('loading')).toBe(true); }); }); describe('when users query succeeds and has pagination', () => { beforeEach(async () => { - fetchGroupEnterpriseUsers.mockResolvedValueOnce( - mockEnterpriseUsersWithPaginationQueryResponse, - ); - fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse); - createComponent({ provide: { restrictReassignmentToEnterprise: true } }); await waitForPromises(); }); describe('when "bottom-reached" event is emitted', () => { - beforeEach(() => { - findListbox().vm.$emit('bottom-reached'); + beforeEach(async () => { + await findListbox().vm.$emit('bottom-reached'); }); - it('calls getGroupEnterpriseUsers again to get next page', () => { - expect(findListbox().props('infiniteScrollLoading')).toBe(true); - - expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); + it('emits the "load-more-users" event', () => { + expect(wrapper.emitted('load-more-users')[0]).toEqual([]); }); + }); + }); - it('appends query results to "items"', async () => { - const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; - - await waitForPromises(); + describe('when users query succeeds and does not have pagination', () => { + beforeEach(async () => { + createComponent({ + provide: { restrictReassignmentToEnterprise: true }, + props: { enterpriseUsersNextPage: null }, + }); + await waitForPromises(); + }); - expect(findListbox().props('infiniteScrollLoading')).toBe(false); + describe('when "bottom-reached" event is emitted', () => { + beforeEach(async () => { + await findListbox().vm.$emit('bottom-reached'); + }); - const dropdownItems = findListbox().props('items'); - expect(dropdownItems).toHaveLength(allUsers.length); - dropdownItems.forEach((user, index) => { - expect(user).toMatchObject({ - id: `gid://gitlab/User/${allUsers[index].id}`, - username: allUsers[index].username, - value: `gid://gitlab/User/${allUsers[index].id}`, - }); - }); + it('does not emit "load-more-users" event', () => { + expect(wrapper.emitted('load-more-users')).toBeUndefined(); }); }); }); + }); - describe('when users query fails', () => { + describe('when restrictReassignmentToEnterprise is false', () => { + describe('when users query succeeds and has pagination', () => { beforeEach(async () => { - fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); - - createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + createComponent({ provide: { restrictReassignmentToEnterprise: false } }); await waitForPromises(); }); - it('creates an alert', () => { - expect(createAlert).toHaveBeenCalledWith({ - message: 'There was a problem fetching users.', + describe('when "bottom-reached" event is emitted', () => { + beforeEach(async () => { + await findListbox().vm.$emit('bottom-reached'); + }); + + it('does not emit "load-more-users" event', () => { + expect(wrapper.emitted('load-more-users')).toBeUndefined(); }); }); }); diff --git a/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js b/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js new file mode 100644 index 00000000000000..486b8f3901588a --- /dev/null +++ b/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js @@ -0,0 +1,158 @@ +import Vue from 'vue'; +import VueApollo from 'vue-apollo'; + +import { mount } from '@vue/test-utils'; +import PlaceholderActions from '~/members/placeholders/components/placeholder_actions.vue'; +import createMockApollo from 'helpers/mock_apollo_helper'; +import { createAlert } from '~/alert'; +import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; +import { createMockDirective } from 'helpers/vue_mock_directive'; + +import PlaceholdersTable from '~/members/placeholders/components/placeholders_table.vue'; +import waitForPromises from 'helpers/wait_for_promises'; + +import importSourceUsersQuery from '~/members/placeholders/graphql/queries/import_source_users.query.graphql'; + +import { + PLACEHOLDER_USER_STATUS, + PLACEHOLDER_SORT_SOURCE_NAME_ASC, +} from '~/import_entities/import_groups/constants'; + +import { + mockEnterpriseUser1, + mockEnterpriseUser2, + mockEnterpriseUsersQueryResponse, + mockEnterpriseUsersWithPaginationQueryResponse, + mockSourceUsersQueryResponse, +} from '../mock_data'; + +jest.mock('~/alert'); +jest.mock('ee_component/api/groups_api'); + +Vue.use(VueApollo); +jest.mock('~/alert'); + +describe('PlaceholdersTable', () => { + let wrapper; + let mockApollo; + + const mockGroup = { + path: 'imported-group', + name: 'Imported group', + }; + + const defaultProps = { + queryStatuses: PLACEHOLDER_USER_STATUS.UNASSIGNED, + reassigned: false, + querySort: PLACEHOLDER_SORT_SOURCE_NAME_ASC, + }; + + const defaultProvide = { + group: mockGroup, + restrictReassignmentToEnterprise: true, + }; + + const sourceUsersQueryHandler = jest.fn().mockResolvedValue(mockSourceUsersQueryResponse()); + + const $toast = { + show: jest.fn(), + }; + + const createComponent = ({ + queryHandler = sourceUsersQueryHandler, + mountFn = mount, + props = {}, + provide = {}, + options = {}, + } = {}) => { + mockApollo = createMockApollo([[importSourceUsersQuery, queryHandler]]); + + wrapper = mountFn(PlaceholdersTable, { + apolloProvider: mockApollo, + propsData: { + ...defaultProps, + ...props, + }, + provide: { + ...defaultProvide, + ...provide, + }, + mocks: { $toast }, + directives: { + GlTooltip: createMockDirective('gl-tooltip'), + }, + ...options, + }); + }; + + const findPlaceholderActions = () => wrapper.findComponent(PlaceholderActions); + + describe('when restrictReassignmentToEnterprise is true', () => { + describe('when users query is loading', () => { + beforeEach(() => { + fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); + }); + + it('calls getGroupEnterpriseUsers', async () => { + createComponent(); + + await waitForPromises(); + + expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(1); + }); + }); + + describe('when users query succeeds and has pagination', () => { + beforeEach(async () => { + fetchGroupEnterpriseUsers.mockResolvedValueOnce( + mockEnterpriseUsersWithPaginationQueryResponse, + ); + fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse); + + createComponent(); + await waitForPromises(); + }); + + describe('when "load-more-users" event is emitted', () => { + beforeEach(() => { + findPlaceholderActions().vm.$emit('load-more-users'); + }); + + it('calls getGroupEnterpriseUsers again to get next page', () => { + expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); + }); + + it('appends query results to "enterpriseUsers"', () => { + const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; + + expect(wrapper.vm.enterpriseUsers).toEqual(expect.arrayContaining(allUsers)); + }); + }); + }); + + describe('when users query fails', () => { + beforeEach(async () => { + fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); + + createComponent(); + await waitForPromises(); + }); + + it('creates an alert', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: 'There was a problem fetching users.', + }); + }); + }); + }); + + describe('when restrictReassignmentToEnterprise is false', () => { + it('does not call getGroupEnterpriseUsers', async () => { + createComponent({ provide: { restrictReassignmentToEnterprise: false } }); + + await waitForPromises(); + + expect(fetchGroupEnterpriseUsers).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/ee/spec/frontend/members/placeholders/mock_data.js b/ee/spec/frontend/members/placeholders/mock_data.js index 8e5cddd104daf6..cb14c0472ad891 100644 --- a/ee/spec/frontend/members/placeholders/mock_data.js +++ b/ee/spec/frontend/members/placeholders/mock_data.js @@ -76,3 +76,24 @@ export const mockEnterpriseUsersWithPaginationQueryResponse = { data: [mockEnterpriseUser2], headers: { 'X-Next-Page': 2 }, }; + +export const mockSourceUsersQueryResponse = ({ nodes = mockSourceUsers, pageInfo = {} } = {}) => ({ + data: { + namespace: { + __typename: 'Namespace', + id: 'gid://gitlab/Group/1', + importSourceUsers: { + __typename: 'ImportSourceUserConnection', + nodes, + pageInfo: { + __typename: 'PageInfo', + hasNextPage: false, + hasPreviousPage: false, + startCursor: '', + endCursor: '', + ...pageInfo, + }, + }, + }, + }, +}); -- GitLab From 3462a860d7092b4bcef4c3a539340d95281278fc Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Thu, 17 Apr 2025 16:27:14 +0100 Subject: [PATCH 15/29] Fix broken method --- .../members/placeholders/components/placeholder_actions.vue | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index db456c5972330f..ecd09f4688f7c6 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -113,8 +113,11 @@ export default { return this.users?.pageInfo?.hasNextPage; }, - // TODO: Make sure this works with Enterprise isLoading() { + if (this.restrictReassignmentToEnterprise) { + return this.enterpriseUsersIsLoadingMore; + } + return this.$apollo.queries.users.loading && !this.apolloIsLoadingMore; }, -- GitLab From dad1443534ddf20fb6bf7e08d65eb9b78ce13cbe Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 23 Apr 2025 12:09:22 +0100 Subject: [PATCH 16/29] Do not load enterprise users on reassigned page --- .../placeholders/components/placeholders_table.vue | 2 +- .../placeholders/components/placeholders_table_spec.js | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue index c30127dc13decd..82d1a9e860c8b2 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue @@ -166,7 +166,7 @@ export default { }, mounted() { - if (this.restrictReassignmentToEnterprise) { + if (this.restrictReassignmentToEnterprise && !this.reassigned) { this.getGroupEnterpriseUsers(); } }, diff --git a/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js b/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js index 486b8f3901588a..eb4119837b5b12 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js @@ -155,4 +155,14 @@ describe('PlaceholdersTable', () => { expect(fetchGroupEnterpriseUsers).not.toHaveBeenCalled(); }); }); + + describe('when reassigned is true', () => { + it('does not call getGroupEnterpriseUsers', async () => { + createComponent({ props: { reassigned: true } }); + + await waitForPromises(); + + expect(fetchGroupEnterpriseUsers).not.toHaveBeenCalled(); + }); + }); }); -- GitLab From 1da173296022c34a386627081f55291a76eb2818 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 23 Apr 2025 12:34:25 +0100 Subject: [PATCH 17/29] Fix loading more users causing refresh --- .../members/placeholders/components/placeholder_actions.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index ecd09f4688f7c6..308277b28e5e22 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -115,7 +115,7 @@ export default { isLoading() { if (this.restrictReassignmentToEnterprise) { - return this.enterpriseUsersIsLoadingMore; + return this.enterpriseUsersIsLoadingInitial; } return this.$apollo.queries.users.loading && !this.apolloIsLoadingMore; -- GitLab From 6ee01a2e0dcbaf85912ea492f93f27ec467a5c1f Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Thu, 24 Apr 2025 16:22:46 +0100 Subject: [PATCH 18/29] Apply feedback from frontend changes - Simplify inject - Refactor fetchEnterpriseUsers - Add extra specs - Do not duplicate mocks - Rename event - Wrap page info in enterpriseUsersPageInfo - Set is loading to false by default --- .../components/placeholder_actions.vue | 16 ++-- .../components/placeholders_table.vue | 49 ++++++++---- .../components/placeholder_actions_spec.js | 24 +++--- .../components/placeholders_table_spec.js | 49 +++++++++--- .../members/placeholders/mock_data.js | 80 ++----------------- .../placeholders/components/app_spec.js | 1 - .../components/placeholder_actions_spec.js | 12 --- .../components/placeholders_table_spec.js | 1 - 8 files changed, 96 insertions(+), 136 deletions(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index 308277b28e5e22..c3c323e5715b89 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -32,7 +32,11 @@ export default { GlButton, GlCollapsibleListbox, }, - inject: ['group', 'restrictReassignmentToEnterprise'], + inject: { + restrictReassignmentToEnterprise: { + default: false, + }, + }, props: { enterpriseUsersIsLoadingInitial: { type: Boolean, @@ -54,10 +58,10 @@ export default { required: false, default: () => [], }, - enterpriseUsersNextPage: { - type: Number, + enterpriseUsersPageInfo: { + type: Object, required: false, - default: null, + default: () => ({ nextPage: null }), }, }, @@ -107,7 +111,7 @@ export default { hasNextPage() { if (this.restrictReassignmentToEnterprise) { - return Boolean(this.enterpriseUsersNextPage); + return Boolean(this.enterpriseUsersPageInfo.nextPage); } return this.users?.pageInfo?.hasNextPage; @@ -194,7 +198,7 @@ export default { if (!this.hasNextPage) return; if (this.restrictReassignmentToEnterprise) { - this.$emit('load-more-users'); + this.$emit('load-more-enterprise-users'); return; } diff --git a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue index 82d1a9e860c8b2..d067b598d484db 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue @@ -50,7 +50,14 @@ export default { directives: { GlTooltip: GlTooltipDirective, }, - inject: ['group', 'restrictReassignmentToEnterprise'], + inject: { + group: { + default: {}, + }, + restrictReassignmentToEnterprise: { + default: false, + }, + }, props: { queryStatuses: { type: Array, @@ -80,9 +87,11 @@ export default { }, enterpriseUsersSearch: null, enterpriseUsers: [], - enterpriseUsersNextPage: 1, - enterpriseUsersIsLoadingInitial: true, - enterpriseUsersIsLoadingMore: true, + enterpriseUsersPageInfo: { + nextPage: null, + }, + enterpriseUsersIsLoadingInitial: false, + enterpriseUsersIsLoadingMore: false, }; }, apollo: { @@ -167,7 +176,7 @@ export default { mounted() { if (this.restrictReassignmentToEnterprise && !this.reassigned) { - this.getGroupEnterpriseUsers(); + this.loadInitialEnterpriseUsers(); } }, @@ -217,32 +226,38 @@ export default { return localeDateFormat.asDateTime.format(new Date(dateString)); }, - async getGroupEnterpriseUsers() { - this.enterpriseUsersIsLoadingMore = true; - + async fetchEnterpriseUsers(page) { try { const { data, headers } = await fetchGroupEnterpriseUsers(this.group.id, { - page: this.enterpriseUsersNextPage, + page, per_page: USERS_PER_PAGE, search: this.enterpriseUsersSearch, }); - this.enterpriseUsersIsLoadingInitial = false; - this.enterpriseUsersNextPage = parseIntPagination(normalizeHeaders(headers)).nextPage; + this.enterpriseUsersPageInfo = parseIntPagination(normalizeHeaders(headers)); this.enterpriseUsers.push(...data); } catch (error) { this.onError(); - } finally { - this.enterpriseUsersIsLoadingMore = false; } }, + async loadInitialEnterpriseUsers() { + this.enterpriseUsersIsLoadingInitial = true; + await this.fetchEnterpriseUsers(1); + this.enterpriseUsersIsLoadingInitial = false; + }, + + async loadMoreEnterpriseUsers() { + this.enterpriseUsersIsLoadingMore = true; + await this.fetchEnterpriseUsers(this.enterpriseUsersPageInfo.nextPage); + this.enterpriseUsersIsLoadingMore = false; + }, + resetEnterpriseSearch(searchTerm) { this.enterpriseUsersSearch = searchTerm; this.enterpriseUsers = []; - this.enterpriseUsersNextPage = 1; - this.getGroupEnterpriseUsers(); + this.loadInitialEnterpriseUsers(); }, onError() { @@ -359,10 +374,10 @@ export default { :enterprise-users="enterpriseUsers" :enterprise-users-is-loading-initial="enterpriseUsersIsLoadingInitial" :enterprise-users-is-loading-more="enterpriseUsersIsLoadingMore" - :enterprise-users-next-page="enterpriseUsersNextPage" + :enterprise-users-page-info="enterpriseUsersPageInfo" @confirm="onConfirm(item)" @reset-enterprise-search="resetEnterpriseSearch" - @load-more-users="getGroupEnterpriseUsers" + @load-more-enterprise-users="loadMoreEnterpriseUsers" /> diff --git a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index 7708b747b5d284..155c5b363acf3a 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -7,7 +7,7 @@ import waitForPromises from 'helpers/wait_for_promises'; import PlaceholderActions from '~/members/placeholders/components/placeholder_actions.vue'; -import { mockSourceUsers } from '../mock_data'; +import { mockSourceUsers } from 'jest/members/placeholders/mock_data'; Vue.use(VueApollo); jest.mock('~/alert'); @@ -16,17 +16,11 @@ jest.mock('ee_component/api/groups_api'); describe('PlaceholderActions', () => { let wrapper; - const group = { - path: 'imported-group', - name: 'Imported group', - }; - const defaultProps = { sourceUser: mockSourceUsers[0], - enterpriseUsersNextPage: 1, + enterpriseUsersPageInfo: { nextPage: 1 }, }; const defaultProvide = { - group, restrictReassignmentToEnterprise: false, }; @@ -74,8 +68,8 @@ describe('PlaceholderActions', () => { await findListbox().vm.$emit('bottom-reached'); }); - it('emits the "load-more-users" event', () => { - expect(wrapper.emitted('load-more-users')[0]).toEqual([]); + it('emits the "load-more-enterprise-users" event', () => { + expect(wrapper.emitted('load-more-enterprise-users')[0]).toEqual([]); }); }); }); @@ -84,7 +78,7 @@ describe('PlaceholderActions', () => { beforeEach(async () => { createComponent({ provide: { restrictReassignmentToEnterprise: true }, - props: { enterpriseUsersNextPage: null }, + props: { enterpriseUsersPageInfo: { nextPage: null } }, }); await waitForPromises(); }); @@ -94,8 +88,8 @@ describe('PlaceholderActions', () => { await findListbox().vm.$emit('bottom-reached'); }); - it('does not emit "load-more-users" event', () => { - expect(wrapper.emitted('load-more-users')).toBeUndefined(); + it('does not emit "load-more-enterprise-users" event', () => { + expect(wrapper.emitted('load-more-enterprise-users')).toBeUndefined(); }); }); }); @@ -113,8 +107,8 @@ describe('PlaceholderActions', () => { await findListbox().vm.$emit('bottom-reached'); }); - it('does not emit "load-more-users" event', () => { - expect(wrapper.emitted('load-more-users')).toBeUndefined(); + it('does not emit "load-more-enterprise-users" event', () => { + expect(wrapper.emitted('load-more-enterprise-users')).toBeUndefined(); }); }); }); diff --git a/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js b/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js index eb4119837b5b12..076657b19b8c0b 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js @@ -18,12 +18,12 @@ import { PLACEHOLDER_SORT_SOURCE_NAME_ASC, } from '~/import_entities/import_groups/constants'; +import { mockSourceUsersQueryResponse } from 'jest/members/placeholders/mock_data'; import { mockEnterpriseUser1, mockEnterpriseUser2, mockEnterpriseUsersQueryResponse, mockEnterpriseUsersWithPaginationQueryResponse, - mockSourceUsersQueryResponse, } from '../mock_data'; jest.mock('~/alert'); @@ -93,7 +93,7 @@ describe('PlaceholdersTable', () => { fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); }); - it('calls getGroupEnterpriseUsers', async () => { + it('calls fetchGroupEnterpriseUsers', async () => { createComponent(); await waitForPromises(); @@ -107,31 +107,62 @@ describe('PlaceholdersTable', () => { fetchGroupEnterpriseUsers.mockResolvedValueOnce( mockEnterpriseUsersWithPaginationQueryResponse, ); - fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse); + fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse()); createComponent(); await waitForPromises(); }); - describe('when "load-more-users" event is emitted', () => { + describe('when "load-more-enterprise-users" event is emitted', () => { beforeEach(() => { - findPlaceholderActions().vm.$emit('load-more-users'); + findPlaceholderActions().vm.$emit('load-more-enterprise-users'); }); - it('calls getGroupEnterpriseUsers again to get next page', () => { + it('calls fetchGroupEnterpriseUsers again to get next page', () => { expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); }); it('appends query results to "enterpriseUsers"', () => { const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; - expect(wrapper.vm.enterpriseUsers).toEqual(expect.arrayContaining(allUsers)); + expect(findPlaceholderActions().props('enterpriseUsers')).toEqual( + expect.arrayContaining(allUsers), + ); + }); + }); + + describe('when "reset-enterprise-search" event is emitted', () => { + beforeEach(() => { + fetchGroupEnterpriseUsers.mockResolvedValueOnce( + mockEnterpriseUsersQueryResponse(mockEnterpriseUser2), + ); + + findPlaceholderActions().vm.$emit('reset-enterprise-search', 'Rookie'); + }); + + it('queries for the search term', () => { + expect(fetchGroupEnterpriseUsers).toHaveBeenCalledWith( + undefined, + expect.objectContaining({ search: 'Rookie' }), + ); + }); + + it('resets the enterpriseUsers and page count', () => { + const searchTermResults = [mockEnterpriseUser2]; + + expect(findPlaceholderActions().props('enterpriseUsers')).toEqual( + expect.arrayContaining(searchTermResults), + ); + expect(findPlaceholderActions().props('enterpriseUsersPageInfo')).toEqual( + expect.objectContaining({ nextPage: 2 }), + ); }); }); }); describe('when users query fails', () => { beforeEach(async () => { + fetchGroupEnterpriseUsers.mockReset(); fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); createComponent(); @@ -147,7 +178,7 @@ describe('PlaceholdersTable', () => { }); describe('when restrictReassignmentToEnterprise is false', () => { - it('does not call getGroupEnterpriseUsers', async () => { + it('does not call fetchGroupEnterpriseUsers', async () => { createComponent({ provide: { restrictReassignmentToEnterprise: false } }); await waitForPromises(); @@ -157,7 +188,7 @@ describe('PlaceholdersTable', () => { }); describe('when reassigned is true', () => { - it('does not call getGroupEnterpriseUsers', async () => { + it('does not call fetchGroupEnterpriseUsers', async () => { createComponent({ props: { reassigned: true } }); await waitForPromises(); diff --git a/ee/spec/frontend/members/placeholders/mock_data.js b/ee/spec/frontend/members/placeholders/mock_data.js index cb14c0472ad891..1949ed121d2981 100644 --- a/ee/spec/frontend/members/placeholders/mock_data.js +++ b/ee/spec/frontend/members/placeholders/mock_data.js @@ -1,54 +1,3 @@ -const createMockPlaceholderUser = (index) => { - return { - __typename: 'UserCore', - id: `gid://gitlab/User/382${index}`, - avatarUrl: '/avatar1', - name: `Placeholder ${index}`, - username: `placeholder_${index}`, - webUrl: '/', - webPath: '/', - }; -}; - -const createMockReassignUser = (index) => { - return { - __typename: 'UserCore', - id: `gid://gitlab/User/741${index}`, - avatarUrl: '/avatar2', - name: `Reassigned ${index}`, - username: `reassigned_${index}`, - webUrl: '/', - webPath: '/', - }; -}; - -export const importSourceUserCreatedAt = '2024-09-17T00:41:28.000Z'; - -const createMockSourceUser = ( - index, - { status, placeholderUser = true, reassignToUser = false, sourceUserExists = true } = {}, -) => { - return { - __typename: 'ImportSourceUser', - createdAt: importSourceUserCreatedAt, - id: `gid://gitlab/Import::SourceUser/${index}`, - sourceHostname: 'https://gitlab.com', - sourceName: sourceUserExists ? `Old User ${index}` : null, - sourceUsername: sourceUserExists ? `old_user_${index}` : null, - sourceUserIdentifier: index, - status, - reassignmentError: null, - placeholderUser: placeholderUser ? createMockPlaceholderUser(index) : null, - reassignToUser: reassignToUser ? createMockReassignUser(index) : null, - }; -}; - -export const mockSourceUsers = [ - createMockSourceUser(1, { - status: 'PENDING_REASSIGNMENT', - }), -]; - export const mockEnterpriseUser1 = { username: 'Administrator', web_url: '/root', @@ -67,33 +16,14 @@ export const mockEnterpriseUser2 = { name: 'Rookie', }; -export const mockEnterpriseUsersQueryResponse = { - data: [mockEnterpriseUser1], +export const mockEnterpriseUsersQueryResponse = ({ + enterpriseUser = mockEnterpriseUser1, +} = {}) => ({ + data: [enterpriseUser], headers: { 'X-Next-Page': null }, -}; +}); export const mockEnterpriseUsersWithPaginationQueryResponse = { data: [mockEnterpriseUser2], headers: { 'X-Next-Page': 2 }, }; - -export const mockSourceUsersQueryResponse = ({ nodes = mockSourceUsers, pageInfo = {} } = {}) => ({ - data: { - namespace: { - __typename: 'Namespace', - id: 'gid://gitlab/Group/1', - importSourceUsers: { - __typename: 'ImportSourceUserConnection', - nodes, - pageInfo: { - __typename: 'PageInfo', - hasNextPage: false, - hasPreviousPage: false, - startCursor: '', - endCursor: '', - ...pageInfo, - }, - }, - }, - }, -}); diff --git a/spec/frontend/members/placeholders/components/app_spec.js b/spec/frontend/members/placeholders/components/app_spec.js index db0c721c388331..d3d9fe6a0cdef6 100644 --- a/spec/frontend/members/placeholders/components/app_spec.js +++ b/spec/frontend/members/placeholders/components/app_spec.js @@ -72,7 +72,6 @@ describe('PlaceholdersTabApp', () => { provide: { group: mockGroup, reassignmentCsvPath: 'foo/bar', - restrictReassignmentToEnterprise: false, ...provide, }, mocks: { $toast }, diff --git a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index 5801e1f9b995e8..a902f0ade66156 100644 --- a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -34,20 +34,9 @@ describe('PlaceholderActions', () => { let wrapper; let mockApollo; - const group = { - path: 'imported-group', - name: 'Imported group', - }; - - const restrictReassignmentToEnterprise = false; - const defaultProps = { sourceUser: mockSourceUsers[0], }; - const provide = { - group, - restrictReassignmentToEnterprise, - }; const usersQueryHandler = jest.fn().mockResolvedValue(mockUsersQueryResponse); const sourceUsersQueryHandler = jest.fn().mockResolvedValue(mockSourceUsersQueryResponse()); @@ -91,7 +80,6 @@ describe('PlaceholderActions', () => { ...defaultProps, ...props, }, - provide, mocks: { $toast }, }); }; diff --git a/spec/frontend/members/placeholders/components/placeholders_table_spec.js b/spec/frontend/members/placeholders/components/placeholders_table_spec.js index d12c7f6e0a20da..eb502f8f1f94f6 100644 --- a/spec/frontend/members/placeholders/components/placeholders_table_spec.js +++ b/spec/frontend/members/placeholders/components/placeholders_table_spec.js @@ -84,7 +84,6 @@ describe('PlaceholdersTable', () => { }, provide: { group: mockGroup, - restrictReassignmentToEnterprise: false, }, mocks: { $toast }, directives: { -- GitLab From 6b824c347277cf30090354798b5e3afc365aeb54 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Mon, 28 Apr 2025 10:54:09 +0100 Subject: [PATCH 19/29] Update docs and error messages --- doc/user/group/import/direct_transfer_migrations.md | 4 ++-- ee/app/services/ee/import/source_users/reassign_service.rb | 3 +-- .../services/ee/import/source_users/reassign_service_spec.rb | 3 +-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/doc/user/group/import/direct_transfer_migrations.md b/doc/user/group/import/direct_transfer_migrations.md index 54b83b1371eb63..da8893551b5d3a 100644 --- a/doc/user/group/import/direct_transfer_migrations.md +++ b/doc/user/group/import/direct_transfer_migrations.md @@ -122,8 +122,8 @@ has an existing membership in the destination namespace with a [higher role](../ the one being mapped, the membership is mapped as a direct membership instead. This ensures the member does not get elevated permissions. -If the root namespace has domain verification enabled with at least one verified domain, then users can only be -reassigned to [Enterprise Users](../../enterprise_user/_index.md). +For top-level groups with at least one verified domain, you can map +contributions and memberships only to [enterprise users](../../enterprise_user/_index.md). {{< alert type="note" >}} diff --git a/ee/app/services/ee/import/source_users/reassign_service.rb b/ee/app/services/ee/import/source_users/reassign_service.rb index c8228005845408..5923cfb1e4169f 100644 --- a/ee/app/services/ee/import/source_users/reassign_service.rb +++ b/ee/app/services/ee/import/source_users/reassign_service.rb @@ -48,8 +48,7 @@ def error_invalid_assignee_due_to_enterprise_users_check end def invalid_assignee_due_to_enterprise_users_check_message - s_('UserMapping|You can assign only to Enterprise Users of the root namespace. ' \ - 'Ensure the user is an Enterprise member of the top level namespace you are importing to.') + s_('UserMapping|You can assign only enterprise users in the top-level group you're importing to.) end def invalid_assignee_due_to_sso_enforcement_message diff --git a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb index df617ec10e0f15..73ab5d5c8a1809 100644 --- a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb +++ b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb @@ -107,8 +107,7 @@ let_it_be(:assignee_user) { create(:user, enterprise_group: nil) } it_behaves_like 'an error response', - error: 'You can assign only to Enterprise Users of the root namespace. ' \ - 'Ensure the user is an Enterprise member of the top level namespace you are importing to.' + error: 'You can assign only enterprise users in the top-level group you're importing to.' context 'when not on .com' do before do -- GitLab From a1ce549e4d5a8898b1a831053540e379c2588b6e Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Mon, 28 Apr 2025 11:55:42 +0100 Subject: [PATCH 20/29] Update direct transfer docs --- doc/user/group/import/direct_transfer_migrations.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/user/group/import/direct_transfer_migrations.md b/doc/user/group/import/direct_transfer_migrations.md index da8893551b5d3a..bc50c77ea2268e 100644 --- a/doc/user/group/import/direct_transfer_migrations.md +++ b/doc/user/group/import/direct_transfer_migrations.md @@ -122,7 +122,8 @@ has an existing membership in the destination namespace with a [higher role](../ the one being mapped, the membership is mapped as a direct membership instead. This ensures the member does not get elevated permissions. -For top-level groups with at least one verified domain, you can map +[In GitLab 18.0 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/510673), +for top-level groups with at least one enterprise user, you can map contributions and memberships only to [enterprise users](../../enterprise_user/_index.md). {{< alert type="note" >}} -- GitLab From 97c4ec40308125981730d537462996eaf7a886cf Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Mon, 28 Apr 2025 11:59:38 +0100 Subject: [PATCH 21/29] Update user mapping error message --- ee/app/services/ee/import/source_users/reassign_service.rb | 2 +- .../services/ee/import/source_users/reassign_service_spec.rb | 2 +- locale/gitlab.pot | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/app/services/ee/import/source_users/reassign_service.rb b/ee/app/services/ee/import/source_users/reassign_service.rb index 5923cfb1e4169f..20ad1261236b1f 100644 --- a/ee/app/services/ee/import/source_users/reassign_service.rb +++ b/ee/app/services/ee/import/source_users/reassign_service.rb @@ -48,7 +48,7 @@ def error_invalid_assignee_due_to_enterprise_users_check end def invalid_assignee_due_to_enterprise_users_check_message - s_('UserMapping|You can assign only enterprise users in the top-level group you're importing to.) + s_("UserMapping|You can assign only enterprise users in the top-level group you're importing to.") end def invalid_assignee_due_to_sso_enforcement_message diff --git a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb index 73ab5d5c8a1809..3c9d0e64bd2555 100644 --- a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb +++ b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb @@ -107,7 +107,7 @@ let_it_be(:assignee_user) { create(:user, enterprise_group: nil) } it_behaves_like 'an error response', - error: 'You can assign only enterprise users in the top-level group you're importing to.' + error: "You can assign only enterprise users in the top-level group you're importing to." context 'when not on .com' do before do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4a471e51ad4a49..d2d3d5e0ada9d2 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -65521,7 +65521,7 @@ msgstr "" msgid "UserMapping|You can assign only active users with regular or auditor access. To assign users with administrator access, ask your GitLab administrator to enable the \"Allow contribution mapping to administrators\" setting." msgstr "" -msgid "UserMapping|You can assign only to Enterprise Users of the root namespace. Ensure the user is an Enterprise member of the top level namespace you are importing to." +msgid "UserMapping|You can assign only enterprise users in the top-level group you're importing to." msgstr "" msgid "UserMapping|You can assign only users with linked SAML and SCIM identities. Ensure the user has signed into GitLab through your SAML SSO provider and has an active SCIM identity for this group." -- GitLab From 50975f63f3cb168e8d8460e767165e36edecf7c6 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 30 Apr 2025 16:50:36 +0100 Subject: [PATCH 22/29] Move loading back to actions component Co-authored by: Justin Ho --- .../components/placeholder_actions.vue | 71 ++++++++++++------ .../components/placeholders_table.vue | 72 +------------------ 2 files changed, 50 insertions(+), 93 deletions(-) diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index c3c323e5715b89..217640bbb0f6d2 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -2,10 +2,15 @@ import produce from 'immer'; import { debounce, isEmpty, isNull } from 'lodash'; import { GlAvatarLabeled, GlButton, GlCollapsibleListbox } from '@gitlab/ui'; -import { getFirstPropertyValue } from '~/lib/utils/common_utils'; +import { + getFirstPropertyValue, + normalizeHeaders, + parseIntPagination, +} from '~/lib/utils/common_utils'; import { __, s__ } from '~/locale'; import { createAlert } from '~/alert'; import searchUsersQuery from '~/graphql_shared/queries/users_search_all_paginated.query.graphql'; +import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; import { PLACEHOLDER_STATUS_AWAITING_APPROVAL, @@ -33,36 +38,19 @@ export default { GlCollapsibleListbox, }, inject: { + group: { + default: {}, + }, restrictReassignmentToEnterprise: { default: false, }, }, props: { - enterpriseUsersIsLoadingInitial: { - type: Boolean, - required: false, - default: true, - }, - enterpriseUsersIsLoadingMore: { - type: Boolean, - required: false, - default: true, - }, sourceUser: { type: Object, required: false, default: () => ({}), }, - enterpriseUsers: { - type: Array, - required: false, - default: () => [], - }, - enterpriseUsersPageInfo: { - type: Object, - required: false, - default: () => ({ nextPage: null }), - }, }, data() { @@ -75,6 +63,12 @@ export default { isValidated: false, search: '', selectedUserToReassign: null, + enterpriseUsers: [], + enterpriseUsersPageInfo: { + nextPage: null, + }, + enterpriseUsersIsLoadingInitial: false, + enterpriseUsersIsLoadingMore: false, }; }, @@ -194,11 +188,41 @@ export default { }, methods: { + async fetchEnterpriseUsers(page) { + try { + const { data, headers } = await fetchGroupEnterpriseUsers(this.group.id, { + page, + per_page: USERS_PER_PAGE, + search: this.search, + }); + + this.enterpriseUsersPageInfo = parseIntPagination(normalizeHeaders(headers)); + this.enterpriseUsers.push(...data); + } catch (error) { + this.onError(); + } + }, + async loadInitialEnterpriseUsers() { + if (!this.restrictReassignmentToEnterprise) { + return; + } + + this.enterpriseUsersIsLoadingInitial = true; + await this.fetchEnterpriseUsers(1); + this.enterpriseUsersIsLoadingInitial = false; + }, + + async loadMoreEnterpriseUsers() { + this.enterpriseUsersIsLoadingMore = true; + await this.fetchEnterpriseUsers(this.enterpriseUsersPageInfo.nextPage); + this.enterpriseUsersIsLoadingMore = false; + }, + async loadMoreUsers() { if (!this.hasNextPage) return; if (this.restrictReassignmentToEnterprise) { - this.$emit('load-more-enterprise-users'); + this.loadMoreEnterpriseUsers(); return; } @@ -233,6 +257,8 @@ export default { this.search = searchTerm; if (this.restrictReassignmentToEnterprise) { + this.enterpriseUsers = []; + this.loadInitialEnterpriseUsers(); this.$emit('reset-enterprise-search', this.search); } }, @@ -379,6 +405,7 @@ export default { :searching="isLoading" infinite-scroll :infinite-scroll-loading="isLoadingMore" + @shown="loadInitialEnterpriseUsers" @search="debouncedSetSearch" @select="onSelect" @bottom-reached="loadMoreUsers" diff --git a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue index d067b598d484db..5edba12a094bbe 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue @@ -11,8 +11,6 @@ import { GlSprintf, GlLink, } from '@gitlab/ui'; -import { normalizeHeaders, parseIntPagination } from '~/lib/utils/common_utils'; -import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; import { createAlert } from '~/alert'; import { __, s__ } from '~/locale'; import { fetchPolicies } from '~/lib/graphql'; @@ -30,7 +28,6 @@ import importSourceUsersQuery from '../graphql/queries/import_source_users.query import PlaceholderActions from './placeholder_actions.vue'; const MINIMUM_QUERY_LENGTH = 3; -const USERS_PER_PAGE = 20; export default { name: 'PlaceholdersTable', @@ -54,9 +51,6 @@ export default { group: { default: {}, }, - restrictReassignmentToEnterprise: { - default: false, - }, }, props: { queryStatuses: { @@ -85,13 +79,6 @@ export default { before: null, after: null, }, - enterpriseUsersSearch: null, - enterpriseUsers: [], - enterpriseUsersPageInfo: { - nextPage: null, - }, - enterpriseUsersIsLoadingInitial: false, - enterpriseUsersIsLoadingMore: false, }; }, apollo: { @@ -174,12 +161,6 @@ export default { }, }, - mounted() { - if (this.restrictReassignmentToEnterprise && !this.reassigned) { - this.loadInitialEnterpriseUsers(); - } - }, - methods: { statusBadge(item) { return placeholderUserBadges[item.status]; @@ -225,46 +206,6 @@ export default { formatDate(dateString) { return localeDateFormat.asDateTime.format(new Date(dateString)); }, - - async fetchEnterpriseUsers(page) { - try { - const { data, headers } = await fetchGroupEnterpriseUsers(this.group.id, { - page, - per_page: USERS_PER_PAGE, - search: this.enterpriseUsersSearch, - }); - - this.enterpriseUsersPageInfo = parseIntPagination(normalizeHeaders(headers)); - this.enterpriseUsers.push(...data); - } catch (error) { - this.onError(); - } - }, - - async loadInitialEnterpriseUsers() { - this.enterpriseUsersIsLoadingInitial = true; - await this.fetchEnterpriseUsers(1); - this.enterpriseUsersIsLoadingInitial = false; - }, - - async loadMoreEnterpriseUsers() { - this.enterpriseUsersIsLoadingMore = true; - await this.fetchEnterpriseUsers(this.enterpriseUsersPageInfo.nextPage); - this.enterpriseUsersIsLoadingMore = false; - }, - - resetEnterpriseSearch(searchTerm) { - this.enterpriseUsersSearch = searchTerm; - this.enterpriseUsers = []; - - this.loadInitialEnterpriseUsers(); - }, - - onError() { - createAlert({ - message: __('There was a problem fetching users.'), - }); - }, }, placeholderUsersHelpPath: helpPagePath('user/project/import/_index', { anchor: 'placeholder-users', @@ -367,18 +308,7 @@ export default { data-testid="placeholder-reassigned" /> - + -- GitLab From bb359b7be84610efa258dfe7a1ed455712de0e90 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Thu, 1 May 2025 09:38:58 +0100 Subject: [PATCH 23/29] Move enterprise user fetching back to actions --- .../components/placeholder_actions.vue | 3 +- .../components/placeholders_table.vue | 6 +- .../components/placeholder_actions_spec.js | 68 +++++- .../components/placeholders_table_spec.js | 199 ------------------ .../components/placeholder_actions_spec.js | 1 - 5 files changed, 62 insertions(+), 215 deletions(-) delete mode 100644 ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js diff --git a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue index 217640bbb0f6d2..f46338636ea3f2 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholder_actions.vue @@ -203,7 +203,7 @@ export default { } }, async loadInitialEnterpriseUsers() { - if (!this.restrictReassignmentToEnterprise) { + if (!this.restrictReassignmentToEnterprise || this.enterpriseUsers.length > 0) { return; } @@ -259,7 +259,6 @@ export default { if (this.restrictReassignmentToEnterprise) { this.enterpriseUsers = []; this.loadInitialEnterpriseUsers(); - this.$emit('reset-enterprise-search', this.search); } }, diff --git a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue index 5edba12a094bbe..69a0fa6c6e6c96 100644 --- a/app/assets/javascripts/members/placeholders/components/placeholders_table.vue +++ b/app/assets/javascripts/members/placeholders/components/placeholders_table.vue @@ -47,11 +47,7 @@ export default { directives: { GlTooltip: GlTooltipDirective, }, - inject: { - group: { - default: {}, - }, - }, + inject: ['group'], props: { queryStatuses: { type: Array, diff --git a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index 155c5b363acf3a..02535691fde148 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -1,6 +1,7 @@ import Vue from 'vue'; import VueApollo from 'vue-apollo'; import { GlCollapsibleListbox } from '@gitlab/ui'; +import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; @@ -9,6 +10,13 @@ import PlaceholderActions from '~/members/placeholders/components/placeholder_ac import { mockSourceUsers } from 'jest/members/placeholders/mock_data'; +import { + mockEnterpriseUser1, + mockEnterpriseUser2, + mockEnterpriseUsersQueryResponse, + mockEnterpriseUsersWithPaginationQueryResponse, +} from '../mock_data'; + Vue.use(VueApollo); jest.mock('~/alert'); jest.mock('ee_component/api/groups_api'); @@ -47,19 +55,57 @@ describe('PlaceholderActions', () => { describe('when restrictReassignmentToEnterprise is true', () => { describe('when users query is loading', () => { - beforeEach(async () => { + it('renders listbox as loading and resets to false after finishing', async () => { + let loadingStateDuringFetch = false; + + jest + .spyOn(PlaceholderActions.methods, 'fetchEnterpriseUsers') + .mockImplementation(async () => { + await Vue.nextTick(); + loadingStateDuringFetch = findListbox().props('loading'); + }); + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + await findListbox().vm.$emit('shown'); await waitForPromises(); + + expect(loadingStateDuringFetch).toBe(true); + expect(findListbox().props('loading')).toBe(false); }); + }); + + describe('when the component is opened multiple times', () => { + beforeEach(() => { + fetchGroupEnterpriseUsers.mockResolvedValueOnce( + mockEnterpriseUsersWithPaginationQueryResponse, + ); + }); + + it('does not call fetchEnterpriseUsers again', async () => { + createComponent({ + provide: { restrictReassignmentToEnterprise: true }, + props: { enterpriseUsers: [mockEnterpriseUser1] }, + }); + await findListbox().vm.$emit('shown'); + await waitForPromises(); + + await findListbox().vm.$emit('shown'); + await waitForPromises(); - it('renders listbox as loading', () => { - expect(findListbox().props('loading')).toBe(true); + expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(1); }); }); describe('when users query succeeds and has pagination', () => { beforeEach(async () => { + fetchGroupEnterpriseUsers.mockResolvedValueOnce( + mockEnterpriseUsersWithPaginationQueryResponse, + ); + fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse()); + createComponent({ provide: { restrictReassignmentToEnterprise: true } }); + + await findListbox().vm.$emit('shown'); await waitForPromises(); }); @@ -68,8 +114,14 @@ describe('PlaceholderActions', () => { await findListbox().vm.$emit('bottom-reached'); }); - it('emits the "load-more-enterprise-users" event', () => { - expect(wrapper.emitted('load-more-enterprise-users')[0]).toEqual([]); + it('calls fetchGroupEnterpriseUsers again to get next page', () => { + expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); + }); + + it('appends query results to "enterpriseUsers"', () => { + const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; + + expect(wrapper.vm.enterpriseUsers).toEqual(expect.arrayContaining(allUsers)); }); }); }); @@ -78,8 +130,8 @@ describe('PlaceholderActions', () => { beforeEach(async () => { createComponent({ provide: { restrictReassignmentToEnterprise: true }, - props: { enterpriseUsersPageInfo: { nextPage: null } }, }); + await findListbox().vm.$emit('shown'); await waitForPromises(); }); @@ -88,8 +140,8 @@ describe('PlaceholderActions', () => { await findListbox().vm.$emit('bottom-reached'); }); - it('does not emit "load-more-enterprise-users" event', () => { - expect(wrapper.emitted('load-more-enterprise-users')).toBeUndefined(); + it('does not call fetchGroupEnterpriseUsers again', () => { + expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(1); }); }); }); diff --git a/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js b/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js deleted file mode 100644 index 076657b19b8c0b..00000000000000 --- a/ee/spec/frontend/members/placeholders/components/placeholders_table_spec.js +++ /dev/null @@ -1,199 +0,0 @@ -import Vue from 'vue'; -import VueApollo from 'vue-apollo'; - -import { mount } from '@vue/test-utils'; -import PlaceholderActions from '~/members/placeholders/components/placeholder_actions.vue'; -import createMockApollo from 'helpers/mock_apollo_helper'; -import { createAlert } from '~/alert'; -import { fetchGroupEnterpriseUsers } from 'ee_else_ce/api/groups_api'; -import { createMockDirective } from 'helpers/vue_mock_directive'; - -import PlaceholdersTable from '~/members/placeholders/components/placeholders_table.vue'; -import waitForPromises from 'helpers/wait_for_promises'; - -import importSourceUsersQuery from '~/members/placeholders/graphql/queries/import_source_users.query.graphql'; - -import { - PLACEHOLDER_USER_STATUS, - PLACEHOLDER_SORT_SOURCE_NAME_ASC, -} from '~/import_entities/import_groups/constants'; - -import { mockSourceUsersQueryResponse } from 'jest/members/placeholders/mock_data'; -import { - mockEnterpriseUser1, - mockEnterpriseUser2, - mockEnterpriseUsersQueryResponse, - mockEnterpriseUsersWithPaginationQueryResponse, -} from '../mock_data'; - -jest.mock('~/alert'); -jest.mock('ee_component/api/groups_api'); - -Vue.use(VueApollo); -jest.mock('~/alert'); - -describe('PlaceholdersTable', () => { - let wrapper; - let mockApollo; - - const mockGroup = { - path: 'imported-group', - name: 'Imported group', - }; - - const defaultProps = { - queryStatuses: PLACEHOLDER_USER_STATUS.UNASSIGNED, - reassigned: false, - querySort: PLACEHOLDER_SORT_SOURCE_NAME_ASC, - }; - - const defaultProvide = { - group: mockGroup, - restrictReassignmentToEnterprise: true, - }; - - const sourceUsersQueryHandler = jest.fn().mockResolvedValue(mockSourceUsersQueryResponse()); - - const $toast = { - show: jest.fn(), - }; - - const createComponent = ({ - queryHandler = sourceUsersQueryHandler, - mountFn = mount, - props = {}, - provide = {}, - options = {}, - } = {}) => { - mockApollo = createMockApollo([[importSourceUsersQuery, queryHandler]]); - - wrapper = mountFn(PlaceholdersTable, { - apolloProvider: mockApollo, - propsData: { - ...defaultProps, - ...props, - }, - provide: { - ...defaultProvide, - ...provide, - }, - mocks: { $toast }, - directives: { - GlTooltip: createMockDirective('gl-tooltip'), - }, - ...options, - }); - }; - - const findPlaceholderActions = () => wrapper.findComponent(PlaceholderActions); - - describe('when restrictReassignmentToEnterprise is true', () => { - describe('when users query is loading', () => { - beforeEach(() => { - fetchGroupEnterpriseUsers.mockResolvedValueOnce({ data: [] }); - }); - - it('calls fetchGroupEnterpriseUsers', async () => { - createComponent(); - - await waitForPromises(); - - expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(1); - }); - }); - - describe('when users query succeeds and has pagination', () => { - beforeEach(async () => { - fetchGroupEnterpriseUsers.mockResolvedValueOnce( - mockEnterpriseUsersWithPaginationQueryResponse, - ); - fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse()); - - createComponent(); - await waitForPromises(); - }); - - describe('when "load-more-enterprise-users" event is emitted', () => { - beforeEach(() => { - findPlaceholderActions().vm.$emit('load-more-enterprise-users'); - }); - - it('calls fetchGroupEnterpriseUsers again to get next page', () => { - expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(2); - }); - - it('appends query results to "enterpriseUsers"', () => { - const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; - - expect(findPlaceholderActions().props('enterpriseUsers')).toEqual( - expect.arrayContaining(allUsers), - ); - }); - }); - - describe('when "reset-enterprise-search" event is emitted', () => { - beforeEach(() => { - fetchGroupEnterpriseUsers.mockResolvedValueOnce( - mockEnterpriseUsersQueryResponse(mockEnterpriseUser2), - ); - - findPlaceholderActions().vm.$emit('reset-enterprise-search', 'Rookie'); - }); - - it('queries for the search term', () => { - expect(fetchGroupEnterpriseUsers).toHaveBeenCalledWith( - undefined, - expect.objectContaining({ search: 'Rookie' }), - ); - }); - - it('resets the enterpriseUsers and page count', () => { - const searchTermResults = [mockEnterpriseUser2]; - - expect(findPlaceholderActions().props('enterpriseUsers')).toEqual( - expect.arrayContaining(searchTermResults), - ); - expect(findPlaceholderActions().props('enterpriseUsersPageInfo')).toEqual( - expect.objectContaining({ nextPage: 2 }), - ); - }); - }); - }); - - describe('when users query fails', () => { - beforeEach(async () => { - fetchGroupEnterpriseUsers.mockReset(); - fetchGroupEnterpriseUsers.mockRejectedValue(new Error('Axios error')); - - createComponent(); - await waitForPromises(); - }); - - it('creates an alert', () => { - expect(createAlert).toHaveBeenCalledWith({ - message: 'There was a problem fetching users.', - }); - }); - }); - }); - - describe('when restrictReassignmentToEnterprise is false', () => { - it('does not call fetchGroupEnterpriseUsers', async () => { - createComponent({ provide: { restrictReassignmentToEnterprise: false } }); - - await waitForPromises(); - - expect(fetchGroupEnterpriseUsers).not.toHaveBeenCalled(); - }); - }); - - describe('when reassigned is true', () => { - it('does not call fetchGroupEnterpriseUsers', async () => { - createComponent({ props: { reassigned: true } }); - - await waitForPromises(); - - expect(fetchGroupEnterpriseUsers).not.toHaveBeenCalled(); - }); - }); -}); diff --git a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index a902f0ade66156..0e35acb78c7a8d 100644 --- a/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -37,7 +37,6 @@ describe('PlaceholderActions', () => { const defaultProps = { sourceUser: mockSourceUsers[0], }; - const usersQueryHandler = jest.fn().mockResolvedValue(mockUsersQueryResponse); const sourceUsersQueryHandler = jest.fn().mockResolvedValue(mockSourceUsersQueryResponse()); const reassignMutationHandler = jest.fn().mockResolvedValue(mockReassignMutationResponse); -- GitLab From 884a6464900764b3c6b0947a0530ba1fae576828 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Fri, 2 May 2025 15:44:49 +0100 Subject: [PATCH 24/29] Update specs based on feedback --- .../components/placeholder_actions_spec.js | 92 ++++++++----------- 1 file changed, 40 insertions(+), 52 deletions(-) diff --git a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js index 02535691fde148..549025dec96769 100644 --- a/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js +++ b/ee/spec/frontend/members/placeholders/components/placeholder_actions_spec.js @@ -26,10 +26,6 @@ describe('PlaceholderActions', () => { const defaultProps = { sourceUser: mockSourceUsers[0], - enterpriseUsersPageInfo: { nextPage: 1 }, - }; - const defaultProvide = { - restrictReassignmentToEnterprise: false, }; const $toast = { @@ -44,7 +40,6 @@ describe('PlaceholderActions', () => { ...props, }, provide: { - ...defaultProvide, ...provide, }, mocks: { $toast }, @@ -53,27 +48,48 @@ describe('PlaceholderActions', () => { const findListbox = () => wrapper.findComponent(GlCollapsibleListbox); - describe('when restrictReassignmentToEnterprise is true', () => { - describe('when users query is loading', () => { - it('renders listbox as loading and resets to false after finishing', async () => { - let loadingStateDuringFetch = false; - - jest - .spyOn(PlaceholderActions.methods, 'fetchEnterpriseUsers') - .mockImplementation(async () => { - await Vue.nextTick(); - loadingStateDuringFetch = findListbox().props('loading'); - }); - - createComponent({ provide: { restrictReassignmentToEnterprise: true } }); - await findListbox().vm.$emit('shown'); - await waitForPromises(); + afterEach(() => { + fetchGroupEnterpriseUsers.mockClear(); + }); + + describe('when restrictReassignmentToEnterprise is false', () => { + beforeEach(() => { + createComponent({ + provide: { restrictReassignmentToEnterprise: false }, + }); + }); + + it('does not call fetchGroupEnterpriseUsers on listbox `shown` event', async () => { + await findListbox().vm.$emit('shown'); + await waitForPromises(); - expect(loadingStateDuringFetch).toBe(true); - expect(findListbox().props('loading')).toBe(false); + expect(fetchGroupEnterpriseUsers).not.toHaveBeenCalled(); + }); + }); + + describe('when restrictReassignmentToEnterprise is true', () => { + beforeEach(() => { + createComponent({ + provide: { restrictReassignmentToEnterprise: true }, }); }); + it('calls fetchGroupEnterpriseUsers on listbox `shown` event', async () => { + fetchGroupEnterpriseUsers.mockResolvedValueOnce( + mockEnterpriseUsersWithPaginationQueryResponse, + ); + + await findListbox().vm.$emit('shown'); + + expect(findListbox().props('loading')).toBe(true); + expect(fetchGroupEnterpriseUsers).toHaveBeenCalledTimes(1); + + await waitForPromises(); + + expect(findListbox().props('loading')).toBe(false); + expect(findListbox().props('items')).toHaveLength(1); + }); + describe('when the component is opened multiple times', () => { beforeEach(() => { fetchGroupEnterpriseUsers.mockResolvedValueOnce( @@ -81,11 +97,7 @@ describe('PlaceholderActions', () => { ); }); - it('does not call fetchEnterpriseUsers again', async () => { - createComponent({ - provide: { restrictReassignmentToEnterprise: true }, - props: { enterpriseUsers: [mockEnterpriseUser1] }, - }); + it('does not call fetchGroupEnterpriseUsers again', async () => { await findListbox().vm.$emit('shown'); await waitForPromises(); @@ -103,8 +115,6 @@ describe('PlaceholderActions', () => { ); fetchGroupEnterpriseUsers.mockResolvedValueOnce(mockEnterpriseUsersQueryResponse()); - createComponent({ provide: { restrictReassignmentToEnterprise: true } }); - await findListbox().vm.$emit('shown'); await waitForPromises(); }); @@ -121,16 +131,13 @@ describe('PlaceholderActions', () => { it('appends query results to "enterpriseUsers"', () => { const allUsers = [mockEnterpriseUser2, mockEnterpriseUser1]; - expect(wrapper.vm.enterpriseUsers).toEqual(expect.arrayContaining(allUsers)); + expect(findListbox().props('items')).toHaveLength(allUsers.length); }); }); }); describe('when users query succeeds and does not have pagination', () => { beforeEach(async () => { - createComponent({ - provide: { restrictReassignmentToEnterprise: true }, - }); await findListbox().vm.$emit('shown'); await waitForPromises(); }); @@ -146,23 +153,4 @@ describe('PlaceholderActions', () => { }); }); }); - - describe('when restrictReassignmentToEnterprise is false', () => { - describe('when users query succeeds and has pagination', () => { - beforeEach(async () => { - createComponent({ provide: { restrictReassignmentToEnterprise: false } }); - await waitForPromises(); - }); - - describe('when "bottom-reached" event is emitted', () => { - beforeEach(async () => { - await findListbox().vm.$emit('bottom-reached'); - }); - - it('does not emit "load-more-enterprise-users" event', () => { - expect(wrapper.emitted('load-more-enterprise-users')).toBeUndefined(); - }); - }); - }); - }); }); -- GitLab From 03f877b77aeb435364d4d52a8057c4d89132c888 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 7 May 2025 09:50:24 +0100 Subject: [PATCH 25/29] Do not check Gitlab.com? for restrict reassignment --- ee/app/helpers/ee/groups/group_members_helper.rb | 2 +- .../helpers/ee/groups/group_members_helper_spec.rb | 14 -------------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/ee/app/helpers/ee/groups/group_members_helper.rb b/ee/app/helpers/ee/groups/group_members_helper.rb index 2ae7d8bef4609d..2ee025301714fb 100644 --- a/ee/app/helpers/ee/groups/group_members_helper.rb +++ b/ee/app/helpers/ee/groups/group_members_helper.rb @@ -33,7 +33,7 @@ def group_members_app_data( promotion_request: { enabled: member_promotion_management_enabled?, total_items: pending_members_count }, can_approve_access_requests: !::Namespaces::FreeUserCap::Enforcement.new(group.root_ancestor).reached_limit?, namespace_user_limit: ::Namespaces::FreeUserCap.dashboard_limit, - restrict_reassignment_to_enterprise: ::Gitlab.com? && group.any_enterprise_users? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks -- We only want this on .com + restrict_reassignment_to_enterprise: group.any_enterprise_users? }) end # rubocop:enable Metrics/ParameterLists diff --git a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb index ece8840c3a6494..50c8496be0dd42 100644 --- a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb +++ b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb @@ -58,23 +58,9 @@ context 'when enterprise users exist for the group' do let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } - before do - allow(::Gitlab).to receive(:com?).and_return(true) - end - it 'sets the restrict_reassignment_to_enterprise flag to true' do expect(subject[:restrict_reassignment_to_enterprise]).to be(true) end - - context 'when not on .com' do - before do - allow(::Gitlab).to receive(:com?).and_return(false) - end - - it 'sets the restrict_reassignment_to_enterprise flag to false' do - expect(subject[:restrict_reassignment_to_enterprise]).to be(false) - end - end end context 'when enterprise users do not exist for the group' do -- GitLab From 6d1b11158ba0bce885146f32b72945ebde0215f4 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 7 May 2025 15:01:58 +0100 Subject: [PATCH 26/29] Remove .com check in reassign service --- .../ee/import/source_users/reassign_service.rb | 2 +- .../ee/import/source_users/reassign_service_spec.rb | 12 ------------ 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/ee/app/services/ee/import/source_users/reassign_service.rb b/ee/app/services/ee/import/source_users/reassign_service.rb index 20ad1261236b1f..dce81c9f1cbdfd 100644 --- a/ee/app/services/ee/import/source_users/reassign_service.rb +++ b/ee/app/services/ee/import/source_users/reassign_service.rb @@ -34,7 +34,7 @@ def error_invalid_assignee_due_to_sso_enforcement end def valid_assignee_if_should_check_enterprise_users? - return true unless ::Gitlab.com? && root_namespace.any_enterprise_users? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks -- We only want this on .com + return true unless root_namespace.any_enterprise_users? assignee_user.enterprise_group == root_namespace end diff --git a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb index 3c9d0e64bd2555..39affad168b9fc 100644 --- a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb +++ b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb @@ -90,10 +90,6 @@ end context 'for enterprise users check' do - before do - allow(::Gitlab).to receive(:com?).and_return(true) - end - context 'when there are enterprise users' do let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } @@ -108,14 +104,6 @@ it_behaves_like 'an error response', error: "You can assign only enterprise users in the top-level group you're importing to." - - context 'when not on .com' do - before do - allow(::Gitlab).to receive(:com?).and_return(false) - end - - it_behaves_like 'success response' - end end end -- GitLab From 78870aaa463007bbc2997458a9ebaf8e9d445eda Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Wed, 7 May 2025 15:18:26 +0100 Subject: [PATCH 27/29] Remove .com? stub in specs Also remove spec for domain_verification_available? --- .../ee/groups/group_members_helper_spec.rb | 19 ------------------- ee/spec/models/ee/namespace_spec.rb | 4 ---- 2 files changed, 23 deletions(-) diff --git a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb index 50c8496be0dd42..e6ddf2ed216aa2 100644 --- a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb +++ b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb @@ -64,30 +64,11 @@ end context 'when enterprise users do not exist for the group' do - before do - allow(::Gitlab).to receive(:com?).and_return(true) - end - it 'sets the restrict_reassignment_to_enterprise flag to true' do expect(subject[:restrict_reassignment_to_enterprise]).to be(false) end end - context 'when domain_verification_available? is false' do - let_it_be(:group, reload: true) { create(:group, path: 'root-path') } - let_it_be(:project) { create(:project, group:) } - let_it_be(:pages_domain) { create(:pages_domain, project:) } - - before do - stub_licensed_features(domain_verification: false) - allow(::Gitlab).to receive(:com?).and_return(true) - end - - it 'sets the restrict_reassignment_to_enterprise flag to false' do - expect(subject[:restrict_reassignment_to_enterprise]).to be(false) - end - end - context 'adds `can_approve_access_requests`' do before do stub_ee_application_setting(dashboard_limit_enabled: true) diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb index 92d3bcdbad2056..e93c4e2bd09455 100644 --- a/ee/spec/models/ee/namespace_spec.rb +++ b/ee/spec/models/ee/namespace_spec.rb @@ -2308,10 +2308,6 @@ subject { group.any_enterprise_users? } - before do - allow(::Gitlab).to receive(:com?).and_return(true) - end - context 'when there are enterprise users for the group' do let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } -- GitLab From d17f9c74b9ac901b5a814304c809735906f5339f Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Thu, 8 May 2025 12:30:31 +0100 Subject: [PATCH 28/29] Check domain verification and managed_by_group Prevents an issue where a group downgrades, but still has enterprise users. --- ee/app/models/ee/namespace.rb | 2 +- .../ee/import/source_users/reassign_service.rb | 2 +- ee/spec/models/ee/namespace_spec.rb | 13 +++++++++++++ .../ee/import/source_users/reassign_service_spec.rb | 6 ++++++ 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/ee/app/models/ee/namespace.rb b/ee/app/models/ee/namespace.rb index afd3e1c93e11c8..fff845981e3a58 100644 --- a/ee/app/models/ee/namespace.rb +++ b/ee/app/models/ee/namespace.rb @@ -612,7 +612,7 @@ def domain_verification_available? end def any_enterprise_users? - enterprise_users.any? + domain_verification_available? && enterprise_users.any? end def enforce_ssh_certificates? diff --git a/ee/app/services/ee/import/source_users/reassign_service.rb b/ee/app/services/ee/import/source_users/reassign_service.rb index dce81c9f1cbdfd..3620df17950754 100644 --- a/ee/app/services/ee/import/source_users/reassign_service.rb +++ b/ee/app/services/ee/import/source_users/reassign_service.rb @@ -36,7 +36,7 @@ def error_invalid_assignee_due_to_sso_enforcement def valid_assignee_if_should_check_enterprise_users? return true unless root_namespace.any_enterprise_users? - assignee_user.enterprise_group == root_namespace + assignee_user.managed_by_group?(root_namespace) end def error_invalid_assignee_due_to_enterprise_users_check diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb index e93c4e2bd09455..c9abd4cb0ddfcb 100644 --- a/ee/spec/models/ee/namespace_spec.rb +++ b/ee/spec/models/ee/namespace_spec.rb @@ -2308,10 +2308,23 @@ subject { group.any_enterprise_users? } + before do + stub_licensed_features(domain_verification: true) + allow(::Gitlab).to receive(:com?).and_return(true) + end + context 'when there are enterprise users for the group' do let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } it { is_expected.to be(true) } + + context 'when domain_verification_available? is false' do + before do + stub_licensed_features(domain_verification: false) + end + + it { is_expected.to be(false) } + end end context 'when there are enterprise users for another group' do diff --git a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb index 39affad168b9fc..fc6aa17f02c194 100644 --- a/ee/spec/services/ee/import/source_users/reassign_service_spec.rb +++ b/ee/spec/services/ee/import/source_users/reassign_service_spec.rb @@ -90,6 +90,12 @@ end context 'for enterprise users check' do + before do + # Needed for managed_by_group? + stub_licensed_features(domain_verification: true) + allow(::Gitlab).to receive(:com?).and_return(true) + end + context 'when there are enterprise users' do let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } -- GitLab From 66843e33a7e272705b2241b3fd0018c0c7eef219 Mon Sep 17 00:00:00 2001 From: Keeyan Nejad Date: Thu, 8 May 2025 14:45:14 +0100 Subject: [PATCH 29/29] Fix failing specs --- ee/spec/helpers/ee/groups/group_members_helper_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb index e6ddf2ed216aa2..15407734c554d7 100644 --- a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb +++ b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb @@ -58,6 +58,11 @@ context 'when enterprise users exist for the group' do let_it_be(:enterprise_user) { create(:user, enterprise_group: group) } + before do + stub_licensed_features(domain_verification: true) + allow(::Gitlab).to receive(:com?).and_return(true) + end + it 'sets the restrict_reassignment_to_enterprise flag to true' do expect(subject[:restrict_reassignment_to_enterprise]).to be(true) end -- GitLab