Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upcoming: [M3-7840] - Placement Groups Query Key Factory #10314

Merged
merged 5 commits into from
Mar 26, 2024
Merged

upcoming: [M3-7840] - Placement Groups Query Key Factory #10314

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
609d416
New queries definition fo placement groups
abailly-akamai Mar 25, 2024
66981b2
invalidate linodes on assign/unassign
abailly-akamai Mar 26, 2024
cf7a3ee
Changeset & code comments
abailly-akamai Mar 26, 2024
4bd63f0
feedback
abailly-akamai Mar 26, 2024
03bbff5
feedback
abailly-akamai Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions packages/manager/src/components/PlacementGroupsSelect/PlacementGroupsSelect.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,20 @@ const props: PlacementGroupsSelectProps = {
};

const queryMocks = vi.hoisted(() => ({
useUnpaginatedPlacementGroupsQuery: vi.fn().mockReturnValue({}),
useAllPlacementGroupsQuery: vi.fn().mockReturnValue({}),
}));

vi.mock('src/queries/placementGroups', async () => {
const actual = await vi.importActual('src/queries/placementGroups');
return {
...actual,
useUnpaginatedPlacementGroupsQuery:
queryMocks.useUnpaginatedPlacementGroupsQuery,
useAllPlacementGroupsQuery: queryMocks.useAllPlacementGroupsQuery,
};
});

