test, upcoming: [M3-7134, M3-7314] - Add integration test for AGLB Edit Certificate flow and improve form validation #9880

mjac0bs · 2023-11-06T22:45:50Z

Description 📝

This PR does two things, because fixing the validation was necessary for writing a complete integration test that confirmed all input errors were handled gracefully.

Add Cypress integration tests to confirm the UI flow for AGLB certification editing works as expected.
Provide form validation to prevent a user from being able to edit a certificate and provide a blank label.

Changes 🔄

Integration tests in load-balancer-certificates.spec.ts test the following:

User can edit certificates from the AGLB certificate page (route: /loadbalancers/:id/certificates)
Certificates page updates to reflect edited certificates
Cloud Manager handles user input errors (e.g. empty or invalid fields/selections) gracefully
Cloud Manager handles server errors (e.g. HTTP 500 responses from the API) gracefully

In the Cloud Manager UI:

A validation error ("Label must not be empty.") is provided when a user attempts to submit the Edit Certificate drawer with a blank label field.

Preview 📷

Before	After
Screen.Recording.2023-11-06.at.3.12.03.PM.mov	Screen.Recording.2023-11-06.at.3.10.57.PM.mov

How to test 🧪

Prerequisites

(How to setup test environment)

Checkout this PR.
Make sure the aglb feature flag is on and turn mocks on.

Reproduction steps

(How to reproduce the issue, if applicable)

