From 0b42e304f86e4d280cc2816d89a1f3b0ad52c77d Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 28 Mar 2023 16:35:55 -0400 Subject: [PATCH] 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; }