Elisa/5818 facility form #5983

emyl3 · 2023-06-14T16:43:39Z

FRONTEND PULL REQUEST

Related Issue

Resolves #5818

Changes Proposed

Fix accessibility concerns raised in [Add Facility] Form not using proper element and error messages are not clear. #5818
Refactor facility form to use react-hook-form library
Remove FacilitySchema.ts

Additional Information

Testing

Ensure facilities can be added or edited
Validation and error messages work correctly
- particularly for edge cases:
Addresses are being properly validated
Axe dev tools scan passes
on dev5 for testing

Screenshots / Demos

For large changes, please pair with a designer to ensure changes are as intended

emyl3 · 2023-06-14T19:38:20Z

frontend/src/app/Settings/Facility/Components/OrderingProvider.tsx

-  const { orderingProvider: provider } = facility;
-  const isRequired = requiresOrderProvider(facility.state || "");
+  const isValidNPI = (npi: string) => {
+    return /^\d{10}$/.test(npi);


⚠️ previously the regex to validate the NPI was /^\d{1,10}$/, which means the NPI could be set to anything with less than 10 digits which according to our error message is incorrect.

Let me know if I should change this back to the previous regex.

emyl3 · 2023-06-15T13:28:39Z

frontend/src/app/Settings/Facility/FacilityForm.tsx

    const zipCodeData = await getZipCodeData(originalFacilityAddress.zipCode);
    const isValidZipForState = isValidZipCodeForState(
      originalFacilityAddress.state,
      zipCodeData
    );

    if (!isValidZipForState) {
-      showError("Invalid ZIP code for this state", "Form Errors");
+      setError("facility.zipCode", {


⚠️ currently only validating if the facility's zip code and state combination is valid

we are not checking if the ordering provider's zip code and state (if provided, since it is not required) are valid combinations -- this is the current behavior as well

emyl3 · 2023-06-15T13:37:44Z

frontend/src/app/Settings/Facility/Components/OrderingProvider.tsx

+          registrationProps={register("orderingProvider.zipCode", {
+            required: false,
+            validate: {
+              validZip: (zipCode: string) =>


⚠️ decided against adding if the zip code was valid for the state validation here because we were making too many requests and getting back 429 errors from SmartyStreets 😅 -- so like it was done before, checking if the zip code and state combination are good only on submit after the form passes initial validation check

BobanL · 2023-06-15T15:35:00Z

frontend/src/app/Settings/Facility/Components/FacilityInformation.tsx

+          validationStatus={errors?.facility?.email?.type ? "error" : undefined}
+          errorMessage={errors?.facility?.email?.message}
+          registrationProps={register("facility.email", {
+            required: false,


nit: I believe required is defaulted to false so we don't need to specify this field,

ooo thank you!

johanna-skylight

Great job on the migration! I tested the form and it works smoothly. I left some questions but overall this looks awesome. Thanks for the work!

johanna-skylight · 2023-06-15T15:53:42Z

frontend/src/app/Settings/Facility/FacilityForm.tsx

+    clearErrors,
+    formState: { errors, isSubmitting, isDirty },
+  } = useForm({
+    mode: "onBlur",


Can the validation be set on submit?. I have found that the validation on blur is disruptive when using screen readers plus it will be more performant. LMK!

ooo ok! Yes, I will remove this line as I think the default onsubmit! Thank you!

johanna-skylight · 2023-06-15T15:56:20Z

frontend/src/app/Settings/Facility/Components/FacilityInformation.tsx

+              </span>
+            )
+          }
+          onChange={() => {}}


Can the onChange prop in the Dropdown component be set as optional so we don't have to set dummy handlers?

oooo good call!

johanna-skylight · 2023-06-15T15:59:50Z

frontend/src/app/Settings/Facility/Components/ManageDevices.tsx

-          onChange={(newDeviceIds) => {
-            updateDevices(newDeviceIds);
-          }}
+          onChange={onChange}


Is this onChange being triggered or is react-hook-forms overriding it? If it is the later I would suggest removing the onChange prop from this component and making the onChange prop in the multi select optional so we can clean up the code that is not really being used.

Yes! This onChange is being triggered to update the selected devices and display an error if the device list is empty. Based on that, let me know if you see room for improvement! Thank you!!

got more clarity on co edge case

BobanL

👍

emyl3 · 2023-06-22T16:43:30Z

@fzhao99 @BobanL @johanna-skylight -- so sorry everyone who reviewed! found one last bug where the suggested address wasn't showing up when provided with a ordering provider address -- added a test for this. Ready for (hopefully - final 🤞 😅) review!!
on dev5 for testing!

fzhao99

thank you for breaking out the strings!

sonarcloud · 2023-06-22T22:08:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.8% Coverage
0.0% Duplication

emyl3 temporarily deployed to Dev5 June 14, 2023 16:56 — with GitHub Actions Inactive

emyl3 force-pushed the elisa/5818-facility-form branch from 56f33d6 to 209d3e8 Compare June 14, 2023 18:31

emyl3 temporarily deployed to Dev5 June 14, 2023 18:54 — with GitHub Actions Inactive

emyl3 force-pushed the elisa/5818-facility-form branch 2 times, most recently from 899a5af to 0875d86 Compare June 14, 2023 19:21

emyl3 commented Jun 14, 2023

View reviewed changes

emyl3 temporarily deployed to Dev5 June 14, 2023 19:42 — with GitHub Actions Inactive

emyl3 force-pushed the elisa/5818-facility-form branch 2 times, most recently from 0cf185a to 4740da9 Compare June 14, 2023 22:34

emyl3 temporarily deployed to Dev5 June 14, 2023 22:49 — with GitHub Actions Inactive

emyl3 force-pushed the elisa/5818-facility-form branch from 4740da9 to 14e7cee Compare June 14, 2023 22:50

emyl3 temporarily deployed to Dev5 June 14, 2023 23:04 — with GitHub Actions Inactive

emyl3 force-pushed the elisa/5818-facility-form branch from 14e7cee to 6a763fd Compare June 14, 2023 23:55

emyl3 temporarily deployed to Dev5 June 15, 2023 00:11 — with GitHub Actions Inactive

emyl3 force-pushed the elisa/5818-facility-form branch from 6a763fd to 68d7196 Compare June 15, 2023 00:33

emyl3 temporarily deployed to Dev5 June 15, 2023 00:47 — with GitHub Actions Inactive

emyl3 force-pushed the elisa/5818-facility-form branch from 68d7196 to e82824d Compare June 15, 2023 01:45

emyl3 temporarily deployed to Dev5 June 15, 2023 03:37 — with GitHub Actions Inactive

Refactor facilityForm to use react-hook-form

876fe4b

emyl3 force-pushed the elisa/5818-facility-form branch from e82824d to 876fe4b Compare June 15, 2023 04:05

emyl3 temporarily deployed to Dev5 June 15, 2023 13:18 — with GitHub Actions Inactive

emyl3 commented Jun 15, 2023

View reviewed changes

emyl3 marked this pull request as ready for review June 15, 2023 14:13

emyl3 requested review from johanna-skylight, mehansen, BobanL and fzhao99 June 15, 2023 14:13

BobanL reviewed Jun 15, 2023

View reviewed changes

johanna-skylight reviewed Jun 15, 2023

View reviewed changes

emyl3 dismissed stale reviews from johanna-skylight and BobanL via 91a7f96 June 20, 2023 23:47

emyl3 temporarily deployed to Dev5 June 21, 2023 00:19 — with GitHub Actions Inactive

fzhao99 self-requested a review June 21, 2023 13:23

fzhao99 removed their request for review June 21, 2023 13:28

Merge branch 'main' into elisa/5818-facility-form

1d52f11

emyl3 temporarily deployed to Dev5 June 21, 2023 21:04 — with GitHub Actions Inactive

Merge branch 'main' into elisa/5818-facility-form

ac3d0f7

emyl3 temporarily deployed to Dev5 June 21, 2023 23:08 — with GitHub Actions Inactive

Merge branch 'main' into elisa/5818-facility-form

293a96b

BobanL previously approved these changes Jun 22, 2023

View reviewed changes

johanna-skylight previously approved these changes Jun 22, 2023

View reviewed changes

Merge branch 'main' into elisa/5818-facility-form

fc47c3c

emyl3 temporarily deployed to Dev5 June 22, 2023 14:56 — with GitHub Actions Inactive

fix ordering provider address suggestion

791727f

emyl3 dismissed stale reviews from johanna-skylight and BobanL via 791727f June 22, 2023 16:04

emyl3 temporarily deployed to Dev5 June 22, 2023 16:28 — with GitHub Actions Inactive

emyl3 requested review from fzhao99, BobanL and johanna-skylight June 22, 2023 16:41

fzhao99 approved these changes Jun 22, 2023

View reviewed changes

BobanL approved these changes Jun 22, 2023

View reviewed changes

Merge branch 'main' into elisa/5818-facility-form

e484529

emyl3 merged commit 7a7363d into main Jun 22, 2023

emyl3 deleted the elisa/5818-facility-form branch June 22, 2023 22:12

emyl3 mentioned this pull request Jun 23, 2023

Fix facilities dropdown #6030

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elisa/5818 facility form #5983

Elisa/5818 facility form #5983

emyl3 commented Jun 14, 2023 •

edited

Loading

emyl3 Jun 14, 2023

emyl3 Jun 15, 2023

emyl3 Jun 15, 2023

BobanL Jun 15, 2023 •

edited

Loading

emyl3 Jun 16, 2023

johanna-skylight left a comment

johanna-skylight Jun 15, 2023 •

edited

Loading

emyl3 Jun 16, 2023

johanna-skylight Jun 15, 2023

emyl3 Jun 16, 2023

johanna-skylight Jun 15, 2023

emyl3 Jun 16, 2023

BobanL left a comment

emyl3 commented Jun 22, 2023

fzhao99 left a comment

sonarcloud bot commented Jun 22, 2023

Elisa/5818 facility form #5983

Elisa/5818 facility form #5983

Conversation

emyl3 commented Jun 14, 2023 • edited Loading

FRONTEND PULL REQUEST

Related Issue

Changes Proposed

Additional Information

Testing

Screenshots / Demos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BobanL Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanna-skylight left a comment

Choose a reason for hiding this comment

johanna-skylight Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BobanL left a comment

Choose a reason for hiding this comment

emyl3 commented Jun 22, 2023

fzhao99 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 22, 2023

emyl3 commented Jun 14, 2023 •

edited

Loading

BobanL Jun 15, 2023 •

edited

Loading

johanna-skylight Jun 15, 2023 •

edited

Loading