diff --git a/app/components/form/fields/NameField.tsx b/app/components/form/fields/NameField.tsx index 181fa426e3..a8fa6d1e16 100644 --- a/app/components/form/fields/NameField.tsx +++ b/app/components/form/fields/NameField.tsx @@ -18,11 +18,15 @@ export function NameField< required = true, name, label = capitalize(name), + validate, ...textFieldProps -}: Omit, 'validate'> & { label?: string }) { +}: TextFieldProps & { label?: string }) { return ( validateName(name, label, required)} + // always check the name rules first, then the other checks if present + validate={(name, formValues) => + validateName(name, label, required) || validate?.(name, formValues) + } required={required} label={label} name={name} diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 81a3bc0299..39dc0a4446 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -104,6 +104,7 @@ const targetDefaultValues: TargetFormValues = { type CommonFieldsProps = { error: ApiError | null control: Control + nameTaken: (name: string) => boolean } function getFilterValueProps(hostType: VpcFirewallRuleHostFilter['type']) { @@ -155,7 +156,7 @@ const DocsLinkMessage = () => ( /> ) -export const CommonFields = ({ error, control }: CommonFieldsProps) => { +export const CommonFields = ({ error, control, nameTaken }: CommonFieldsProps) => { const portRangeForm = useForm({ defaultValues: portRangeDefaultValues }) const ports = useController({ name: 'ports', control }).field const submitPortRange = portRangeForm.handleSubmit(({ portRange }) => { @@ -199,7 +200,16 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { Enabled - + { + if (nameTaken(name)) { + // TODO: might be worth mentioning that the names are unique per VPC as opposed to globally + return 'Name taken. To update an existing rule, edit it directly.' + } + }} + /> { - // TODO: this silently overwrites existing rules with the current name. - // we should probably at least warn and confirm, if not reject as invalid - const otherRules = existingRules - .filter((r) => r.name !== values.name) - .map(firewallRuleGetToPut) updateRules.mutate({ query: vpcSelector, body: { - rules: [...otherRules, valuesToRuleUpdate(values)], + rules: [...existingRules.map(firewallRuleGetToPut), valuesToRuleUpdate(values)], }, }) }} @@ -605,7 +610,18 @@ export function CreateFirewallRuleForm() { submitError={updateRules.error} submitLabel="Add rule" > - + !!data.rules.find((r) => r.name === name)} + + // TODO: there should also be a form-level error so if the name is off + // screen, it doesn't look like the submit button isn't working. Maybe + // instead of setting a root error, it would be more robust to show a + // top level error if any errors are found in the form. We might want to + // expand the submitError prop on SideModalForm for this + /> ) } diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index 8a37e3f13b..e03fadcb55 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -61,8 +61,12 @@ export function EditFirewallRuleForm() { const updateRules = useApiMutation('vpcFirewallRulesUpdate', { onSuccess() { - queryClient.invalidateQueries('vpcFirewallRulesView') + // Nav before the invalidate because I once saw the above invariant fail + // briefly after successful edit (error page flashed but then we land + // on the rules list ok) and I think it was a race condition where the + // invalidate managed to complete while the modal was still open. onDismiss() + queryClient.invalidateQueries('vpcFirewallRulesView') }, }) @@ -87,32 +91,35 @@ export function EditFirewallRuleForm() { // TODO: uhhhh how can this happen if (Object.keys(originalRule).length === 0) return null + // note different filter logic from create: filter out the rule with the + // *original* name because we need to overwrite that rule + const otherRules = data.rules.filter((r) => r.name !== originalRule.name) + return ( { - // note different filter logic from create: filter out the rule with the - // *original* name because we need to overwrite that rule - const otherRules = data.rules - .filter((r) => r.name !== originalRule.name) - .map(firewallRuleGetToPut) - + onSubmit={(values) => updateRules.mutate({ query: vpcSelector, body: { - rules: [...otherRules, valuesToRuleUpdate(values)], + rules: [...otherRules.map(firewallRuleGetToPut), valuesToRuleUpdate(values)], }, }) - }} + } // validationSchema={validationSchema} // validateOnBlur loading={updateRules.isPending} submitError={updateRules.error} > - + !!otherRules.find((r) => r.name === name)} + /> ) } diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 239c7792d3..9949affa2b 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -371,3 +371,48 @@ test('404s on edit non-existent rule', async ({ page }) => { await page.goto(rulePath.replace('icmp', 'boop')) await expect(page.getByText('Page not found')).toBeVisible() }) + +// when creating a rule, giving it the same name as an existing rule is an +// error. if you want to overwrite a rule, you need to edit it +test('name conflict error on create', async ({ page }) => { + await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new') + + await page.getByRole('textbox', { name: 'Name', exact: true }).fill('allow-ssh') + + const error = page.getByText('Name taken').first() + await expect(error).toBeHidden() + + await page.getByRole('button', { name: 'Add rule' }).click() + await expect(error).toBeVisible() +}) + +// when editing a rule, on the other hand, we only check for conflicts against rules +// other than the one being edited, because of course its name can stay the same +test('name conflict error on edit', async ({ page }) => { + await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules/allow-icmp/edit') + + // changing the name to one taken by another rule is an error + const nameField = page.getByRole('textbox', { name: 'Name', exact: true }) + await nameField.fill('allow-ssh') + + const error = page.getByText('Name taken').first() + await expect(error).toBeHidden() + + await page.getByRole('button', { name: 'Update rule' }).click() + await expect(error).toBeVisible() + + // change name back + await nameField.fill('allow-icmp') + + // changing a value _without_ changing the name is allowed + await page.getByRole('textbox', { name: 'Priority' }).fill('37') + await page.getByRole('button', { name: 'Update rule' }).click() + await expect(error).toBeHidden() + await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp', Priority: '37' }) + + // changing the name to a non-conflicting name is allowed + await page.getByRole('link', { name: 'allow-icmp' }).click() + await nameField.fill('allow-icmp2') + await page.getByRole('button', { name: 'Update rule' }).click() + await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp2', Priority: '37' }) +})