diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx index 652555cf64..00b820f5b2 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx @@ -14,15 +14,20 @@ * limitations under the License. */ -import { Entity, parseEntityRef } from '@backstage/catalog-model'; +import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; import { fireEvent, screen } from '@testing-library/react'; import React from 'react'; import { MockEntityListContextProvider } from '../../testUtils/providers'; import { EntityOwnerFilter } from '../../filters'; import { EntityOwnerPicker } from './EntityOwnerPicker'; import { ApiProvider } from '@backstage/core-app-api'; -import { renderWithEffects, TestApiRegistry } from '@backstage/test-utils'; +import { + MockErrorApi, + renderWithEffects, + TestApiRegistry, +} from '@backstage/test-utils'; import { catalogApiRef, CatalogApi } from '../..'; +import { errorApiRef } from '@backstage/core-plugin-api'; const ownerEntities: Entity[] = [ { @@ -38,12 +43,18 @@ const ownerEntities: Entity[] = [ metadata: { name: 'some-owner-2', }, + spec: { + profile: { + displayName: 'Some Owner 2', + }, + }, }, { apiVersion: '1', kind: 'Group', metadata: { name: 'another-owner', + title: 'Another Owner', }, }, ]; @@ -96,57 +107,20 @@ const sampleEntities: Entity[] = [ const getEntitiesByRefs = jest.fn(async ({ entityRefs }) => ({ items: entityRefs.map((e: string) => - ownerEntities.find(f => f.metadata.name === e), + ownerEntities.find(f => stringifyEntityRef(f) === e), ), })); const mockCatalogApi: Partial = { getEntitiesByRefs, }; +const mockErrorApi = new MockErrorApi(); describe('', () => { - const mockApis = TestApiRegistry.from([catalogApiRef, mockCatalogApi]); + const mockApis = TestApiRegistry.from( + [catalogApiRef, mockCatalogApi], + [errorApiRef, mockErrorApi], + ); - it('renders display name when available', async () => { - const names: Record = { - 'some-owner': 'Some Team', - 'some-owner-2': 'Other Team', - 'another-owner': 'AnotherTeam', - }; - getEntitiesByRefs.mockResolvedValueOnce({ - items: ownerEntities.map(e => { - e.spec = { - ...e.spec, - profile: { - displayName: names[e.metadata.name], - }, - }; - return e; - }), - }); - await renderWithEffects( - - - - - , - ); - expect(screen.getByText('Owner')).toBeInTheDocument(); - - fireEvent.click(screen.getByTestId('owner-picker-expand')); - sampleEntities - .flatMap(e => e.relations?.map(r => parseEntityRef(r.targetRef).name)) - .forEach(owner => { - expect(screen.getByText(names[owner as string])).toBeInTheDocument(); - }); - }); - - /** - * All previous test cases are still applicable for the case where there is no - * owner entity returned from the API or the owner entity returned does not have - * a display name. - */ it('renders all owners', async () => { await renderWithEffects( @@ -160,11 +134,9 @@ describe('', () => { expect(screen.getByText('Owner')).toBeInTheDocument(); fireEvent.click(screen.getByTestId('owner-picker-expand')); - sampleEntities - .flatMap(e => e.relations?.map(r => parseEntityRef(r.targetRef).name)) - .forEach(owner => { - expect(screen.getByText(owner as string)).toBeInTheDocument(); - }); + ['Another Owner', 'some-owner', 'Some Owner 2'].forEach(owner => { + expect(screen.getByText(owner)).toBeInTheDocument(); + }); }); it('renders unique owners in alphabetical order', async () => { @@ -182,9 +154,9 @@ describe('', () => { fireEvent.click(screen.getByTestId('owner-picker-expand')); expect(screen.getAllByRole('option').map(o => o.textContent)).toEqual([ - 'another-owner', + 'Another Owner', 'some-owner', - 'some-owner-2', + 'Some Owner 2', ]); }); diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index c3ac658238..f3d656b5ee 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -16,7 +16,6 @@ import { Entity, - parseEntityRef, RELATION_OWNED_BY, stringifyEntityRef, } from '@backstage/catalog-model'; @@ -36,11 +35,10 @@ import React, { useEffect, useMemo, useState } from 'react'; import { useEntityList } from '../../hooks/useEntityListProvider'; import { EntityOwnerFilter } from '../../filters'; import { getEntityRelations } from '../../utils'; -import { humanizeEntityRef } from '../EntityRefLink'; import useAsync from 'react-use/lib/useAsync'; -import { useApi } from '@backstage/core-plugin-api'; +import { errorApiRef, useApi } from '@backstage/core-plugin-api'; import { catalogApiRef } from '../../api'; -import { humanizeEntity } from '../EntityRefLink/humanize'; +import { humanizeEntity, humanizeEntityRef } from '../EntityRefLink/humanize'; /** @public */ export type CatalogReactEntityOwnerPickerClassKey = 'input'; @@ -67,12 +65,74 @@ export const EntityOwnerPicker = () => { queryParameters: { owners: ownersParameter }, } = useEntityList(); const catalogApi = useApi(catalogApiRef); + const errorApi = useApi(errorApiRef); const queryParamOwners = useMemo( () => [ownersParameter].flat().filter(Boolean) as string[], [ownersParameter], ); + const { + loading, + error, + value: ownerEntities, + } = useAsync(async () => { + const availableOwners = [ + ...new Set( + backendEntities + .flatMap((e: Entity) => + getEntityRelations(e, RELATION_OWNED_BY).map(o => + stringifyEntityRef(o), + ), + ) + .filter(Boolean) as string[], + ), + ]; + const { items } = await catalogApi.getEntitiesByRefs({ + entityRefs: availableOwners, + fields: [ + 'kind', + 'metadata.name', + 'metadata.title', + 'metadata.namespace', + 'spec.profile.displayName', + ], + }); + return ( + availableOwners + .map( + (e, i) => + items[i] || ({ metadata: { name: e }, kind: 'Group' } as Entity), + ) + // Keep the previous sorting logic. + .sort((a, b) => { + const nameA = humanizeEntity(a).toLocaleUpperCase('en-US'); // ignore upper and lowercase + const nameB = humanizeEntity(b).toLocaleUpperCase('en-US'); // ignore upper and lowercase + if (nameA < nameB) { + return -1; + } + if (nameA > nameB) { + return 1; + } + + // names must be equal + return 0; + }) + ); + }, [backendEntities]); + + useEffect(() => { + if (error) { + errorApi.post( + { + ...error, + message: `EntityOwnerPicker failed to initialize: ${error.message}`, + }, + {}, + ); + } + }, [error, errorApi]); + const [selectedOwners, setSelectedOwners] = useState( queryParamOwners.length ? queryParamOwners : filters.owners?.values ?? [], ); @@ -85,55 +145,16 @@ export const EntityOwnerPicker = () => { } }, [queryParamOwners]); - const availableOwners = useMemo( - () => - [ - ...new Set( - backendEntities - .flatMap((e: Entity) => - getEntityRelations(e, RELATION_OWNED_BY).map(o => - humanizeEntityRef(o, { defaultKind: 'group' }), - ), - ) - .filter(Boolean) as string[], - ), - ].sort(), - [backendEntities], - ); - - const { - loading, - error, - value: ownerEntities, - } = useAsync(async () => { - const { items } = await catalogApi.getEntitiesByRefs({ - entityRefs: availableOwners.map(ref => - stringifyEntityRef(parseEntityRef(ref, { defaultKind: 'Group' })), - ), - fields: [ - 'kind', - 'metadata.name', - 'metadata.title', - 'spec.profile.displayName', - ], - }); - return availableOwners.map( - (e, i) => - items.at(i) || ({ metadata: { name: e }, kind: 'Group' } as Entity), - ); - }, [availableOwners]); - useEffect(() => { - updateFilters({ - owners: - selectedOwners.length && availableOwners.length - ? new EntityOwnerFilter(selectedOwners) - : undefined, - }); - }, [selectedOwners, updateFilters, availableOwners]); - - if (!availableOwners.length) return null; - if (error) throw error; + if (!loading && ownerEntities) { + updateFilters({ + owners: + selectedOwners.length && ownerEntities.length + ? new EntityOwnerFilter(selectedOwners) + : undefined, + }); + } + }, [selectedOwners, updateFilters, ownerEntities, loading]); return ( @@ -152,7 +173,9 @@ export const EntityOwnerPicker = () => { ) ?? [] } onChange={(_: object, value: Entity[]) => - setSelectedOwners(value.map(e => e.metadata.name)) + setSelectedOwners( + value.map(e => humanizeEntityRef(e, { defaultKind: 'Group' })), + ) } getOptionLabel={option => humanizeEntity(option, { defaultKind: 'Group' }) @@ -175,6 +198,7 @@ export const EntityOwnerPicker = () => { renderInput={params => ( diff --git a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts index 1e377e1859..7f1501280a 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts @@ -18,8 +18,8 @@ import { Entity, CompoundEntityRef, DEFAULT_NAMESPACE, - UserEntity, } from '@backstage/catalog-model'; +import get from 'lodash/get'; /** * @param defaultNamespace - if set to false then namespace is never omitted, @@ -88,14 +88,12 @@ export function humanizeEntity( defaultNamespace?: string | false; }, ) { - let title: string | undefined = undefined; - switch (entity.kind) { - case 'User': - case 'Group': - title = (entity as UserEntity).spec?.profile?.displayName; - break; - default: - title = entity.metadata.title; + for (const path of ['spec.profile.displayName', 'metadata.title']) { + const value = get(entity, path); + if (value && typeof value === 'string') { + return value; + } } - return title || humanizeEntityRef(entity, opts); + + return humanizeEntityRef(entity, opts); }