From 6ffa47bb0ac7527730211bcb124b73f58f77965e Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Thu, 27 Oct 2022 19:34:58 +0200 Subject: [PATCH 1/7] Catalog kind filter fixes & clean-up Various fixes: * CatalogDefaultPage cannot handle Capitalized kinds from the kind query parameter. This breaks ownership cards and both CatalogKindHeader and EntityKindPicker turns up empty. There may be other places where the catalog expects to handle capitalized Kinds. After all, the default Kinds are capitalized in the entities. * If a non existing kind is specified, CatalogKindHeader works but not the EntityKindPicker. Well, at least as long as the non-existing kind is not capitalized(!) * The CatalogKindHeader supports allowed kinds, but not the EntityKindFilter. So depending on Catalog-setup they will list different kinds. Refactoring: * Refactored out helper functions to a util library. This ensured that similar logic can be shared between the Kind pickers (and in turn their behaviour is similar). Tests: * Relevant tests added Signed-off-by: Gustaf Lundh --- .changeset/brave-bags-sniff.md | 5 + .changeset/neat-lies-know.md | 5 + plugins/catalog-react/api-report.md | 15 +++ .../EntityKindPicker.test.tsx | 57 ++++++++++ .../EntityKindPicker/EntityKindPicker.tsx | 106 +++++------------- plugins/catalog-react/src/index.ts | 2 + plugins/catalog-react/src/utils/index.ts | 1 + .../src/utils/kindFilterUtils.ts | 82 ++++++++++++++ .../CatalogKindHeader/CatalogKindHeader.tsx | 41 ++----- 9 files changed, 207 insertions(+), 107 deletions(-) create mode 100644 .changeset/brave-bags-sniff.md create mode 100644 .changeset/neat-lies-know.md create mode 100644 plugins/catalog-react/src/utils/kindFilterUtils.ts diff --git a/.changeset/brave-bags-sniff.md b/.changeset/brave-bags-sniff.md new file mode 100644 index 0000000000..2608ca7f1e --- /dev/null +++ b/.changeset/brave-bags-sniff.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog': minor +--- + +Fixes in kind selectors (now OwnershipCards work again) diff --git a/.changeset/neat-lies-know.md b/.changeset/neat-lies-know.md new file mode 100644 index 0000000000..cb355c638f --- /dev/null +++ b/.changeset/neat-lies-know.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-react': minor +--- + +Cleanup and small fixes for the kind selector diff --git a/plugins/catalog-react/api-report.md b/plugins/catalog-react/api-report.md index e8f0b0e6ee..ea67702c9b 100644 --- a/plugins/catalog-react/api-report.md +++ b/plugins/catalog-react/api-report.md @@ -175,6 +175,7 @@ export const EntityKindPicker: ( // @public export interface EntityKindPickerProps { + allowedKinds?: string[]; // (undocumented) hidden?: boolean; // (undocumented) @@ -431,6 +432,13 @@ export type FavoriteEntityProps = ComponentProps & { entity: Entity; }; +// @public +export function filterAndCapitalize( + allKinds: string[], + allowedKinds?: string[], + forcedKinds?: string[], +): Record; + // @public export function getEntityRelations( entity: Entity | undefined, @@ -503,6 +511,13 @@ export type UnregisterEntityDialogProps = { entity: Entity; }; +// @public +export function useAllKinds(): { + loading: boolean; + error?: Error; + allKinds: string[]; +}; + // @public export function useAsyncEntity< TEntity extends Entity = Entity, diff --git a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx index 65ad3f227a..a62669caf9 100644 --- a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx @@ -147,6 +147,43 @@ describe('', () => { }); }); + it('renders unknown kinds provided in query parameters', async () => { + const rendered = await renderWithEffects( + + + + + , + ); + + expect(rendered.getByText('Frob')).toBeInTheDocument(); + }); + + it('limits kinds when allowedKinds is set', async () => { + const rendered = await renderWithEffects( + + + + + , + ); + + const input = rendered.getByTestId('select'); + fireEvent.click(input); + + expect( + rendered.getByRole('option', { name: 'Component' }), + ).toBeInTheDocument(); + expect( + rendered.getByRole('option', { name: 'Domain' }), + ).toBeInTheDocument(); + expect( + rendered.queryByRole('option', { name: 'Template' }), + ).not.toBeInTheDocument(); + }); + it('responds to external queryParameters changes', async () => { const updateFilters = jest.fn(); const rendered = await renderWithEffects( @@ -180,4 +217,24 @@ describe('', () => { kind: new EntityKindFilter('domain'), }); }); + + it('renders kind from the query parameter even when not in allowedKinds', async () => { + const rendered = await renderWithEffects( + + + + + , + ); + + expect(rendered.getByText('Frob')).toBeInTheDocument(); + + const input = rendered.getByTestId('select'); + fireEvent.click(input); + expect( + rendered.getByRole('option', { name: 'Domain' }), + ).toBeInTheDocument(); + }); }); diff --git a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx index c626a68d10..89313790f5 100644 --- a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx +++ b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx @@ -17,64 +17,15 @@ import { Select } from '@backstage/core-components'; import { alertApiRef, useApi } from '@backstage/core-plugin-api'; import { Box } from '@material-ui/core'; -import capitalize from 'lodash/capitalize'; -import sortBy from 'lodash/sortBy'; -import React, { useEffect, useMemo, useRef, useState } from 'react'; -import useAsync from 'react-use/lib/useAsync'; -import { catalogApiRef } from '../../api'; +import React, { useEffect, useMemo, useState } from 'react'; import { EntityKindFilter } from '../../filters'; import { useEntityList } from '../../hooks'; - -function useAvailableKinds() { - const catalogApi = useApi(catalogApiRef); - - const [availableKinds, setAvailableKinds] = useState([]); - - const { - error, - loading, - value: facets, - } = useAsync(async () => { - const facet = 'kind'; - const items = await catalogApi - .getEntityFacets({ - facets: [facet], - }) - .then(response => response.facets[facet] || []); - - return items; - }, [catalogApi]); - - const facetsRef = useRef(facets); - useEffect(() => { - const oldFacets = facetsRef.current; - facetsRef.current = facets; - // Delay processing hook until facets load updates have settled to generate list of kinds; - // This prevents resetting the kind filter due to saved kind value from query params not matching the - // empty set of kind values while values are still being loaded; also only run this hook on changes - // to facets - if (loading || oldFacets === facets || !facets) { - return; - } - - const newKinds = [ - ...new Set( - sortBy(facets, f => f.value).map(f => - f.value.toLocaleLowerCase('en-US'), - ), - ), - ]; - - setAvailableKinds(newKinds); - }, [loading, facets, setAvailableKinds]); - - return { loading, error, availableKinds }; -} +import { filterAndCapitalize, useAllKinds } from '../../utils/kindFilterUtils'; function useEntityKindFilter(opts: { initialFilter: string }): { loading: boolean; error?: Error; - availableKinds: string[]; + allKinds: string[]; selectedKind: string; setSelectedKind: (kind: string) => void; } { @@ -84,23 +35,15 @@ function useEntityKindFilter(opts: { initialFilter: string }): { updateFilters, } = useEntityList(); - const flattenedQueryKind = useMemo( + const queryParamKind = useMemo( () => [kindParameter].flat()[0], [kindParameter], ); const [selectedKind, setSelectedKind] = useState( - flattenedQueryKind ?? filters.kind?.value ?? opts.initialFilter, + queryParamKind ?? filters.kind?.value ?? opts.initialFilter, ); - // Set selected kinds on query parameter updates; this happens at initial page load and from - // external updates to the page location. - useEffect(() => { - if (flattenedQueryKind) { - setSelectedKind(flattenedQueryKind); - } - }, [flattenedQueryKind]); - // Set selected kind from filters; this happens when the kind filter is // updated from another component useEffect(() => { @@ -109,18 +52,26 @@ function useEntityKindFilter(opts: { initialFilter: string }): { } }, [filters.kind]); - const { availableKinds, loading, error } = useAvailableKinds(); - useEffect(() => { updateFilters({ kind: selectedKind ? new EntityKindFilter(selectedKind) : undefined, }); }, [selectedKind, updateFilters]); + // Set selected kinds on query parameter updates; this happens at initial page load and from + // external updates to the page location. + useEffect(() => { + if (queryParamKind) { + setSelectedKind(queryParamKind); + } + }, [queryParamKind]); + + const { allKinds, loading, error } = useAllKinds(); + return { loading, error, - availableKinds, + allKinds: allKinds ?? [], selectedKind, setSelectedKind, }; @@ -132,17 +83,22 @@ function useEntityKindFilter(opts: { initialFilter: string }): { * @public */ export interface EntityKindPickerProps { + /** + * Entity kinds to show in the dropdown; by default all kinds are fetched from the catalog and + * displayed. + */ + allowedKinds?: string[]; initialFilter?: string; hidden?: boolean; } /** @public */ export const EntityKindPicker = (props: EntityKindPickerProps) => { - const { hidden, initialFilter = 'component' } = props; + const { allowedKinds, hidden, initialFilter = 'component' } = props; const alertApi = useApi(alertApiRef); - const { error, availableKinds, selectedKind, setSelectedKind } = + const { error, allKinds, selectedKind, setSelectedKind } = useEntityKindFilter({ initialFilter: initialFilter, }); @@ -156,21 +112,21 @@ export const EntityKindPicker = (props: EntityKindPickerProps) => { } }, [error, alertApi]); - if (availableKinds?.length === 0 || error) return null; + if (error) return null; - const items = [ - ...availableKinds.map((kind: string) => ({ - value: kind, - label: capitalize(kind), - })), - ]; + const options = filterAndCapitalize(allKinds, allowedKinds, [selectedKind]); + + const items = Object.keys(options).map(key => ({ + value: key, + label: options[key], + })); return hidden ? null : ( } - value={selectedKind} + input={} + value={selectedKind.toLocaleLowerCase('en-US')} onChange={e => setSelectedKind(e.target.value as string)} classes={classes} > From 8ba66bd0ac044759e1ca70ba4566cfbe1af9897b Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Sun, 20 Nov 2022 22:53:31 +0100 Subject: [PATCH 2/7] Fixed review comments * Now we don't react on external changes to the query kindParameter * Casing is always correct (APIs instead of Apis etc) Signed-off-by: Gustaf Lundh --- .changeset/brave-bags-sniff.md | 1 + .changeset/neat-lies-know.md | 2 +- plugins/catalog-react/api-report.md | 6 ++ .../EntityKindPicker.test.tsx | 62 ++++--------------- .../EntityKindPicker/EntityKindPicker.tsx | 12 +--- plugins/catalog-react/src/index.ts | 2 +- plugins/catalog-react/src/utils/index.ts | 2 +- .../src/utils/kindFilterUtils.ts | 43 +++++++------ .../CatalogKindHeader.test.tsx | 38 +----------- .../CatalogKindHeader/CatalogKindHeader.tsx | 12 +--- 10 files changed, 51 insertions(+), 129 deletions(-) diff --git a/.changeset/brave-bags-sniff.md b/.changeset/brave-bags-sniff.md index 2608ca7f1e..3fa542a4e3 100644 --- a/.changeset/brave-bags-sniff.md +++ b/.changeset/brave-bags-sniff.md @@ -3,3 +3,4 @@ --- Fixes in kind selectors (now OwnershipCards work again) +EntityKindPicker now accepts an optional allowedKinds prop, just like CatalogKindHeader. diff --git a/.changeset/neat-lies-know.md b/.changeset/neat-lies-know.md index cb355c638f..01fc002668 100644 --- a/.changeset/neat-lies-know.md +++ b/.changeset/neat-lies-know.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-catalog-react': minor +'@backstage/plugin-catalog-react': patch --- Cleanup and small fixes for the kind selector diff --git a/plugins/catalog-react/api-report.md b/plugins/catalog-react/api-report.md index ea67702c9b..91c57c3498 100644 --- a/plugins/catalog-react/api-report.md +++ b/plugins/catalog-react/api-report.md @@ -432,6 +432,9 @@ export type FavoriteEntityProps = ComponentProps & { entity: Entity; }; +// Warning: (tsdoc-undefined-tag) The TSDoc tag "@private" is not defined in this configuration +// Warning: (ae-missing-release-tag) "filterAndCapitalize" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// // @public export function filterAndCapitalize( allKinds: string[], @@ -511,6 +514,9 @@ export type UnregisterEntityDialogProps = { entity: Entity; }; +// Warning: (tsdoc-undefined-tag) The TSDoc tag "@private" is not defined in this configuration +// Warning: (ae-missing-release-tag) "useAllKinds" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// // @public export function useAllKinds(): { loading: boolean; diff --git a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx index 784580b636..04d8df3d16 100644 --- a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx @@ -148,21 +148,21 @@ describe('', () => { }); it('renders unknown kinds provided in query parameters', async () => { - const rendered = await renderWithEffects( + await renderWithEffects( , ); - expect(rendered.getByText('Frob')).toBeInTheDocument(); + expect(screen.getByText('FROb')).toBeInTheDocument(); }); it('limits kinds when allowedKinds is set', async () => { - const rendered = await renderWithEffects( + await renderWithEffects( @@ -170,56 +170,20 @@ describe('', () => { , ); - const input = rendered.getByTestId('select'); + const input = screen.getByTestId('select'); fireEvent.click(input); expect( - rendered.getByRole('option', { name: 'Component' }), + screen.getByRole('option', { name: 'Component' }), ).toBeInTheDocument(); + expect(screen.getByRole('option', { name: 'Domain' })).toBeInTheDocument(); expect( - rendered.getByRole('option', { name: 'Domain' }), - ).toBeInTheDocument(); - expect( - rendered.queryByRole('option', { name: 'Template' }), + screen.queryByRole('option', { name: 'Template' }), ).not.toBeInTheDocument(); }); - it('responds to external queryParameters changes', async () => { - const updateFilters = jest.fn(); - const rendered = await renderWithEffects( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - kind: new EntityKindFilter('component'), - }); - rendered.rerender( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - kind: new EntityKindFilter('domain'), - }); - }); - it('renders kind from the query parameter even when not in allowedKinds', async () => { - const rendered = await renderWithEffects( + await renderWithEffects( ', () => { , ); - expect(rendered.getByText('Frob')).toBeInTheDocument(); + expect(screen.getByText('Frob')).toBeInTheDocument(); - const input = rendered.getByTestId('select'); + const input = screen.getByTestId('select'); fireEvent.click(input); - expect( - rendered.getByRole('option', { name: 'Domain' }), - ).toBeInTheDocument(); + expect(screen.getByRole('option', { name: 'Domain' })).toBeInTheDocument(); }); }); diff --git a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx index 89313790f5..b0d1565cfb 100644 --- a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx +++ b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx @@ -20,7 +20,7 @@ import { Box } from '@material-ui/core'; import React, { useEffect, useMemo, useState } from 'react'; import { EntityKindFilter } from '../../filters'; import { useEntityList } from '../../hooks'; -import { filterAndCapitalize, useAllKinds } from '../../utils/kindFilterUtils'; +import { filterKinds, useAllKinds } from '../../utils/kindFilterUtils'; function useEntityKindFilter(opts: { initialFilter: string }): { loading: boolean; @@ -58,14 +58,6 @@ function useEntityKindFilter(opts: { initialFilter: string }): { }); }, [selectedKind, updateFilters]); - // Set selected kinds on query parameter updates; this happens at initial page load and from - // external updates to the page location. - useEffect(() => { - if (queryParamKind) { - setSelectedKind(queryParamKind); - } - }, [queryParamKind]); - const { allKinds, loading, error } = useAllKinds(); return { @@ -114,7 +106,7 @@ export const EntityKindPicker = (props: EntityKindPickerProps) => { if (error) return null; - const options = filterAndCapitalize(allKinds, allowedKinds, [selectedKind]); + const options = filterKinds(allKinds, allowedKinds, selectedKind); const items = Object.keys(options).map(key => ({ value: key, diff --git a/plugins/catalog-react/src/index.ts b/plugins/catalog-react/src/index.ts index 783b9c1959..de8b32282a 100644 --- a/plugins/catalog-react/src/index.ts +++ b/plugins/catalog-react/src/index.ts @@ -36,6 +36,6 @@ export { getEntitySourceLocation, isOwnerOf, useAllKinds, - filterAndCapitalize, + filterKinds, } from './utils'; export type { EntitySourceLocation } from './utils'; diff --git a/plugins/catalog-react/src/utils/index.ts b/plugins/catalog-react/src/utils/index.ts index 197eff3a55..8262d1cc81 100644 --- a/plugins/catalog-react/src/utils/index.ts +++ b/plugins/catalog-react/src/utils/index.ts @@ -18,4 +18,4 @@ export { getEntityRelations } from './getEntityRelations'; export { getEntitySourceLocation } from './getEntitySourceLocation'; export type { EntitySourceLocation } from './getEntitySourceLocation'; export { isOwnerOf } from './isOwnerOf'; -export { useAllKinds, filterAndCapitalize } from './kindFilterUtils'; +export { useAllKinds, filterKinds } from './kindFilterUtils'; diff --git a/plugins/catalog-react/src/utils/kindFilterUtils.ts b/plugins/catalog-react/src/utils/kindFilterUtils.ts index 939c7296dd..a0441db33f 100644 --- a/plugins/catalog-react/src/utils/kindFilterUtils.ts +++ b/plugins/catalog-react/src/utils/kindFilterUtils.ts @@ -15,14 +15,12 @@ */ import { useApi } from '@backstage/core-plugin-api'; -import { capitalize } from '@material-ui/core'; import useAsync from 'react-use/lib/useAsync'; import { catalogApiRef } from '../api'; /** * Fetch and return all availible kinds. - * - * @public + * @internal */ export function useAllKinds(): { loading: boolean; @@ -48,35 +46,40 @@ export function useAllKinds(): { /** * Filter and capitalize accessible kinds. * - * @public + * @internal */ -export function filterAndCapitalize( +export function filterKinds( allKinds: string[], allowedKinds?: string[], - forcedKinds?: string[], + forcedKinds?: string, ): Record { // Before allKinds is loaded, or when a kind is entered manually in the URL, selectedKind may not // be present in allKinds. It should still be shown in the dropdown, but may not have the nice // enforced casing from the catalog-backend. This makes a key/value record for the Select options, // including selectedKind if it's unknown - but allows the selectedKind to get clobbered by the // more proper catalog kind if it exists. - const availableKinds = allKinds - .concat(forcedKinds ?? []) - .filter(k => - allowedKinds - ? allowedKinds.some( - a => a.toLocaleLowerCase('en-US') === k.toLocaleLowerCase('en-US'), - ) || - forcedKinds?.some( - f => f.toLocaleLowerCase('en-US') === k.toLocaleLowerCase('en-US'), - ) - : true, + let availableKinds = allKinds; + if (allowedKinds) { + availableKinds = availableKinds.filter(k => + allowedKinds.some( + a => a.toLocaleLowerCase('en-US') === k.toLocaleLowerCase('en-US'), + ), ); + } + if ( + forcedKinds && + !allKinds.some( + a => + a.toLocaleLowerCase('en-US') === forcedKinds.toLocaleLowerCase('en-US'), + ) + ) { + availableKinds = availableKinds.concat([forcedKinds]); + } - const capitalizedKinds = availableKinds.sort().reduce((acc, kind) => { - acc[kind.toLocaleLowerCase('en-US')] = capitalize(kind); + const kindsMap = availableKinds.sort().reduce((acc, kind) => { + acc[kind.toLocaleLowerCase('en-US')] = kind; return acc; }, {} as Record); - return capitalizedKinds; + return kindsMap; } diff --git a/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.test.tsx b/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.test.tsx index 9f68252ca7..acff9e3cda 100644 --- a/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.test.tsx +++ b/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.test.tsx @@ -105,14 +105,14 @@ describe('', () => { await renderWithEffects( , ); - expect(screen.getByText('Frobs')).toBeInTheDocument(); + expect(screen.getByText('FRObs')).toBeInTheDocument(); }); it('updates the kind filter', async () => { @@ -136,40 +136,6 @@ describe('', () => { }); }); - it('responds to external queryParameters changes', async () => { - const updateFilters = jest.fn(); - const rendered = await renderWithEffects( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - kind: new EntityKindFilter('components'), - }); - rendered.rerender( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - kind: new EntityKindFilter('template'), - }); - }); - it('limits kinds when allowedKinds is set', async () => { await renderWithEffects( diff --git a/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.tsx b/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.tsx index 7de842abe6..5a69f89106 100644 --- a/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.tsx +++ b/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.tsx @@ -25,7 +25,7 @@ import { } from '@material-ui/core'; import { EntityKindFilter, - filterAndCapitalize, + filterKinds, useAllKinds, useEntityList, } from '@backstage/plugin-catalog-react'; @@ -90,15 +90,7 @@ export function CatalogKindHeader(props: CatalogKindHeaderProps) { }); }, [selectedKind, updateFilters]); - // Set selected Kind on query parameter updates; this happens at initial page load and from - // external updates to the page location. - useEffect(() => { - if (queryParamKind) { - setSelectedKind(queryParamKind); - } - }, [queryParamKind]); - - const options = filterAndCapitalize(allKinds, allowedKinds, [selectedKind]); + const options = filterKinds(allKinds, allowedKinds, selectedKind); return (