upcoming: [M3-7940] - Update and cleanup Placement Group assign/unassign features #10328

abailly-akamai · 2024-03-29T02:25:34Z

Description 📝

Assigning/Unassigning linodes to/from placement groups has now been merged to alpha, warranting code updates to support the UI/UX for real API transactions (props/queries, loading states, invalidation).

While this touches a fair amount of files, we're looking at a +493 −386 diff refactor aimed to simplify and optimize the feature code.

Changes 🔄

Better handling of props VS queries - case per case (PG features use path based modal/drawers - aka you can link to those) so the code to handle both case had to be touch in a number of files
Modify the RemovableSelectionsList component to handle API request better (disabled and loading states). These updates include new non breaking, optional false defaulted props
Removal of the usePlacementGroupData hook. It was an early over-optimization on my end from early development considerations, resulting in over-fetching and over-complicated loading states.
Fix permissions
Improve linode invalidation on assigning/unassigning
Improve loading states for delete modal & Edit and Assign drawers
Update tests and improve coverage
Update some Naming conventions

Preview 📷

Screen.Recording.2024-04-01.at.11.31.24.mov

How to test 🧪

⚠️ Important Note:
We're getting gradual alpha API implementation. This means some features are not yet functional. Here is what is not implemented as of this PR:

regions limits (you will see unknown for limit values (ex: linodes column, "2 out of unknown")
linode POST, DELETE endpoints. You will not see PG assignment working while creating a linode, not will a linode be unassigned when deleted

Prerequisites

Have the dev admin placement-group customer tag
Switch to the dev API
Have the "Placement Group" feature flag on
Have MSW off

Verification steps

Navigate to http://localhost:3000/placement-groups and play around with
- Create Drawer
- Editing drawer
- Delete Modal
Navigate to http://localhost:3000/placement-groups/{id}/linodes and play around with
- Assign Linode drawer
- Unassign Modal

When testing both, navigate back and forth to confirm query invalidations and that the data is updated according to your latest actions

As an Author I have considered 🤔

Check all that apply

abailly-akamai · 2024-04-01T14:52:44Z

packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.tsx

+              'data-testid': 'cancel',
+              label: 'Cancel',
+              onClick: handleDrawerClose,
+            }}


This was a missing button all along

abailly-akamai · 2024-04-01T14:57:31Z

packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsList.tsx

@@ -95,20 +110,32 @@ export const RemovableSelectionsList = (
    onRemove,
    preferredDataLabel,
    selectionData,
+    showLoadingIndicatorOnRemove = false,


three new props added here to handle loading and disabled states.

this results in the ability to:

Add a loading spinner for the current item being removed (not all)

Add a disabled state for the other items when one is deleted

Handle reseting those states if the query does not succeed

abailly-akamai · 2024-04-01T15:11:44Z

packages/manager/src/features/PlacementGroups/PlacementGroupsLanding/PlacementGroupsLanding.tsx

+      }
+    }
+  }, [id, placementGroups]);
+


this block is the more consequential of this PR.

The landing page table contains table rows with linode counts and a tooltip with the linode labels, therefore we need to fetch linodes...

Previously we would query for each row - this was ok but as the feature grow and people may have many PGs, this is less than ideal to send 10+ different queries (they are x-filtering linode Ids on each row) which may never be invalidated during the session.

Instead, I've updated this page to gather all linode IDs assigned to all placement groups, query all those once, then dispatch them to each row with the getPlacementGroupLinodes util.

abailly-akamai · 2024-04-01T15:22:03Z

packages/manager/src/features/PlacementGroups/PlacementGroupsUnassignModal.tsx

@@ -50,7 +63,7 @@ export const PlacementGroupsUnassignModal = (props: Props) => {
    onClose();
  };

