diff --git a/.changeset/itchy-bulldogs-dance.md b/.changeset/itchy-bulldogs-dance.md new file mode 100644 index 0000000000..a2acad4d34 --- /dev/null +++ b/.changeset/itchy-bulldogs-dance.md @@ -0,0 +1,10 @@ +--- +'@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 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. diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx index f6a4c59374..b776f26926 100644 --- a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx +++ b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx @@ -69,11 +69,13 @@ 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'; +const mockIsStarredEntity = jest.fn( + (entity: Entity) => entity.metadata.name === 'component-3', +); jest.mock('../../hooks', () => { const actual = jest.requireActual('../../hooks'); @@ -250,4 +252,86 @@ describe('', () => { ), }); }); + + describe.each` + type | filterFn + ${'owned'} | ${mockIsOwnedEntity} + ${'starred'} | ${mockIsStarredEntity} + `('filter resetting for $type entities', ({ type, filterFn }) => { + let updateFilters: jest.Mock; + + const picker = ({ loading }: { loading: boolean }) => ( + + + + + + ); + + beforeEach(() => { + updateFilters = jest.fn(); + }); + + describe(`when there are no ${type} entities match the filter`, () => { + beforeEach(() => { + filterFn.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 there are some ${type} entities present`, () => { + beforeEach(() => { + filterFn.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( + type, + mockIsOwnedEntity, + mockIsStarredEntity, + ), + }); + }); + }); + }); }); diff --git a/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx b/plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx index 8746752b08..d731a185fc 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,46 @@ 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 filterCounts = useMemo>( + () => ({ + all: entitiesWithoutUserFilter.length, + starred: entitiesWithoutUserFilter.filter(entity => + starredFilter.filterEntity(entity), + ).length, + owned: entitiesWithoutUserFilter.filter(entity => + ownedFilter.filterEntity(entity), + ).length, + }), + [entitiesWithoutUserFilter, starredFilter, ownedFilter], + ); + + useEffect(() => { + if ( + !loading && + !!selectedUserFilter && + selectedUserFilter !== 'all' && + filterCounts[selectedUserFilter] === 0 + ) { + setSelectedUserFilter('all'); + } + }, [loading, filterCounts, selectedUserFilter, setSelectedUserFilter]); + useEffect(() => { updateFilters({ user: selectedUserFilter @@ -186,26 +213,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': - return totalOwnedUserEntities; - case 'starred': - return entitiesWithoutUserFilter.filter(entity => - starredFilter.filterEntity(entity), - ).length; - default: - return entitiesWithoutUserFilter.length; - } - } - return ( {filterGroups.map(group => ( @@ -223,6 +230,8 @@ export const UserListPicker = ({ onClick={() => setSelectedUserFilter(item.id)} selected={item.id === filters.user?.value} className={classes.menuItem} + disabled={filterCounts[item.id] === 0} + data-testid={`user-picker-${item.id}`} > {item.icon && ( @@ -230,15 +239,10 @@ export const UserListPicker = ({ )} - - {item.label} - + {item.label} - {getFilterCount(item.id) ?? '-'} + {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\)/),