From 4248afb99442c063ef2dfd8773005a06ce504af8 Mon Sep 17 00:00:00 2001 From: Banks Nussman Date: Mon, 6 Mar 2023 19:27:44 -0500 Subject: [PATCH 1/2] React Query-ify Firewall Devices --- .../Devices/FirewallDevicesTable.tsx | 31 ++----- .../Devices/FirewallLinodesLanding.tsx | 79 ++++++---------- .../Devices/RemoveDeviceDialog.tsx | 91 +++++++++---------- .../Firewalls/FirewallLanding/FirewallRow.tsx | 16 +--- .../LinodeRescue/RescueDescription.tsx | 2 +- .../manager/src/hooks/useFirewallDevices.ts | 55 ----------- packages/manager/src/queries/base.ts | 4 +- packages/manager/src/queries/firewalls.ts | 59 ++++++++++-- .../manager/src/queries/linodeFirewalls.ts | 14 --- packages/manager/src/queries/linodes.ts | 9 ++ 10 files changed, 150 insertions(+), 210 deletions(-) delete mode 100644 packages/manager/src/hooks/useFirewallDevices.ts delete mode 100644 packages/manager/src/queries/linodeFirewalls.ts diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallDevicesTable.tsx b/packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallDevicesTable.tsx index 3f605aef39a..fd7ed107201 100644 --- a/packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallDevicesTable.tsx +++ b/packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallDevicesTable.tsx @@ -1,7 +1,6 @@ import { FirewallDevice } from '@linode/api-v4/lib/firewalls/types'; import { APIError } from '@linode/api-v4/lib/types'; import * as React from 'react'; -import { compose } from 'recompose'; import TableBody from 'src/components/core/TableBody'; import TableHead from 'src/components/core/TableHead'; import OrderBy from 'src/components/OrderBy'; @@ -19,27 +18,16 @@ interface Props { error?: APIError[]; loading: boolean; disabled: boolean; - lastUpdated: number; - triggerRemoveDevice: (deviceID: number, deviceLabel: string) => void; + triggerRemoveDevice: (deviceID: number) => void; } -type CombinedProps = Props; +const FirewallTable = (props: Props) => { + const { devices, error, loading, disabled, triggerRemoveDevice } = props; -const FirewallTable: React.FC = (props) => { - const { - devices, - error, - lastUpdated, - loading, - disabled, - triggerRemoveDevice, - } = props; - - const _error = - error && lastUpdated === 0 - ? // @todo change to Devices or make dynamic when NBs are possible as Devices - getAPIErrorOrDefault(error, 'Unable to retrieve Linodes') - : undefined; + const _error = error + ? // @todo change to Devices or make dynamic when NBs are possible as Devices + getAPIErrorOrDefault(error, 'Unable to retrieve Linodes') + : undefined; return ( @@ -72,9 +60,8 @@ const FirewallTable: React.FC = (props) => { {paginatedAndOrderedData.map((thisDevice) => ( = (props) => { ); }; -export default compose(React.memo)(FirewallTable); +export default React.memo(FirewallTable); diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallLinodesLanding.tsx b/packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallLinodesLanding.tsx index e7b09f3f698..0c4abf1d249 100644 --- a/packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallLinodesLanding.tsx +++ b/packages/manager/src/features/Firewalls/FirewallDetail/Devices/FirewallLinodesLanding.tsx @@ -11,12 +11,13 @@ import Notice from 'src/components/Notice'; import withLinodes, { Props as LinodesProps, } from 'src/containers/withLinodes.container'; -import { useDialog } from 'src/hooks/useDialog'; -import useFirewallDevices from 'src/hooks/useFirewallDevices'; -import { getAPIErrorOrDefault } from 'src/utilities/errorUtils'; import AddDeviceDrawer from './AddDeviceDrawer'; import FirewallDevicesTable from './FirewallDevicesTable'; import RemoveDeviceDialog from './RemoveDeviceDialog'; +import { + useAddFirewallDeviceMutation, + useAllFirewallDevicesQuery, +} from 'src/queries/firewalls'; const useStyles = makeStyles((theme: Theme) => ({ copy: { @@ -51,37 +52,23 @@ type CombinedProps = RouteComponentProps & Props & LinodesProps; const FirewallLinodesLanding: React.FC = (props) => { const { firewallID, firewallLabel, disabled } = props; const classes = useStyles(); - const { - devices, - requestDevices, - removeDevice, - addDevice, - } = useFirewallDevices(firewallID); - const deviceList = Object.values(devices.itemsById ?? {}); // Gives the devices as an array or [] if nothing is found - React.useEffect(() => { - if (devices.lastUpdated === 0 && !(devices.loading || devices.error.read)) { - requestDevices(); - } - }, [ - devices.error, - devices.lastUpdated, - devices.loading, - firewallID, - requestDevices, - ]); + const { data: devices, isLoading, error } = useAllFirewallDevicesQuery( + firewallID + ); + + const { mutateAsync: addDevice } = useAddFirewallDeviceMutation(firewallID); + + const [ + isRemoveDeviceDialogOpen, + setIsRemoveDeviceDialogOpen, + ] = React.useState(false); - const { - dialog, - openDialog, - closeDialog, - handleError, - submitDialog, - } = useDialog(removeDevice); + const [selectedDeviceId, setSelectedDeviceId] = React.useState(-1); - const _openDialog = React.useCallback(openDialog, [dialog, openDialog]); - const _closeDialog = React.useCallback(closeDialog, [dialog, closeDialog]); - const _submitDialog = React.useCallback(submitDialog, [dialog, submitDialog]); + const selectedDevice = devices?.find( + (device) => device.id === selectedDeviceId + ); const [addDeviceDrawerOpen, setDeviceDrawerOpen] = React.useState( false @@ -134,12 +121,6 @@ const FirewallLinodesLanding: React.FC = (props) => { setDeviceSubmitting(false); }; - const handleRemoveDevice = () => { - _submitDialog(dialog.entityID).catch((e) => - handleError(getAPIErrorOrDefault(e, 'Error removing Device')[0].reason) - ); - }; - return ( <> {disabled ? ( @@ -170,30 +151,30 @@ const FirewallLinodesLanding: React.FC = (props) => { { + setSelectedDeviceId(id); + setIsRemoveDeviceDialogOpen(true); + }} /> thisDevice.entity.id)} + currentDevices={devices?.map((device) => device.entity.id) ?? []} firewallLabel={firewallLabel} onClose={handleClose} addDevice={handleAddDevice} /> setIsRemoveDeviceDialogOpen(false)} + device={selectedDevice} firewallLabel={firewallLabel} + firewallId={firewallID} /> ); diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Devices/RemoveDeviceDialog.tsx b/packages/manager/src/features/Firewalls/FirewallDetail/Devices/RemoveDeviceDialog.tsx index 7007711f3f4..74446e1fe08 100644 --- a/packages/manager/src/features/Firewalls/FirewallDetail/Devices/RemoveDeviceDialog.tsx +++ b/packages/manager/src/features/Firewalls/FirewallDetail/Devices/RemoveDeviceDialog.tsx @@ -1,71 +1,66 @@ +import { FirewallDevice } from '@linode/api-v4'; import * as React from 'react'; import ActionsPanel from 'src/components/ActionsPanel'; import Button from 'src/components/Button'; import ConfirmationDialog from 'src/components/ConfirmationDialog'; import Typography from 'src/components/core/Typography'; -import Notice from 'src/components/Notice'; +import { useRemoveFirewallDeviceMutation } from 'src/queries/firewalls'; export interface Props { open: boolean; - error?: string; - loading: boolean; - deviceLabel: string; - firewallLabel: string; onClose: () => void; - onRemove: () => void; + device: FirewallDevice | undefined; + firewallLabel: string; + firewallId: number; } -export const RemoveDeviceDialog: React.FC = (props) => { - const { - deviceLabel, - error, - firewallLabel, - loading, - onClose, - onRemove, - open, - } = props; +export const RemoveDeviceDialog = (props: Props) => { + const { device, firewallLabel, firewallId, onClose, open } = props; + + const { mutateAsync, isLoading, error } = useRemoveFirewallDeviceMutation( + firewallId, + device?.id ?? -1 + ); + + const onDelete = async () => { + await mutateAsync(); + onClose(); + }; + return ( + + + + } > - {error && } - Are you sure you want to remove {deviceLabel} from {firewallLabel}? + Are you sure you want to remove {device?.entity.label} from{' '} + {firewallLabel}? ); }; -const renderActions = ( - loading: boolean, - onClose: () => void, - onDelete: () => void -) => { - return ( - - - - - ); -}; - export default React.memo(RemoveDeviceDialog); diff --git a/packages/manager/src/features/Firewalls/FirewallLanding/FirewallRow.tsx b/packages/manager/src/features/Firewalls/FirewallLanding/FirewallRow.tsx index 2091be2e428..9f82006188c 100644 --- a/packages/manager/src/features/Firewalls/FirewallLanding/FirewallRow.tsx +++ b/packages/manager/src/features/Firewalls/FirewallLanding/FirewallRow.tsx @@ -8,7 +8,7 @@ import { makeStyles, Theme } from 'src/components/core/styles'; import StatusIcon from 'src/components/StatusIcon'; import TableCell from 'src/components/TableCell'; import TableRow from 'src/components/TableRow'; -import useFirewallDevices from 'src/hooks/useFirewallDevices'; +import { useAllFirewallDevicesQuery } from 'src/queries/firewalls'; import capitalize from 'src/utilities/capitalize'; import ActionMenu, { ActionHandlers } from './FirewallActionMenu'; @@ -31,17 +31,7 @@ export const FirewallRow: React.FC = (props) => { const { id, label, status, rules, ...actionHandlers } = props; - const { - devices: { itemsById, error, loading, lastUpdated }, - requestDevices, - } = useFirewallDevices(id); - const devices = Object.values(itemsById); - - React.useEffect(() => { - if (lastUpdated === 0 && !(loading || error.read)) { - requestDevices(); - } - }, [error, lastUpdated, loading, requestDevices]); + const { data: devices, isLoading, error } = useAllFirewallDevicesQuery(id); const count = getCountOfRules(rules); @@ -63,7 +53,7 @@ export const FirewallRow: React.FC = (props) => { {getRuleString(count)} - {getLinodesCellString(devices, loading, error.read)} + {getLinodesCellString(devices ?? [], isLoading, error ?? undefined)} diff --git a/packages/manager/src/features/linodes/LinodesDetail/LinodeRescue/RescueDescription.tsx b/packages/manager/src/features/linodes/LinodesDetail/LinodeRescue/RescueDescription.tsx index c4e05e33943..97f44647fe1 100644 --- a/packages/manager/src/features/linodes/LinodesDetail/LinodeRescue/RescueDescription.tsx +++ b/packages/manager/src/features/linodes/LinodesDetail/LinodeRescue/RescueDescription.tsx @@ -4,7 +4,7 @@ import Typography from 'src/components/core/Typography'; import Link from 'src/components/Link'; import Notice from 'src/components/Notice'; import { lishLaunch } from 'src/features/Lish/lishUtils'; -import { useLinodeFirewalls } from 'src/queries/linodeFirewalls'; +import { useLinodeFirewalls } from 'src/queries/linodes'; const rescueDescription = { text: `If you suspect that your primary filesystem is corrupt, use the Linode Manager to boot your Linode into Rescue Mode. This is a safe environment for performing many system recovery and disk management tasks.`, diff --git a/packages/manager/src/hooks/useFirewallDevices.ts b/packages/manager/src/hooks/useFirewallDevices.ts deleted file mode 100644 index 63a15f141e3..00000000000 --- a/packages/manager/src/hooks/useFirewallDevices.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { - FirewallDevice, - FirewallDevicePayload, -} from '@linode/api-v4/lib/firewalls/types'; - -import { useDispatch, useSelector } from 'react-redux'; -import { ApplicationState } from 'src/store'; -import { - addFirewallDevice, - getAllFirewallDevices, - removeFirewallDevice, -} from 'src/store/firewalls/devices.requests'; -import { - EntityError, - MappedEntityState2, - ThunkDispatch, -} from 'src/store/types'; - -export interface UseDevicesProps { - devices: MappedEntityState2; - addDevice: (newDevice: FirewallDevicePayload) => Promise; - removeDevice: (deviceID: number) => Promise<{}>; - requestDevices: () => Promise; -} - -const defaultState: MappedEntityState2 = { - loading: false, - lastUpdated: 0, - itemsById: {}, - error: {}, - results: 0, -}; - -/** - * Hook for viewing and working with Firewall devices - */ -export const useFirewallDevices = (firewallID: number): UseDevicesProps => { - const dispatch: ThunkDispatch = useDispatch(); - const devices = useSelector( - (state: ApplicationState) => - state.firewallDevices[firewallID] ?? { ...defaultState } - ); - const requestDevices = () => - dispatch(getAllFirewallDevices({ firewallID })) - .then((response) => response.data) - .catch((_) => null); // Handle errors through Redux - const addDevice = (newDevice: FirewallDevicePayload) => - dispatch(addFirewallDevice({ firewallID, ...newDevice })); - const removeDevice = (deviceID: number) => - dispatch(removeFirewallDevice({ firewallID, deviceID })); - - return { devices, requestDevices, addDevice, removeDevice }; -}; - -export default useFirewallDevices; diff --git a/packages/manager/src/queries/base.ts b/packages/manager/src/queries/base.ts index 53fe5e4916c..9a436e2f014 100644 --- a/packages/manager/src/queries/base.ts +++ b/packages/manager/src/queries/base.ts @@ -159,7 +159,7 @@ export const itemInListMutationHandler = < }; export const itemInListCreationHandler = ( - queryKey: string + queryKey: QueryKey ): UseMutationOptions void> => { return { onSuccess: (createdEntity) => { @@ -181,7 +181,7 @@ export const itemInListDeletionHandler = < V extends { id?: number | string }, E = APIError[] >( - queryKey: string + queryKey: QueryKey ): UseMutationOptions void> => { return { onSuccess: (_, variables) => { diff --git a/packages/manager/src/queries/firewalls.ts b/packages/manager/src/queries/firewalls.ts index 84d78384282..67d0370aabc 100644 --- a/packages/manager/src/queries/firewalls.ts +++ b/packages/manager/src/queries/firewalls.ts @@ -1,16 +1,21 @@ +import { APIError } from '@linode/api-v4/lib/types'; +import { useMutation, useQuery } from 'react-query'; +import { getAll } from 'src/utilities/getAll'; import { + addFirewallDevice, createFirewall, CreateFirewallPayload, deleteFirewall, + deleteFirewallDevice, Firewall, + FirewallDevice, + FirewallDevicePayload, FirewallRules, + getFirewallDevices, getFirewalls, updateFirewall, updateFirewallRules as _updateFirewallRules, } from '@linode/api-v4/lib/firewalls'; -import { APIError } from '@linode/api-v4/lib/types'; -import { useMutation, useQuery } from 'react-query'; -import { getAll } from 'src/utilities/getAll'; import { mutationHandlers, listToItemsByID, @@ -19,8 +24,53 @@ import { deletionHandlers, queryClient, ItemsByID, + itemInListCreationHandler, } from './base'; +const queryKey = 'firewall'; + +export const useAllFirewallDevicesQuery = (id: number) => + useQuery([queryKey, id, 'devices'], () => + getAllFirewallDevices(id) + ); + +export const useAddFirewallDeviceMutation = (id: number) => + useMutation( + (data) => addFirewallDevice(id, data), + itemInListCreationHandler([queryKey, id, 'devices']) + ); + +export const useRemoveFirewallDeviceMutation = ( + firewallId: number, + deviceId: number +) => + useMutation<{}, APIError[]>( + () => deleteFirewallDevice(firewallId, deviceId), + { + onSuccess() { + queryClient.setQueryData( + [queryKey, firewallId, 'devices'], + (oldData) => { + return oldData?.filter((device) => device.id !== deviceId) ?? []; + } + ); + }, + } + ); + +const getAllFirewallDevices = ( + id: number, + passedParams: any = {}, + passedFilter: any = {} +) => + getAll((params, filter) => + getFirewallDevices( + id, + { ...params, ...passedParams }, + { ...filter, ...passedFilter } + ) + )().then((data) => data.data); + const getAllFirewallsRequest = () => getAll((passedParams, passedFilter) => getFirewalls(passedParams, passedFilter) @@ -31,8 +81,6 @@ const getAllPlainFirewallsRequest = () => getFirewalls(passedParams, passedFilter) )().then((data) => data.data); -const queryKey = 'queryFirewalls'; - export const useFirewallQuery = () => { return useQuery, APIError[]>( queryKey, @@ -51,7 +99,6 @@ export const useAllFirewallsQuery = (enabled: boolean = true) => { type MutateFirewall = { id: number; payload: Partial }; -// @TODO: Refactor so these are combined? export const useMutateFirewall = () => { return useMutation((mutateData) => { return updateFirewall(mutateData.id, mutateData.payload); diff --git a/packages/manager/src/queries/linodeFirewalls.ts b/packages/manager/src/queries/linodeFirewalls.ts deleted file mode 100644 index 29b8475cb5b..00000000000 --- a/packages/manager/src/queries/linodeFirewalls.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { getLinodeFirewalls } from '@linode/api-v4/lib/linodes'; -import { Firewall } from '@linode/api-v4/lib/firewalls/types'; -import { APIError, ResourcePage } from '@linode/api-v4/lib/types'; -import { useQuery } from 'react-query'; -import { queryPresets } from './base'; - -const queryKey = 'queryLinodeFirewalls'; - -export const useLinodeFirewalls = (linodeID: number) => - useQuery, APIError[]>( - [queryKey, linodeID], - () => getLinodeFirewalls(linodeID), - queryPresets.oneTimeFetch - ); diff --git a/packages/manager/src/queries/linodes.ts b/packages/manager/src/queries/linodes.ts index 8a67dfa869b..29a8878f828 100644 --- a/packages/manager/src/queries/linodes.ts +++ b/packages/manager/src/queries/linodes.ts @@ -18,7 +18,9 @@ import { Config, LinodeType, getLinodeTypes, + getLinodeFirewalls, } from '@linode/api-v4/lib/linodes'; +import { Firewall } from '@linode/api-v4'; export const STATS_NOT_READY_API_MESSAGE = 'Stats are unavailable at this time.'; @@ -157,6 +159,13 @@ export const useAllLinodeTypesQuery = () => { ); }; +export const useLinodeFirewalls = (linodeID: number) => + useQuery, APIError[]>( + [queryKey, linodeID, 'linodes'], + () => getLinodeFirewalls(linodeID), + queryPresets.oneTimeFetch + ); + /** Use with care; originally added to request all Linodes in a given region for IP sharing and transfer */ const getAllLinodesRequest = (passedParams: any = {}, passedFilter: any = {}) => getAll((params, filter) => From e5e8d70946b1fc35e2bc97328fe250cdfccb2563 Mon Sep 17 00:00:00 2001 From: Banks Nussman Date: Mon, 6 Mar 2023 19:48:20 -0500 Subject: [PATCH 2/2] the actual bug fix - invalidate devices query when linode is deleted --- packages/api-v4/src/linodes/linodes.ts | 10 +++++++-- packages/manager/src/queries/firewalls.ts | 2 +- packages/manager/src/queries/linodes.ts | 13 +++++++++++ .../src/store/linodes/linode.requests.ts | 22 ++++++++++++++++++- 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/packages/api-v4/src/linodes/linodes.ts b/packages/api-v4/src/linodes/linodes.ts index 707dab47597..4a5628e2759 100644 --- a/packages/api-v4/src/linodes/linodes.ts +++ b/packages/api-v4/src/linodes/linodes.ts @@ -145,8 +145,14 @@ export const changeLinodePassword = (linodeId: number, root_pass: string) => * View Firewall information for Firewalls associated with this Linode */ -export const getLinodeFirewalls = (linodeId: number) => +export const getLinodeFirewalls = ( + linodeId: number, + params?: any, + filter?: any +) => Request>( setURL(`${API_ROOT}/linode/instances/${linodeId}/firewalls`), - setMethod('GET') + setMethod('GET'), + setXFilter(filter), + setParams(params) ); diff --git a/packages/manager/src/queries/firewalls.ts b/packages/manager/src/queries/firewalls.ts index 67d0370aabc..ebed1221384 100644 --- a/packages/manager/src/queries/firewalls.ts +++ b/packages/manager/src/queries/firewalls.ts @@ -27,7 +27,7 @@ import { itemInListCreationHandler, } from './base'; -const queryKey = 'firewall'; +export const queryKey = 'firewall'; export const useAllFirewallDevicesQuery = (id: number) => useQuery([queryKey, id, 'devices'], () => diff --git a/packages/manager/src/queries/linodes.ts b/packages/manager/src/queries/linodes.ts index 29a8878f828..6acca254926 100644 --- a/packages/manager/src/queries/linodes.ts +++ b/packages/manager/src/queries/linodes.ts @@ -187,3 +187,16 @@ const getAllLinodeTypes = () => getAll((params) => getLinodeTypes(params))().then( (data) => data.data ); + +export const getAllLinodeFirewalls = ( + linodeId: number, + passedParams: any = {}, + passedFilter: any = {} +) => + getAll((params, filter) => + getLinodeFirewalls( + linodeId, + { ...params, ...passedParams }, + { ...filter, ...passedFilter } + ) + )().then((data) => data.data); diff --git a/packages/manager/src/store/linodes/linode.requests.ts b/packages/manager/src/store/linodes/linode.requests.ts index 4142c21cab1..189c657a635 100644 --- a/packages/manager/src/store/linodes/linode.requests.ts +++ b/packages/manager/src/store/linodes/linode.requests.ts @@ -7,6 +7,9 @@ import { linodeReboot as _rebootLinode, updateLinode as _updateLinode, } from '@linode/api-v4/lib/linodes'; +import { queryClient } from 'src/queries/base'; +import { queryKey as firewallsQueryKey } from 'src/queries/firewalls'; +import { getAllLinodeFirewalls } from 'src/queries/linodes'; import { getAll } from 'src/utilities/getAll'; import { createRequestThunk } from '../store.helpers'; import { ThunkActionCreator } from '../types'; @@ -36,7 +39,24 @@ export const createLinode = createRequestThunk(createLinodeActions, (data) => export const deleteLinode = createRequestThunk( deleteLinodeActions, - ({ linodeId }) => _deleteLinode(linodeId) + async ({ linodeId }) => { + // Before we delete a Linode, we need to see what firewalls + // are associated with it so that we can update the firewalls + // UI to no longer include the deleted linode. + const firewalls = await getAllLinodeFirewalls(linodeId); + + const response = await _deleteLinode(linodeId); + + for (const firewall of firewalls) { + queryClient.invalidateQueries([ + firewallsQueryKey, + firewall.id, + 'devices', + ]); + } + + return response; + } ); export const rebootLinode = createRequestThunk(