From d7fa409a72100b56bb80ab97fe801b732fd7bd8e Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Thu, 9 Feb 2023 19:50:22 -0500 Subject: [PATCH 01/20] Set up an async select for user picker to pull additional data. Signed-off-by: Aramis Sennyey --- .../EntityOwnerPicker/EntityOwnerPicker.tsx | 43 ++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index 185de3535a..72ced126e5 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -14,7 +14,12 @@ * limitations under the License. */ -import { Entity, RELATION_OWNED_BY } from '@backstage/catalog-model'; +import { + Entity, + parseEntityRef, + RELATION_OWNED_BY, + stringifyEntityRef, +} from '@backstage/catalog-model'; import { Box, Checkbox, @@ -32,6 +37,9 @@ 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 { catalogApiRef } from '../../api'; /** @public */ export type CatalogReactEntityOwnerPickerClassKey = 'input'; @@ -57,6 +65,7 @@ export const EntityOwnerPicker = () => { filters, queryParameters: { owners: ownersParameter }, } = useEntityList(); + const catalogApi = useApi(catalogApiRef); const queryParamOwners = useMemo( () => [ownersParameter].flat().filter(Boolean) as string[], @@ -91,6 +100,21 @@ export const EntityOwnerPicker = () => { [backendEntities], ); + const { + loading, + error, + value: owners, + } = useAsync(async () => { + const { items } = await catalogApi.getEntitiesByRefs({ + entityRefs: availableOwners.map(ref => + stringifyEntityRef(parseEntityRef(ref, { defaultKind: 'Group' })), + ), + }); + return availableOwners.map( + (e, i) => items.at(i) || ({ metadata: { name: e } } as Entity), + ); + }, [availableOwners]); + useEffect(() => { updateFilters({ owners: @@ -101,6 +125,7 @@ export const EntityOwnerPicker = () => { }, [selectedOwners, updateFilters, availableOwners]); if (!availableOwners.length) return null; + if (error) throw error; return ( @@ -109,9 +134,17 @@ export const EntityOwnerPicker = () => { setSelectedOwners(value)} + loading={loading} + options={owners || []} + getOptionLabel={option => + option.metadata.title || option.metadata.name + } + value={owners?.filter(e => + selectedOwners.some(f => f === e.metadata.name), + )} + onChange={(_: object, value: Entity[]) => + setSelectedOwners(value.map(e => e.metadata.name)) + } renderOption={(option, { selected }) => ( { /> } onClick={event => event.preventDefault()} - label={option} + label={option.metadata.title || option.metadata.name} /> )} size="small" From c66531a8ebc78ca510fdc46120e107fdc7c9a11a Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Fri, 10 Feb 2023 18:46:00 -0500 Subject: [PATCH 02/20] Adding test cases and cleaning up. Signed-off-by: Aramis Sennyey --- .../EntityOwnerPicker.test.tsx | 257 ++++++++++++------ .../EntityOwnerPicker/EntityOwnerPicker.tsx | 26 +- .../components/EntityRefLink/humanize.test.ts | 59 +++- .../src/components/EntityRefLink/humanize.ts | 26 ++ .../src/components/EntityRefLink/index.ts | 2 +- 5 files changed, 280 insertions(+), 90 deletions(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx index b6e928b519..652555cf64 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx @@ -15,11 +15,38 @@ */ import { Entity, parseEntityRef } from '@backstage/catalog-model'; -import { fireEvent, render, screen } from '@testing-library/react'; +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 { catalogApiRef, CatalogApi } from '../..'; + +const ownerEntities: Entity[] = [ + { + apiVersion: '1', + kind: 'Group', + metadata: { + name: 'some-owner', + }, + }, + { + apiVersion: '1', + kind: 'Group', + metadata: { + name: 'some-owner-2', + }, + }, + { + apiVersion: '1', + kind: 'Group', + metadata: { + name: 'another-owner', + }, + }, +]; const sampleEntities: Entity[] = [ { @@ -67,14 +94,68 @@ const sampleEntities: Entity[] = [ }, ]; +const getEntitiesByRefs = jest.fn(async ({ entityRefs }) => ({ + items: entityRefs.map((e: string) => + ownerEntities.find(f => f.metadata.name === e), + ), +})); +const mockCatalogApi: Partial = { + getEntitiesByRefs, +}; + describe('', () => { - it('renders all owners', () => { - render( - - - , + const mockApis = TestApiRegistry.from([catalogApiRef, mockCatalogApi]); + + 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( + + + + + , ); expect(screen.getByText('Owner')).toBeInTheDocument(); @@ -86,13 +167,15 @@ describe('', () => { }); }); - it('renders unique owners in alphabetical order', () => { - render( - - - , + it('renders unique owners in alphabetical order', async () => { + await renderWithEffects( + + + + + , ); expect(screen.getByText('Owner')).toBeInTheDocument(); @@ -105,20 +188,22 @@ describe('', () => { ]); }); - it('respects the query parameter filter value', () => { + it('respects the query parameter filter value', async () => { const updateFilters = jest.fn(); const queryParameters = { owners: ['another-owner'] }; - render( - - - , + await renderWithEffects( + + + + + , ); expect(updateFilters).toHaveBeenLastCalledWith({ @@ -126,18 +211,20 @@ describe('', () => { }); }); - it('adds owners to filters', () => { + it('adds owners to filters', async () => { const updateFilters = jest.fn(); - render( - - - , + await renderWithEffects( + + + + + , ); expect(updateFilters).toHaveBeenLastCalledWith({ owners: undefined, @@ -150,19 +237,21 @@ describe('', () => { }); }); - it('removes owners from filters', () => { + it('removes owners from filters', async () => { const updateFilters = jest.fn(); - render( - - - , + await renderWithEffects( + + + + + , ); expect(updateFilters).toHaveBeenLastCalledWith({ owners: new EntityOwnerFilter(['some-owner']), @@ -176,49 +265,55 @@ describe('', () => { }); }); - it('responds to external queryParameters changes', () => { + it('responds to external queryParameters changes', async () => { const updateFilters = jest.fn(); - const rendered = render( - - - , + const rendered = await renderWithEffects( + + + + + , ); expect(updateFilters).toHaveBeenLastCalledWith({ owners: new EntityOwnerFilter(['team-a']), }); rendered.rerender( - - - , + + + + + , ); expect(updateFilters).toHaveBeenLastCalledWith({ owners: new EntityOwnerFilter(['team-b']), }); }); - it('removes owners from filters if there are none available', () => { + it('removes owners from filters if there are none available', async () => { const updateFilters = jest.fn(); - render( - - - , + await renderWithEffects( + + + + + , ); expect(updateFilters).toHaveBeenLastCalledWith({ owners: undefined, diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index 72ced126e5..ac4304ce60 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -40,6 +40,7 @@ import { humanizeEntityRef } from '../EntityRefLink'; import useAsync from 'react-use/lib/useAsync'; import { useApi } from '@backstage/core-plugin-api'; import { catalogApiRef } from '../../api'; +import { humanizeEntity } from '../EntityRefLink/humanize'; /** @public */ export type CatalogReactEntityOwnerPickerClassKey = 'input'; @@ -109,9 +110,16 @@ export const EntityOwnerPicker = () => { 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 } } as Entity), + (e, i) => + items.at(i) || ({ metadata: { name: e }, kind: 'Group' } as Entity), ); }, [availableOwners]); @@ -136,15 +144,19 @@ export const EntityOwnerPicker = () => { disableCloseOnSelect loading={loading} options={owners || []} - getOptionLabel={option => - option.metadata.title || option.metadata.name + value={ + owners?.filter(e => + selectedOwners.some( + f => f === humanizeEntityRef(e, { defaultKind: 'Group' }), + ), + ) ?? [] } - value={owners?.filter(e => - selectedOwners.some(f => f === e.metadata.name), - )} onChange={(_: object, value: Entity[]) => setSelectedOwners(value.map(e => e.metadata.name)) } + getOptionLabel={option => + humanizeEntity(option, { defaultKind: 'Group' }) + } renderOption={(option, { selected }) => ( { /> } onClick={event => event.preventDefault()} - label={option.metadata.title || option.metadata.name} + label={humanizeEntity(option, { defaultKind: 'Group' })} /> )} size="small" diff --git a/plugins/catalog-react/src/components/EntityRefLink/humanize.test.ts b/plugins/catalog-react/src/components/EntityRefLink/humanize.test.ts index 80ce2100f6..e7d1649a29 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/humanize.test.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/humanize.test.ts @@ -14,7 +14,8 @@ * limitations under the License. */ -import { humanizeEntityRef } from './humanize'; +import { Entity } from '@backstage/catalog-model'; +import { humanizeEntity, humanizeEntityRef } from './humanize'; describe('humanizeEntityRef', () => { it('formats entity in default namespace', () => { @@ -210,3 +211,59 @@ describe('humanizeEntityRef', () => { expect(title).toEqual('component:default/software'); }); }); + +describe('humanizeEntity', () => { + it('gives a readable name when one is provided at metadata.title', () => { + expect( + humanizeEntity({ + metadata: { name: 'my-entity', title: 'My Title' }, + } as Entity), + ).toBe('My Title'); + }); + + it.each([ + [ + 'User', + { + apiVersion: '1', + kind: 'User', + metadata: { + name: 'user-name', + }, + spec: { + profile: { + displayName: 'User Name', + }, + }, + }, + 'User Name', + ], + [ + 'Group', + { + apiVersion: '1', + kind: 'User', + metadata: { + name: 'team-name', + }, + spec: { + profile: { + displayName: 'Team Name', + }, + }, + }, + 'Team Name', + ], + ])( + 'gives a readable name for kind %s when one is provided at spec.profile.displayName', + (_, entity: Entity, expected) => { + expect(humanizeEntity(entity)).toBe(expected); + }, + ); + + it('should pass through to humanizeEntityRef when nothing matches', () => { + expect( + humanizeEntity({ kind: 'Group', metadata: { name: 'test' } } as Entity), + ).toBe('group:test'); + }); +}); diff --git a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts index bfc8e2af84..3781935ad5 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts @@ -18,6 +18,7 @@ import { Entity, CompoundEntityRef, DEFAULT_NAMESPACE, + UserEntity, } from '@backstage/catalog-model'; /** @@ -65,3 +66,28 @@ export function humanizeEntityRef( : kind; return `${kind ? `${kind}:` : ''}${namespace ? `${namespace}/` : ''}${name}`; } + +/** + * Convert an entity to its more common name. + * + * @param entity Entity to convert. + * @returns Readable name, defaults to unique identifier. + */ +export function humanizeEntity( + entity: Entity, + opts?: { + defaultKind?: string; + 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; + } + return title || humanizeEntityRef(entity, opts); +} diff --git a/plugins/catalog-react/src/components/EntityRefLink/index.ts b/plugins/catalog-react/src/components/EntityRefLink/index.ts index 50ac3d4534..4d71e8b10e 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/index.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/index.ts @@ -18,4 +18,4 @@ export { EntityRefLink } from './EntityRefLink'; export type { EntityRefLinkProps } from './EntityRefLink'; export { EntityRefLinks } from './EntityRefLinks'; export type { EntityRefLinksProps } from './EntityRefLinks'; -export { humanizeEntityRef } from './humanize'; +export { humanizeEntityRef, humanizeEntity } from './humanize'; From a49fb39df5ac4440de719617a263d0d873eb2bb7 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Mon, 13 Feb 2023 11:17:07 -0500 Subject: [PATCH 03/20] Add changeset Signed-off-by: Aramis Sennyey --- .changeset/plenty-eels-listen.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/plenty-eels-listen.md diff --git a/.changeset/plenty-eels-listen.md b/.changeset/plenty-eels-listen.md new file mode 100644 index 0000000000..6a14d8be2a --- /dev/null +++ b/.changeset/plenty-eels-listen.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-react': minor +--- + +Attempt to load entity owner names in the EntityOwnerPicker through the `by-refs` endpoint. If `spec.profile.displayName` or `metadata.title` are populated, we now attempt to show those before showing the `humanizeEntityRef`'d version. From 8b7a55b55d9062c600fb8218f84dfec746f00d56 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Mon, 13 Feb 2023 11:20:10 -0500 Subject: [PATCH 04/20] Add comments Signed-off-by: Aramis Sennyey --- .../src/components/EntityRefLink/humanize.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts index 3781935ad5..815dac0758 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts @@ -68,9 +68,15 @@ export function humanizeEntityRef( } /** - * Convert an entity to its more common name. + * Convert an entity to its more readable name if available. + * + * If an entity is either User or Group, this will be its `spec.profile.displayName`. + * Otherwise, this is `metadata.title`. + * + * If neither of those are found or populated, fallback to `humanizeEntityRef`. * * @param entity Entity to convert. + * @param opts If entity readable name is not available, opts will be used to specify humanizeEntityRef options. * @returns Readable name, defaults to unique identifier. */ export function humanizeEntity( From 6bd5cade395efce94f06d00047bcee88e77b4140 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Mon, 13 Feb 2023 13:53:02 -0500 Subject: [PATCH 05/20] Add catalog API report and fix test cases. Signed-off-by: Aramis Sennyey --- plugins/catalog-react/api-report.md | 9 +++++++++ .../src/components/EntityRefLink/humanize.ts | 6 ++++-- .../components/CatalogPage/DefaultCatalogPage.test.tsx | 7 +++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/plugins/catalog-react/api-report.md b/plugins/catalog-react/api-report.md index 77f439018f..9b318ec116 100644 --- a/plugins/catalog-react/api-report.md +++ b/plugins/catalog-react/api-report.md @@ -453,6 +453,15 @@ export function getEntitySourceLocation( scmIntegrationsApi: ScmIntegrationRegistry, ): EntitySourceLocation | undefined; +// @public +export function humanizeEntity( + entity: Entity, + opts?: { + defaultKind?: string; + defaultNamespace?: string | false; + }, +): string; + // @public (undocumented) export function humanizeEntityRef( entityRef: Entity | CompoundEntityRef, diff --git a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts index 815dac0758..1e377e1859 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts @@ -75,9 +75,11 @@ export function humanizeEntityRef( * * If neither of those are found or populated, fallback to `humanizeEntityRef`. * - * @param entity Entity to convert. - * @param opts If entity readable name is not available, opts will be used to specify humanizeEntityRef options. + * @param entity - Entity to convert. + * @param opts - If entity readable name is not available, opts will be used to specify humanizeEntityRef options. * @returns Readable name, defaults to unique identifier. + * + * @public */ export function humanizeEntity( entity: Entity, diff --git a/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx b/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx index 7b92df5809..3411656426 100644 --- a/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx +++ b/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx @@ -119,6 +119,13 @@ describe('DefaultCatalogPage', () => { ], }; }, + /** + * For the purposes of this test case, use existing functionality. The picker + * isn't being tested, just needs this method to render correctly. + */ + getEntitiesByRefs: async refs => { + return { items: refs.entityRefs.map(() => undefined) }; + }, }; const testProfile: Partial = { displayName: 'Display Name', From 487d699993a9c86d9d61a240d11345f24b3450db Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 21 Feb 2023 12:26:42 -0500 Subject: [PATCH 06/20] Update variable name. Signed-off-by: Aramis Sennyey --- .../src/components/EntityOwnerPicker/EntityOwnerPicker.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index ac4304ce60..c3ac658238 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -104,7 +104,7 @@ export const EntityOwnerPicker = () => { const { loading, error, - value: owners, + value: ownerEntities, } = useAsync(async () => { const { items } = await catalogApi.getEntitiesByRefs({ entityRefs: availableOwners.map(ref => @@ -143,9 +143,9 @@ export const EntityOwnerPicker = () => { multiple disableCloseOnSelect loading={loading} - options={owners || []} + options={ownerEntities || []} value={ - owners?.filter(e => + ownerEntities?.filter(e => selectedOwners.some( f => f === humanizeEntityRef(e, { defaultKind: 'Group' }), ), From 3555c2741eb4bd0c6a90bd6323a20c09588f470d Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Wed, 22 Feb 2023 11:52:57 -0500 Subject: [PATCH 07/20] Fix test cases and update async logic. Signed-off-by: Aramis Sennyey --- .../EntityOwnerPicker.test.tsx | 76 ++++------ .../EntityOwnerPicker/EntityOwnerPicker.tsx | 130 +++++++++++------- .../src/components/EntityRefLink/humanize.ts | 18 ++- 3 files changed, 109 insertions(+), 115 deletions(-) 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); } From 436b4f50bb1e697391dd071eff78729605f77c1a Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Wed, 22 Feb 2023 12:00:09 -0500 Subject: [PATCH 08/20] Add namespace check as well. Signed-off-by: Aramis Sennyey --- .../EntityOwnerPicker.test.tsx | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx index 00b820f5b2..f33fde19b0 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx @@ -57,6 +57,15 @@ const ownerEntities: Entity[] = [ title: 'Another Owner', }, }, + { + apiVersion: '1', + kind: 'Group', + metadata: { + namespace: 'test-namespace', + name: 'another-owner-2', + title: 'Another Owner in Another Namespace', + }, + }, ]; const sampleEntities: Entity[] = [ @@ -88,6 +97,10 @@ const sampleEntities: Entity[] = [ type: 'ownedBy', targetRef: 'group:default/another-owner', }, + { + type: 'ownedBy', + targetRef: 'group:test-namespace/another-owner-2', + }, ], }, { @@ -134,7 +147,12 @@ describe('', () => { expect(screen.getByText('Owner')).toBeInTheDocument(); fireEvent.click(screen.getByTestId('owner-picker-expand')); - ['Another Owner', 'some-owner', 'Some Owner 2'].forEach(owner => { + [ + 'Another Owner', + 'some-owner', + 'Some Owner 2', + 'Another Owner in Another Namespace', + ].forEach(owner => { expect(screen.getByText(owner)).toBeInTheDocument(); }); }); @@ -155,6 +173,7 @@ describe('', () => { expect(screen.getAllByRole('option').map(o => o.textContent)).toEqual([ 'Another Owner', + 'Another Owner in Another Namespace', 'some-owner', 'Some Owner 2', ]); From 46c7bf0a441d7cfeea5bb05e3c5c041504e2f56c Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Wed, 22 Feb 2023 13:47:40 -0500 Subject: [PATCH 09/20] Return null when no entities to render. Signed-off-by: Aramis Sennyey --- .../EntityOwnerPicker/EntityOwnerPicker.tsx | 46 ++++++++----------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index f3d656b5ee..9cf84fff08 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -72,6 +72,10 @@ export const EntityOwnerPicker = () => { [ownersParameter], ); + const [selectedOwners, setSelectedOwners] = useState( + queryParamOwners.length ? queryParamOwners : filters.owners?.values ?? [], + ); + const { loading, error, @@ -98,27 +102,17 @@ export const EntityOwnerPicker = () => { '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; - }) - ); + return availableOwners + .map( + (e, i) => + items[i] || ({ metadata: { name: e }, kind: 'Group' } as Entity), + ) + .sort((a, b) => + new Intl.Collator().compare( + humanizeEntity(a).toLocaleUpperCase('en-US'), + humanizeEntity(b).toLocaleUpperCase('en-US'), + ), + ); }, [backendEntities]); useEffect(() => { @@ -133,10 +127,6 @@ export const EntityOwnerPicker = () => { } }, [error, errorApi]); - const [selectedOwners, setSelectedOwners] = useState( - queryParamOwners.length ? queryParamOwners : filters.owners?.values ?? [], - ); - // Set selected owners on query parameter updates; this happens at initial page load and from // external updates to the page location. useEffect(() => { @@ -156,6 +146,8 @@ export const EntityOwnerPicker = () => { } }, [selectedOwners, updateFilters, ownerEntities, loading]); + if (!loading && !ownerEntities?.length) return null; + return ( @@ -168,7 +160,8 @@ export const EntityOwnerPicker = () => { value={ ownerEntities?.filter(e => selectedOwners.some( - f => f === humanizeEntityRef(e, { defaultKind: 'Group' }), + (f: string) => + f === humanizeEntityRef(e, { defaultKind: 'Group' }), ), ) ?? [] } @@ -198,7 +191,6 @@ export const EntityOwnerPicker = () => { renderInput={params => ( From c2fbe4b145bd45311c3a8ca01e8106a0734a883e Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Wed, 22 Feb 2023 17:13:58 -0500 Subject: [PATCH 10/20] Fix test cases. Signed-off-by: Aramis Sennyey --- .../components/ApiExplorerPage/DefaultApiExplorerPage.test.tsx | 1 + .../techdocs/src/home/components/DefaultTechDocsHome.test.tsx | 1 + 2 files changed, 2 insertions(+) diff --git a/plugins/api-docs/src/components/ApiExplorerPage/DefaultApiExplorerPage.test.tsx b/plugins/api-docs/src/components/ApiExplorerPage/DefaultApiExplorerPage.test.tsx index bc413661a8..f89897524d 100644 --- a/plugins/api-docs/src/components/ApiExplorerPage/DefaultApiExplorerPage.test.tsx +++ b/plugins/api-docs/src/components/ApiExplorerPage/DefaultApiExplorerPage.test.tsx @@ -64,6 +64,7 @@ describe('DefaultApiExplorerPage', () => { }), getLocationByRef: () => Promise.resolve({ id: 'id', type: 'url', target: 'url' }), + getEntitiesByRefs: () => Promise.resolve({ items: [] }), getEntityByRef: async entityRef => { return { apiVersion: 'backstage.io/v1alpha1', diff --git a/plugins/techdocs/src/home/components/DefaultTechDocsHome.test.tsx b/plugins/techdocs/src/home/components/DefaultTechDocsHome.test.tsx index 48c1fa248c..9ac2ac4ef4 100644 --- a/plugins/techdocs/src/home/components/DefaultTechDocsHome.test.tsx +++ b/plugins/techdocs/src/home/components/DefaultTechDocsHome.test.tsx @@ -38,6 +38,7 @@ import { DefaultTechDocsHome } from './DefaultTechDocsHome'; const mockCatalogApi = { getEntityByRef: () => Promise.resolve(), + getEntitiesByRefs: () => Promise.resolve({ items: [] }), getEntities: async () => ({ items: [ { From 77d3d93c7e381b4824e15414b685dd46ddbed21e Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Mon, 6 Mar 2023 11:13:29 -0500 Subject: [PATCH 11/20] Fix sorting logic and how we handle entities vs entityRefs. Signed-off-by: Aramis Sennyey --- .../EntityOwnerPicker/EntityOwnerPicker.tsx | 54 ++++++++++--------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index 9cf84fff08..d4a21a4dd5 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -16,6 +16,7 @@ import { Entity, + parseEntityRef, RELATION_OWNED_BY, stringifyEntityRef, } from '@backstage/catalog-model'; @@ -81,7 +82,7 @@ export const EntityOwnerPicker = () => { error, value: ownerEntities, } = useAsync(async () => { - const availableOwners = [ + const ownerEntityRefs = [ ...new Set( backendEntities .flatMap((e: Entity) => @@ -93,7 +94,7 @@ export const EntityOwnerPicker = () => { ), ]; const { items } = await catalogApi.getEntitiesByRefs({ - entityRefs: availableOwners, + entityRefs: ownerEntityRefs, fields: [ 'kind', 'metadata.name', @@ -102,17 +103,29 @@ export const EntityOwnerPicker = () => { 'spec.profile.displayName', ], }); - return availableOwners - .map( - (e, i) => - items[i] || ({ metadata: { name: e }, kind: 'Group' } as Entity), - ) - .sort((a, b) => - new Intl.Collator().compare( - humanizeEntity(a).toLocaleUpperCase('en-US'), - humanizeEntity(b).toLocaleUpperCase('en-US'), + const owners = ownerEntityRefs.map((ref, index) => { + const entity = items[index]; + if (entity) { + return { + label: humanizeEntity(entity, { defaultKind: 'Group' }), + entityRef: humanizeEntityRef(entity, { defaultKind: 'Group' }), + }; + } + return { + label: humanizeEntityRef( + parseEntityRef(ref, { defaultKind: 'Group' }), + { defaultKind: 'group' }, ), - ); + entityRef: ref, + }; + }); + + return owners.sort((a, b) => + a.label.localeCompare(b.label, 'en-US', { + ignorePunctuation: true, + caseFirst: 'upper', + }), + ); }, [backendEntities]); useEffect(() => { @@ -159,20 +172,13 @@ export const EntityOwnerPicker = () => { options={ownerEntities || []} value={ ownerEntities?.filter(e => - selectedOwners.some( - (f: string) => - f === humanizeEntityRef(e, { defaultKind: 'Group' }), - ), + selectedOwners.some((f: string) => f === e.entityRef), ) ?? [] } - onChange={(_: object, value: Entity[]) => - setSelectedOwners( - value.map(e => humanizeEntityRef(e, { defaultKind: 'Group' })), - ) - } - getOptionLabel={option => - humanizeEntity(option, { defaultKind: 'Group' }) + onChange={(_: object, value: { entityRef: string }[]) => + setSelectedOwners(value.map(e => e.entityRef)) } + getOptionLabel={option => option.label} renderOption={(option, { selected }) => ( { /> } onClick={event => event.preventDefault()} - label={humanizeEntity(option, { defaultKind: 'Group' })} + label={option.label} /> )} size="small" From b39b897a7bf2d8260fe6bdee8079a268a8cec1c1 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Mon, 6 Mar 2023 11:25:44 -0500 Subject: [PATCH 12/20] Fix variable names. Signed-off-by: Aramis Sennyey --- .../components/EntityOwnerPicker/EntityOwnerPicker.tsx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index d4a21a4dd5..d755f2a84f 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -93,7 +93,7 @@ export const EntityOwnerPicker = () => { .filter(Boolean) as string[], ), ]; - const { items } = await catalogApi.getEntitiesByRefs({ + const { items: ownerEntitiesOrNull } = await catalogApi.getEntitiesByRefs({ entityRefs: ownerEntityRefs, fields: [ 'kind', @@ -103,8 +103,7 @@ export const EntityOwnerPicker = () => { 'spec.profile.displayName', ], }); - const owners = ownerEntityRefs.map((ref, index) => { - const entity = items[index]; + const owners = ownerEntitiesOrNull.map((entity, index) => { if (entity) { return { label: humanizeEntity(entity, { defaultKind: 'Group' }), @@ -113,10 +112,10 @@ export const EntityOwnerPicker = () => { } return { label: humanizeEntityRef( - parseEntityRef(ref, { defaultKind: 'Group' }), + parseEntityRef(ownerEntityRefs[index], { defaultKind: 'Group' }), { defaultKind: 'group' }, ), - entityRef: ref, + entityRef: ownerEntityRefs[index], }; }); From 0b42e304f86e4d280cc2816d89a1f3b0ad52c77d Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 28 Mar 2023 16:35:55 -0400 Subject: [PATCH 13/20] Force entity references to be full in the owner picker. Signed-off-by: Aramis Sennyey --- .../EntityOwnerPicker.test.tsx | 11 ++-- .../EntityOwnerPicker/EntityOwnerPicker.tsx | 12 ++--- plugins/catalog-react/src/filters.test.ts | 50 ++++++++++++++++++- plugins/catalog-react/src/filters.ts | 31 ++++++++++-- 4 files changed, 88 insertions(+), 16 deletions(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx index f33fde19b0..e4dc322c7c 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx @@ -198,7 +198,7 @@ describe('', () => { ); expect(updateFilters).toHaveBeenLastCalledWith({ - owners: new EntityOwnerFilter(['another-owner']), + owners: new EntityOwnerFilter(['group:default/another-owner']), }); }); @@ -224,7 +224,7 @@ describe('', () => { fireEvent.click(screen.getByTestId('owner-picker-expand')); fireEvent.click(screen.getByText('some-owner')); expect(updateFilters).toHaveBeenLastCalledWith({ - owners: new EntityOwnerFilter(['some-owner']), + owners: new EntityOwnerFilter(['group:default/some-owner']), }); }); @@ -245,9 +245,10 @@ describe('', () => { , ); expect(updateFilters).toHaveBeenLastCalledWith({ - owners: new EntityOwnerFilter(['some-owner']), + owners: new EntityOwnerFilter(['group:default/some-owner']), }); fireEvent.click(screen.getByTestId('owner-picker-expand')); + expect(screen.getByLabelText('some-owner')).toBeChecked(); fireEvent.click(screen.getByLabelText('some-owner')); @@ -272,7 +273,7 @@ describe('', () => { , ); expect(updateFilters).toHaveBeenLastCalledWith({ - owners: new EntityOwnerFilter(['team-a']), + owners: new EntityOwnerFilter(['group:default/team-a']), }); rendered.rerender( @@ -288,7 +289,7 @@ describe('', () => { , ); expect(updateFilters).toHaveBeenLastCalledWith({ - owners: new EntityOwnerFilter(['team-b']), + owners: new EntityOwnerFilter(['group:default/team-b']), }); }); it('removes owners from filters if there are none available', async () => { diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index d755f2a84f..c1ed4fba18 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -107,14 +107,13 @@ export const EntityOwnerPicker = () => { if (entity) { return { label: humanizeEntity(entity, { defaultKind: 'Group' }), - entityRef: humanizeEntityRef(entity, { defaultKind: 'Group' }), + entityRef: stringifyEntityRef(entity), }; } return { - label: humanizeEntityRef( - parseEntityRef(ownerEntityRefs[index], { defaultKind: 'Group' }), - { defaultKind: 'group' }, - ), + label: humanizeEntityRef(parseEntityRef(ownerEntityRefs[index]), { + defaultKind: 'group', + }), entityRef: ownerEntityRefs[index], }; }); @@ -143,7 +142,8 @@ export const EntityOwnerPicker = () => { // external updates to the page location. useEffect(() => { if (queryParamOwners.length) { - setSelectedOwners(queryParamOwners); + const filter = new EntityOwnerFilter(queryParamOwners); + setSelectedOwners(filter.values); } }, [queryParamOwners]); diff --git a/plugins/catalog-react/src/filters.test.ts b/plugins/catalog-react/src/filters.test.ts index 2a67190633..ac9e7c0e8a 100644 --- a/plugins/catalog-react/src/filters.test.ts +++ b/plugins/catalog-react/src/filters.test.ts @@ -15,11 +15,12 @@ */ import { AlphaEntity } from '@backstage/catalog-model/alpha'; -import { Entity } from '@backstage/catalog-model'; +import { Entity, RELATION_OWNED_BY } from '@backstage/catalog-model'; import { TemplateEntityV1beta3 } from '@backstage/plugin-scaffolder-common'; import { EntityErrorFilter, EntityOrphanFilter, + EntityOwnerFilter, EntityTextFilter, } from './filters'; @@ -143,3 +144,50 @@ describe('EntityErrorFilter', () => { expect(filter.filterEntity(entities[1])).toBeFalsy(); }); }); + +describe('EntityOwnerFilter', () => { + it('should handle humanizedEntityRefs', () => { + const filter = new EntityOwnerFilter(['my-user']); + expect( + filter.filterEntity({ + relations: [ + { + type: RELATION_OWNED_BY, + targetRef: 'group:default/my-user', + }, + ], + } as Entity), + ).toBeTruthy(); + expect(filter.values).toStrictEqual(['group:default/my-user']); + }); + + it('should also handle full entityRefs', () => { + const filter = new EntityOwnerFilter(['group:default/my-user']); + expect( + filter.filterEntity({ + relations: [ + { + type: RELATION_OWNED_BY, + targetRef: 'group:default/my-user', + }, + ], + } as Entity), + ).toBeTruthy(); + expect(filter.values).toStrictEqual(['group:default/my-user']); + }); + + it('should also gracefully reject non-entity refs', () => { + const filter = new EntityOwnerFilter(['group:default/my-user', '']); + expect( + filter.filterEntity({ + relations: [ + { + type: RELATION_OWNED_BY, + targetRef: 'group:default/my-user', + }, + ], + } as Entity), + ).toBeTruthy(); + expect(filter.values).toStrictEqual(['group:default/my-user']); + }); +}); diff --git a/plugins/catalog-react/src/filters.ts b/plugins/catalog-react/src/filters.ts index 5bd81780d6..ad9e917699 100644 --- a/plugins/catalog-react/src/filters.ts +++ b/plugins/catalog-react/src/filters.ts @@ -14,9 +14,13 @@ * limitations under the License. */ -import { Entity, RELATION_OWNED_BY } from '@backstage/catalog-model'; +import { + Entity, + parseEntityRef, + RELATION_OWNED_BY, + stringifyEntityRef, +} from '@backstage/catalog-model'; import { AlphaEntity } from '@backstage/catalog-model/alpha'; -import { humanizeEntityRef } from './components/EntityRefLink'; import { EntityFilter, UserListFilterKind } from './types'; import { getEntityRelations } from './utils'; @@ -113,18 +117,37 @@ export class EntityTextFilter implements EntityFilter { /** * Filter matching entities that are owned by group. * @public + * + * CAUTION: This class may contain both full and partial entity refs. */ export class EntityOwnerFilter implements EntityFilter { - constructor(readonly values: string[]) {} + readonly values: string[]; + constructor(values: string[]) { + this.values = values.reduce((fullRefs, ref) => { + // Attempt to remove bad entity references here. + try { + fullRefs.push( + stringifyEntityRef(parseEntityRef(ref, { defaultKind: 'Group' })), + ); + return fullRefs; + } catch (err) { + return fullRefs; + } + }, [] as string[]); + } filterEntity(entity: Entity): boolean { return this.values.some(v => getEntityRelations(entity, RELATION_OWNED_BY).some( - o => humanizeEntityRef(o, { defaultKind: 'group' }) === v, + o => stringifyEntityRef(o) === v, ), ); } + /** + * Get the URL query parameter value. May be a mix of full and humanized entity refs. + * @returns list of entity refs. + */ toQueryValue(): string[] { return this.values; } From 820365a34a3f14c0c9dd959d502f4d46fcd7e252 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 28 Mar 2023 16:37:52 -0400 Subject: [PATCH 14/20] Add test case for non-group refs. Signed-off-by: Aramis Sennyey --- plugins/catalog-react/src/filters.test.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/plugins/catalog-react/src/filters.test.ts b/plugins/catalog-react/src/filters.test.ts index ac9e7c0e8a..8ddac7e53b 100644 --- a/plugins/catalog-react/src/filters.test.ts +++ b/plugins/catalog-react/src/filters.test.ts @@ -161,7 +161,7 @@ describe('EntityOwnerFilter', () => { expect(filter.values).toStrictEqual(['group:default/my-user']); }); - it('should also handle full entityRefs', () => { + it('should handle full entityRefs', () => { const filter = new EntityOwnerFilter(['group:default/my-user']); expect( filter.filterEntity({ @@ -176,7 +176,7 @@ describe('EntityOwnerFilter', () => { expect(filter.values).toStrictEqual(['group:default/my-user']); }); - it('should also gracefully reject non-entity refs', () => { + it('should gracefully reject non-entity refs', () => { const filter = new EntityOwnerFilter(['group:default/my-user', '']); expect( filter.filterEntity({ @@ -190,4 +190,19 @@ describe('EntityOwnerFilter', () => { ).toBeTruthy(); expect(filter.values).toStrictEqual(['group:default/my-user']); }); + + it('should handle non group full entity refs', () => { + const filter = new EntityOwnerFilter(['user:default/my-user', '']); + expect( + filter.filterEntity({ + relations: [ + { + type: RELATION_OWNED_BY, + targetRef: 'user:default/my-user', + }, + ], + } as Entity), + ).toBeTruthy(); + expect(filter.values).toStrictEqual(['user:default/my-user']); + }); }); From bba158e7ed743cdf149268b407b0bb6e23a1c42c Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 28 Mar 2023 16:39:58 -0400 Subject: [PATCH 15/20] Adjust query params on first render. Signed-off-by: Aramis Sennyey --- .../src/components/EntityOwnerPicker/EntityOwnerPicker.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index c1ed4fba18..8c4f2bfea0 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -74,7 +74,9 @@ export const EntityOwnerPicker = () => { ); const [selectedOwners, setSelectedOwners] = useState( - queryParamOwners.length ? queryParamOwners : filters.owners?.values ?? [], + queryParamOwners.length + ? new EntityOwnerFilter(queryParamOwners).values + : filters.owners?.values ?? [], ); const { From 483f0cc57fdf6f8591247a4c70c1a19fdc8951a8 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 28 Mar 2023 16:51:29 -0400 Subject: [PATCH 16/20] Update changeset. Signed-off-by: Aramis Sennyey --- .changeset/plenty-eels-listen.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/.changeset/plenty-eels-listen.md b/.changeset/plenty-eels-listen.md index 6a14d8be2a..ebffd1084b 100644 --- a/.changeset/plenty-eels-listen.md +++ b/.changeset/plenty-eels-listen.md @@ -3,3 +3,23 @@ --- Attempt to load entity owner names in the EntityOwnerPicker through the `by-refs` endpoint. If `spec.profile.displayName` or `metadata.title` are populated, we now attempt to show those before showing the `humanizeEntityRef`'d version. + +**BREAKING**: This updates the `EntityOwnerFilter` to use the full entity ref instead of the `humanizeEntityRef`. If you rely on `EntityOwnerFilter.values` or the `owners` query parameter of the catalog page, you will need to adjust your code to use + +```tsx +const {queryParameters: {owners: oldEntityOwnerFilterRef}} = useEntityList(); +// or +const {filter: {owners}} = useEntityList(); + +// Instead of, +... +if(owners.some(ref=>ref === humanizeEntityRef(myEntity))){ + ... +} + +// You'll need to use, +... +if(owners.some(ref=>ref === stringifyEntityRef(myEntity))){ + ... +} +``` From dbecde0168ed15c17f86b88d614949a61329d764 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 28 Mar 2023 16:52:09 -0400 Subject: [PATCH 17/20] Update message. Signed-off-by: Aramis Sennyey --- .changeset/plenty-eels-listen.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/plenty-eels-listen.md b/.changeset/plenty-eels-listen.md index ebffd1084b..6e6bc5d664 100644 --- a/.changeset/plenty-eels-listen.md +++ b/.changeset/plenty-eels-listen.md @@ -4,7 +4,7 @@ Attempt to load entity owner names in the EntityOwnerPicker through the `by-refs` endpoint. If `spec.profile.displayName` or `metadata.title` are populated, we now attempt to show those before showing the `humanizeEntityRef`'d version. -**BREAKING**: This updates the `EntityOwnerFilter` to use the full entity ref instead of the `humanizeEntityRef`. If you rely on `EntityOwnerFilter.values` or the `owners` query parameter of the catalog page, you will need to adjust your code to use +**BREAKING**: `EntityOwnerFilter` now uses the full entity ref instead of the `humanizeEntityRef`. If you rely on `EntityOwnerFilter.values` or the `queryParameters.owners` of `useEntityList`, you will need to adjust your code like the following. ```tsx const {queryParameters: {owners: oldEntityOwnerFilterRef}} = useEntityList(); From d3b8d08bb435d0720d154ef4d490ff891faaf661 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Wed, 29 Mar 2023 17:38:22 -0400 Subject: [PATCH 18/20] Update API report. Signed-off-by: Aramis Sennyey --- plugins/catalog-react/api-report.md | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/catalog-react/api-report.md b/plugins/catalog-react/api-report.md index 9b318ec116..72b654527b 100644 --- a/plugins/catalog-react/api-report.md +++ b/plugins/catalog-react/api-report.md @@ -242,7 +242,6 @@ export class EntityOwnerFilter implements EntityFilter { constructor(values: string[]); // (undocumented) filterEntity(entity: Entity): boolean; - // (undocumented) toQueryValue(): string[]; // (undocumented) readonly values: string[]; From 44f723b8d2cc55110f82079cc94d7b71e9aa0023 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Thu, 30 Mar 2023 11:01:09 -0400 Subject: [PATCH 19/20] Update changeset. Signed-off-by: Aramis Sennyey --- .changeset/plenty-eels-listen.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/plenty-eels-listen.md b/.changeset/plenty-eels-listen.md index 6e6bc5d664..fd2213b7b4 100644 --- a/.changeset/plenty-eels-listen.md +++ b/.changeset/plenty-eels-listen.md @@ -7,7 +7,7 @@ Attempt to load entity owner names in the EntityOwnerPicker through the `by-refs **BREAKING**: `EntityOwnerFilter` now uses the full entity ref instead of the `humanizeEntityRef`. If you rely on `EntityOwnerFilter.values` or the `queryParameters.owners` of `useEntityList`, you will need to adjust your code like the following. ```tsx -const {queryParameters: {owners: oldEntityOwnerFilterRef}} = useEntityList(); +const {queryParameters: {owners}} = useEntityList(); // or const {filter: {owners}} = useEntityList(); From cd874bfaf64adaf2ff31153700f41a4d785190d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 6 Apr 2023 19:03:46 +0200 Subject: [PATCH 20/20] hide the export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/plenty-eels-listen.md | 14 ++++---------- plugins/catalog-react/api-report.md | 9 --------- .../src/components/EntityRefLink/index.ts | 2 +- 3 files changed, 5 insertions(+), 20 deletions(-) diff --git a/.changeset/plenty-eels-listen.md b/.changeset/plenty-eels-listen.md index fd2213b7b4..6b4776f808 100644 --- a/.changeset/plenty-eels-listen.md +++ b/.changeset/plenty-eels-listen.md @@ -7,19 +7,13 @@ Attempt to load entity owner names in the EntityOwnerPicker through the `by-refs **BREAKING**: `EntityOwnerFilter` now uses the full entity ref instead of the `humanizeEntityRef`. If you rely on `EntityOwnerFilter.values` or the `queryParameters.owners` of `useEntityList`, you will need to adjust your code like the following. ```tsx -const {queryParameters: {owners}} = useEntityList(); +const { queryParameters: { owners } } = useEntityList(); // or -const {filter: {owners}} = useEntityList(); +const { filter: { owners } } = useEntityList(); // Instead of, -... -if(owners.some(ref=>ref === humanizeEntityRef(myEntity))){ - ... -} +if (owners.some(ref => ref === humanizeEntityRef(myEntity))) ... // You'll need to use, -... -if(owners.some(ref=>ref === stringifyEntityRef(myEntity))){ - ... -} +if (owners.some(ref => ref === stringifyEntityRef(myEntity))) ... ``` diff --git a/plugins/catalog-react/api-report.md b/plugins/catalog-react/api-report.md index 72b654527b..20b10f18cc 100644 --- a/plugins/catalog-react/api-report.md +++ b/plugins/catalog-react/api-report.md @@ -452,15 +452,6 @@ export function getEntitySourceLocation( scmIntegrationsApi: ScmIntegrationRegistry, ): EntitySourceLocation | undefined; -// @public -export function humanizeEntity( - entity: Entity, - opts?: { - defaultKind?: string; - defaultNamespace?: string | false; - }, -): string; - // @public (undocumented) export function humanizeEntityRef( entityRef: Entity | CompoundEntityRef, diff --git a/plugins/catalog-react/src/components/EntityRefLink/index.ts b/plugins/catalog-react/src/components/EntityRefLink/index.ts index 4d71e8b10e..50ac3d4534 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/index.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/index.ts @@ -18,4 +18,4 @@ export { EntityRefLink } from './EntityRefLink'; export type { EntityRefLinkProps } from './EntityRefLink'; export { EntityRefLinks } from './EntityRefLinks'; export type { EntityRefLinksProps } from './EntityRefLinks'; -export { humanizeEntityRef, humanizeEntity } from './humanize'; +export { humanizeEntityRef } from './humanize';