-  const isAllowedToEditPlacementGroup = useIsResourceRestricted({
+  const isLinodeReadOnly = useIsResourceRestricted({


this fixes a false positive check introduced in #10257

…features

github-actions · 2024-04-01T16:06:40Z

Coverage Report: ❌
Base Coverage: 81.75%
Current Coverage: 81.72%

bnussman-akamai · 2024-04-01T19:43:36Z

packages/manager/src/features/PlacementGroups/PlacementGroupsLanding/PlacementGroupsLanding.tsx

+        (pg) => pg.id === Number(id)
+      );
+      if (placementGroup) {
+        setSelectedPlacementGroup(placementGroup);


Can we remove the selectedPlacementGroup state and just derive it from the param and API data? It would allow us to eliminate the useEffect, thesetSelectedPlacementGroup calls, and the onExited.

const selectedPlacementGroup = placementGroups?.data.find( (pg) => pg.id === Number(id) );

Regarding this comment and others, i had added states to prevent the drawer to be emptied out before closing, which causes a flash of content. That being said I agree this is cleaner, and better than over-optimizing for something barely noticeable. Made that change 👍

bnussman-akamai · 2024-04-01T19:50:07Z

packages/manager/src/features/PlacementGroups/PlacementGroupsEditDrawer.tsx

+  const [placementGroup, setPlacementGroup] = React.useState(
+    selectedPlacementGroup


I see the thought here, but I'm not sure how I feel about needing the useEffect to hold extra state along with the useQuery. I wonder if there is clean way we can just rely on the useQuery. Maybe we could use the initialData react query option?

yup, used a useMemo as suggested by @hkhalil-akamai which makes things cleaner

hkhalil-akamai · 2024-04-01T19:21:37Z

packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsList.tsx

+  /**
+   * If true, reset loading states.
+   */
+  hasEncounteredError?: boolean;


This pattern strikes me as a little unusual... for example, what happens when an error is encountered, the loading state is reset (by setting this prop to true), and then another error is encountered? Would the parent component be responsible for resetting this prop after each error?

A similar problem is solved elsewhere in the CM using promises, which allows this component to track the status of each removal, including whether it succeeded or failed.

I don't really see the point of what you are describing here (tracking each individual error). This seems over complicated. Here we have one unassignment error at a time, reset after each error encountered (the boolean is based on mutation error, which we can only execute one at a time, so yes, the parent component be responsible for resetting this prop after each error). I confirmed the pattern working well without bells and whistles.
Perhaps the naming convention is what creates confusion?

i improved the naming convention and description

hkhalil-akamai · 2024-04-01T19:40:35Z

...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx

    history.replace(`/placement-groups/${placementGroup.id}/linodes/assign`);
  };
+  const handleUnassignLinodeModal = (linode: Linode) => {
+    setSelectedLinode(linode);
+    history.replace(


Is it better to use history.push here?

Agree with @hkhalil-akamai unless we don't want it to be in the browser history.

I added this change too, and used history.push for all methods. The only drawback is to history -1 on the delete modal (since you would go back to an empty modal, but it is handled with <NotFound /> and this is the proper, expected behavior 👍

hkhalil-akamai · 2024-04-01T19:44:14Z

packages/manager/src/features/PlacementGroups/PlacementGroupsEditDrawer.tsx

+  React.useEffect(() => {
+    if (open) {
+      setPlacementGroup(selectedPlacementGroup ?? placementGroupFromParam);
+    }
+  }, [open, selectedPlacementGroup, placementGroupFromParam]);
+


useState + useEffect = useMemo ?

Suggested change

React.useEffect(() => {

if (open) {

setPlacementGroup(selectedPlacementGroup ?? placementGroupFromParam);

}

}, [open, selectedPlacementGroup, placementGroupFromParam]);

const placementGroup = React.useMemo(() => selectedPlacementGroup ?? placementGroupFromParam, [open, selectedPlacementGroup, placementGroupFromParam]);

yup, good suggestion. had to modify slightly to handle open and have a proper dependency array, but i like this pattern!

hkhalil-akamai · 2024-04-01T19:59:29Z

packages/manager/src/features/PlacementGroups/PlacementGroupsLanding/PlacementGroupsLanding.tsx

+  React.useEffect(() => {
+    if (id) {
+      const placementGroup = placementGroups?.data.find(
+        (pg) => pg.id === Number(id)
+      );
+      if (placementGroup) {
+        setSelectedPlacementGroup(placementGroup);
+      }
+    }
+  }, [id, placementGroups]);


This might also be able to be written slightly simpler as a useMemo

bnussman-akamai · 2024-04-01T20:35:24Z

packages/manager/src/queries/placementGroups.ts

-    onSuccess: () => {
+    mutationFn: (req) =>
+      unassignLinodesFromPlacementGroup(placementGroupId, req),
+    onSuccess: (_, ctx) => {


Can ctx be renamed to variables? Context is technically the 3ed parameter of an onSuccess function and this might cause confusion

Oh yes thx for pointing this out. Named arguments would be nicer ... 😢

bnussman-akamai

Thanks for addressing those changes ✅

bnussman-akamai · 2024-04-02T14:27:40Z

packages/manager/src/queries/placementGroups.ts

+      queryClient.invalidateQueries([
+        linodeQueryKey,
+        'linode',
+        variables.linodes[0],
+      ]);


Should we go ahead and invalidate all Linodes for the sake of future-proofing?

Also, we might want to invalidate ['linodes', 'linode', id, 'details'] instead of ['linodes', 'linode', id] so that extra stuff like the disks and configs don't get invalidated

Suggested change

queryClient.invalidateQueries([

linodeQueryKey,

'linode',

variables.linodes[0],

]);

for (const linode of variables.linodes) {

queryClient.invalidateQueries([

linodeQueryKey,

'linode',

linode,

'details',

]);

}

Why do you want to invalidate those? there's no placement group info displayed in the linode/detail UI

What is the intention of

queryClient.invalidateQueries([ linodeQueryKey, 'linode', variables.linodes[0], ]);

I might not understand its purpose.

We invalidate this linode so that it won't be available to be assigned to another placement group, but now that I am looking at it we actually populate the linode select via looking at all placement group linodes already assigned so this may not be needed at all.
However, I dislike the idea of having a cache not reflecting API data even we we don't use it. Do you think it matters?

Yeah, I think we should perform invalidations with the goal to keep the cache reflecting the API

cpathipa

Overall LGTM!

Confirming on the below.

Observations:

Since the validation says min 3 chars, is it expected to consider space as a character when creating a PG?

abailly-akamai · 2024-04-02T19:15:04Z

thanks @cpathipa

if the API allows it, then so do we. I agree it's odd, i'll bring it up to them and we'll see if they want to modify the behavior

…ign features (linode#10328) * Fix permission error and improve unassign modal * improve patterns * quey invalidation and fetching states * delete modal unassign * cleaning up logic * Fix tests * Fix tests 2 * remove remaining instances of obsolete hook * Small tooltip fix * More cleanup and coverage * Add missing cancel button in assign drawer * Added changeset: Update and clean up Placement Group assign/unassign features * Address feedback * Improve nomenclature and description * small cleanup

Fix permission error and improve unassign modal

f03a5e7

abailly-akamai self-assigned this Mar 29, 2024

abailly-akamai added the Placement Groups label Mar 29, 2024

abailly-akamai added 10 commits March 29, 2024 00:10

improve patterns

c24d70b

quey invalidation and fetching states

d70e929

delete modal unassign

f95c48e

cleaning up logic

7eb8f00

Fix tests

221b105

Fix tests 2

a89ad64

remove remaining instances of obsolete hook

c8d9c9e

Small tooltip fix

950b49b

More cleanup and coverage

bb69a47

Add missing cancel button in assign drawer

47147bb

abailly-akamai changed the title ~~upcoming: [M3-7940]~~ upcoming: [M3-7940] - Update and cleanup Placement Group assign/unassign features Apr 1, 2024

abailly-akamai commented Apr 1, 2024

View reviewed changes

abailly-akamai marked this pull request as ready for review April 1, 2024 15:43

abailly-akamai requested a review from a team as a code owner April 1, 2024 15:43

abailly-akamai requested review from bnussman-akamai, cpathipa and carrillo-erik and removed request for a team April 1, 2024 15:43

Added changeset: Update and clean up Placement Group assign/unassign …

aa80ff7

…features

bnussman-akamai reviewed Apr 1, 2024

View reviewed changes

hkhalil-akamai reviewed Apr 1, 2024

View reviewed changes

bnussman-akamai requested changes Apr 1, 2024

View reviewed changes

Address feedback

8b9b134

abailly-akamai requested review from bnussman-akamai and hkhalil-akamai April 2, 2024 14:15

Improve nomenclature and description

6c0f6e9

bnussman-akamai approved these changes Apr 2, 2024

View reviewed changes

small cleanup

23d5f03

cpathipa approved these changes Apr 2, 2024

View reviewed changes

abailly-akamai merged commit a520d9c into linode:develop Apr 2, 2024
18 checks passed

This was referenced Apr 11, 2024

Release v1.117.0 - release → staging #10368

Merged

Release v1.117.0 - staging → master #10377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [M3-7940] - Update and cleanup Placement Group assign/unassign features #10328

upcoming: [M3-7940] - Update and cleanup Placement Group assign/unassign features #10328

abailly-akamai commented Mar 29, 2024 •

edited

abailly-akamai Apr 1, 2024

abailly-akamai Apr 1, 2024

abailly-akamai Apr 1, 2024

abailly-akamai Apr 1, 2024

github-actions bot commented Apr 1, 2024 •

edited

bnussman-akamai Apr 1, 2024

abailly-akamai Apr 2, 2024

bnussman-akamai Apr 1, 2024

abailly-akamai Apr 2, 2024

hkhalil-akamai Apr 1, 2024

abailly-akamai Apr 2, 2024

abailly-akamai Apr 2, 2024

hkhalil-akamai Apr 1, 2024

cpathipa Apr 1, 2024

abailly-akamai Apr 2, 2024

hkhalil-akamai Apr 1, 2024

abailly-akamai Apr 2, 2024

hkhalil-akamai Apr 1, 2024

abailly-akamai Apr 2, 2024

bnussman-akamai Apr 1, 2024

abailly-akamai Apr 2, 2024

bnussman-akamai left a comment

bnussman-akamai Apr 2, 2024 •

edited

abailly-akamai Apr 2, 2024

bnussman-akamai Apr 2, 2024 •

edited

abailly-akamai Apr 2, 2024 •

edited

bnussman-akamai Apr 2, 2024

cpathipa left a comment

abailly-akamai commented Apr 2, 2024

		const [placementGroup, setPlacementGroup] = React.useState(
		selectedPlacementGroup

upcoming: [M3-7940] - Update and cleanup Placement Group assign/unassign features #10328

upcoming: [M3-7940] - Update and cleanup Placement Group assign/unassign features #10328

Conversation

abailly-akamai commented Mar 29, 2024 • edited

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

bnussman-akamai Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai Apr 2, 2024 • edited

Choose a reason for hiding this comment

abailly-akamai Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpathipa left a comment

Choose a reason for hiding this comment

Observations:

abailly-akamai commented Apr 2, 2024

abailly-akamai commented Mar 29, 2024 •

edited

github-actions bot commented Apr 1, 2024 •

edited

bnussman-akamai Apr 2, 2024 •

edited

bnussman-akamai Apr 2, 2024 •

edited

abailly-akamai Apr 2, 2024 •

edited