diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index 923baca23..3b9ee63f5 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -25,7 +25,6 @@ export type ComboboxFieldProps< name: TName control: Control onChange?: (value: string | null | undefined) => void - disabled?: boolean } & ComboboxBaseProps export function ComboboxField< @@ -38,15 +37,29 @@ export function ComboboxField< label = capitalize(name), required, onChange, - disabled, + allowArbitraryValues, + placeholder, + // Intent is to not show both a placeholder and a description, while still having good defaults; prefer a description to a placeholder + /* + * If description is provided, use it. + * If not, but a placeholder is provided, the default description should be undefined. + * If no placeholder is provided and arbitrary values are allowed, the default description should be 'Select an option or enter a custom value'. + * If no placeholder is provided and arbitrary values are not allowed, the default description should be 'Select an option'. + */ + description = placeholder + ? undefined + : allowArbitraryValues + ? 'Select an option or enter a custom value' + : 'Select an option', ...props }: ComboboxFieldProps) { const { field, fieldState } = useController({ name, control, rules: { required } }) return (
diff --git a/app/components/form/fields/ListboxField.tsx b/app/components/form/fields/ListboxField.tsx index 965a92081..38e644acb 100644 --- a/app/components/form/fields/ListboxField.tsx +++ b/app/components/form/fields/ListboxField.tsx @@ -35,6 +35,7 @@ export type ListboxFieldProps< onChange?: (value: string | null | undefined) => void isLoading?: boolean noItemsPlaceholder?: string + hideOptionalTag?: boolean } export function ListboxField< @@ -54,6 +55,7 @@ export function ListboxField< onChange, isLoading, noItemsPlaceholder, + hideOptionalTag, }: ListboxFieldProps) { // TODO: recreate this logic // validate: (v) => (required && !v ? `${name} is required` : undefined), @@ -80,6 +82,7 @@ export function ListboxField< hasError={fieldState.error !== undefined} isLoading={isLoading} buttonRef={field.ref} + hideOptionalTag={hideOptionalTag} />
diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index ecc3d5e4e..3c9906ace 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,17 +6,27 @@ * Copyright Oxide Computer Company */ -import { useController, type Control } from 'react-hook-form' - -import type { ApiError, VpcFirewallRuleHostFilter, VpcFirewallRuleTarget } from '~/api' +import { useController, type Control, type ControllerRenderProps } from 'react-hook-form' + +import { + usePrefetchedApiQuery, + type ApiError, + type Instance, + type Vpc, + type VpcFirewallRuleHostFilter, + type VpcFirewallRuleTarget, + type VpcSubnet, +} from '~/api' import { parsePortRange } from '~/api/util' import { CheckboxField } from '~/components/form/fields/CheckboxField' +import { ComboboxField } from '~/components/form/fields/ComboboxField' import { DescriptionField } from '~/components/form/fields/DescriptionField' import { ListboxField } from '~/components/form/fields/ListboxField' import { NameField } from '~/components/form/fields/NameField' import { NumberField } from '~/components/form/fields/NumberField' import { RadioField } from '~/components/form/fields/RadioField' import { TextField, TextFieldInner } from '~/components/form/fields/TextField' +import { useVpcSelector } from '~/hooks' import { useForm } from '~/hooks/use-form' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' @@ -26,45 +36,35 @@ import * as MiniTable from '~/ui/lib/MiniTable' import { TextInputHint } from '~/ui/lib/TextInput' import { KEYS } from '~/ui/util/keys' import { links } from '~/util/links' +import { capitalize } from '~/util/str' import { type FirewallRuleValues } from './firewall-rules-util' -type PortRangeFormValues = { - portRange: string -} - -const portRangeDefaultValues: PortRangeFormValues = { - portRange: '', -} - -type HostFormValues = { - type: VpcFirewallRuleHostFilter['type'] - value: string -} +/** + * This is a large file. The main thing to be aware of is that the firewall rules + * form is made up of two main sections: Targets and Filters. Filters, then, has + * a few sub-sections (Ports, Protocols, and Hosts). + * + * The Targets section and the Filters:Hosts section are very similar, so we've + * pulled common code to the DynamicTypeAndValueFields and ClearAndAddButtons + * components. We also then set up the Targets / Ports / Hosts variables at the + * top of the CommonFields component. + */ -const hostDefaultValues: HostFormValues = { - type: 'vpc', - value: '', -} +type TargetAndHostFilterType = + | VpcFirewallRuleTarget['type'] + | VpcFirewallRuleHostFilter['type'] -type TargetFormValues = { - type: VpcFirewallRuleTarget['type'] +type TargetAndHostFormValues = { + type: TargetAndHostFilterType value: string + subnetVpc?: string } -const targetDefaultValues: TargetFormValues = { - type: 'vpc', - value: '', -} - -type CommonFieldsProps = { - error: ApiError | null - control: Control - nameTaken: (name: string) => boolean -} - -function getFilterValueProps(hostType: VpcFirewallRuleHostFilter['type']) { - switch (hostType) { +// these are part of the target and host filter form; +// the specific values depend on the target or host filter type selected +const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { + switch (targetOrHostType) { case 'vpc': return { label: 'VPC name' } case 'subnet': @@ -72,49 +72,241 @@ function getFilterValueProps(hostType: VpcFirewallRuleHostFilter['type']) { case 'instance': return { label: 'Instance name' } case 'ip': - return { label: 'IP address', helpText: 'An IPv4 or IPv6 address' } + return { label: 'IP address', description: 'Enter an IPv4 or IPv6 address' } case 'ip_net': return { label: 'IP network', - helpText: 'Looks like 192.168.0.0/16 or fd00:1122:3344:0001::1/64', + description: 'Looks like 192.168.0.0/16 or fd00:1122:3344:0001::1/64', } } } -const DocsLinkMessage = () => ( - - Read the{' '} - - guest networking guide - {' '} - and{' '} - + valueType: TargetAndHostFilterType + items: Array<{ value: string; label: string }> + isDisabled?: boolean + onInputChange?: (value: string) => void + onTypeChange: () => void + onSubmitTextField: (e: React.KeyboardEvent) => void +}) => { + return ( + <> + + {/* In the firewall rules form, a few types get comboboxes instead of text fields */} + {valueType === 'vpc' || valueType === 'subnet' || valueType === 'instance' ? ( + + ) : ( + { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + onSubmitTextField(e) + } + }} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected + /> + )} + + ) +} + +// The "Clear" and "Add …" buttons that appear below the filter input fields +const ClearAndAddButtons = ({ + isDirty, + onClear, + onSubmit, + buttonCopy, +}: { + isDirty: boolean + onClear: () => void + onSubmit: () => void + buttonCopy: string +}) => ( +
+ + +
+) + +type TypeAndValueTableProps = { + sectionType: 'target' | 'host' + items: ControllerRenderProps +} +const TypeAndValueTable = ({ sectionType, items }: TypeAndValueTableProps) => ( + + + Type + Value + {/* For remove button */} + + + + {items.value.map(({ type, value }, index) => ( + - API docs - {' '} - to learn more about firewall rules. - - } - /> + + {type} + + {value} + + items.onChange( + items.value.filter((i) => !(i.value === value && i.type === type)) + ) + } + label={`remove ${sectionType} ${value}`} + /> + + ))} + + +) + +// Given an array of committed items (VPCs, Subnets, Instances) and +// a list of all items, return the items that are available +const availableItems = ( + committedItems: Array, + itemType: 'vpc' | 'subnet' | 'instance', + items: Array +) => { + if (!items) return [] + return ( + items + .map((item) => item.name) + // remove any items that match the committed items on both type and value + .filter( + (name) => + !committedItems.filter((ci) => ci.type === itemType && ci.value === name).length + ) + .map((name) => ({ label: name, value: name })) + ) +} + +type ProtocolFieldProps = { + control: Control + protocol: 'TCP' | 'UDP' | 'ICMP' +} +const ProtocolField = ({ control, protocol }: ProtocolFieldProps) => ( +
+ + {protocol} + +
) -export const CommonFields = ({ error, control, nameTaken }: CommonFieldsProps) => { - const portRangeForm = useForm({ defaultValues: portRangeDefaultValues }) +type CommonFieldsProps = { + control: Control + nameTaken: (name: string) => boolean + error: ApiError | null +} + +export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { + const { project, vpc } = useVpcSelector() + const targetAndHostDefaultValues: TargetAndHostFormValues = { + type: 'vpc', + value: '', + // only becomes relevant when the type is 'VPC subnet'; ignored otherwise + subnetVpc: vpc, + } + // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit + const { + data: { items: instances }, + } = usePrefetchedApiQuery('instanceList', { + query: { project, limit: 1000 }, + }) + const { + data: { items: vpcs }, + } = usePrefetchedApiQuery('vpcList', { + query: { project, limit: 1000 }, + }) + const { + data: { items: vpcSubnets }, + } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) + + // Targets + const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) + const targets = useController({ name: 'targets', control }).field + const targetType = targetForm.watch('type') + const targetValue = targetForm.watch('value') + // get the list of items that are not already in the list of targets + const targetItems = { + vpc: availableItems(targets.value, 'vpc', vpcs), + subnet: availableItems(targets.value, 'subnet', vpcSubnets), + instance: availableItems(targets.value, 'instance', instances), + ip: [], + ip_net: [], + } + const submitTarget = targetForm.handleSubmit(({ type, value }) => { + // TODO: do this with a normal validation + // ignore click if empty or a duplicate + // TODO: show error instead of ignoring click + if (!type || !value) return + if (targets.value.some((t) => t.value === value && t.type === type)) return + targets.onChange([...targets.value, { type, value }]) + targetForm.reset() + }) + + // Ports + const portRangeForm = useForm({ defaultValues: { portRange: '' } }) const ports = useController({ name: 'ports', control }).field + const portValue = portRangeForm.watch('portRange') const submitPortRange = portRangeForm.handleSubmit(({ portRange }) => { const portRangeValue = portRange.trim() // at this point we've already validated in validate() that it parses and @@ -123,34 +315,58 @@ export const CommonFields = ({ error, control, nameTaken }: CommonFieldsProps) = portRangeForm.reset() }) - const hostForm = useForm({ defaultValues: hostDefaultValues }) + // Host Filters + const hostForm = useForm({ defaultValues: targetAndHostDefaultValues }) const hosts = useController({ name: 'hosts', control }).field + const hostType = hostForm.watch('type') + const hostValue = hostForm.watch('value') + // get the list of items that are not already in the list of host filters + const hostFilterItems = { + vpc: availableItems(hosts.value, 'vpc', vpcs), + subnet: availableItems(hosts.value, 'subnet', vpcSubnets), + instance: availableItems(hosts.value, 'instance', instances), + ip: [], + ip_net: [], + } const submitHost = hostForm.handleSubmit(({ type, value }) => { // ignore click if empty or a duplicate // TODO: show error instead of ignoring click if (!type || !value) return if (hosts.value.some((t) => t.value === value && t.type === type)) return - hosts.onChange([...hosts.value, { type, value }]) hostForm.reset() }) - const targetForm = useForm({ defaultValues: targetDefaultValues }) - const targets = useController({ name: 'targets', control }).field - const submitTarget = targetForm.handleSubmit(({ type, value }) => { - // TODO: do this with a normal validation - // ignore click if empty or a duplicate - // TODO: show error instead of ignoring click - if (!type || !value) return - if (targets.value.some((t) => t.value === value && t.type === type)) return - - targets.onChange([...targets.value, { type, value }]) - targetForm.reset() - }) - return ( <> - + + Read the{' '} + + guest networking guide + {' '} + and{' '} + + API docs + {' '} + to learn more about firewall rules. + + } + /> {/* omitting value prop makes it a boolean value. beautiful */} {/* TODO: better text or heading or tip or something on this checkbox */} @@ -204,106 +420,41 @@ export const CommonFields = ({ error, control, nameTaken }: CommonFieldsProps) = {/* Really this should be its own
, but you can't have a form inside a form, so we just stick the submit handler in a button onClick */} -

Targets

- - Targets determine the instances to which this rule applies. You can target - instances directly by name, or specify a VPC, VPC subnet, IP, or IP subnet, - which will apply the rule to traffic going to all matching instances. Targets - are additive: the rule applies to instances matching{' '} - any target. - - } - /> - {/* TODO: make ListboxField smarter with the values like RadioField is */} - -
- Targets + + Targets determine the instances to which this rule applies. You can target + instances directly by name, or specify a VPC, VPC subnet, IP, or IP subnet, + which will apply the rule to traffic going to all matching instances. Targets + are additive: the rule applies to instances matching{' '} + any target. + + } + /> + { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitTarget(e) - } - }} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected + valueType={targetType} + items={targetItems[targetType]} + onTypeChange={() => targetForm.setValue('value', '')} + onInputChange={(value) => targetForm.setValue('value', value)} + onSubmitTextField={submitTarget} + /> + targetForm.reset()} + onSubmit={submitTarget} + buttonCopy="Add target" /> - -
- - -
- - {!!targets.value.length && ( - - - Type - Value - {/* For remove button */} - - - - {targets.value.map((t, index) => ( - - - {t.type} - - {t.value} - - targets.onChange( - targets.value.filter( - (i) => !(i.value === t.value && i.type === t.type) - ) - ) - } - label={`remove target ${t.value}`} - /> - - ))} - - - )} + {!!targets.value.length && }