describe('PlacementGroupSelect', () => {
beforeEach(() => {
queryMocks.useUnpaginatedPlacementGroupsQuery.mockReturnValue({
queryMocks.useAllPlacementGroupsQuery.mockReturnValue({
data: [
placementGroupFactory.build({
affinity_type: 'affinity:local',
Expand Down Expand Up @@ -67,7 +66,7 @@ describe('PlacementGroupSelect', () => {
});

it('should have a disabled option if the region has reached its placement group capacity', async () => {
queryMocks.useUnpaginatedPlacementGroupsQuery.mockReturnValue({
queryMocks.useAllPlacementGroupsQuery.mockReturnValue({
data: [
placementGroupFactory.build({
affinity_type: 'affinity:local',
Expand Down
4 changes: 2 additions & 2 deletions packages/manager/src/components/PlacementGroupsSelect/PlacementGroupsSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import * as React from 'react';
import { Autocomplete } from 'src/components/Autocomplete/Autocomplete';
import { TextFieldProps } from 'src/components/TextField';
import { hasPlacementGroupReachedCapacity } from 'src/features/PlacementGroups/utils';
import { useUnpaginatedPlacementGroupsQuery } from 'src/queries/placementGroups';
import { useAllPlacementGroupsQuery } from 'src/queries/placementGroups';

import { PlacementGroupSelectOption } from './PlacementGroupSelectOption';

Expand Down Expand Up @@ -51,7 +51,7 @@ export const PlacementGroupsSelect = (props: PlacementGroupsSelectProps) => {
data: placementGroups,
error,
isLoading,
} = useUnpaginatedPlacementGroupsQuery(Boolean(selectedRegion?.id));
} = useAllPlacementGroupsQuery(Boolean(selectedRegion?.id));

const isDisabledPlacementGroup = (
selectedPlacementGroup: PlacementGroup,
Expand Down
7 changes: 3 additions & 4 deletions packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const queryMocks = vi.hoisted(() => ({
useAllLinodesQuery: vi.fn().mockReturnValue({}),
useAssignLinodesToPlacementGroup: vi.fn().mockReturnValue({}),
useRegionsQuery: vi.fn().mockReturnValue({}),
useUnpaginatedPlacementGroupsQuery: vi.fn().mockReturnValue({}),
useAllPlacementGroupsQuery: vi.fn().mockReturnValue({}),
}));

vi.mock('src/queries/linodes/linodes', async () => {
Expand All @@ -29,8 +29,7 @@ vi.mock('src/queries/placementGroups', async () => {
const actual = await vi.importActual('src/queries/placementGroups');
return {
...actual,
useUnpaginatedPlacementGroupsQuery:
queryMocks.useUnpaginatedPlacementGroupsQuery,
useAllPlacementGroupsQuery: queryMocks.useAllPlacementGroupsQuery,
};
});

Expand Down Expand Up @@ -77,7 +76,7 @@ describe('PlacementGroupsAssignLinodesDrawer', () => {
],
});
queryMocks.useRegionsQuery.mockReturnValue(regionFactory.buildList(5));
queryMocks.useUnpaginatedPlacementGroupsQuery.mockReturnValue({
queryMocks.useAllPlacementGroupsQuery.mockReturnValue({
data: placementGroupFactory.build(),
});
queryMocks.useAssignLinodesToPlacementGroup.mockReturnValue(
Expand Down
4 changes: 2 additions & 2 deletions packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { usePlacementGroupData } from 'src/hooks/usePlacementGroupsData';
import { useAllLinodesQuery } from 'src/queries/linodes/linodes';
import {
useAssignLinodesToPlacementGroup,
useUnpaginatedPlacementGroupsQuery,
useAllPlacementGroupsQuery,
} from 'src/queries/placementGroups';

import { LinodeSelect } from '../Linodes/LinodeSelect/LinodeSelect';
Expand Down Expand Up @@ -43,7 +43,7 @@ export const PlacementGroupsAssignLinodesDrawer = (
const {
data: allPlacementGroups,
error: allPlacementGroupsError,
} = useUnpaginatedPlacementGroupsQuery();
} = useAllPlacementGroupsQuery();
const { enqueueSnackbar } = useSnackbar();

// We display a notice and disable inputs in case the user reaches this drawer somehow
Expand Down
7 changes: 3 additions & 4 deletions packages/manager/src/features/PlacementGroups/PlacementGroupsDetailPanel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const defaultProps = {

const queryMocks = vi.hoisted(() => ({
useRegionsQuery: vi.fn().mockReturnValue({}),
useUnpaginatedPlacementGroupsQuery: vi.fn().mockReturnValue({}),
useAllPlacementGroupsQuery: vi.fn().mockReturnValue({}),
}));

vi.mock('src/queries/regions/regions', async () => {
Expand All @@ -26,8 +26,7 @@ vi.mock('src/queries/placementGroups', async () => {
const actual = await vi.importActual('src/queries/placementGroups');
return {
...actual,
useUnpaginatedPlacementGroupsQuery:
queryMocks.useUnpaginatedPlacementGroupsQuery,
useAllPlacementGroupsQuery: queryMocks.useAllPlacementGroupsQuery,
};
});

Expand All @@ -54,7 +53,7 @@ describe('PlacementGroupsDetailPanel', () => {
}),
],
});
queryMocks.useUnpaginatedPlacementGroupsQuery.mockReturnValue({
queryMocks.useAllPlacementGroupsQuery.mockReturnValue({
data: [
placementGroupFactory.build({
affinity_type: 'affinity:local',
Expand Down
4 changes: 2 additions & 2 deletions packages/manager/src/features/PlacementGroups/PlacementGroupsDetailPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { Typography } from 'src/components/Typography';
import { PlacementGroupsCreateDrawer } from 'src/features/PlacementGroups/PlacementGroupsCreateDrawer';
import { hasRegionReachedPlacementGroupCapacity } from 'src/features/PlacementGroups/utils';
import { useRestrictedGlobalGrantCheck } from 'src/hooks/useRestrictedGlobalGrantCheck';
import { useUnpaginatedPlacementGroupsQuery } from 'src/queries/placementGroups';
import { useAllPlacementGroupsQuery } from 'src/queries/placementGroups';
import { useRegionsQuery } from 'src/queries/regions/regions';

import { PLACEMENT_GROUP_SELECT_TOOLTIP_COPY } from './constants';
Expand All @@ -27,7 +27,7 @@ interface Props {
export const PlacementGroupsDetailPanel = (props: Props) => {
const theme = useTheme();
const { handlePlacementGroupChange, selectedRegionId } = props;
const { data: allPlacementGroups } = useUnpaginatedPlacementGroupsQuery();
const { data: allPlacementGroups } = useAllPlacementGroupsQuery();
const { data: regions } = useRegionsQuery();
const [
isCreatePlacementGroupDrawerOpen,
Expand Down
77 changes: 49 additions & 28 deletions packages/manager/src/queries/placementGroups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ import {
Params,
ResourcePage,
} from '@linode/api-v4/lib/types';
import { createQueryKeys } from '@lukemorales/query-key-factory';
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query';

import { queryKey as linodeQueryKey } from 'src/queries/linodes/linodes';
import { getAll } from 'src/utilities/getAll';

import { profileQueries } from './profile';
Expand All @@ -27,13 +29,25 @@ import type {
UpdatePlacementGroupPayload,
} from '@linode/api-v4';

export const queryKey = 'placement-groups';
export const placementGroupQueries = createQueryKeys('placement-groups', {
all: (params: Params = {}, filters: Filter = {}) => ({
queryFn: () => getAllPlacementGroupsRequest(),
queryKey: [params, filters],
}),
abailly-akamai marked this conversation as resolved.
Show resolved Hide resolved
paginated: (params: Params, filters: Filter) => ({
queryFn: () => getPlacementGroups(params, filters),
queryKey: [params, filters],
}),
placementGroup: (placementGroupId: number) => ({
queryFn: () => getPlacementGroup(placementGroupId),
queryKey: [placementGroupId],
}),
});

export const useUnpaginatedPlacementGroupsQuery = (enabled = true) =>
export const useAllPlacementGroupsQuery = (enabled = true) =>
useQuery<PlacementGroup[], APIError[]>({
enabled,
queryFn: () => getAllPlacementGroupsRequest(),
queryKey: [queryKey, 'all'],
...placementGroupQueries.all(),
});

const getAllPlacementGroupsRequest = () =>
Expand All @@ -49,8 +63,7 @@ export const usePlacementGroupsQuery = (
useQuery<ResourcePage<PlacementGroup>, APIError[]>({
enabled,
keepPreviousData: true,
queryFn: () => getPlacementGroups(params, filter),
queryKey: [queryKey, 'paginated', params, filter],
...placementGroupQueries.paginated(params, filter),
});

export const usePlacementGroupQuery = (
Expand All @@ -59,8 +72,7 @@ export const usePlacementGroupQuery = (
) => {
return useQuery<PlacementGroup, APIError[]>({
enabled,
queryFn: () => getPlacementGroup(placementGroupId),
queryKey: [queryKey, 'placement-group', placementGroupId],
...placementGroupQueries.placementGroup(placementGroupId),
});
};

Expand All @@ -70,12 +82,13 @@ export const useCreatePlacementGroup = () => {
return useMutation<PlacementGroup, APIError[], CreatePlacementGroupPayload>({
mutationFn: createPlacementGroup,
onSuccess: (placementGroup) => {
queryClient.invalidateQueries([queryKey, 'paginated']);
queryClient.invalidateQueries([queryKey, 'all']);
queryClient.setQueryData(
[queryKey, 'placement-group', placementGroup.id],
queryClient.invalidateQueries(placementGroupQueries.paginated._def);
queryClient.invalidateQueries(placementGroupQueries.all._def);
queryClient.setQueryData<PlacementGroup>(
placementGroupQueries.placementGroup(placementGroup.id).queryKey,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when creating a new PG, we're really only concerned about the Placement Group queries since at the time of creation the PG will be empty

placementGroup
);

// If a restricted user creates an entity, we must make sure grants are up to date.
queryClient.invalidateQueries(profileQueries.grants.queryKey);
},
Expand All @@ -88,10 +101,10 @@ export const useMutatePlacementGroup = (id: number) => {
return useMutation<PlacementGroup, APIError[], UpdatePlacementGroupPayload>({
mutationFn: (data) => updatePlacementGroup(id, data),
onSuccess: (placementGroup) => {
queryClient.invalidateQueries([queryKey, 'paginated']);
queryClient.invalidateQueries([queryKey, 'all']);
queryClient.invalidateQueries(placementGroupQueries.paginated._def);
queryClient.invalidateQueries(placementGroupQueries.all._def);
queryClient.setQueryData(
[queryKey, 'placement-group', id],
placementGroupQueries.placementGroup(id).queryKey,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When editing a PG, we're only editing its label, so no need to invalidate any Linode query here since the PG is not represented in the Linode details UI

placementGroup
);
},
Expand All @@ -104,9 +117,11 @@ export const useDeletePlacementGroup = (id: number) => {
return useMutation<{}, APIError[]>({
mutationFn: () => deletePlacementGroup(id),
onSuccess: () => {
queryClient.invalidateQueries([queryKey, 'paginated']);
queryClient.invalidateQueries([queryKey, 'all']);
queryClient.removeQueries([queryKey, 'placement-group', id]);
queryClient.invalidateQueries(placementGroupQueries.paginated._def);
queryClient.invalidateQueries(placementGroupQueries.all._def);
queryClient.removeQueries(
placementGroupQueries.placementGroup(id).queryKey
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't delete a PG with Linodes in it, so at the time of deletion, it is empty therefore no need to invalidate any Linode

},
});
};
Expand All @@ -120,12 +135,15 @@ export const useAssignLinodesToPlacementGroup = (placementGroupId: number) => {
AssignLinodesToPlacementGroupPayload
>({
mutationFn: (data) => assignLinodesToPlacementGroup(placementGroupId, data),
onSuccess: (updatedPlacementGroup) => {
queryClient.invalidateQueries([queryKey, 'paginated']);
queryClient.setQueryData(
[queryKey, 'placement-group', placementGroupId],
updatedPlacementGroup
onSuccess: () => {
queryClient.invalidateQueries(placementGroupQueries.paginated._def);
queryClient.invalidateQueries(placementGroupQueries.all._def);
queryClient.invalidateQueries(
placementGroupQueries.placementGroup(placementGroupId).queryKey
);

// Invalidate all linodes query since we use the list to populate the select in the assign drawer
queryClient.invalidateQueries([linodeQueryKey, 'all']);
},
});
};
Expand All @@ -141,12 +159,15 @@ export const useUnassignLinodesFromPlacementGroup = (
>({
mutationFn: (data) =>
unassignLinodesFromPlacementGroup(placementGroupId, data),
onSuccess: (updatedPlacementGroup) => {
queryClient.invalidateQueries([queryKey, 'paginated']);
queryClient.setQueryData(
[queryKey, 'placement-group', placementGroupId],
updatedPlacementGroup
onSuccess: () => {
queryClient.invalidateQueries(placementGroupQueries.paginated._def);
queryClient.invalidateQueries(placementGroupQueries.all._def);
queryClient.invalidateQueries(
placementGroupQueries.placementGroup(placementGroupId).queryKey
);

// Invalidate all linodes query since we use the list to populate the select in the assign drawer
queryClient.invalidateQueries([linodeQueryKey, 'all']);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For assign and unassign, we're only worried about invalidating the allLinodesQuery because that's the only relationship query we use at the moment (for the PG select).

There is no representation of the PG in the linode details UI as it stands

},
});
};
Loading