diff --git a/.changeset/selfish-mugs-itch.md b/.changeset/selfish-mugs-itch.md new file mode 100644 index 0000000000..39ed236352 --- /dev/null +++ b/.changeset/selfish-mugs-itch.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-cost-insights': minor +--- + +Remove product filters from query parameters diff --git a/plugins/cost-insights/package.json b/plugins/cost-insights/package.json index 68561fc041..f94c467ab3 100644 --- a/plugins/cost-insights/package.json +++ b/plugins/cost-insights/package.json @@ -40,7 +40,8 @@ "react-dom": "^16.13.1", "react-router-dom": "6.0.0-beta.0", "react-use": "^15.3.3", - "recharts": "^1.8.5" + "recharts": "^1.8.5", + "yup": "^0.29.3" }, "devDependencies": { "@backstage/cli": "^0.1.1-alpha.25", @@ -50,6 +51,7 @@ "@testing-library/user-event": "^12.0.7", "@types/jest": "^26.0.7", "@types/node": "^12.0.0", + "@types/yup": "^0.29.8", "jest-fetch-mock": "^3.0.3", "msw": "^0.20.5", "node-fetch": "^2.6.1" diff --git a/plugins/cost-insights/src/components/ProductInsightsCard/ProductInsightsCard.tsx b/plugins/cost-insights/src/components/ProductInsightsCard/ProductInsightsCard.tsx index c9790a5bfa..13bb990740 100644 --- a/plugins/cost-insights/src/components/ProductInsightsCard/ProductInsightsCard.tsx +++ b/plugins/cost-insights/src/components/ProductInsightsCard/ProductInsightsCard.tsx @@ -51,7 +51,7 @@ const ProductInsightsCard = ({ product }: ProductInsightsCardProps) => { const dispatchLoadingProduct = useCallback(dispatchLoading, [product.kind]); const amount = resource?.entities?.length || 0; - const hasCostsWithinTimeframe = resource?.change && amount; + const hasCostsWithinTimeframe = resource?.change && !!amount; const subheader = amount ? `${amount} ${pluralOf(amount, 'entity', 'entities')}, sorted by cost` diff --git a/plugins/cost-insights/src/hooks/index.ts b/plugins/cost-insights/src/hooks/index.ts index 7065bcf91b..8998a10e2c 100644 --- a/plugins/cost-insights/src/hooks/index.ts +++ b/plugins/cost-insights/src/hooks/index.ts @@ -20,5 +20,4 @@ export * from './useFilters'; export * from './useCurrency'; export * from './useGroups'; export * from './useLoading'; -export * from './useQueryParams'; export * from './useScroll'; diff --git a/plugins/cost-insights/src/hooks/useFilters.tsx b/plugins/cost-insights/src/hooks/useFilters.tsx index 7dcf8ddb26..22ef735186 100644 --- a/plugins/cost-insights/src/hooks/useFilters.tsx +++ b/plugins/cost-insights/src/hooks/useFilters.tsx @@ -16,114 +16,98 @@ import React, { Dispatch, - ReactNode, + PropsWithChildren, SetStateAction, useContext, useEffect, - useRef, useState, } from 'react'; -import { - getDefaultPageFilters, - PageFilters, - ProductFilters, - Duration, - Group, -} from '../types'; +import { Alert } from '@material-ui/lab'; +import { Maybe, PageFilters, ProductFilters } from '../types'; import { useLocation, useNavigate } from 'react-router-dom'; -import { useQueryParams } from './useQueryParams'; -import { stringify } from '../utils/history'; +import { + stringify, + validate, + getInitialPageState, + getInitialProductState, +} from '../utils/history'; import { useGroups } from './useGroups'; import { useConfig } from './useConfig'; -const getInitialPageState = ( - groups: Group[], - queryParams?: Partial, -) => { - // The group is written initially to queryParams as null, since user groups are asynchronously - // loaded. We preserve nulls in queryParams for other parameters where null is meaningful; for - // group, avoid overwriting the default with null after groups are loaded. - const { group, ...otherParams } = queryParams || {}; - return { - ...getDefaultPageFilters(groups), - ...otherParams, - ...(group ? { group: group } : {}), - }; -}; - export type FilterContextProps = { pageFilters: PageFilters; productFilters: ProductFilters; - setPageFilters: Dispatch>; - setProductFilters: Dispatch>; + setPageFilters: Dispatch>>; + setProductFilters: Dispatch>>; }; export type MapFiltersToProps = (props: FilterContextProps) => T; -export type FilterProviderProps = { - children: ReactNode; -}; - export const FilterContext = React.createContext< FilterContextProps | undefined >(undefined); -export const FilterProvider = ({ children }: FilterProviderProps) => { +export const FilterProvider = ({ children }: PropsWithChildren<{}>) => { const config = useConfig(); const navigate = useNavigate(); const location = useLocation(); - const queryParams = useQueryParams(); - const qsRef = useRef(''); const groups = useGroups(); - - const defaultProductFilters = config.products.map(product => ({ - productType: product.kind, - duration: Duration.P1M, - })); - - const getInitialProductState = (productFilters?: ProductFilters) => { - if (!productFilters) return defaultProductFilters; - return defaultProductFilters.map(product => { - return ( - productFilters.find( - param => param.productType === product.productType, - ) || product - ); - }); - }; - - const [productFilters, setProductFilters] = useState( - getInitialProductState(queryParams.productFilters), - ); - const [pageFilters, setPageFilters] = useState( - getInitialPageState(groups, queryParams.pageFilters), + const [error, setError] = useState>(null); + const [pageFilters, setPageFilters] = useState>(null); + const [productFilters, setProductFilters] = useState>( + null, ); - // TODO: Figure out why pageFilters doesn't get updated by the above when groups are loaded. useEffect(() => { - const initialState = getInitialPageState(groups, queryParams.pageFilters); - const defaultMetric = config.metrics.find(m => m.default); - setPageFilters({ ...initialState, metric: defaultMetric?.kind ?? null }); + async function setPageFiltersFromLocation() { + try { + // strip extraneous parameters, validate and transform + const queryParams = await validate(location.search); + const defaultMetric = config.metrics.find(m => m.default)?.kind ?? null; + + // Group or project parameters should override defaults + const initialPageState = getInitialPageState(groups, queryParams); + const initialProductState = getInitialProductState(config); + + setProductFilters(initialProductState); + setPageFilters({ ...initialPageState, metric: defaultMetric }); + } catch (e) { + setError(e); + } + } + + setPageFiltersFromLocation(); }, [groups]); // eslint-disable-line react-hooks/exhaustive-deps useEffect(() => { - const queryString = stringify({ ...pageFilters, products: productFilters }); - if (queryString === qsRef.current) return; - qsRef.current = queryString; - // TODO Remove workaround once issue is resolved in react-router - // (https://github.com/ReactTraining/react-router/issues/7496) - // navigate({ ...location, search: queryString }); - navigate({ ...location, search: `?${queryString}` }); - }, [pageFilters, productFilters]); // eslint-disable-line react-hooks/exhaustive-deps + function setLocationFromPageFilters(filters: PageFilters) { + const queryString = stringify({ + group: filters.group, + ...(filters.project ? { project: filters.project } : {}), + }); + // TODO Remove workaround once issue is resolved in react-router + // (https://github.com/ReactTraining/react-router/issues/7496) + // navigate({ ...location, search: queryString }); + navigate({ ...location, search: `?${queryString}` }); + } + + if (pageFilters) { + setLocationFromPageFilters(pageFilters); + } + }, [pageFilters]); // eslint-disable-line react-hooks/exhaustive-deps + + if (error) { + return {error.message}; + } + + // Wait for filters to load + if (!pageFilters || !productFilters) { + return null; + } return ( {children} @@ -132,17 +116,7 @@ export const FilterProvider = ({ children }: FilterProviderProps) => { export function useFilters(mapFiltersToProps: MapFiltersToProps): T { const context = useContext(FilterContext); - - if (!context) { - assertNever(); - } - - return mapFiltersToProps({ - pageFilters: context.pageFilters, - productFilters: context.productFilters, - setPageFilters: context.setPageFilters, - setProductFilters: context.setProductFilters, - }); + return context ? mapFiltersToProps(context) : assertNever(); } function assertNever(): never { diff --git a/plugins/cost-insights/src/hooks/useGroups.tsx b/plugins/cost-insights/src/hooks/useGroups.tsx index 02c32e7920..8f11c88e60 100644 --- a/plugins/cost-insights/src/hooks/useGroups.tsx +++ b/plugins/cost-insights/src/hooks/useGroups.tsx @@ -14,11 +14,17 @@ * limitations under the License. */ -import React, { ReactNode, useContext, useEffect, useState } from 'react'; +import React, { + PropsWithChildren, + useContext, + useEffect, + useState, +} from 'react'; +import { Alert } from '@material-ui/lab'; import { useApi, identityApiRef } from '@backstage/core'; import { costInsightsApiRef } from '../api'; import { MapLoadingToProps, useLoading } from './useLoading'; -import { DefaultLoadingAction, Group } from '../types'; +import { DefaultLoadingAction, Group, Maybe } from '../types'; type GroupsProviderLoadingProps = { dispatchLoadingGroups: (isLoading: boolean) => void; @@ -35,33 +41,41 @@ type GroupsContextProps = { groups: Group[]; }; -export type GroupsProviderProps = { - children: ReactNode; -}; +export const GroupsContext = React.createContext< + GroupsContextProps | undefined +>(undefined); -export const GroupsContext = React.createContext({ - groups: [], -}); - -export const GroupsProvider = ({ children }: GroupsProviderProps) => { +export const GroupsProvider = ({ children }: PropsWithChildren<{}>) => { const userId = useApi(identityApiRef).getUserId(); const client = useApi(costInsightsApiRef); + const [error, setError] = useState>(null); const { dispatchLoadingGroups } = useLoading(mapLoadingToProps); - const [groups, setGroups] = useState([]); + const [groups, setGroups] = useState>(null); useEffect(() => { dispatchLoadingGroups(true); async function getUserGroups() { - const g = await client.getUserGroups(userId); - setGroups(g); - dispatchLoadingGroups(false); + try { + const g = await client.getUserGroups(userId); + setGroups(g); + } catch (e) { + setError(e); + } finally { + dispatchLoadingGroups(false); + } } getUserGroups(); }, [userId, client]); // eslint-disable-line react-hooks/exhaustive-deps + if (error) { + return {error.message}; + } + + if (!groups) return null; + return ( {children} @@ -70,13 +84,8 @@ export const GroupsProvider = ({ children }: GroupsProviderProps) => { }; export function useGroups(): Group[] { - const { groups } = useContext(GroupsContext); - - if (!groups) { - assertNever(); - } - - return groups; + const context = useContext(GroupsContext); + return context ? context.groups : assertNever(); } function assertNever(): never { diff --git a/plugins/cost-insights/src/hooks/useQueryParams.tsx b/plugins/cost-insights/src/hooks/useQueryParams.tsx deleted file mode 100644 index 1e6ce8f0fd..0000000000 --- a/plugins/cost-insights/src/hooks/useQueryParams.tsx +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Copyright 2020 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { useLocation } from 'react-router-dom'; -import { Location } from 'history'; -import { PageFilters, ProductFilters } from '../types'; -import { parse } from '../utils/history'; - -export type FilterParams = { - pageFilters?: Partial; - productFilters?: ProductFilters; -}; - -export function useQueryParams(): FilterParams { - const location: Location = useLocation(); - const { products: productFilters, ...pageFilters } = parse(location.search); - - return { - productFilters: productFilters, - pageFilters: pageFilters, - }; -} diff --git a/plugins/cost-insights/src/types/Filters.ts b/plugins/cost-insights/src/types/Filters.ts index f90b9b4c78..b1fbbb2848 100644 --- a/plugins/cost-insights/src/types/Filters.ts +++ b/plugins/cost-insights/src/types/Filters.ts @@ -27,8 +27,6 @@ export interface PageFilters { export type ProductFilters = Array; -export type QueryParams = PageFilters & { products: ProductFilters }; - export interface ProductPeriod { duration: Duration; productType: string; diff --git a/plugins/cost-insights/src/utils/history.test.ts b/plugins/cost-insights/src/utils/history.test.ts new file mode 100644 index 0000000000..e4310e13c6 --- /dev/null +++ b/plugins/cost-insights/src/utils/history.test.ts @@ -0,0 +1,80 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { validate, getInitialPageState } from './history'; + +describe('getInitialPageState', () => { + describe('groups', () => { + it('should set defaults if params or group is not provided', () => { + const initialState = getInitialPageState([]); + expect(initialState.group).toBe(null); + }); + + it('should set defaults if a group is fetched but no group is present on query params', () => { + const initialState = getInitialPageState([{ id: 'group' }]); + expect(initialState.group).toMatch(/group/); + }); + + it('group param should always override fetched group', () => { + const initialState = getInitialPageState( + [{ id: 'group' }, { id: 'second-group' }], + { group: 'other-group' }, + ); + expect(initialState.group).toMatch(/other-group/); + }); + + it('first group should be set as default group if user belongs to multiple groups', () => { + const initialState = getInitialPageState([ + { id: 'group' }, + { id: 'other-group' }, + ]); + expect(initialState.group).toMatch(/group/); + }); + }); + + describe('projects', () => { + it("should set defaults if project param doesn't exist", () => { + const initialState = getInitialPageState([], {}); + expect(initialState.project).toBeNull(); + }); + + it('should override defaults if project param exists', () => { + const initialState = getInitialPageState([], { project: 'some-project' }); + expect(initialState.project).toMatch(/some-project/); + }); + }); +}); + +describe.each` + params | expected + ${''} | ${{}} + ${'?foo=bar'} | ${{}} + ${'?project'} | ${{ project: null }} + ${'?group=some-group'} | ${{ group: 'some-group' }} + ${'?group=some-group&project'} | ${{ group: 'some-group', project: null }} + ${'?group=some-group&project=some-project'} | ${{ group: 'some-group', project: 'some-project' }} + ${'?group=some-group&project=some-project&foo=bar'} | ${{ group: 'some-group', project: 'some-project' }} +`('validate', ({ params, expected }) => { + it(`should validate ${params}`, async () => { + const pageFilters = await validate(params); + expect(pageFilters).toMatchObject(expected); + }); +}); + +describe('invalidate', () => { + it("should throw an error if param values don't match schema", async () => { + await expect(validate('?group')).rejects.toThrowError(); + }); +}); diff --git a/plugins/cost-insights/src/utils/history.ts b/plugins/cost-insights/src/utils/history.ts index 71e8e2c219..90df574acf 100644 --- a/plugins/cost-insights/src/utils/history.ts +++ b/plugins/cost-insights/src/utils/history.ts @@ -15,9 +15,44 @@ */ import qs from 'qs'; -import { QueryParams } from '../types'; +import * as yup from 'yup'; +import { Group, PageFilters, getDefaultPageFilters, Duration } from '../types'; +import { ConfigContextProps } from '../hooks/useConfig'; -export const stringify = (queryParams: Partial) => +const schema = yup + .object() + .shape({ + group: yup.string(), + project: yup.string().nullable(), + }) + .required(); + +export const stringify = (queryParams: Partial) => qs.stringify(queryParams, { strictNullHandling: true }); -export const parse = (queryString: string): Partial => + +export const parse = (queryString: string): Partial => qs.parse(queryString, { ignoreQueryPrefix: true, strictNullHandling: true }); + +export const validate = (queryString: string): Promise => { + return schema.validate(parse(queryString), { + stripUnknown: true, + strict: true, + }) as Promise; +}; + +export const getInitialPageState = ( + groups: Group[], + queryParams: Partial = {}, +) => { + return { + ...getDefaultPageFilters(groups), + ...(queryParams.project ? { project: queryParams.project } : {}), + ...(queryParams.group ? { group: queryParams.group } : {}), + }; +}; + +export const getInitialProductState = (config: ConfigContextProps) => + config.products.map(product => ({ + productType: product.kind, + duration: Duration.P30D, + })); diff --git a/plugins/cost-insights/src/utils/tests.tsx b/plugins/cost-insights/src/utils/tests.tsx index 807232555b..552c524afe 100644 --- a/plugins/cost-insights/src/utils/tests.tsx +++ b/plugins/cost-insights/src/utils/tests.tsx @@ -16,6 +16,7 @@ import React, { Dispatch, ReactNode, SetStateAction } from 'react'; import { getDefaultPageFilters, + Maybe, PageFilters, ProductFilters, Group, @@ -29,8 +30,8 @@ import { MockProductFilters } from './mockData'; export const MockGroups: Group[] = [{ id: 'tech' }, { id: 'mock-group' }]; type MockFilterProviderProps = { - setPageFilters: Dispatch>; - setProductFilters: Dispatch>; + setPageFilters: Dispatch>>; + setProductFilters: Dispatch>>; children: ReactNode; };