From 38a8a8114da16f82dfd4a386e3e0bc68cca6df0c Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 17 Nov 2023 17:32:39 +0100 Subject: [PATCH 1/2] catalog-react: fix UserListPicker initial filter Signed-off-by: Vincenzo Scamporlino --- .../UserListPicker/UserListPicker.test.tsx | 66 ++++++++++++- .../useOwnedEntitiesCount.test.tsx | 4 +- .../UserListPicker/useOwnedEntitiesCount.ts | 94 ++++++++----------- 3 files changed, 104 insertions(+), 60 deletions(-) diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx index ad56681d6c..5667447503 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx @@ -40,6 +40,7 @@ import { storageApiRef, } from '@backstage/core-plugin-api'; import { MockStarredEntitiesApi, starredEntitiesApiRef } from '../../apis'; +import { DefaultEntityFilters } from '../../hooks'; const mockUser: UserEntity = { apiVersion: 'backstage.io/v1alpha1', @@ -289,7 +290,12 @@ describe('', () => { const updateFilters = jest.fn(); render( - + , @@ -297,6 +303,25 @@ describe('', () => { fireEvent.click(screen.getByText('Starred')); + // wait until the component has finished loading + await waitFor(() => { + expect(mockCatalogApi.queryEntities).toHaveBeenCalledWith({ + filter: { kind: 'component', 'metadata.name': ['e-1', 'e-2'] }, + limit: 1000, + }); + expect(mockCatalogApi.queryEntities).toHaveBeenCalledWith({ + filter: { kind: 'component' }, + limit: 0, + }); + expect(mockCatalogApi.queryEntities).toHaveBeenCalledWith({ + filter: { + kind: 'component', + 'relations.ownedBy': ['user:default/testuser'], + }, + limit: 0, + }); + }); + await waitFor(() => expect(updateFilters).toHaveBeenLastCalledWith({ user: EntityUserFilter.starred([ @@ -360,12 +385,17 @@ describe('', () => { describe('filter resetting', () => { const updateFilters = jest.fn(); - const Picker = ({ ...props }: UserListPickerProps) => ( + const Picker = ({ + filters, + ...props + }: UserListPickerProps & { filters?: DefaultEntityFilters }) => ( @@ -413,6 +443,21 @@ describe('', () => { }); }); + it('does not reset the filter when request is empty', async () => { + render(); + + await waitFor(() => { + expect(mockCatalogApi.queryEntities).toHaveBeenCalledTimes(1); + expect(mockCatalogApi.queryEntities).toHaveBeenCalledWith({ + filter: { 'metadata.name': ['e-1', 'e-2'] }, + limit: 1000, + }); + }); + expect(updateFilters).not.toHaveBeenCalledWith({ + user: expect.any(Object), + }); + }); + it('resets the filter to "all" when entities are loaded', async () => { mockCatalogApi.queryEntities?.mockImplementation(async request => { if ( @@ -541,6 +586,21 @@ describe('', () => { }), ); }); + + it('does not reset the filter when request is empty xxxx', async () => { + render(); + + await waitFor(() => { + expect(mockCatalogApi.queryEntities).toHaveBeenCalledTimes(1); + expect(mockCatalogApi.queryEntities).toHaveBeenCalledWith({ + filter: { 'metadata.name': ['e-1', 'e-2'] }, + limit: 1000, + }); + }); + expect(updateFilters).not.toHaveBeenCalledWith({ + user: expect.any(Object), + }); + }); }); describe(`when there are some starred entities present`, () => { diff --git a/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.test.tsx b/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.test.tsx index 1f6fbfd53d..aeaa20c535 100644 --- a/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.test.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.test.tsx @@ -93,8 +93,8 @@ describe('useOwnedEntitiesCount', () => { ).rejects.toThrow(); expect(result.current).toEqual({ - count: 0, - loading: false, + count: undefined, + loading: true, filter: EntityUserFilter.owned([ 'user:default/spiderman', 'user:group/a-group', diff --git a/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.ts b/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.ts index 1adab49361..2f03a0b285 100644 --- a/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.ts +++ b/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.ts @@ -14,16 +14,16 @@ * limitations under the License. */ -import { QueryEntitiesInitialRequest } from '@backstage/catalog-client'; import { identityApiRef, useApi } from '@backstage/core-plugin-api'; -import { compact, intersection, isEqual } from 'lodash'; -import { useEffect, useMemo, useRef } from 'react'; +import { compact, intersection } from 'lodash'; +import { useMemo } from 'react'; import useAsync from 'react-use/lib/useAsync'; import { catalogApiRef } from '../../api'; import { EntityOwnerFilter, EntityUserFilter } from '../../filters'; import { useEntityList } from '../../hooks'; import { reduceCatalogFilters } from '../../utils'; import useAsyncFn from 'react-use/lib/useAsyncFn'; +import useDeepCompareEffect from 'react-use/lib/useDeepCompareEffect'; export function useOwnedEntitiesCount() { const identityApi = useApi(identityApiRef); @@ -37,79 +37,63 @@ export function useOwnedEntitiesCount() { [], ); - const prevRequest = useRef(); - - const request = useMemo(() => { - const { user, owners, ...allFilters } = filters; - const compacted = compact(Object.values(allFilters)); - const allFilter = reduceCatalogFilters(compacted); - const { ['metadata.name']: metadata, ...filter } = allFilter; - - const countFilter = getOwnedCountClaims(owners, ownershipEntityRefs); - - if ( - ownershipEntityRefs?.length === 0 || - countFilter === undefined || - Object.keys(filter).length === 0 - ) { - prevRequest.current = undefined; - return undefined; - } - const newRequest: QueryEntitiesInitialRequest = { - filter: { - ...filter, - 'relations.ownedBy': countFilter, - }, - limit: 0, - }; - - if (isEqual(newRequest, prevRequest.current)) { - return prevRequest.current; - } - - prevRequest.current = newRequest; - - return newRequest; - }, [filters, ownershipEntityRefs]); + const { user, owners, ...allFilters } = filters; + const { ['metadata.name']: metadata, ...filter } = reduceCatalogFilters( + compact(Object.values(allFilters)), + ); const [{ value: count, loading: loadingEntityOwnership }, fetchEntities] = useAsyncFn( - async ( - req: QueryEntitiesInitialRequest | undefined, - ownershipEntityRefsParam: string[], - ) => { - if (ownershipEntityRefsParam && !req) { + async (req: { + ownershipEntityRefs: string[]; + owners: EntityOwnerFilter | undefined; + filter: Record; + }) => { + const ownedClaims = getOwnedCountClaims( + req.owners, + req.ownershipEntityRefs, + ); + if (ownedClaims === undefined) { // this implicitly means that there aren't claims in common with // the logged in users, so avoid invoking the queryEntities endpoint // which will implicitly returns 0 return 0; } - const { totalItems } = await catalogApi.queryEntities(req); + + const { totalItems } = await catalogApi.queryEntities({ + filter: { + ...req.filter, + 'relations.ownedBy': ownedClaims, + }, + limit: 0, + }); return totalItems; }, [], { loading: true }, ); - useEffect(() => { - if (ownershipEntityRefs) { - if (request && Object.keys(request).length === 0) { - return; - } - fetchEntities(request, ownershipEntityRefs); + useDeepCompareEffect(() => { + // context contains no filter, wait + if (Object.keys(filter).length === 0) { + return; } - }, [fetchEntities, request, ownershipEntityRefs]); + // ownershipEntityRefs is loading, wait + if (ownershipEntityRefs === undefined) { + return; + } + fetchEntities({ ownershipEntityRefs, owners, filter }); + }, [ownershipEntityRefs, owners, filter]); const loading = loadingEntityRefs || loadingEntityOwnership; - const filter = useMemo( - () => EntityUserFilter.owned(ownershipEntityRefs ?? []), - [ownershipEntityRefs], - ); return { count, loading, - filter, + filter: useMemo( + () => EntityUserFilter.owned(ownershipEntityRefs ?? []), + [ownershipEntityRefs], + ), ownershipEntityRefs, }; } From eee0ff29464d78c2332b1515acd7d1df076385ac Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 17 Nov 2023 17:38:46 +0100 Subject: [PATCH 2/2] catalog-react: UserListPicker fix changeset Signed-off-by: Vincenzo Scamporlino --- .changeset/twenty-beans-laugh.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/twenty-beans-laugh.md diff --git a/.changeset/twenty-beans-laugh.md b/.changeset/twenty-beans-laugh.md new file mode 100644 index 0000000000..5dc537d93b --- /dev/null +++ b/.changeset/twenty-beans-laugh.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-react': patch +--- + +Fixed a issue where `CatalogPage` wasn't using the chosen `initiallySelectedFilter` as intended.