-
Notifications
You must be signed in to change notification settings - Fork 12
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
Move IP Pools edit form to view page #2405
Changes from all commits
3da57c1
6f784e3
40d914d
6f2fb6c
393207c
22af2df
08eaa7c
ab54174
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -30,23 +30,27 @@ EditIpPoolSideModalForm.loader = async ({ params }: LoaderFunctionArgs) => { | |||||||||||||||||||||||||||
export function EditIpPoolSideModalForm() { | ||||||||||||||||||||||||||||
const queryClient = useApiQueryClient() | ||||||||||||||||||||||||||||
const navigate = useNavigate() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const poolSelector = useIpPoolSelector() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const onDismiss = () => navigate(pb.ipPools()) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const { data: pool } = usePrefetchedApiQuery('ipPoolView', { path: poolSelector }) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const form = useForm({ defaultValues: pool }) | ||||||||||||||||||||||||||||
const onDismiss = () => navigate(pb.ipPool({ pool: poolSelector.pool })) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const editPool = useApiMutation('ipPoolUpdate', { | ||||||||||||||||||||||||||||
onSuccess(_pool) { | ||||||||||||||||||||||||||||
queryClient.invalidateQueries('ipPoolList') | ||||||||||||||||||||||||||||
if (pool.name !== _pool.name) { | ||||||||||||||||||||||||||||
// as the pool's name has changed, we need to navigate to an updated URL | ||||||||||||||||||||||||||||
navigate(pb.ipPool({ pool: _pool.name })) | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
queryClient.invalidateQueries('ipPoolView') | ||||||||||||||||||||||||||||
onDismiss() | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
Comment on lines
42
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is equivalent since
Suggested change
Is the reason for the conditional that you were getting that error flash on nav? Otherwise this would work, wouldn't it? queryClient.invalidateQueries('ipPoolList')
queryClient.invalidateQueries('ipPoolView')
navigate(pb.ipPool({ pool: _pool.name })) Or maybe navigate first I guess? |
||||||||||||||||||||||||||||
addToast({ content: 'Your IP pool has been updated' }) | ||||||||||||||||||||||||||||
onDismiss() | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const form = useForm({ defaultValues: pool }) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||
<SideModalForm | ||||||||||||||||||||||||||||
form={form} | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,7 +110,7 @@ test('IP pool link silo', async ({ page }) => { | |
await expectRowVisible(table, { Silo: 'myriad', 'Pool is silo default': '' }) | ||
}) | ||
|
||
test('IP pool delete', async ({ page }) => { | ||
test('IP pool delete from IP Pools list page', async ({ page }) => { | ||
await page.goto('/system/networking/ip-pools') | ||
|
||
// can't delete a pool containing ranges | ||
|
@@ -133,6 +133,24 @@ test('IP pool delete', async ({ page }) => { | |
await expect(page.getByRole('cell', { name: 'ip-pool-3' })).toBeHidden() | ||
}) | ||
|
||
test('IP pool delete from IP Pool view page', async ({ page }) => { | ||
// can't delete a pool containing ranges | ||
await page.goto('/system/networking/ip-pools/ip-pool-1') | ||
await page.getByRole('button', { name: 'IP pool actions' }).click() | ||
await expect(page.getByRole('menuitem', { name: 'Delete' })).toBeDisabled() | ||
|
||
// can delete a pool with no ranges | ||
await page.goto('/system/networking/ip-pools/ip-pool-3') | ||
await page.getByRole('button', { name: 'IP pool actions' }).click() | ||
await page.getByRole('menuitem', { name: 'Delete' }).click() | ||
await expect(page.getByRole('dialog', { name: 'Confirm delete' })).toBeVisible() | ||
await page.getByRole('button', { name: 'Confirm' }).click() | ||
|
||
// get redirected back to the list after successful delete | ||
await expect(page).toHaveURL('/system/networking/ip-pools') | ||
await expect(page.getByRole('cell', { name: 'ip-pool-3' })).toBeHidden() | ||
}) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wonderful |
||
test('IP pool create', async ({ page }) => { | ||
await page.goto('/system/networking/ip-pools') | ||
await expect(page.getByRole('cell', { name: 'another-pool' })).toBeHidden() | ||
|
@@ -155,6 +173,23 @@ test('IP pool create', async ({ page }) => { | |
}) | ||
}) | ||
|
||
test('IP pool edit', async ({ page }) => { | ||
await page.goto('/system/networking/ip-pools/ip-pool-3') | ||
await page.getByRole('button', { name: 'IP pool actions' }).click() | ||
await page.getByRole('menuitem', { name: 'Edit' }).click() | ||
|
||
const modal = page.getByRole('dialog', { name: 'Edit IP pool' }) | ||
await expect(modal).toBeVisible() | ||
|
||
await page.getByRole('textbox', { name: 'Name' }).fill('updated-pool') | ||
await page.getByRole('textbox', { name: 'Description' }).fill('an updated description') | ||
await page.getByRole('button', { name: 'Update IP pool' }).click() | ||
|
||
await expect(modal).toBeHidden() | ||
await expect(page).toHaveURL('/system/networking/ip-pools/updated-pool') | ||
await expect(page.getByRole('heading', { name: 'updated-pool' })).toBeVisible() | ||
}) | ||
|
||
test('IP range validation and add', async ({ page }) => { | ||
await page.goto('/system/networking/ip-pools/ip-pool-2') | ||
|
||
|
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.
would add a comment explaining why the difference. also the navigate line is the same in both cases, you can just do
navigate(pb.ipPool({ pool: _pool.name }))
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 realized that we don't actually need the
navigate
function when the description has changed; just an onDismiss(). Have added a comment on the firstnavigate
(when name has changed), though.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 the difference in invalidations?
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 can maybe drop the View one; will walk through the various elements of it.
The
ipPoolList
invalidation is needed because of the presence of the IP pools in the header nav. Without that invalidation, even though thenavigate
takes us to the new page and the header is correct, the dropdown has old data (this showsip-pool-1
renamed toip-pool-updated
, without the invalidation:Adding
queryClient.invalidateQueries('ipPoolList')
corrects that:The other invalidation would be useful if the page showed the description (the only form fields in the "Edit IP pool" form are name and description), but since the description is not shown on the IP Pool page, we can actually just remove that invalidation. We'd need to add it back in if we decide down the road to show the description on this page.
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.
If so, isn't the
ipPoolList
invalidation needed regardless of whether the name changed? And on the view one, I think we should always do the corresponding invalidation even if we aren't relying on it directly yet. Leaving out an invalidation was how we got this head-scratcher #2392 (comment).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.
Sure, getting the View invalidation back in makes sense, in case we add descriptions in there.
For List, my thought was that it was only needed when the name changed, as the description changing didn't impact that navigation dropdown, so it wouldn't be necessary to invalidate that; every time I manually checked the list page after editing the description it seemed to show the updated description state in the table of IP pools. But perhaps there's an aspect of the List invalidation I'm missing. Will move the it up a level.