Merge pull request #3006 from spotify/ryanv/simpler-query-params

Simpler query params

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"

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`

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';

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<PageFilters>,
-) => {
-  // 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<SetStateAction<PageFilters>>;
-  setProductFilters: Dispatch<SetStateAction<ProductFilters>>;
+  setPageFilters: Dispatch<SetStateAction<Maybe<PageFilters>>>;
+  setProductFilters: Dispatch<SetStateAction<Maybe<ProductFilters>>>;
 };
 
 export type MapFiltersToProps<T> = (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<Maybe<Error>>(null);
+  const [pageFilters, setPageFilters] = useState<Maybe<PageFilters>>(null);
+  const [productFilters, setProductFilters] = useState<Maybe<ProductFilters>>(
+    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 <Alert severity="error">{error.message}</Alert>;
+  }
+
+  // Wait for filters to load
+  if (!pageFilters || !productFilters) {
+    return null;
+  }
 
   return (
     <FilterContext.Provider
-      value={{
-        pageFilters: pageFilters,
-        productFilters: productFilters,
-        setPageFilters: setPageFilters,
-        setProductFilters: setProductFilters,
-      }}
+      value={{ pageFilters, productFilters, setPageFilters, setProductFilters }}
     >
       {children}
     </FilterContext.Provider>
@@ -132,17 +116,7 @@ export const FilterProvider = ({ children }: FilterProviderProps) => {
 
 export function useFilters<T>(mapFiltersToProps: MapFiltersToProps<T>): 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 {

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<GroupsContextProps>({
-  groups: [],
-});
-
-export const GroupsProvider = ({ children }: GroupsProviderProps) => {
+export const GroupsProvider = ({ children }: PropsWithChildren<{}>) => {
   const userId = useApi(identityApiRef).getUserId();
   const client = useApi(costInsightsApiRef);
+  const [error, setError] = useState<Maybe<Error>>(null);
   const { dispatchLoadingGroups } = useLoading(mapLoadingToProps);
 
-  const [groups, setGroups] = useState<Group[]>([]);
+  const [groups, setGroups] = useState<Maybe<Group[]>>(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 <Alert severity="error">{error.message}</Alert>;
+  }
+
+  if (!groups) return null;
+
   return (
     <GroupsContext.Provider value={{ groups: groups }}>
       {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 {

plugins/cost-insights/src/hooks/useQueryParams.tsx

@@ -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<PageFilters>;
-  productFilters?: ProductFilters;
-};
-
-export function useQueryParams(): FilterParams {
-  const location: Location = useLocation();
-  const { products: productFilters, ...pageFilters } = parse(location.search);
-
-  return {
-    productFilters: productFilters,
-    pageFilters: pageFilters,
-  };
-}

plugins/cost-insights/src/types/Filters.ts

@@ -27,8 +27,6 @@ export interface PageFilters {
 
 export type ProductFilters = Array<ProductPeriod>;
 
-export type QueryParams = PageFilters & { products: ProductFilters };
-
 export interface ProductPeriod {
   duration: Duration;
   productType: string;

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();
+  });
+});

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<QueryParams>) =>
+const schema = yup
+  .object()
+  .shape({
+    group: yup.string(),
+    project: yup.string().nullable(),
+  })
+  .required();
+
+export const stringify = (queryParams: Partial<PageFilters>) =>
   qs.stringify(queryParams, { strictNullHandling: true });
-export const parse = (queryString: string): Partial<QueryParams> =>
+
+export const parse = (queryString: string): Partial<PageFilters> =>
   qs.parse(queryString, { ignoreQueryPrefix: true, strictNullHandling: true });
+
+export const validate = (queryString: string): Promise<PageFilters> => {
+  return schema.validate(parse(queryString), {
+    stripUnknown: true,
+    strict: true,
+  }) as Promise<PageFilters>;
+};
+
+export const getInitialPageState = (
+  groups: Group[],
+  queryParams: Partial<PageFilters> = {},
+) => {
+  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,
+  }));

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<SetStateAction<PageFilters>>;
-  setProductFilters: Dispatch<SetStateAction<ProductFilters>>;
+  setPageFilters: Dispatch<SetStateAction<Maybe<PageFilters>>>;
+  setProductFilters: Dispatch<SetStateAction<Maybe<ProductFilters>>>;
   children: ReactNode;
 };