Skip to content

Commit

Permalink
Improve Application Configuration UX
Browse files Browse the repository at this point in the history 
This commit improves on multiple issues with the existing application
configuration page in the admin UI:
1. deleting a service now actually deletes the errors for that service
   from the top banner
2. creating a new service automatically opens the accordion banner for
   that new service
3. the validation error banner won't be shown for new services until any
   error is made (assuming there are not existing errors in other
   services)
4. the accordion name is now controlled directly by the form, rather
   than an alternate state

4/ in the above list should also address awslabs#474, but until we have
reproduction we cannot know for sure.
  • Loading branch information
PoeppingT committed Feb 8, 2023
1 parent ed0255f commit f052660
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 89 deletions.
76 changes: 21 additions & 55 deletions client/web/src/settings/ApplicationComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ export function ApplicationComponent(props) {
provisionObjectStorage: !!thisService?.s3,
windowsVersion: windowsVersion,
tiers: initialTierValues,
tombstone: false,
}
}

Expand Down Expand Up @@ -199,27 +198,20 @@ export function ApplicationComponent(props) {
}

// min, max, computeSize, cpu/memory/instanceType (not in form), filesystem, database
const singleTierValidationSpec = (tombstone) => {
const singleTierValidationSpec = () => {
return Yup.object({
min: requiredIfNotTombstoned(
tombstone,
Yup.number()
.integer('Minimum count must be an integer value')
.min(1, 'Minimum count must be at least ${min}'),
'Minimum count is a required field.'
),
min: Yup.number()
.integer('Minimum count must be an integer value')
.min(1, 'Minimum count must be at least ${min}')
.required('Minimum count is a required field.'),
max: Yup.number()
.required('Maximum count is a required field.')
.integer('Maximum count must be an integer value')
.max(10, 'Maximum count can be no larger than ${max}')
.test('match', 'Max cannot be smaller than min', function (max) {
return max >= this.parent.min
}),
computeSize: requiredIfNotTombstoned(
tombstone,
Yup.string(),
'Compute size is a required field.'
),
computeSize: Yup.string().required('Compute size is a required field.'),
filesystem: Yup.object().when(['provisionFS', 'filesystemType'], (provisionFS, filesystemType) => {
if (provisionFS) {
return FILESYSTEM_TYPES[filesystemType]?.validationSchema || Yup.object()
Expand All @@ -229,34 +221,26 @@ export function ApplicationComponent(props) {
})
}

const allTiersValidationSpec = (tombstone) => {
const allTiersValidationSpec = () => {
let allTiers = {}
for (var i = 0; i < tiers.length; i++) {
var tierName = tiers[i].name
allTiers[tierName] = singleTierValidationSpec(tombstone)
allTiers[tierName] = singleTierValidationSpec()
}
return Yup.object(allTiers)
}

const allTiersDatabaseValidationSpec = (tombstone) => {
const allTiersDatabaseValidationSpec = () => {
let allTiers = {}
for (var i = 0; i < tiers.length; i++) {
var tierName = tiers[i].name
allTiers[tierName] = Yup.object({
instance: requiredIfNotTombstoned(
tombstone,
Yup.string(),
'Database instance is required.'
)
instance: Yup.string().required('Database instance is required.')
})
}
return Yup.object(allTiers)
}

const requiredIfNotTombstoned = (tombstone, schema, requiredMessage) => {
return tombstone ? schema : schema.required(requiredMessage)
}

// TODO public service paths cannot match
const validationSpecs = Yup.object({
name: Yup.string().required('Name is a required field.'),
Expand All @@ -268,26 +252,16 @@ export function ApplicationComponent(props) {
Yup.object({
public: Yup.boolean().required(),
name: Yup.string()
.when('tombstone', (tombstone, schema) => {
return requiredIfNotTombstoned(
tombstone,
schema,
'Service Name is a required field.'
)
})
.required('Service Name is a required field.')
.matches(
/^[\.\-_a-zA-Z0-9]+$/,
'Name must only contain alphanumeric characters or .-_'
),
description: Yup.string(),
path: Yup.string()
.when(['public', 'tombstone'], (isPublic, tombstone, schema) => {
.when(['public'], (isPublic, schema) => {
if (isPublic) {
return requiredIfNotTombstoned(
tombstone,
schema,
'Path is required for public services'
)
return schema.required('Path is required for public services')
}
return schema
})
Expand All @@ -304,12 +278,11 @@ export function ApplicationComponent(props) {
healthCheckUrl: Yup.string()
.required('Health Check URL is a required field')
.matches(/^\//, 'Health Check must start with forward slash (/)'),
operatingSystem: Yup.string().when('tombstone', (tombstone, schema) => {
return requiredIfNotTombstoned(
tombstone,
schema,
'Container OS is a required field.'
)
operatingSystem: Yup.string().required('Container OS is a required field.'),
ecsLaunchType: Yup.string().when('operatingSystem', {
is: (containerOs) => containerOs && containerOs === LINUX,
then: Yup.string().required('Container launch type must be chosen'),
otherwise: Yup.string().nullable(),
}),
windowsVersion: Yup.string().when('operatingSystem', {
is: (containerOs) => containerOs && containerOs === WINDOWS,
Expand All @@ -322,9 +295,7 @@ export function ApplicationComponent(props) {
then: Yup.object({
engine: Yup.string().required('Engine is required'),
version: Yup.string().required('Version is required'),
tiers: Yup.object().when('tombstone', (tombstone, schema) => {
return allTiersDatabaseValidationSpec(tombstone)
}),
tiers: allTiersDatabaseValidationSpec(),
username: Yup.string()
.matches('^[a-zA-Z]+[a-zA-Z0-9_$]*$', 'Username is not valid')
.required('Username is required'),
Expand All @@ -342,10 +313,7 @@ export function ApplicationComponent(props) {
otherwise: Yup.object(),
}),
provisionObjectStorage: Yup.boolean(),
tiers: Yup.object().when(['tombstone'], (tombstone) => {
return allTiersValidationSpec(tombstone)
}),
tombstone: Yup.boolean(),
tiers: allTiersValidationSpec(),
})
).min(1, 'Application must have at least ${min} service(s).'),
provisionBilling: Yup.boolean(),
Expand Down Expand Up @@ -404,7 +372,6 @@ export function ApplicationComponent(props) {
</span>
</Alert>
)}
{/* {loading !== "idle" && <div>Loading...</div>} */}
{!!error && (
<Alert variant="danger" isOpen={!!error} toggle={dismissError}>
{error}
Expand All @@ -419,8 +386,8 @@ export function ApplicationComponent(props) {
initialValues={initialValues}
validationSchema={validationSpecs}
validateOnChange={true}
validateOnBlur={false}
onSubmit={updateConfig}
enableReinitialize={true}
>
{(formik) => {
return (
Expand All @@ -438,7 +405,6 @@ export function ApplicationComponent(props) {
></AppSettingsSubform>
<ServicesComponent
formik={formik}
formikErrors={formik.errors}
hasTenants={hasTenants}
osOptions={osOptions}
dbOptions={dbOptions}
Expand Down
2 changes: 0 additions & 2 deletions client/web/src/settings/ApplicationContainer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ export function ApplicationContainer(props) {
let cleanedServicesMap = {}
for (var serviceIndex in services) {
let thisService = services[serviceIndex]
if (thisService.tombstone) continue
// update the tier config
let cleanedTiersMap = {}
for (var tierName in thisService.tiers) {
Expand All @@ -203,7 +202,6 @@ export function ApplicationContainer(props) {
ecsLaunchType,
provisionDb,
provisionObjectStorage,
tombstone,
database,
...rest
} = thisService
Expand Down
22 changes: 3 additions & 19 deletions client/web/src/settings/ServiceSettingsSubform.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import React from 'react'
import { Row, Col, Card, CardBody, CardHeader, FormGroup, Label, FormFeedback } from 'reactstrap'
import { Row, Col, Card, CardBody, CardHeader, FormGroup, Label } from 'reactstrap'
import { Field } from 'formik'
import { SaasBoostSelect, SaasBoostCheckbox, SaasBoostInput, SaasBoostTextarea } from '../components/FormComponents'
import { PropTypes } from 'prop-types'
Expand All @@ -25,7 +25,7 @@ import DatabaseSubform from './DatabaseSubform'
import ObjectStoreSubform from './components/ObjectStoreSubform'

const ServiceSettingsSubform = (props) => {
const { formikErrors, serviceValues, osOptions, dbOptions, serviceName, onFileSelected, isLocked, serviceIndex } = props
const { serviceValues, osOptions, dbOptions, serviceName, onFileSelected, isLocked, serviceIndex } = props
const getWinServerOptions = (serviceIndex) => {
if (!osOptions) {
return null
Expand Down Expand Up @@ -54,8 +54,6 @@ const ServiceSettingsSubform = (props) => {
) : null
}

const operatingSystemFeedback = !!formikErrors.services ? formikErrors.services[serviceIndex]?.operatingSystem : undefined

const getLaunchTypeOptions = (serviceIndex) => {
return serviceValues?.operatingSystem === 'LINUX' && (
<FormGroup>
Expand Down Expand Up @@ -86,15 +84,6 @@ const ServiceSettingsSubform = (props) => {
Fargate
</Label>
</FormGroup>
<FormFeedback
invalid={
formikErrors.ecsLaunchType
? formikErrors.ecsLaunchType
: undefined
}
>
{formikErrors.ecsLaunchType}
</FormFeedback>
</FormGroup>
)
}
Expand All @@ -115,6 +104,7 @@ const ServiceSettingsSubform = (props) => {
label="Service Name"
name={"services[" + serviceIndex + "].name"}
type="text"
autoFocus
/>
</Col>
<Col className="d-flex align-items-center">
Expand Down Expand Up @@ -169,11 +159,6 @@ const ServiceSettingsSubform = (props) => {
<CIcon icon={cibWindows} /> Windows
</Label>
</FormGroup>
<FormFeedback
invalid={!!operatingSystemFeedback}
>
{operatingSystemFeedback}
</FormFeedback>
</FormGroup>
{getLaunchTypeOptions(serviceIndex)}
{getWinServerOptions(serviceIndex)}
Expand Down Expand Up @@ -242,7 +227,6 @@ ServiceSettingsSubform.propTypes = {
dbOptions: PropTypes.array,
onFileSelected: PropTypes.func,
serviceValues: PropTypes.object,
formikErrors: PropTypes.object,
serviceIndex: PropTypes.number,
}

Expand Down
26 changes: 13 additions & 13 deletions client/web/src/settings/ServicesComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import { PropTypes } from 'prop-types'
const ServicesComponent = (props) => {
const {
formik,
formikErrors,
hasTenants,
osOptions,
dbOptions,
Expand All @@ -33,25 +32,27 @@ const ServicesComponent = (props) => {
initService
} = props

const [services, setServices] = useState(formik.values.services)
const [showModal, setShowModal] = useState(false)
const [openAccordionItem, setOpenAccordionItem] = useState()

const toggleModal = () => {
setShowModal((state) => !state)
}

const addService = (serviceName) => {
let newService = initService(serviceName)
setOpenAccordionItem(formik.values.services.length)
formik.values.services.push(newService)
setServices([...formik.values.services])
formik.validateForm()
}

const deleteService = (index) => {
// we can't just remove this service from the list because it'll mess with our indices
formik.values.services[index].tombstone = true
setServices(formik.values.services)
// kick off validation so the schema recognizes the tombstone and clears any pending errors
// remove one entry starting from `index` from services
formik.values.services.splice(index, 1)
// manually remove this entry from errors if necessary, since Formik apparently doesn't do it itself
formik.errors.services?.splice(index, 1)
if (openAccordionItem === index) {
setOpenAccordionItem(null)
}
formik.validateForm()
}

Expand All @@ -76,18 +77,16 @@ const ServicesComponent = (props) => {
</Row>
</Card.Header>
<Card.Body>
<Accordion>
{services.map(
(service, index) =>
!service.tombstone && (
<Accordion activeKey={openAccordionItem} onSelect={(index) => setOpenAccordionItem(index)}>
{formik.values.services?.map(
(service, index) => (
<Accordion.Item key={service.name} eventKey={index}>
<Accordion.Header>{service.name}</Accordion.Header>
<Accordion.Body>
<ServiceSettingsSubform
formik={formik}
isLocked={hasTenants}
serviceValues={formik.values.services[index]}
formikErrors={formikErrors}
osOptions={osOptions}
dbOptions={dbOptions}
onFileSelected={onFileSelected}
Expand All @@ -104,6 +103,7 @@ const ServicesComponent = (props) => {
dbOptions={dbOptions}
formikServicePrefix={'services[' + index + ']'}
setFieldValue={(k, v) => formik.setFieldValue(k, v)}
validate={formik.validateForm}
></TierServiceSettingsSubform>
<Container className="mt-3">
<Row>
Expand Down

0 comments on commit f052660

Please sign in to comment.