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

Move IP Pools edit form to view page #2405

Merged
merged 8 commits into from
Aug 30, 2024
Merged

Move IP Pools edit form to view page #2405

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3da57c1
Move edit form to view page for IP Pools
charliepark Aug 30, 2024
6f784e3
Add deletion restriction, disabling when IP ranges present
charliepark Aug 30, 2024
40d914d
Add test for view-page deletion
charliepark Aug 30, 2024
6f2fb6c
Add test for IP Pool edit
charliepark Aug 30, 2024
393207c
Add comment; remove unnecessary navigate function call
charliepark Aug 30, 2024
22af2df
Clean up invalidations
charliepark Aug 30, 2024
08eaa7c
mutateAsync update
charliepark Aug 30, 2024
ab54174
Always invalidate ipPoolList on successful edit; add ipPoolView inval…
charliepark Aug 30, 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
18 changes: 11 additions & 7 deletions app/forms/ip-pool-edit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,26 @@ 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 editPool = useApiMutation('ipPoolUpdate', {
onSuccess(_pool) {
queryClient.invalidateQueries('ipPoolList')
if (pool.name !== _pool.name) {
queryClient.invalidateQueries('ipPoolList')
// 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()
}
Copy link
Collaborator

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 }))

Copy link
Contributor Author

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 first navigate (when name has changed), though.

Copy link
Collaborator

@david-crespo david-crespo Aug 30, 2024 •
edited
Loading

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?

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 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 the navigate takes us to the new page and the header is correct, the dropdown has old data (this shows ip-pool-1 renamed to ip-pool-updated, without the invalidation:
Screenshot 2024-08-30 at 2 26 47 PM

Adding queryClient.invalidateQueries('ipPoolList') corrects that:
Screenshot 2024-08-30 at 2 27 22 PM

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.

Copy link
Collaborator

@david-crespo david-crespo Aug 30, 2024 •
edited
Loading

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).

Copy link
Contributor Author

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.

addToast({ content: 'Your IP pool has been updated' })
onDismiss()
},
})

const form = useForm({ defaultValues: pool })
const onDismiss = () => navigate(pb.ipPool({ pool: poolSelector.pool }))

