From 300f8cdaee54fc40fc94c493e0c1a40cdd50fcfe Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Fri, 14 Jan 2022 13:35:24 +0000 Subject: [PATCH 1/7] catalog-react: fix filter resetting bug in UserListPicker Signed-off-by: MT Lewis --- .changeset/itchy-bulldogs-dance.md | 5 ++ .../UserListPicker/UserListPicker.test.tsx | 83 ++++++++++++++++++- .../UserListPicker/UserListPicker.tsx | 44 ++++++---- 3 files changed, 113 insertions(+), 19 deletions(-) create mode 100644 .changeset/itchy-bulldogs-dance.md diff --git a/.changeset/itchy-bulldogs-dance.md b/.changeset/itchy-bulldogs-dance.md new file mode 100644 index 0000000000..a5123c0c27 --- /dev/null +++ b/.changeset/itchy-bulldogs-dance.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-react': patch +--- + +Fix bug with filter resetting based on user ownership diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx index f6a4c59374..a032d8fc46 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx @@ -69,8 +69,9 @@ const apis = TestApiRegistry.from( [storageApiRef, MockStorageApi.create()], ); -const mockIsOwnedEntity = (entity: Entity) => - entity.metadata.name === 'component-1'; +const mockIsOwnedEntity = jest.fn( + (entity: Entity) => entity.metadata.name === 'component-1', +); const mockIsStarredEntity = (entity: Entity) => entity.metadata.name === 'component-3'; @@ -250,4 +251,82 @@ describe('', () => { ), }); }); + + describe('filter resetting', () => { + let updateFilters: jest.Mock; + + const picker = ({ loading }: { loading: boolean }) => ( + + + + + + ); + + beforeEach(() => { + updateFilters = jest.fn(); + }); + + describe('when the user does not own any entities', () => { + beforeEach(() => { + mockIsOwnedEntity.mockReturnValue(false); + }); + + it('does not reset the filter while entities are loading', () => { + render(picker({ loading: true })); + + expect(updateFilters).not.toHaveBeenCalledWith({ + user: new UserListFilter( + 'all', + mockIsOwnedEntity, + mockIsStarredEntity, + ), + }); + }); + + it('resets the filter to "all" when entities are loaded', () => { + render(picker({ loading: false })); + + expect(updateFilters).toHaveBeenLastCalledWith({ + user: new UserListFilter( + 'all', + mockIsOwnedEntity, + mockIsStarredEntity, + ), + }); + }); + }); + + describe('when the user owns entities', () => { + beforeEach(() => { + mockIsOwnedEntity.mockReturnValue(true); + }); + + it('does not reset the filter while entities are loading', () => { + render(picker({ loading: true })); + + expect(updateFilters).not.toHaveBeenCalledWith({ + user: new UserListFilter( + 'all', + mockIsOwnedEntity, + mockIsStarredEntity, + ), + }); + }); + + it('does not reset the filter when entities are loaded', () => { + render(picker({ loading: false })); + + expect(updateFilters).toHaveBeenLastCalledWith({ + user: new UserListFilter( + 'owned', + mockIsOwnedEntity, + mockIsStarredEntity, + ), + }); + }); + }); + }); }); diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx index 8746752b08..dcad74f760 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx @@ -130,7 +130,7 @@ export const UserListPicker = ({ const classes = useStyles(); const configApi = useApi(configApiRef); const orgName = configApi.getOptionalString('organization.name') ?? 'Company'; - const { filters, updateFilters, backendEntities, queryParameters } = + const { filters, updateFilters, backendEntities, queryParameters, loading } = useEntityListProvider(); // Remove group items that aren't in availableFilters and exclude @@ -161,19 +161,36 @@ export const UserListPicker = ({ [isOwnedEntity, isStarredEntity], ); + const [selectedUserFilter, setSelectedUserFilter] = useState( + [queryParameters.user].flat()[0] ?? initialFilter, + ); + // To show proper counts for each section, apply all other frontend filters _except_ the user // filter that's controlled by this picker. - const [entitiesWithoutUserFilter, setEntitiesWithoutUserFilter] = - useState(backendEntities); - const totalOwnedUserEntities = entitiesWithoutUserFilter.filter(entity => - ownedFilter.filterEntity(entity), - ).length; - const [selectedUserFilter, setSelectedUserFilter] = useState( - totalOwnedUserEntities > 0 - ? [queryParameters.user].flat()[0] ?? initialFilter - : 'all', + const entitiesWithoutUserFilter = useMemo( + () => + backendEntities.filter( + reduceEntityFilters( + compact(Object.values({ ...filters, user: undefined })), + ), + ), + [filters, backendEntities], ); + const totalOwnedUserEntities = useMemo( + () => + entitiesWithoutUserFilter.filter(entity => + ownedFilter.filterEntity(entity), + ).length, + [entitiesWithoutUserFilter, ownedFilter], + ); + + useEffect(() => { + if (!loading && totalOwnedUserEntities === 0) { + setSelectedUserFilter('all'); + } + }, [loading, totalOwnedUserEntities, setSelectedUserFilter]); + useEffect(() => { updateFilters({ user: selectedUserFilter @@ -186,13 +203,6 @@ export const UserListPicker = ({ }); }, [selectedUserFilter, isOwnedEntity, isStarredEntity, updateFilters]); - useEffect(() => { - const filterFn = reduceEntityFilters( - compact(Object.values({ ...filters, user: undefined })), - ); - setEntitiesWithoutUserFilter(backendEntities.filter(filterFn)); - }, [filters, backendEntities]); - function getFilterCount(id: UserListFilterKind) { switch (id) { case 'owned': From dc3fe7c3ead198bb778399ef7e1f04345cfe3cf8 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Fri, 14 Jan 2022 14:20:39 +0000 Subject: [PATCH 2/7] catalog-react: disable groups with 0 entries in UserListPicker Signed-off-by: MT Lewis --- .../src/components/UserListPicker/UserListPicker.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx index dcad74f760..7c909167c9 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx @@ -233,6 +233,7 @@ export const UserListPicker = ({ onClick={() => setSelectedUserFilter(item.id)} selected={item.id === filters.user?.value} className={classes.menuItem} + disabled={getFilterCount(item.id) === 0} > {item.icon && ( From 17d812bdf25434e4e4c4cb259b4a96da957438a6 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Fri, 14 Jan 2022 14:36:16 +0000 Subject: [PATCH 3/7] catalog-react: generalize empty group handling Signed-off-by: MT Lewis --- .../UserListPicker/UserListPicker.tsx | 39 +++++++++---------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx index 7c909167c9..30102681d7 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx @@ -177,19 +177,29 @@ export const UserListPicker = ({ [filters, backendEntities], ); - const totalOwnedUserEntities = useMemo( - () => - entitiesWithoutUserFilter.filter(entity => + const filterCounts = useMemo>( + () => ({ + all: entitiesWithoutUserFilter.length, + starred: entitiesWithoutUserFilter.filter(entity => + starredFilter.filterEntity(entity), + ).length, + owned: entitiesWithoutUserFilter.filter(entity => ownedFilter.filterEntity(entity), ).length, - [entitiesWithoutUserFilter, ownedFilter], + }), + [entitiesWithoutUserFilter, starredFilter, ownedFilter], ); useEffect(() => { - if (!loading && totalOwnedUserEntities === 0) { + if ( + !loading && + !!selectedUserFilter && + selectedUserFilter !== 'all' && + filterCounts[selectedUserFilter] === 0 + ) { setSelectedUserFilter('all'); } - }, [loading, totalOwnedUserEntities, setSelectedUserFilter]); + }, [loading, filterCounts, selectedUserFilter, setSelectedUserFilter]); useEffect(() => { updateFilters({ @@ -203,19 +213,6 @@ export const UserListPicker = ({ }); }, [selectedUserFilter, isOwnedEntity, isStarredEntity, updateFilters]); - function getFilterCount(id: UserListFilterKind) { - switch (id) { - case 'owned': - return totalOwnedUserEntities; - case 'starred': - return entitiesWithoutUserFilter.filter(entity => - starredFilter.filterEntity(entity), - ).length; - default: - return entitiesWithoutUserFilter.length; - } - } - return ( {filterGroups.map(group => ( @@ -233,7 +230,7 @@ export const UserListPicker = ({ onClick={() => setSelectedUserFilter(item.id)} selected={item.id === filters.user?.value} className={classes.menuItem} - disabled={getFilterCount(item.id) === 0} + disabled={filterCounts[item.id] === 0} > {item.icon && ( @@ -249,7 +246,7 @@ export const UserListPicker = ({ - {getFilterCount(item.id) ?? '-'} + {filterCounts[item.id] ?? '-'} ))} From ce383faaef7edcb371c73e49668212b62f68d148 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Fri, 28 Jan 2022 10:26:20 +0000 Subject: [PATCH 4/7] catalog-react: generalise UserListPicker filter resetting tests Signed-off-by: MT Lewis --- .../UserListPicker/UserListPicker.test.tsx | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx index a032d8fc46..b776f26926 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx @@ -73,8 +73,9 @@ const mockIsOwnedEntity = jest.fn( (entity: Entity) => entity.metadata.name === 'component-1', ); -const mockIsStarredEntity = (entity: Entity) => - entity.metadata.name === 'component-3'; +const mockIsStarredEntity = jest.fn( + (entity: Entity) => entity.metadata.name === 'component-3', +); jest.mock('../../hooks', () => { const actual = jest.requireActual('../../hooks'); @@ -252,7 +253,11 @@ describe('', () => { }); }); - describe('filter resetting', () => { + describe.each` + type | filterFn + ${'owned'} | ${mockIsOwnedEntity} + ${'starred'} | ${mockIsStarredEntity} + `('filter resetting for $type entities', ({ type, filterFn }) => { let updateFilters: jest.Mock; const picker = ({ loading }: { loading: boolean }) => ( @@ -260,7 +265,7 @@ describe('', () => { - + ); @@ -269,9 +274,9 @@ describe('', () => { updateFilters = jest.fn(); }); - describe('when the user does not own any entities', () => { + describe(`when there are no ${type} entities match the filter`, () => { beforeEach(() => { - mockIsOwnedEntity.mockReturnValue(false); + filterFn.mockReturnValue(false); }); it('does not reset the filter while entities are loading', () => { @@ -299,9 +304,9 @@ describe('', () => { }); }); - describe('when the user owns entities', () => { + describe(`when there are some ${type} entities present`, () => { beforeEach(() => { - mockIsOwnedEntity.mockReturnValue(true); + filterFn.mockReturnValue(true); }); it('does not reset the filter while entities are loading', () => { @@ -321,7 +326,7 @@ describe('', () => { expect(updateFilters).toHaveBeenLastCalledWith({ user: new UserListFilter( - 'owned', + type, mockIsOwnedEntity, mockIsStarredEntity, ), From 91ffc2ade2c5e8739ddc899db174700aeeb354b1 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Fri, 28 Jan 2022 10:29:27 +0000 Subject: [PATCH 5/7] catalog: update DefaultCatalogPage test This test appears to be checking for a regression in which the filter reset from starred to owned after starring an entity. As a precursor, it tried to select all three possible filters, but now it's no longer possible to select filters that don't have any matching entities. Now that the behaviour of menu items when there are no items in that filter has changed, this commit updates the test to confirm that the menu item starts disabled, and then enables when an entity is starred. Signed-off-by: MT Lewis --- .../components/UserListPicker/UserListPicker.tsx | 8 ++------ .../CatalogPage/DefaultCatalogPage.test.tsx | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx index 30102681d7..d731a185fc 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx @@ -231,6 +231,7 @@ export const UserListPicker = ({ selected={item.id === filters.user?.value} className={classes.menuItem} disabled={filterCounts[item.id] === 0} + data-testid={`user-picker-${item.id}`} > {item.icon && ( @@ -238,12 +239,7 @@ export const UserListPicker = ({ )} - - {item.label} - + {item.label} {filterCounts[item.id] ?? '-'} diff --git a/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx b/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx index 135a2a590d..20acff3c50 100644 --- a/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx +++ b/plugins/catalog/src/components/CatalogPage/DefaultCatalogPage.test.tsx @@ -263,10 +263,12 @@ describe('DefaultCatalogPage', () => { const { getByTestId } = await renderWrapped(); fireEvent.click(getByTestId('user-picker-owned')); await expect(screen.findByText(/Owned \(1\)/)).resolves.toBeInTheDocument(); - fireEvent.click(screen.getByTestId('user-picker-starred')); - await expect( - screen.findByText(/Starred \(0\)/), - ).resolves.toBeInTheDocument(); + // The "Starred" menu option should initially be disabled, since there + // aren't any starred entities. + await expect(screen.getByTestId('user-picker-starred')).toHaveAttribute( + 'aria-disabled', + 'true', + ); fireEvent.click(screen.getByTestId('user-picker-all')); await expect(screen.findByText(/All \(2\)/)).resolves.toBeInTheDocument(); @@ -274,6 +276,12 @@ describe('DefaultCatalogPage', () => { fireEvent.click(starredIcons[0]); await expect(screen.findByText(/All \(2\)/)).resolves.toBeInTheDocument(); + // Now that we've starred an entity, the "Starred" menu option should be + // enabled. + await expect(screen.getByTestId('user-picker-starred')).not.toHaveAttribute( + 'aria-disabled', + 'true', + ); fireEvent.click(screen.getByTestId('user-picker-starred')); await expect( screen.findByText(/Starred \(1\)/), From 569715b9f0a43922cd1519ca1f98151e5fb94393 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 1 Feb 2022 11:37:12 +0000 Subject: [PATCH 6/7] catalog-react: expand bugfix description in changeset Signed-off-by: MT Lewis --- .changeset/itchy-bulldogs-dance.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/itchy-bulldogs-dance.md b/.changeset/itchy-bulldogs-dance.md index a5123c0c27..b3d34654e8 100644 --- a/.changeset/itchy-bulldogs-dance.md +++ b/.changeset/itchy-bulldogs-dance.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-react': patch --- -Fix bug with filter resetting based on user ownership +Fix bug: previously the filter would be set to "all" on page load, even if the `initiallySelectedFilter` on the `DefaultCatalogPage` was set to something else, or a different query param was supplied. Now, the prop and query params control the filter as expected. Additionally, after this change any filters which match 0 items will be disabled, and the filter will be reverted to 'all' if they're set on page load. From 06f84f7e678fca28f5621613e86b274ce39f515f Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 1 Feb 2022 14:21:39 +0000 Subject: [PATCH 7/7] Stop abbreviating 'params' to make Vale happy Signed-off-by: MT Lewis --- .changeset/itchy-bulldogs-dance.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.changeset/itchy-bulldogs-dance.md b/.changeset/itchy-bulldogs-dance.md index b3d34654e8..a2acad4d34 100644 --- a/.changeset/itchy-bulldogs-dance.md +++ b/.changeset/itchy-bulldogs-dance.md @@ -2,4 +2,9 @@ '@backstage/plugin-catalog-react': patch --- -Fix bug: previously the filter would be set to "all" on page load, even if the `initiallySelectedFilter` on the `DefaultCatalogPage` was set to something else, or a different query param was supplied. Now, the prop and query params control the filter as expected. Additionally, after this change any filters which match 0 items will be disabled, and the filter will be reverted to 'all' if they're set on page load. +Fix bug: previously the filter would be set to "all" on page load, even if the +`initiallySelectedFilter` on the `DefaultCatalogPage` was set to something else, +or a different query parameter was supplied. Now, the prop and query parameters +control the filter as expected. Additionally, after this change any filters +which match 0 items will be disabled, and the filter will be reverted to 'all' +if they're set on page load.