From 78b2610717546e406def7f2a55c3c385b7f0b656 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 26 May 2023 14:14:48 +0200 Subject: [PATCH] catalog-react: remove UserListFilter usage Signed-off-by: Vincenzo Scamporlino --- plugins/catalog-react/api-report.md | 43 ++-- .../UserListPicker/UserListPicker.test.tsx | 215 +----------------- .../UserListPicker/UserListPicker.tsx | 50 ++-- plugins/catalog-react/src/filters.ts | 9 + .../src/hooks/useEntityListProvider.test.tsx | 23 +- .../src/hooks/useEntityOwnership.ts | 14 +- 6 files changed, 73 insertions(+), 281 deletions(-) diff --git a/plugins/catalog-react/api-report.md b/plugins/catalog-react/api-report.md index 3faa5524c9..e73441f7dc 100644 --- a/plugins/catalog-react/api-report.md +++ b/plugins/catalog-react/api-report.md @@ -142,7 +142,7 @@ export const columnFactories: Readonly<{ export type DefaultEntityFilters = { kind?: EntityKindFilter; type?: EntityTypeFilter; - user?: UserListFilter | UserOwnersFilter; + user?: UserListFilter | EntityUserListFilter; owners?: EntityOwnerFilter; lifecycles?: EntityLifecycleFilter; tags?: EntityTagFilter; @@ -507,6 +507,26 @@ export interface EntityTypePickerProps { initialFilter?: string; } +// @public (undocumented) +export class EntityUserListFilter implements EntityFilter { + // (undocumented) + static all(): EntityUserListFilter; + // (undocumented) + filterEntity(entity: Entity): boolean; + // (undocumented) + getCatalogFilters(): Record; + // (undocumented) + static owned(ownershipEntityRefs: string[]): EntityUserListFilter; + // (undocumented) + readonly refs?: string[] | undefined; + // (undocumented) + static starred(starredEntityRefs: string[]): EntityUserListFilter; + // (undocumented) + toQueryValue(): string; + // (undocumented) + readonly value: UserListFilterKind; +} + // @public export const FavoriteEntity: ( props: FavoriteEntityProps, @@ -661,29 +681,8 @@ export const UserListPicker: ( export type UserListPickerProps = { initialFilter?: UserListFilterKind; availableFilters?: UserListFilterKind[]; - useServerSideFilters?: boolean; }; -// @public (undocumented) -export class UserOwnersFilter implements EntityFilter { - // (undocumented) - static all(): UserOwnersFilter; - // (undocumented) - filterEntity(entity: Entity): boolean; - // (undocumented) - getCatalogFilters(): Record; - // (undocumented) - static owned(ownershipEntityRefs: string[]): UserOwnersFilter; - // (undocumented) - readonly refs?: string[] | undefined; - // (undocumented) - static starred(starredEntityRefs: string[]): UserOwnersFilter; - // (undocumented) - toQueryValue(): string; - // (undocumented) - readonly value: UserListFilterKind; -} - // @public (undocumented) export function useStarredEntities(): { starredEntities: Set; diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx index 20dedf23a6..ed6c43d12e 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx @@ -16,14 +16,10 @@ import React from 'react'; import { fireEvent, render, waitFor, screen } from '@testing-library/react'; -import { Entity, UserEntity } from '@backstage/catalog-model'; +import { UserEntity } from '@backstage/catalog-model'; import { UserListPicker, UserListPickerProps } from './UserListPicker'; import { MockEntityListContextProvider } from '../../testUtils/providers'; -import { - EntityTagFilter, - UserListFilter, - EntityUserListFilter, -} from '../../filters'; +import { EntityTagFilter, EntityUserListFilter } from '../../filters'; import { CatalogApi, QueryEntitiesInitialRequest, @@ -75,14 +71,6 @@ const apis = TestApiRegistry.from( [starredEntitiesApiRef, mockStarredEntitiesApi], ); -const mockIsOwnedEntity = jest.fn( - (entity: Entity) => entity.metadata.name === 'component-1', -); - -const mockIsStarredEntity = jest.fn( - (entity: Entity) => entity.metadata.name === 'component-3', -); - const ownershipEntityRefs = ['user:default/testuser']; describe('', () => { const mockQueryEntitiesImplementation: CatalogApi['queryEntities'] = @@ -233,44 +221,6 @@ describe('', () => { }); }); - it('respects the query parameter filter value, legacy', async () => { - const updateFilters = jest.fn(); - const queryParameters = { user: 'owned' }; - render( - - - - - , - ); - - await waitFor(() => - expect(updateFilters).toHaveBeenLastCalledWith({ - user: new UserListFilter( - 'owned', - expect.any(Function), - expect.any(Function), - ), - }), - ); - expect(mockCatalogApi.queryEntities).toHaveBeenCalledWith({ - filter: {}, - limit: 0, - }); - expect(mockCatalogApi.queryEntities).toHaveBeenCalledWith({ - filter: { 'metadata.name': ['e-1', 'e-2'] }, - limit: 1000, - }); - expect(mockCatalogApi.queryEntities).toHaveBeenCalledWith({ - filter: { - 'relations.ownedBy': ['user:default/testuser'], - }, - limit: 0, - }); - }); - it('respects the query parameter filter value', async () => { const updateFilters = jest.fn(); const queryParameters = { user: 'owned' }; @@ -279,7 +229,7 @@ describe('', () => { - + , ); @@ -306,35 +256,12 @@ describe('', () => { }); }); - it('updates user filter when a menuitem is selected, legacy', async () => { - const updateFilters = jest.fn(); - render( - - - - - , - ); - - fireEvent.click(screen.getByText('Starred')); - - await waitFor(() => - expect(updateFilters).toHaveBeenLastCalledWith({ - user: new UserListFilter( - 'starred', - expect.any(Function), - expect.any(Function), - ), - }), - ); - }); - it('updates user filter when a menuitem is selected', async () => { const updateFilters = jest.fn(); render( - + , ); @@ -351,52 +278,6 @@ describe('', () => { ); }); - it('responds to external queryParameters changes, legacy', async () => { - const updateFilters = jest.fn(); - const rendered = render( - - - - - , - ); - - await waitFor(() => - expect(updateFilters).toHaveBeenLastCalledWith({ - user: new UserListFilter( - 'all', - expect.any(Function), - expect.any(Function), - ), - }), - ); - - rendered.rerender( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - user: new UserListFilter( - 'owned', - expect.any(Function), - expect.any(Function), - ), - }); - }); - it('responds to external queryParameters changes', async () => { const updateFilters = jest.fn(); const rendered = render( @@ -407,7 +288,7 @@ describe('', () => { queryParameters: { user: ['all'] }, }} > - + , ); @@ -426,7 +307,7 @@ describe('', () => { queryParameters: { user: ['owned'] }, }} > - + , ); @@ -489,34 +370,6 @@ describe('', () => { }); }); - it('resets the filter to "all" when entities are loaded, legacy', async () => { - mockCatalogApi.queryEntities?.mockImplementation(async request => { - if ( - ( - (request as QueryEntitiesInitialRequest).filter as Record< - string, - string - > - )['relations.ownedBy'] - ) { - return { items: [], totalItems: 0, pageInfo: {} }; - } - return mockQueryEntitiesImplementation(request); - }); - - render(); - - await waitFor(() => - expect(updateFilters).toHaveBeenLastCalledWith({ - user: new UserListFilter( - 'all', - expect.any(Function), - expect.any(Function), - ), - }), - ); - }); - it('resets the filter to "all" when entities are loaded', async () => { mockCatalogApi.queryEntities?.mockImplementation(async request => { if ( @@ -532,7 +385,7 @@ describe('', () => { return mockQueryEntitiesImplementation(request); }); - render(); + render(); await waitFor(() => expect(updateFilters).toHaveBeenLastCalledWith({ @@ -581,34 +434,6 @@ describe('', () => { }); }); - it('resets the filter to "all" when entities are loaded, legacy', async () => { - mockCatalogApi.queryEntities?.mockImplementation(async request => { - if ( - ( - (request as QueryEntitiesInitialRequest).filter as Record< - string, - string - > - )['metadata.name'] - ) { - return { items: [], totalItems: 0, pageInfo: {} }; - } - return mockQueryEntitiesImplementation(request); - }); - - render(); - - await waitFor(() => - expect(updateFilters).toHaveBeenLastCalledWith({ - user: new UserListFilter( - 'all', - expect.any(Function), - expect.any(Function), - ), - }), - ); - }); - it('resets the filter to "all" when entities are loaded', async () => { mockCatalogApi.queryEntities?.mockImplementation(async request => { if ( @@ -624,7 +449,7 @@ describe('', () => { return mockQueryEntitiesImplementation(request); }); - render(); + render(); await waitFor(() => expect(updateFilters).toHaveBeenLastCalledWith({ @@ -658,11 +483,7 @@ describe('', () => { expect(mockCatalogApi.queryEntities).toHaveBeenCalledTimes(3), ); expect(updateFilters).not.toHaveBeenCalledWith({ - user: new UserListFilter( - 'all', - mockIsOwnedEntity, - mockIsStarredEntity, - ), + user: EntityUserListFilter.all(), }); }); @@ -675,11 +496,7 @@ describe('', () => { await waitFor(() => expect(updateFilters).toHaveBeenLastCalledWith({ - user: new UserListFilter( - 'owned', - expect.any(Function), - expect.any(Function), - ), + user: EntityUserListFilter.owned(expect.any(Array)), }), ); }); @@ -709,11 +526,7 @@ describe('', () => { expect(mockCatalogApi.queryEntities).toHaveBeenCalledTimes(3), ); expect(updateFilters).not.toHaveBeenCalledWith({ - user: new UserListFilter( - 'all', - mockIsOwnedEntity, - mockIsStarredEntity, - ), + user: EntityUserListFilter.all(), }); }); @@ -726,11 +539,7 @@ describe('', () => { await waitFor(() => expect(updateFilters).toHaveBeenLastCalledWith({ - user: new UserListFilter( - 'starred', - expect.any(Function), - expect.any(Function), - ), + user: EntityUserListFilter.starred(expect.any(Array)), }), ); }); diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx index dff4f995ae..b0a86b1322 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx @@ -33,13 +33,12 @@ import { import SettingsIcon from '@material-ui/icons/Settings'; import StarIcon from '@material-ui/icons/Star'; import React, { Fragment, useEffect, useMemo, useState } from 'react'; -import { UserListFilter, EntityUserListFilter } from '../../filters'; -import { useEntityList, useStarredEntities } from '../../hooks'; +import { EntityUserListFilter } from '../../filters'; +import { useEntityList } from '../../hooks'; import { UserListFilterKind } from '../../types'; import { useOwnedEntitiesCount } from './useOwnedEntitiesCount'; import { useAllEntitiesCount } from './useAllEntitiesCount'; import { useStarredEntitiesCount } from './useStarredEntitiesCount'; -import { useIsOwnedEntity } from '../../hooks/useEntityOwnership'; /** @public */ export type CatalogReactUserListPickerClassKey = @@ -120,12 +119,11 @@ function getFilterGroups(orgName: string | undefined): ButtonGroup[] { export type UserListPickerProps = { initialFilter?: UserListFilterKind; availableFilters?: UserListFilterKind[]; - useServerSideFilters?: boolean; }; /** @public */ export const UserListPicker = (props: UserListPickerProps) => { - const { initialFilter, availableFilters, useServerSideFilters } = props; + const { initialFilter, availableFilters } = props; const classes = useStyles(); const configApi = useApi(configApiRef); const orgName = configApi.getOptionalString('organization.name') ?? 'Company'; @@ -154,7 +152,6 @@ export const UserListPicker = (props: UserListPickerProps) => { count: ownedEntitiesCount, loading: loadingOwnedEntities, filter: ownedEntitiesFilter, - ownershipEntityRefs, } = useOwnedEntitiesCount(); const { count: allCount } = useAllEntitiesCount(); const { @@ -180,9 +177,6 @@ export const UserListPicker = (props: UserListPickerProps) => { }; }, [starredEntitiesCount, ownedEntitiesCount, allCount]); - const { isStarredEntity } = useStarredEntities(); - const isOwnedEntity = useIsOwnedEntity(ownershipEntityRefs); - // Set selected user filter on query parameter updates; this happens at initial page load and from // external updates to the page location. useEffect(() => { @@ -211,38 +205,24 @@ export const UserListPicker = (props: UserListPickerProps) => { if (loading) { return; } - if (useServerSideFilters) { - const getFilter = () => { - if (selectedUserFilter === 'owned') { - return ownedEntitiesFilter; - } - if (selectedUserFilter === 'starred') { - return starredEntitiesFilter; - } - return EntityUserListFilter.all(); - }; - updateFilters({ user: getFilter() }); - } else { - // legacy - updateFilters({ - user: selectedUserFilter - ? new UserListFilter( - selectedUserFilter as UserListFilterKind, - isOwnedEntity, - isStarredEntity, - ) - : undefined, - }); - } + const getFilter = () => { + if (selectedUserFilter === 'owned') { + return ownedEntitiesFilter; + } + if (selectedUserFilter === 'starred') { + return starredEntitiesFilter; + } + return EntityUserListFilter.all(); + }; + + updateFilters({ user: getFilter() }); }, [ selectedUserFilter, starredEntitiesFilter, ownedEntitiesFilter, updateFilters, - useServerSideFilters, - isOwnedEntity, - isStarredEntity, + loading, ]); diff --git a/plugins/catalog-react/src/filters.ts b/plugins/catalog-react/src/filters.ts index d0ab093e3c..bafaae4fb2 100644 --- a/plugins/catalog-react/src/filters.ts +++ b/plugins/catalog-react/src/filters.ts @@ -237,6 +237,15 @@ export class EntityUserListFilter implements EntityFilter { if (this.value === 'starred') { return this.refs?.includes(stringifyEntityRef(entity)) ?? true; } + if (this.value === 'owned') { + return ( + this.refs?.some(v => + getEntityRelations(entity, RELATION_OWNED_BY).some( + o => stringifyEntityRef(o) === v, + ), + ) ?? false + ); + } return true; } diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx index d77655ebcc..a4401e3441 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx +++ b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx @@ -32,7 +32,11 @@ import { MemoryRouter } from 'react-router-dom'; import { catalogApiRef } from '../api'; import { starredEntitiesApiRef, MockStarredEntitiesApi } from '../apis'; import { EntityKindPicker, UserListPicker } from '../components'; -import { EntityKindFilter, EntityTypeFilter, UserListFilter } from '../filters'; +import { + EntityKindFilter, + EntityTypeFilter, + EntityUserListFilter, +} from '../filters'; import { UserListFilterKind } from '../types'; import { EntityListProvider, useEntityList } from './useEntityListProvider'; @@ -62,11 +66,14 @@ const entities: Entity[] = [ const mockConfigApi = { getOptionalString: () => '', } as Partial; + +const ownershipEntityRefs = ['user:default/guest']; + const mockIdentityApi: Partial = { getBackstageIdentity: async () => ({ type: 'user', userEntityRef: 'user:default/guest', - ownershipEntityRefs: [], + ownershipEntityRefs, }), getCredentials: async () => ({ token: undefined }), }; @@ -148,11 +155,7 @@ describe('', () => { act(() => result.current.updateFilters({ - user: new UserListFilter( - 'owned', - entity => entity.metadata.name === 'component-1', - () => true, - ), + user: EntityUserListFilter.owned(ownershipEntityRefs), }), ); @@ -193,11 +196,7 @@ describe('', () => { act(() => result.current.updateFilters({ - user: new UserListFilter( - 'owned', - entity => entity.metadata.name === 'component-1', - () => true, - ), + user: EntityUserListFilter.owned(ownershipEntityRefs), }), ); diff --git a/plugins/catalog-react/src/hooks/useEntityOwnership.ts b/plugins/catalog-react/src/hooks/useEntityOwnership.ts index 1f1e200634..9c86d6e01a 100644 --- a/plugins/catalog-react/src/hooks/useEntityOwnership.ts +++ b/plugins/catalog-react/src/hooks/useEntityOwnership.ts @@ -46,15 +46,10 @@ export function useEntityOwnership(): { return ownershipEntityRefs; }, []); - const isOwnedEntity = useIsOwnedEntity(refs); - - return useMemo(() => ({ loading, isOwnedEntity }), [loading, isOwnedEntity]); -} - -export function useIsOwnedEntity(refs?: string[]) { - return useMemo(() => { + const isOwnedEntity = useMemo(() => { const myOwnerRefs = new Set(refs ?? []); - const isOwnedEntity = (entity: Entity) => { + + return (entity: Entity) => { const entityOwnerRefs = getEntityRelations(entity, RELATION_OWNED_BY).map( stringifyEntityRef, ); @@ -65,6 +60,7 @@ export function useIsOwnedEntity(refs?: string[]) { } return false; }; - return isOwnedEntity; }, [refs]); + + return { loading, isOwnedEntity }; }