Improve Application Config UX (awslabs#477)

* Improve Application Configuration UX 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. * small pre-PR cleanup * Selected tier should be default tier on appconfig page --------- Co-authored-by: PoeppingT <[email protected]>
beansandroasters · Feb 10, 2023 · f843997 · f843997
1 parent ed0255f
commit f843997
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 93 deletions.
diff --git a/client/web/src/settings/ApplicationComponent.js b/client/web/src/settings/ApplicationComponent.js
@@ -171,7 +171,6 @@ export function ApplicationComponent(props) {
       provisionObjectStorage: !!thisService?.s3,
       windowsVersion: windowsVersion,
       tiers: initialTierValues,
-      tombstone: false,
     }
   }
 
@@ -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()
@@ -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.'),
@@ -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
           })
@@ -304,13 +278,7 @@ 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.'),
         windowsVersion: Yup.string().when('operatingSystem', {
           is: (containerOs) => containerOs && containerOs === WINDOWS,
           then: Yup.string().required('Windows version is a required field'),
@@ -322,9 +290,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'),
@@ -342,10 +308,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(),
@@ -404,7 +367,6 @@ export function ApplicationComponent(props) {
             </span>
           </Alert>
         )}
-        {/* {loading !== "idle" && <div>Loading...</div>} */}
         {!!error && (
           <Alert variant="danger" isOpen={!!error} toggle={dismissError}>
             {error}
@@ -419,8 +381,8 @@ export function ApplicationComponent(props) {
           initialValues={initialValues}
           validationSchema={validationSpecs}
           validateOnChange={true}
+          validateOnBlur={false}
           onSubmit={updateConfig}
-          enableReinitialize={true}
         >
           {(formik) => {
             return (
@@ -438,7 +400,6 @@ export function ApplicationComponent(props) {
                   ></AppSettingsSubform>
                   <ServicesComponent
                     formik={formik}
-                    formikErrors={formik.errors}
                     hasTenants={hasTenants}
                     osOptions={osOptions}
                     dbOptions={dbOptions}

diff --git a/client/web/src/settings/ApplicationContainer.js b/client/web/src/settings/ApplicationContainer.js
@@ -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) {
@@ -203,7 +202,6 @@ export function ApplicationContainer(props) {
           ecsLaunchType,
           provisionDb,
           provisionObjectStorage,
-          tombstone,
           database,
           ...rest
         } = thisService

diff --git a/client/web/src/settings/ServiceSettingsSubform.js b/client/web/src/settings/ServiceSettingsSubform.js
@@ -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'
@@ -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
@@ -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>
@@ -86,15 +84,6 @@ const ServiceSettingsSubform = (props) => {
            Fargate
          </Label>
        </FormGroup>
-       <FormFeedback
-         invalid={
-           formikErrors.ecsLaunchType
-             ? formikErrors.ecsLaunchType
-             : undefined
-         }
-       >
-         {formikErrors.ecsLaunchType}
-       </FormFeedback>
      </FormGroup>
     )
   }
@@ -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">
@@ -169,11 +159,6 @@ const ServiceSettingsSubform = (props) => {
                       <CIcon icon={cibWindows} /> Windows
                       </Label>
                     </FormGroup>
-                    <FormFeedback
-                      invalid={!!operatingSystemFeedback}
-                    >
-                      {operatingSystemFeedback}
-                    </FormFeedback>
                   </FormGroup>
                   {getLaunchTypeOptions(serviceIndex)}
                   {getWinServerOptions(serviceIndex)}
@@ -242,7 +227,6 @@ ServiceSettingsSubform.propTypes = {
   dbOptions: PropTypes.array,
   onFileSelected: PropTypes.func,
   serviceValues: PropTypes.object,
-  formikErrors: PropTypes.object,
   serviceIndex: PropTypes.number,
 }
 

diff --git a/client/web/src/settings/ServicesComponent.js b/client/web/src/settings/ServicesComponent.js
@@ -24,7 +24,6 @@ import { PropTypes } from 'prop-types'
 const ServicesComponent = (props) => {
   const {
     formik,
-    formikErrors,
     hasTenants,
     osOptions,
     dbOptions,
@@ -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()
   }
 
@@ -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}

diff --git a/client/web/src/settings/TierServiceSettingsSubform.js b/client/web/src/settings/TierServiceSettingsSubform.js
@@ -32,9 +32,7 @@ const TierServiceSettingsSubform = (props) => {
     setFieldValue
   } = props
 
-  const [selectedTier, setSelectedTier] = useState(
-    !!tiers && !!tiers[0] ? tiers[0].name : ''
-  )
+  const [selectedTier, setSelectedTier] = useState(defaultTier)
 
   const formikTierPrefix = formikServicePrefix + '.tiers[' + selectedTier + ']'