Checkout develop.
Go to http://localhost:3000/loadbalancers/0/certificates.
Use the action menu (...) to select the Edit action and open the Edit Certificate drawer.
Clear the pre-populated label field.
Submit the form.
Observe there is no validation error and in the request payload, the blank label is submitted. (Due to the mocks, this isn't visible in the UI.)

Verification steps

(How to verify changes)

On this branch, go to http://localhost:3000/loadbalancers/0/certificates.
Use the action menu (...) to select the Edit action and open the Edit Certificate drawer.
Clear the pre-populated label field.
Submit the form.
Observe there is a validation error Label must not be empty and the form will not submit until a label is provided.

Also confirm the following validation still works as expected by checking the request payload:

When editing a TLS certificate:
- A user can edit just the form label, and the form will submit. The certificate will be excluded from the payload since it has not changed.
- A user can edit just the key, and the form will submit. The existing certificate will be included in the payload, since updating a key requires a cert.
- A user cannot edit just the cert and will see a validation error on Private Key. This is invalid because updating a cert requires a key, and API does not return the key for security.
- A user can edit both the cert and the key, and the form will submit. Both the edited cert and edited key will be in the payload.

Confirm the integration test passes:

yarn cy:run -s "cypress/e2e/core/loadBalancers/load-balancer-certificates.spec.ts"

As an Author I have considered 🤔

Check all that apply

mjac0bs · 2023-11-06T23:26:30Z

...manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx

      {!certificate ? (
        <Notice variant="error">Error loading certificate.</Notice>
      ) : (
        <form onSubmit={formik.handleSubmit}>
-          {errorMap.none && <Notice text={errorMap.none} variant="error" />}


Somehow an extra error notice had slipped in originally -- cleaned this up here.

mjac0bs · 2023-11-06T23:28:36Z

...manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx

+    validateOnBlur: !error,
+    validateOnChange: !error,


Following suit with what we did in another edit drawer even though this doesn't look as nice, it seems better than have disappearing errors if a user were to clear the label and update the cert, but not provide a key (see PR description video).

bnussman-akamai

Changes look great! Thanks for all of the tests too 🧪

coliu-akamai

Looks good! Confirmed everything listed, just had one small comment below.

(Edit: not sure why GitHub isn't showing the rest of my comment, so I'll just put the checks here above the video
Confirmed that all tests pass! ✅
Confirmed that error appears! ✅
Confirmed previous validation: ✅

✅ A user can edit just the form label, and the form will submit. The certificate will be excluded from the payload since it has not changed.
✅ A user can edit just the key, and the form will submit. The existing certificate will be included in the payload, since updating a key requires a cert.
✅ A user cannot edit just the cert and will see a validation error on Private Key. This is invalid because updating a cert requires a key, and API does not return the key for security.
✅ A user can edit both the cert and the key, and the form will submit. Both the edited cert and edited key will be in the payload.)

Could we add a scroll to error, especially for the forms that have the Private Key field?

Screen.Recording.2023-11-07.at.10.45.36.AM.mov

dwiley-akamai

Integration test passes ✅

"Label must not be empty" validation error when attempting to submit an edited cert with no label ✅

Edit just the form label, and the form will submit. The certificate will be excluded from the payload since it has not changed. ✅

Edit just the key, and the form will submit. The existing certificate will be included in the payload, since updating a key requires a cert. ✅

Cannot edit just the cert. See a validation error on Private Key. ✅

Can edit both the cert and the key, and the form will submit. Both the edited cert and edited key are in the payload. ✅

mjac0bs · 2023-11-07T23:03:33Z

Could we add a scroll to error, especially for the forms that have the Private Key field?

@coliu-akamai Good call out for smaller screens!

I implemented scroll-into-view for the Edit drawer in 423e64c.

Screen.Recording.2023-11-07.at.3.00.23.PM.mov

Since we'd want the same behavior in the Create drawer, so I made the change there too. (Also noticed that the Create drawer is not an expanded width, which is a bad UX when the user pastes long certs, so I adjusted in 7624cec. Not very evident in the screencap because I squished my viewport to get the drawer to scroll.)

Screen.Recording.2023-11-07.at.3.01.04.PM.mov

coliu-akamai

awesome, thanks Mariah! 🎉

mjac0bs added 4 commits November 6, 2023 14:28

WIP

70c2874

Add label validation to Edit Cert Drawer

e3e732a

Add label validation check to test

765cf02

Clean up after rebase

a88c425

mjac0bs added the ACLB Relating to the Akamai Cloud Load Balancer label Nov 6, 2023

mjac0bs self-assigned this Nov 6, 2023

Add changesets

b19b43f

mjac0bs commented Nov 6, 2023

View reviewed changes

mjac0bs marked this pull request as ready for review November 6, 2023 23:29

mjac0bs requested a review from a team as a code owner November 6, 2023 23:29

mjac0bs requested review from dwiley-akamai, coliu-akamai, bnussman-akamai and cliu-akamai and removed request for a team November 6, 2023 23:29

mjac0bs added the Ready for Review label Nov 6, 2023

bnussman-akamai approved these changes Nov 7, 2023

View reviewed changes

coliu-akamai reviewed Nov 7, 2023

View reviewed changes

dwiley-akamai approved these changes Nov 7, 2023

View reviewed changes

mjac0bs added 3 commits November 7, 2023 14:32

Address feedback: scroll error into view

423e64c

Make Create drawer error handling and drawer width consistent

7624cec

Missed an import; fix failing typecheck

84c8ea0

mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Nov 7, 2023

coliu-akamai approved these changes Nov 7, 2023

View reviewed changes

mjac0bs merged commit 2a863b5 into linode:develop Nov 7, 2023
13 checks passed

This was referenced Nov 8, 2023

Release v1.107.0 - release → staging #9889

Merged

Release v1.107.0 - staging → master #9898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test, upcoming: [M3-7134, M3-7314] - Add integration test for AGLB Edit Certificate flow and improve form validation #9880

test, upcoming: [M3-7134, M3-7314] - Add integration test for AGLB Edit Certificate flow and improve form validation #9880

mjac0bs commented Nov 6, 2023 •

edited

Loading

mjac0bs Nov 6, 2023

mjac0bs Nov 6, 2023

bnussman-akamai left a comment

coliu-akamai left a comment •

edited

Loading

dwiley-akamai left a comment

mjac0bs commented Nov 7, 2023 •

edited

Loading

coliu-akamai left a comment

test, upcoming: [M3-7134, M3-7314] - Add integration test for AGLB Edit Certificate flow and improve form validation #9880

test, upcoming: [M3-7134, M3-7314] - Add integration test for AGLB Edit Certificate flow and improve form validation #9880

Conversation

mjac0bs commented Nov 6, 2023 • edited Loading

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Reproduction steps

Verification steps

As an Author I have considered 🤔

mjac0bs Nov 6, 2023

Choose a reason for hiding this comment

mjac0bs Nov 6, 2023

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

coliu-akamai left a comment • edited Loading

Choose a reason for hiding this comment

dwiley-akamai left a comment

Choose a reason for hiding this comment

mjac0bs commented Nov 7, 2023 • edited Loading

coliu-akamai left a comment

Choose a reason for hiding this comment

mjac0bs commented Nov 6, 2023 •

edited

Loading

coliu-akamai left a comment •

edited

Loading

mjac0bs commented Nov 7, 2023 •

edited

Loading