-
Notifications
You must be signed in to change notification settings - Fork 357
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.
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-7940] - Update and cleanup Placement Group assign/unassign features #10328
Conversation
'data-testid': 'cancel', | ||
label: 'Cancel', | ||
onClick: handleDrawerClose, | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a missing button all along
@@ -95,20 +110,32 @@ export const RemovableSelectionsList = ( | |||
onRemove, | |||
preferredDataLabel, | |||
selectionData, | |||
showLoadingIndicatorOnRemove = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
} | ||
}, [id, placementGroups]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -50,7 +63,7 @@ export const PlacementGroupsUnassignModal = (props: Props) => { | |||
onClose(); | |||
}; | |||
|
|||
const isAllowedToEditPlacementGroup = useIsResourceRestricted({ | |||
const isLinodeReadOnly = useIsResourceRestricted({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this fixes a false positive check introduced in #10257
Coverage Report: ❌ |
(pg) => pg.id === Number(id) | ||
); | ||
if (placementGroup) { | ||
setSelectedPlacementGroup(placementGroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
const [placementGroup, setPlacementGroup] = React.useState( | ||
selectedPlacementGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, used a useMemo as suggested by @hkhalil-akamai which makes things cleaner
/** | ||
* If true, reset loading states. | ||
*/ | ||
hasEncounteredError?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i improved the naming convention and description
history.replace(`/placement-groups/${placementGroup.id}/linodes/assign`); | ||
}; | ||
const handleUnassignLinodeModal = (linode: Linode) => { | ||
setSelectedLinode(linode); | ||
history.replace( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to use history.push
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @hkhalil-akamai unless we don't want it to be in the browser history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
React.useEffect(() => { | ||
if (open) { | ||
setPlacementGroup(selectedPlacementGroup ?? placementGroupFromParam); | ||
} | ||
}, [open, selectedPlacementGroup, placementGroupFromParam]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useState
+ useEffect
= useMemo
?
React.useEffect(() => { | |
if (open) { | |
setPlacementGroup(selectedPlacementGroup ?? placementGroupFromParam); | |
} | |
}, [open, selectedPlacementGroup, placementGroupFromParam]); | |
const placementGroup = React.useMemo(() => selectedPlacementGroup ?? placementGroupFromParam, [open, selectedPlacementGroup, placementGroupFromParam]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, good suggestion. had to modify slightly to handle open
and have a proper dependency array, but i like this pattern!
React.useEffect(() => { | ||
if (id) { | ||
const placementGroup = placementGroups?.data.find( | ||
(pg) => pg.id === Number(id) | ||
); | ||
if (placementGroup) { | ||
setSelectedPlacementGroup(placementGroup); | ||
} | ||
} | ||
}, [id, placementGroups]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might also be able to be written slightly simpler as a useMemo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
onSuccess: () => { | ||
mutationFn: (req) => | ||
unassignLinodesFromPlacementGroup(placementGroupId, req), | ||
onSuccess: (_, ctx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can ctx
be renamed to variables
? Context is technically the 3ed parameter of an onSuccess function and this might cause confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes thx for pointing this out. Named arguments would be nicer ... 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing those changes ✅
queryClient.invalidateQueries([ | ||
linodeQueryKey, | ||
'linode', | ||
variables.linodes[0], | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
queryClient.invalidateQueries([ | |
linodeQueryKey, | |
'linode', | |
variables.linodes[0], | |
]); | |
for (const linode of variables.linodes) { | |
queryClient.invalidateQueries([ | |
linodeQueryKey, | |
'linode', | |
linode, | |
'details', | |
]); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want to invalidate those? there's no placement group info displayed in the linode/detail UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the intention of
queryClient.invalidateQueries([
linodeQueryKey,
'linode',
variables.linodes[0],
]);
I might not understand its purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we should perform invalidations with the goal to keep the cache reflecting the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 🔄
RemovableSelectionsList
component to handle API request better (disabled and loading states). These updates include new non breaking, optionalfalse
defaulted propsusePlacementGroupData
hook. It was an early over-optimization on my end from early development considerations, resulting in over-fetching and over-complicated loading states.Preview 📷
Screen.Recording.2024-04-01.at.11.31.24.mov
How to test 🧪
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:
unknown
for limit values (ex: linodes column, "2
out ofunknown
")Prerequisites
placement-group
customer tagVerification steps
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