Filters

- -
- - -
+ - {!!ports.value.length && ( @@ -382,21 +524,9 @@ export const CommonFields = ({ error, control, nameTaken }: CommonFieldsProps) =
Protocol filters -
- - TCP - -
-
- - UDP - -
-
- - ICMP - -
+ + +
@@ -411,90 +541,23 @@ export const CommonFields = ({ error, control, nameTaken }: CommonFieldsProps) = } /> - hostForm.setValue('value', '')} + onInputChange={(value) => hostForm.setValue('value', value)} + onSubmitTextField={submitHost} /> - - {/* For everything but IP this is a name, but for IP it's an IP. - So we should probably have the label on this field change when the - host type changes. Also need to confirm that it's just an IP and - not a block. */} - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitHost(e) - } - }} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected + hostForm.reset()} + onSubmit={submitHost} + buttonCopy="Add host filter" /> - -
- - -
- - {!!hosts.value.length && ( - - - Type - Value - {/* For remove button */} - - - - {hosts.value.map((h, index) => ( - - - {h.type} - - {h.value} - - hosts.onChange( - hosts.value.filter( - (i) => !(i.value === h.value && i.type === h.type) - ) - ) - } - label={`remove host ${h.value}`} - /> - - ))} - - - )}
+ {!!hosts.value.length && } {error && ( <> diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 5b79075b6..167021448 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -55,9 +55,14 @@ const ruleToValues = (rule: VpcFirewallRule): FirewallRuleValues => ({ }) CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { - await apiQueryClient.prefetchQuery('vpcFirewallRulesView', { - query: getVpcSelector(params), - }) + const { project, vpc } = getVpcSelector(params) + await Promise.all([ + apiQueryClient.prefetchQuery('vpcFirewallRulesView', { query: { project, vpc } }), + apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: 1000 } }), + apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: 1000 } }), + apiQueryClient.prefetchQuery('vpcSubnetList', { query: { project, vpc } }), + ]) + return null } @@ -76,10 +81,13 @@ export function CreateFirewallRuleForm() { }, }) - const { data } = usePrefetchedApiQuery('vpcFirewallRulesView', { + const { data: vpcFirewallRules } = usePrefetchedApiQuery('vpcFirewallRulesView', { query: vpcSelector, }) - const existingRules = useMemo(() => R.sortBy(data.rules, (r) => r.priority), [data]) + const existingRules = useMemo( + () => R.sortBy(vpcFirewallRules.rules, (r) => r.priority), + [vpcFirewallRules] + ) // The :rule path param is optional. If it is present, we are creating a // rule from an existing one, so we find that rule and copy it into the form @@ -115,11 +123,10 @@ export function CreateFirewallRuleForm() { submitLabel="Add rule" > !!data.rules.find((r) => r.name === name)} - + nameTaken={(name) => !!existingRules.find((r) => r.name === name)} + error={updateRules.error} // 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 diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index b71617f5c..9d99ea874 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -32,26 +32,29 @@ import { valuesToRuleUpdate, type FirewallRuleValues } from './firewall-rules-ut EditFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { const { project, vpc, rule } = getFirewallRuleSelector(params) - const data = await apiQueryClient.fetchQuery('vpcFirewallRulesView', { - query: { project, vpc }, - }) - - const originalRule = data.rules.find((r) => r.name === rule) + const [firewallRules] = await Promise.all([ + apiQueryClient.fetchQuery('vpcFirewallRulesView', { query: { project, vpc } }), + apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: 1000 } }), + apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: 1000 } }), + apiQueryClient.prefetchQuery('vpcSubnetList', { query: { project, vpc } }), + ]) + + const originalRule = firewallRules.rules.find((r) => r.name === rule) if (!originalRule) throw trigger404 return null } export function EditFirewallRuleForm() { - const { vpc, project, rule } = useFirewallRuleSelector() + const { project, vpc, rule } = useFirewallRuleSelector() const vpcSelector = useVpcSelector() const queryClient = useApiQueryClient() - const { data } = usePrefetchedApiQuery('vpcFirewallRulesView', { + const { data: firewallRules } = usePrefetchedApiQuery('vpcFirewallRulesView', { query: { project, vpc }, }) - const originalRule = data.rules.find((r) => r.name === rule) + const originalRule = firewallRules.rules.find((r) => r.name === rule) // we shouldn't hit this because of the trigger404 in the loader invariant(originalRule, 'Firewall rule must exist') @@ -93,7 +96,7 @@ export function EditFirewallRuleForm() { // 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) + const otherRules = firewallRules.rules.filter((r) => r.name !== originalRule.name) return ( !!otherRules.find((r) => r.name === name)} + error={updateRules.error} /> ) diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 623a010fd..685032060 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -24,18 +24,29 @@ import { FieldLabel } from './FieldLabel' import { usePopoverZIndex } from './SideModal' import { TextInputHint } from './TextInput' -export type ComboboxItem = { label: string; value: string } - /** Simple non-generic props shared with ComboboxField */ export type ComboboxBaseProps = { description?: React.ReactNode isDisabled?: boolean isLoading?: boolean - items: ComboboxItem[] + items: Array<{ label: string; value: string }> label: string placeholder?: string required?: boolean tooltipText?: string + ariaLabel?: string + hideOptionalTag?: boolean + /** + * Pass in `allowArbitraryValues` as `true` when the user should be able to + * type in new values that aren't in the list [default is `false`] + */ + allowArbitraryValues?: boolean + /** + * Pass in `onInputChange` when an event should be triggered when the user types in the field; + * This is distinct from `onChange` which is triggered when the user selects an item from the list. + * For example, if a form value should be set when the user types, use `onInputChange`. + */ + onInputChange?: (value: string) => void } type ComboboxProps = { @@ -46,7 +57,7 @@ type ComboboxProps = { export const Combobox = ({ description, - items, + items = [], selected, label, placeholder, @@ -56,6 +67,10 @@ export const Combobox = ({ isDisabled, isLoading, onChange, + onInputChange, + allowArbitraryValues = false, + ariaLabel, + hideOptionalTag, }: ComboboxProps) => { const [query, setQuery] = useState(selected || '') @@ -80,7 +95,12 @@ export const Combobox = ({ {label && ( // TODO: FieldLabel needs a real ID
- + {description && {description}} @@ -100,9 +120,12 @@ export const Combobox = ({ )} > (selected ? selected : query)} - onChange={(event) => setQuery(event.target.value)} + onChange={(event) => { + setQuery(event.target.value) + onInputChange?.(event.target.value) + }} placeholder={placeholder} disabled={isDisabled || isLoading} className={cn( @@ -113,48 +136,52 @@ export const Combobox = ({ hasError && 'focus-error' )} /> -
- -
- - - {filteredItems.length === 0 && ( - -
No items match
- + {items.length > 0 && ( +
+ +
)} - {filteredItems.map((item) => ( - { - onChange(item.label) - setQuery(item.label) - }} - > - {({ focus, selected }) => ( - // This *could* be done with data-[focus] and data-[selected] instead, but - // it would be a lot more verbose. those can only be used with TW classes, - // not our .is-selected and .is-highlighted, so we'd have to copy the pieces - // of those rules one by one. Better to rely on the shared classes. -
- {item.label} -
- )} - - ))} - + + {items.length > 0 && ( + + {!allowArbitraryValues && filteredItems.length === 0 && ( + +
No items match
+ + )} + {filteredItems.map((item) => ( + { + onChange(item.label) + setQuery(item.label) + }} + > + {({ focus, selected }) => ( + // This *could* be done with data-[focus] and data-[selected] instead, but + // it would be a lot more verbose. those can only be used with TW classes, + // not our .is-selected and .is-highlighted, so we'd have to copy the pieces + // of those rules one by one. Better to rely on the shared classes. +
+ {item.label} +
+ )} + + ))} + + )} ) diff --git a/app/ui/lib/Listbox.tsx b/app/ui/lib/Listbox.tsx index 32289a81b..a910d4bbe 100644 --- a/app/ui/lib/Listbox.tsx +++ b/app/ui/lib/Listbox.tsx @@ -44,6 +44,7 @@ export interface ListboxProps { isLoading?: boolean /** Necessary if you want RHF to be able to focus it on error */ buttonRef?: Ref + hideOptionalTag?: boolean } export const Listbox = ({ @@ -62,6 +63,7 @@ export const Listbox = ({ disabled, isLoading = false, buttonRef, + hideOptionalTag, ...props }: ListboxProps) => { const selectedItem = selected && items.find((i) => i.value === selected) @@ -83,7 +85,12 @@ export const Listbox = ({ <> {label && (
- + {description && {description}} diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 9949affa2..7046fedbd 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -6,7 +6,14 @@ * Copyright Oxide Computer Company */ -import { clickRowAction, expect, expectRowVisible, test } from './utils' +import { + clickRowAction, + expect, + expectRowVisible, + selectOption, + test, + type Locator, +} from './utils' const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp'] @@ -38,17 +45,15 @@ test('can create firewall rule', async ({ page }) => { // add targets with overlapping names and types to test delete const targets = page.getByRole('table', { name: 'Targets' }) - await page.getByRole('button', { name: 'Target type' }).click() - await page.getByRole('option', { name: 'IP', exact: true }).click() + await selectOption(page, 'Target type', 'IP') await page.getByRole('textbox', { name: 'IP address' }).fill('192.168.0.1') await page.getByRole('button', { name: 'Add target' }).click() await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) // add host filter instance "host-filter-instance" - await page.locator('role=button[name*="Host type"]').click() - await page.locator('role=option[name="Instance"]').click() - await page.fill('role=textbox[name="Instance name"]', 'host-filter-instance') - await page.locator('text="Add host filter"').click() + await selectOption(page, 'Host type', 'Instance') + await page.getByRole('combobox', { name: 'Instance name' }).fill('host-filter-instance') + await page.getByRole('button', { name: 'Add host filter' }).click() // host is added to hosts table const hosts = page.getByRole('table', { name: 'Host filters' }) @@ -136,6 +141,13 @@ test('firewall rule targets and filters overflow', async ({ page }) => { await expect(tooltip).toBeVisible() }) +const deleteRowAndVerifyRowCount = async (table: Locator, expectedCount: number) => { + const rows = table.getByRole('row') + // skip the header row + await rows.nth(1).getByRole('button', { name: 'remove' }).click() + await expect(rows).toHaveCount(expectedCount) +} + test('firewall rule form targets table', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc') await page.getByRole('tab', { name: 'Firewall Rules' }).click() @@ -144,43 +156,49 @@ test('firewall rule form targets table', async ({ page }) => { await page.getByRole('link', { name: 'New rule' }).click() const targets = page.getByRole('table', { name: 'Targets' }) + const targetVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(0) const addButton = page.getByRole('button', { name: 'Add target' }) // add targets with overlapping names and types to test delete - // there are two of these because the hosts table also defaults to VPC - await page.getByRole('textbox', { name: 'VPC name' }).nth(0).fill('abc') + await targetVpcNameField.fill('abc') await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' }) - await page.getByRole('textbox', { name: 'VPC name' }).nth(0).fill('def') + // enter a VPC called 'mock-subnet', even if that doesn't make sense here, to test dropdown later + await targetVpcNameField.fill('mock-subnet') await addButton.click() - await expectRowVisible(targets, { Type: 'vpc', Value: 'def' }) + await expectRowVisible(targets, { Type: 'vpc', Value: 'mock-subnet' }) + + // add VPC subnet targets + const subnetNameField = page.getByRole('combobox', { name: 'Subnet name' }) - await page.getByRole('button', { name: 'Target type' }).click() - await page.getByRole('option', { name: 'VPC Subnet' }).click() - await page.getByRole('textbox', { name: 'Subnet name' }).fill('abc') + // add a subnet by selecting from a dropdown; make sure 'mock-subnet' is present in the dropdown, + // even though VPC:'mock-subnet' has already been added + await selectOption(page, 'Target type', 'VPC subnet') + await selectOption(page, subnetNameField, 'mock-subnet') + await addButton.click() + await expectRowVisible(targets, { Type: 'subnet', Value: 'mock-subnet' }) + + // now add a subnet by entering text + await selectOption(page, 'Target type', 'VPC subnet') + await subnetNameField.fill('abc') await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' }) - await page.getByRole('button', { name: 'Target type' }).click() - await page.getByRole('option', { name: 'IP', exact: true }).click() + // add IP target + await selectOption(page, 'Target type', 'IP') await page.getByRole('textbox', { name: 'IP address' }).fill('192.168.0.1') await addButton.click() await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) - // test table row delete, only keep the IP one - const firstDeleteButton = targets - .getByRole('row') - .nth(1) - .getByRole('button', { name: 'remove' }) - await expect(targets.getByRole('row')).toHaveCount(5) - await firstDeleteButton.click() - await expect(targets.getByRole('row')).toHaveCount(4) - await firstDeleteButton.click() - await expect(targets.getByRole('row')).toHaveCount(3) - await firstDeleteButton.click() - await expect(targets.getByRole('row')).toHaveCount(2) + // test table row delete; only keep the IP one + // we start with 6 rows, because the header row counts as one + await expect(targets.getByRole('row')).toHaveCount(6) + await deleteRowAndVerifyRowCount(targets, 5) + await deleteRowAndVerifyRowCount(targets, 4) + await deleteRowAndVerifyRowCount(targets, 3) + await deleteRowAndVerifyRowCount(targets, 2) // we still have the IP target await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) @@ -194,43 +212,40 @@ test('firewall rule form hosts table', async ({ page }) => { await page.getByRole('link', { name: 'New rule' }).click() const hosts = page.getByRole('table', { name: 'Host filters' }) + const hostFiltersVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(1) const addButton = page.getByRole('button', { name: 'Add host filter' }) // add hosts with overlapping names and types to test delete - // there are two of these because the targets table also defaults to VPC - await page.getByRole('textbox', { name: 'VPC name' }).nth(1).fill('abc') + await hostFiltersVpcNameField.fill('abc') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' }) - await page.getByRole('textbox', { name: 'VPC name' }).nth(1).fill('def') + await hostFiltersVpcNameField.fill('def') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'def' }) - await page.getByRole('button', { name: 'Host type' }).click() - await page.getByRole('option', { name: 'VPC Subnet' }).click() - await page.getByRole('textbox', { name: 'Subnet name' }).fill('abc') + await selectOption(page, 'Host type', 'VPC subnet') + await selectOption(page, 'Subnet name', 'mock-subnet') + await addButton.click() + await expectRowVisible(hosts, { Type: 'subnet', Value: 'mock-subnet' }) + + await selectOption(page, 'Host type', 'VPC subnet') + await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc') await addButton.click() await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' }) - await page.getByRole('button', { name: 'Host type' }).click() - await page.getByRole('option', { name: 'IP', exact: true }).click() + await selectOption(page, 'Host type', 'IP') await page.getByRole('textbox', { name: 'IP address' }).fill('192.168.0.1') await addButton.click() await expectRowVisible(hosts, { Type: 'ip', Value: '192.168.0.1' }) - // test table row delete, only keep the IP one - const firstDeleteButton = hosts - .getByRole('row') - .nth(1) - .getByRole('button', { name: 'remove' }) - await expect(hosts.getByRole('row')).toHaveCount(5) - await firstDeleteButton.click() - await expect(hosts.getByRole('row')).toHaveCount(4) - await firstDeleteButton.click() - await expect(hosts.getByRole('row')).toHaveCount(3) - await firstDeleteButton.click() - await expect(hosts.getByRole('row')).toHaveCount(2) + // test table row delete; only keep the IP one + await expect(hosts.getByRole('row')).toHaveCount(6) + await deleteRowAndVerifyRowCount(hosts, 5) + await deleteRowAndVerifyRowCount(hosts, 4) + await deleteRowAndVerifyRowCount(hosts, 3) + await deleteRowAndVerifyRowCount(hosts, 2) // we still have the IP target await expectRowVisible(hosts, { Type: 'ip', Value: '192.168.0.1' }) @@ -243,7 +258,7 @@ test('can update firewall rule', async ({ page }) => { const rows = page.locator('tbody >> tr') await expect(rows).toHaveCount(3) - // allow-icmp is the one we're doing to change + // allow-icmp is the one we're going to change const oldNameCell = page.locator('td >> text="allow-icmp"') await expect(oldNameCell).toBeVisible() @@ -259,13 +274,11 @@ test('can update firewall rule', async ({ page }) => { // modal is now open await expect(modal).toBeVisible() - // TODO: get these by their label when that becomes easier to do - // name is populated - await expect(page.locator('input[name=name]')).toHaveValue('allow-icmp') + await expect(page.getByRole('textbox', { name: 'Name' })).toHaveValue('allow-icmp') // priority is populated - await expect(page.locator('role=textbox[name=Priority]')).toHaveValue('65534') + await expect(page.getByRole('textbox', { name: 'Priority' })).toHaveValue('65534') // protocol is populated await expect(page.locator('label >> text=ICMP')).toBeChecked() @@ -277,19 +290,19 @@ test('can update firewall rule', async ({ page }) => { // screen.getByRole('cell', { name: 'default' }) // update name - await page.fill('input[name=name]', 'new-rule-name') + await page.getByRole('textbox', { name: 'Name' }).fill('new-rule-name') // add host filter - await page.locator('role=button[name*="Host type"]').click() - await page.locator('role=option[name="VPC Subnet"]').click() - await page.fill('role=textbox[name="Subnet name"]', 'edit-filter-subnet') - await page.locator('text="Add host filter"').click() + await selectOption(page, 'Host type', 'VPC subnet') + await page.getByRole('combobox', { name: 'Subnet name' }).fill('edit-filter-subnet') + await page.getByRole('button', { name: 'Add host filter' }).click() // new host is added to hosts table - await expect(page.locator('role=cell >> text="edit-filter-subnet"')).toBeVisible() + const hosts = page.getByRole('table', { name: 'Host filters' }) + await expectRowVisible(hosts, { Type: 'subnet', Value: 'edit-filter-subnet' }) // submit the form - await page.locator('text="Update rule"').click() + await page.getByRole('button', { name: 'Update rule' }).click() // modal closes again await expect(modal).toBeHidden() diff --git a/test/e2e/silos.e2e.ts b/test/e2e/silos.e2e.ts index 136057e8e..b9e5a9279 100644 --- a/test/e2e/silos.e2e.ts +++ b/test/e2e/silos.e2e.ts @@ -263,6 +263,10 @@ test('Silo IP pools link pool', async ({ page }) => { await page.getByRole('button', { name: 'Link pool' }).click() await expect(modal).toBeVisible() + // verify that combobox's "no items match" appears when addNewItems prop is false or missing + await page.getByPlaceholder('Select a pool').fill('x') + await expect(page.getByText('No items match')).toBeVisible() + // select silo in combobox and click link await page.getByPlaceholder('Select a pool').fill('ip-pool') await page.getByRole('option', { name: 'ip-pool-3' }).click() diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index fe141ebf4..285bb313c 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -140,6 +140,28 @@ export async function clickRowAction(page: Page, rowText: string, actionName: st await page.getByRole('menuitem', { name: actionName }).click() } +/** + * Select an option from a dropdown + * buttonLocator can either be the drodown's label text or a more elaborate Locator + * optionLocator can either be the drodown's label text or a more elaborate Locator + * */ +export async function selectOption( + page: Page, + buttonLocator: string | Locator, + optionLocator: string | Locator +) { + if (typeof buttonLocator === 'string') { + await page.getByRole('button', { name: buttonLocator }).click() + } else { + await buttonLocator.click() + } + if (typeof optionLocator === 'string') { + await page.getByRole('option', { name: optionLocator, exact: true }).click() + } else { + await optionLocator.click() + } +} + export async function getPageAsUser( browser: Browser, user: 'Hans Jonas' | 'Simone de Beauvoir'