return (
<SideModalForm
Expand Down
57 changes: 49 additions & 8 deletions app/pages/system/networking/IpPoolPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import { createColumnHelper } from '@tanstack/react-table'
import { useCallback, useMemo, useState } from 'react'
import { Outlet, type LoaderFunctionArgs } from 'react-router-dom'
import { Outlet, useNavigate, type LoaderFunctionArgs } from 'react-router-dom'

import {
apiQueryClient,
Expand All @@ -26,9 +26,11 @@ import { CapacityBar } from '~/components/CapacityBar'
import { DocsPopover } from '~/components/DocsPopover'
import { ComboboxField } from '~/components/form/fields/ComboboxField'
import { HL } from '~/components/HL'
import { MoreActionsMenu } from '~/components/MoreActionsMenu'
import { QueryParamTabs } from '~/components/QueryParamTabs'
import { getIpPoolSelector, useForm, useIpPoolSelector } from '~/hooks'
import { confirmAction } from '~/stores/confirm-action'
import { confirmDelete } from '~/stores/confirm-delete'
import { addToast } from '~/stores/toast'
import { DefaultPoolCell } from '~/table/cells/DefaultPoolCell'
import { SkeletonCell } from '~/table/cells/EmptyCell'
Expand All @@ -46,9 +48,10 @@ import { TipIcon } from '~/ui/lib/TipIcon'
import { docLinks } from '~/util/links'
import { pb } from '~/util/path-builder'

const query = { limit: PAGE_SIZE }

IpPoolPage.loader = async function ({ params }: LoaderFunctionArgs) {
const { pool } = getIpPoolSelector(params)
const query = { limit: PAGE_SIZE }
await Promise.all([
apiQueryClient.prefetchQuery('ipPoolView', { path: { pool } }),
apiQueryClient.prefetchQuery('ipPoolSiloList', { path: { pool }, query }),
Expand All @@ -70,16 +73,54 @@ IpPoolPage.loader = async function ({ params }: LoaderFunctionArgs) {
export function IpPoolPage() {
const poolSelector = useIpPoolSelector()
const { data: pool } = usePrefetchedApiQuery('ipPoolView', { path: poolSelector })
const { data: ranges } = usePrefetchedApiQuery('ipPoolRangeList', {
path: poolSelector,
query,
})
const navigate = useNavigate()
const deletePool = useApiMutation('ipPoolDelete', {
david-crespo marked this conversation as resolved.
Show resolved Hide resolved
onSuccess() {
apiQueryClient.invalidateQueries('ipPoolList')
navigate(pb.ipPools())
addToast({ content: 'IP pool deleted' })
},
})

const actions = useMemo(
() => [
{
label: 'Edit',
onActivate() {
navigate(pb.ipPoolEdit(poolSelector))
},
},
{
label: 'Delete',
onActivate: confirmDelete({
doDelete: () => deletePool.mutateAsync({ path: { pool: pool.name } }),
label: pool.name,
}),
disabled:
!!ranges.items.length && 'IP pool cannot be deleted while it contains IP ranges',
className: ranges.items.length ? '' : 'destructive',
},
],
[deletePool, navigate, poolSelector, pool.name, ranges.items]
)

return (
<>
<PageHeader>
<PageTitle icon={<IpGlobal24Icon />}>{pool.name}</PageTitle>
<DocsPopover
heading="IP pools"
icon={<IpGlobal16Icon />}
summary="IP pools are collections of external IPs you can assign to silos. When a pool is linked to a silo, users in that silo can allocate IPs from the pool for their instances."
links={[docLinks.systemIpPools]}
/>
<div className="inline-flex gap-2">
<DocsPopover
heading="IP pools"
icon={<IpGlobal16Icon />}
summary="IP pools are collections of external IPs you can assign to silos. When a pool is linked to a silo, users in that silo can allocate IPs from the pool for their instances."
links={[docLinks.systemIpPools]}
/>
<MoreActionsMenu label="IP pool actions" actions={actions} />
</div>
</PageHeader>
<UtilizationBars />
<QueryParamTabs className="full-width" defaultValue="ranges">
Expand Down
2 changes: 2 additions & 0 deletions app/pages/system/networking/IpPoolsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { DocsPopover } from '~/components/DocsPopover'
import { IpUtilCell } from '~/components/IpPoolUtilization'
import { useQuickActions } from '~/hooks'
import { confirmDelete } from '~/stores/confirm-delete'
import { addToast } from '~/stores/toast'
import { SkeletonCell } from '~/table/cells/EmptyCell'
import { makeLinkCell } from '~/table/cells/LinkCell'
import { useColsWithActions, type MenuAction } from '~/table/columns/action-col'
Expand Down Expand Up @@ -79,6 +80,7 @@ export function IpPoolsPage() {
const deletePool = useApiMutation('ipPoolDelete', {
onSuccess() {
apiQueryClient.invalidateQueries('ipPoolList')
addToast({ content: 'IP pool deleted' })
},
})

Expand Down
12 changes: 6 additions & 6 deletions app/routes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,6 @@ export const routes = createRoutesFromElements(
>
<Route path="ip-pools" element={null} />
<Route path="ip-pools-new" element={<CreateIpPoolSideModalForm />} />
<Route
path="ip-pools/:pool/edit"
element={<EditIpPoolSideModalForm />}
loader={EditIpPoolSideModalForm.loader}
handle={{ crumb: 'Edit IP pool' }}
/>
</Route>
</Route>
<Route path="networking/ip-pools" handle={{ crumb: 'IP pools' }}>
Expand All @@ -213,6 +207,12 @@ export const routes = createRoutesFromElements(
loader={IpPoolPage.loader}
handle={{ crumb: poolCrumb }}
>
<Route
path="edit"
element={<EditIpPoolSideModalForm />}
loader={EditIpPoolSideModalForm.loader}
handle={{ crumb: 'Edit IP pool' }}
/>
<Route path="ranges-add" element={<IpPoolAddRangeSideModalForm />} />
</Route>
</Route>
Expand Down
37 changes: 36 additions & 1 deletion test/e2e/ip-pools.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
})

Copy link
Collaborator

Choose a reason for hiding this comment

The 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()
Expand All @@ -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')

Expand Down
Loading