From 74268e36790efef96516c41fe675a033ce380453 Mon Sep 17 00:00:00 2001 From: Peter Makowski Date: Wed, 19 Oct 2022 10:37:24 +0200 Subject: [PATCH] fix(machines): repeated machine.count API calls --- .../MachinesHeader/MachinesHeader.test.tsx | 2 +- .../node/MachinesHeader/MachinesHeader.tsx | 9 +-- .../MachineListHeader.test.tsx | 24 +++----- .../MachineListHeader/MachineListHeader.tsx | 21 ++++--- src/app/pools/views/Pools.tsx | 3 + src/app/store/machine/utils/hooks.test.tsx | 44 ++++++++++++++ src/app/store/machine/utils/hooks.ts | 59 +++++++++++++------ src/app/store/machine/utils/search.test.ts | 8 +++ src/app/store/machine/utils/search.ts | 6 ++ .../tags/components/TagsHeader/TagsHeader.tsx | 3 + 10 files changed, 130 insertions(+), 49 deletions(-) diff --git a/src/app/base/components/node/MachinesHeader/MachinesHeader.test.tsx b/src/app/base/components/node/MachinesHeader/MachinesHeader.test.tsx index b5d13a9a45..d03de7ccce 100644 --- a/src/app/base/components/node/MachinesHeader/MachinesHeader.test.tsx +++ b/src/app/base/components/node/MachinesHeader/MachinesHeader.test.tsx @@ -77,7 +77,7 @@ describe("MachinesHeader", () => { initialEntries={[{ pathname: "/machines", key: "testKey" }]} > - + diff --git a/src/app/base/components/node/MachinesHeader/MachinesHeader.tsx b/src/app/base/components/node/MachinesHeader/MachinesHeader.tsx index c363e216a5..8236989962 100644 --- a/src/app/base/components/node/MachinesHeader/MachinesHeader.tsx +++ b/src/app/base/components/node/MachinesHeader/MachinesHeader.tsx @@ -8,20 +8,21 @@ import { matchPath, Link } from "react-router-dom-v5-compat"; import type { SectionHeaderProps } from "app/base/components/SectionHeader"; import SectionHeader from "app/base/components/SectionHeader"; import urls from "app/base/urls"; -import { useFetchMachineCount } from "app/store/machine/utils/hooks"; import { actions as resourcePoolActions } from "app/store/resourcepool"; import resourcePoolSelectors from "app/store/resourcepool/selectors"; import { actions as tagActions } from "app/store/tag"; import tagSelectors from "app/store/tag/selectors"; -type Props = SectionHeaderProps; +type Props = SectionHeaderProps & { machineCount: number }; -export const MachinesHeader = (props: Props): JSX.Element => { +export const MachinesHeader = ({ + machineCount, + ...props +}: Props): JSX.Element => { const dispatch = useDispatch(); const location = useLocation(); const poolCount = useSelector(resourcePoolSelectors.count); const tagCount = useSelector(tagSelectors.count); - const { machineCount } = useFetchMachineCount(); useEffect(() => { dispatch(resourcePoolActions.fetch()); diff --git a/src/app/machines/views/MachineList/MachineListHeader/MachineListHeader.test.tsx b/src/app/machines/views/MachineList/MachineListHeader/MachineListHeader.test.tsx index 716e3ed0d7..15df14510c 100644 --- a/src/app/machines/views/MachineList/MachineListHeader/MachineListHeader.test.tsx +++ b/src/app/machines/views/MachineList/MachineListHeader/MachineListHeader.test.tsx @@ -65,7 +65,11 @@ describe("MachineListHeader", () => { }); it("displays a loader if machines have not loaded", () => { - state.machine.counts["mocked-nanoid-3"] = machineStateCountFactory({ + state.machine.selectedMachines = { + groups: ["admin"], + grouping: FetchGroupKey.Owner, + }; + state.machine.counts["mocked-nanoid-2"] = machineStateCountFactory({ loading: true, }); const store = mockStore(state); @@ -89,7 +93,7 @@ describe("MachineListHeader", () => { }); it("displays a machine count if machines have loaded", () => { - state.machine.counts["mocked-nanoid-2"] = machineStateCountFactory({ + state.machine.counts["mocked-nanoid-1"] = machineStateCountFactory({ count: 2, loaded: true, }); @@ -121,10 +125,6 @@ describe("MachineListHeader", () => { grouping: FetchGroupKey.Owner, }; state.machine.counts["mocked-nanoid-2"] = machineStateCountFactory({ - count: 10, - loaded: true, - }); - state.machine.counts["mocked-nanoid-3"] = machineStateCountFactory({ loading: true, }); renderWithBrowserRouter( @@ -188,10 +188,6 @@ describe("MachineListHeader", () => { grouping: FetchGroupKey.Owner, }; state.machine.counts["mocked-nanoid-2"] = machineStateCountFactory({ - count: 10, - loaded: true, - }); - state.machine.counts["mocked-nanoid-3"] = machineStateCountFactory({ count: 2, loaded: true, }); @@ -214,10 +210,6 @@ describe("MachineListHeader", () => { grouping: FetchGroupKey.Owner, }; state.machine.counts["mocked-nanoid-2"] = machineStateCountFactory({ - count: 10, - loaded: true, - }); - state.machine.counts["mocked-nanoid-3"] = machineStateCountFactory({ count: 2, loaded: true, }); @@ -235,11 +227,11 @@ describe("MachineListHeader", () => { it("displays a message when all machines have been selected", () => { state.machine.selectedMachines = { filter: {} }; - state.machine.counts["mocked-nanoid-2"] = machineStateCountFactory({ + state.machine.counts["mocked-nanoid-1"] = machineStateCountFactory({ count: 10, loaded: true, }); - state.machine.counts["mocked-nanoid-3"] = machineStateCountFactory({ + state.machine.counts["mocked-nanoid-2"] = machineStateCountFactory({ count: 10, loaded: true, }); diff --git a/src/app/machines/views/MachineList/MachineListHeader/MachineListHeader.tsx b/src/app/machines/views/MachineList/MachineListHeader/MachineListHeader.tsx index 8f7dcf8acc..6b97664f5d 100644 --- a/src/app/machines/views/MachineList/MachineListHeader/MachineListHeader.tsx +++ b/src/app/machines/views/MachineList/MachineListHeader/MachineListHeader.tsx @@ -49,15 +49,17 @@ export const MachineListHeader = ({ "machineViewTagsSeen", false ); - // Get the count of all machines that match the current filters. - const { machineCount } = useFetchMachineCount( - FilterMachineItems.parseFetchFilters(searchFilter) - ); - const { selectedCount, selectedCountLoading } = useMachineSelectedCount( - FilterMachineItems.parseFetchFilters(searchFilter) - ); + const filter = FilterMachineItems.parseFetchFilters(searchFilter); + // Get the count of all machines + const { machineCount: allMachineCount } = useFetchMachineCount(); + // Get the count of all machines that match the current filter + const { machineCount: availableMachineCount } = useFetchMachineCount(filter, { + isEnabled: FilterMachineItems.isNonEmptyFilter(searchFilter), + }); + // Get the count of selected machines that match the current filter + const { selectedCount, selectedCountLoading } = + useMachineSelectedCount(filter); const previousSelectedCount = usePrevious(selectedCount); - const selectedMachines = useSelector(machineSelectors.selectedMachines); useEffect(() => { @@ -133,9 +135,10 @@ export const MachineListHeader = ({ /> ) } + machineCount={allMachineCount} subtitle={ diff --git a/src/app/pools/views/Pools.tsx b/src/app/pools/views/Pools.tsx index 36ad823bf8..408530f4fb 100644 --- a/src/app/pools/views/Pools.tsx +++ b/src/app/pools/views/Pools.tsx @@ -9,10 +9,12 @@ import urls from "app/base/urls"; import NotFound from "app/base/views/NotFound"; import PoolAdd from "app/pools/views/PoolAdd"; import PoolEdit from "app/pools/views/PoolEdit"; +import { useFetchMachineCount } from "app/store/machine/utils/hooks"; import { getRelativeRoute } from "app/utils"; const Pools = (): JSX.Element => { const base = urls.pools.index; + const { machineCount } = useFetchMachineCount(); return (
{ Add pool , ]} + machineCount={machineCount} /> } > diff --git a/src/app/store/machine/utils/hooks.test.tsx b/src/app/store/machine/utils/hooks.test.tsx index 392bcf9ecf..7857b6e86f 100644 --- a/src/app/store/machine/utils/hooks.test.tsx +++ b/src/app/store/machine/utils/hooks.test.tsx @@ -149,6 +149,28 @@ describe("machine hook utils", () => { expect(getDispatches()).toHaveLength(1); }); + it("fetches if isEnabled changes back to true", async () => { + const store = mockStore(state); + const { rerender } = renderHook( + (queryOptions: UseFetchQueryOptions) => + useFetchMachineCount({ hostname: "spotted-quoll" }, queryOptions), + { + initialProps: { isEnabled: true }, + wrapper: generateWrapper(store), + } + ); + const expectedActionType = machineActions.count("mocked-nanoid-1").type; + const getDispatches = () => + store + .getActions() + .filter((action) => action.type === expectedActionType); + expect(getDispatches()).toHaveLength(1); + rerender({ isEnabled: false }); + expect(getDispatches()).toHaveLength(1); + rerender({ isEnabled: true }); + expect(getDispatches()).toHaveLength(2); + }); + it("returns the machine count", async () => { jest.restoreAllMocks(); jest.spyOn(reduxToolkit, "nanoid").mockReturnValueOnce("mocked-nanoid"); @@ -345,6 +367,28 @@ describe("machine hook utils", () => { expect(getDispatches()).toHaveLength(1); }); + it("fetches again if isEnabled changes back to true", async () => { + const store = mockStore(state); + const { rerender } = renderHook( + (queryOptions: UseFetchQueryOptions) => + useFetchMachines(undefined, queryOptions), + { + initialProps: { isEnabled: true }, + wrapper: generateWrapper(store), + } + ); + const expectedActionType = machineActions.fetch("mocked-nanoid-1").type; + const getDispatches = () => + store + .getActions() + .filter((action) => action.type === expectedActionType); + expect(getDispatches()).toHaveLength(1); + rerender({ isEnabled: false }); + expect(getDispatches()).toHaveLength(1); + rerender({ isEnabled: true }); + expect(getDispatches()).toHaveLength(2); + }); + it("does not fetch again if the options haven't changed including empty objects", () => { const store = mockStore(state); const { rerender } = renderHook( diff --git a/src/app/store/machine/utils/hooks.ts b/src/app/store/machine/utils/hooks.ts index 1c8769a90b..b4ea440210 100644 --- a/src/app/store/machine/utils/hooks.ts +++ b/src/app/store/machine/utils/hooks.ts @@ -255,11 +255,13 @@ export const useSelectedMachinesActionsDispatch = ({ }; export const useMachineSelectedCount = ( - filters?: FetchFilters | null + filters?: FetchFilters | null, + queryOptions?: UseFetchQueryOptions ): { selectedCount: number; selectedCountLoading: boolean; } => { + const { isEnabled } = queryOptions || { isEnabled: true }; let selectedState = useSelector(machineSelectors.selectedMachines); let selectedCount = 0; // Shallow clone the selected state so that object can be modified. @@ -274,11 +276,13 @@ export const useMachineSelectedCount = ( } // Get the count of machines in selected groups or filters. const filtersFromSelected = selectedToFilters(selectedMachines); - // refactor to use a separate count for groups and separate for items const { machineCount: fetchedSelectedCount, machineCountLoading: selectedLoading, - } = useFetchMachineCount({ ...filters, ...filtersFromSelected }); + } = useFetchMachineCount( + { ...filters, ...filtersFromSelected }, + { isEnabled: isEnabled && filtersFromSelected !== null } + ); // Only add the count if there are filters as sending `null` filters // to the count API will return a count of all machines. if (filtersFromSelected) { @@ -388,13 +392,23 @@ export const useFetchMachineCount = ( useEffect(() => { // undefined, null and {} are all equivalent i.e. no filters so compare the // current and previous filters using an empty object if the filters are falsy. - if ( - (isEnabled && !fastDeepEqual(filters || {}, previousFilters || {})) || - (isEnabled && !callId) - ) { - setCallId(nanoid()); + if (isEnabled) { + if ( + !fastDeepEqual(filters || {}, previousFilters || {}) || + !callId || + isEnabled !== previousIsEnabled + ) { + setCallId(nanoid()); + } } - }, [callId, dispatch, filters, previousFilters, isEnabled]); + }, [ + callId, + dispatch, + filters, + previousFilters, + isEnabled, + previousIsEnabled, + ]); useEffect(() => { return () => { @@ -554,19 +568,26 @@ export const useFetchMachines = ( useEffect(() => { // undefined, null and {} are all equivalent i.e. no filters so compare the // current and previous filters using an empty object if the filters are falsy. - if ( - (isEnabled && !fastDeepEqual(options || {}, previousOptions || {})) || - !callId - ) { - setCallId(nanoid()); + if (isEnabled) { + if ( + !fastDeepEqual(options || {}, previousOptions || {}) || + !callId || + isEnabled !== previousIsEnabled + ) { + setCallId(nanoid()); + } } - }, [callId, options, previousOptions, isEnabled]); + }, [ + callId, + dispatch, + options, + previousOptions, + isEnabled, + previousIsEnabled, + ]); useEffect(() => { - if ( - (isEnabled && callId && callId !== previousCallId) || - (isEnabled !== previousIsEnabled && callId) - ) { + if (isEnabled && callId && callId !== previousCallId) { dispatch( machineActions.fetch( callId, diff --git a/src/app/store/machine/utils/search.test.ts b/src/app/store/machine/utils/search.test.ts index 82984cf878..6118997c48 100644 --- a/src/app/store/machine/utils/search.test.ts +++ b/src/app/store/machine/utils/search.test.ts @@ -218,3 +218,11 @@ describe("client side search", () => { }); }); }); + +it("isNonEmptyFilter returns false for empty search string", () => { + expect(FilterMachineItems.isNonEmptyFilter("")).toBe(false); +}); + +it("isNonEmptyFilter returns true for defined search string", () => { + expect(FilterMachineItems.isNonEmptyFilter("status:(=broken)")).toBe(true); +}); diff --git a/src/app/store/machine/utils/search.ts b/src/app/store/machine/utils/search.ts index de1dddf6d7..c93f03b0cd 100644 --- a/src/app/store/machine/utils/search.ts +++ b/src/app/store/machine/utils/search.ts @@ -1,3 +1,5 @@ +import fastDeepEqual from "fast-deep-equal"; + import { MachineMeta } from "app/store/machine/types"; import type { Machine, FetchFilters } from "app/store/machine/types"; import type { Tag } from "app/store/tag/types"; @@ -242,6 +244,10 @@ class FilterMachineHandlers extends FilterHandlers { ); }; + isNonEmptyFilter = (filter: string) => { + return !fastDeepEqual(this.parseFetchFilters(filter), {}); + }; + /** * Merge a new filter into the existing list of filters. This is required * because multiple DSL filter names can be mapped to the same API key name. diff --git a/src/app/tags/components/TagsHeader/TagsHeader.tsx b/src/app/tags/components/TagsHeader/TagsHeader.tsx index 7af0ec1615..9e7ce687cc 100644 --- a/src/app/tags/components/TagsHeader/TagsHeader.tsx +++ b/src/app/tags/components/TagsHeader/TagsHeader.tsx @@ -1,6 +1,7 @@ import { Button } from "@canonical/react-components"; import MachinesHeader from "app/base/components/node/MachinesHeader"; +import { useFetchMachineCount } from "app/store/machine/utils/hooks"; import TagHeaderForms from "app/tags/components/TagsHeader/TagHeaderForms"; import { TagHeaderViews } from "app/tags/constants"; import type { TagHeaderContent, TagSetHeaderContent } from "app/tags/types"; @@ -37,6 +38,7 @@ export const TagsHeader = ({ setHeaderContent, tagViewState, }: Props): JSX.Element => { + const { machineCount } = useFetchMachineCount(); return ( ) } + machineCount={machineCount} title={getHeaderTitle(headerContent)} /> );