[PWA-336] Cart/Checkout Input Errors UI cases #2495

tjwiebell · 2020-06-17T20:26:04Z

Description

After Cart/Checkout is done we should make a general pass over the error states to make sure we represent them correctly according to UX desires and the style guide.

Cases to consider:

User errors
System errors

h5. Open Questions:

How should we handle form fields that are out of view of the shopper when the error occurs?
Are we going to have both form and field level reporting?
Do we have the most common error scenarios / use-cases documented?

Summary of GraphQL Operations that may have error state: https://wiki.corp.magento.com/pages/viewpage.action?pageId=144314396

Related Issue

[PWA-336] Cart/Checkout Input Errors UI cases

Acceptance

Verification Stakeholders

Specification

Verification Steps

This guy is going to be a little difficult to test, it's quite tricky to simulate errors. You might be able to go offline and use network errors (we've discovered this actually doesn't work in most places), but modifying code to break the queries or mutations is what I did. I'll outline the components touched, and the verification steps are all the same; simulate an error somehow and verify output. I have also included screenshots in the HLD for UX, to make visual review easier.

CartPage > Product Edit > Loading Options (now renders error)
CartPage > Remove Item (throws Toast)
CartPage > Product Edit > Update (renders quantity and option errors now and doesn't close form)
CartPage > Remove Coupon (now renders error)
CartPage > Estimate Shipping > Set Address (now renders error)
CheckoutPage > Select Shipping Method (now renders error and doesn't close if in modal)
CheckoutPage > Address Book > Apply Address (throws Toast, doesn't close)
CheckoutPage > Auth/Guest Address > Add/Update Address (renders error, auto scrolls to form top, doesn't close anymore)

Screenshots / Screen Captures (if appropriate)

See HLD for tons of screenshots.

Checklist

I have added tests to cover my changes, if necessary.

I have updated the documentation accordingly, if necessary.

packages/peregrine/lib/talons/CartPage/ProductListing/useProduct.js

PWAStudioBot · 2020-06-17T20:35:51Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-336.

Generated by 🚫 dangerJS against 49cc995

devops-pwa-codebuild · 2020-06-17T20:42:55Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2495.pwa-venia.com : LH Performance Expected 0.85 Actual 0.57, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 90 Actual 88
https://pr-2495.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.34, LH Best Practices Expected 1 Actual 0.92
https://pr-2495.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.49, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

…quantity component.

jimbo · 2020-06-24T16:46:16Z

packages/peregrine/lib/talons/CartPage/PriceAdjustments/ShippingMethods/useShippingForm.js

-        { called: setShippingAddressCalled, loading: isSetShippingLoading }
+        {
+            called: setShippingAddressCalled,
+            error: setShippingAddressError,


It's a bit counterintuitive to see setShippingAddressCalled and setShippingAddressError used as object names, since it's our most common format for naming functions.

Consider calledSetShippingAddress or errorSettingShippingAddress, I suppose?

This pattern is used all over the place. If we want to switch it up let's make sure everyone is aware.

Renamed in 57ab1a7.

jimbo · 2020-06-24T16:48:30Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/ShippingMethods/shippingForm.css

@@ -27,6 +27,10 @@
    padding: 0.75rem 1rem;
 }

+.error {
+    color: rgb(var(--venia-error));


I'm not sure this is a property anymore.

See tokens.css for new tokens

If you replace this one, make sure to replace all of the others as well

Suggested change

color: rgb(var(--venia-error));

color: rgb(var(--venia-global-color-error));

@jimbo - that token doesn't seem to exist. Would you like me to add it, or pick one of the --venia-global-color-red-xxx values?

Strike that, we cut the release branch before design tokens landed, so the new tokens are not available. Going to leave this as-is, and we'll resolve when we merge release back into develop.

Ah, my mistake. I assumed this was targeting develop but it's actually targeting the release branch. Carry on. 👍

jimbo · 2020-06-24T16:50:25Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/ShippingMethods/shippingForm.js

        handleOnSubmit,
        handleZipChange,
        isSetShippingLoading
    } = talonProps;

    const classes = mergeClasses(defaultClasses, props.classes);

+    const errorElement = errorMessage ? (
+        <span className={classes.error}>{errorMessage}</span>


I might have liked to see a FormError component for form-level errors, since I imagine the presentation will be nearly identical for all of them. Such a component could render null when props.message was empty, too, cutting down on a lot of the boilerplate.

Added FormError component in 57ab1a7, and changed all usages in this PR to use it (where it didn't dramatically alter the UX).

I thought we had discussed deferring a dedicated component to a followup Story, but I've gone ahead and implemented one in this scope.

Thanks. I apologize if we backtracked on an agreement. It's not the duplication of code that bothered me; I just saw that we were adding new classnames to a lot of form components as a result of the new error elements, and thought it'd be nice to avoid that churn. Glad to have a component now.

jimbo · 2020-06-24T16:55:07Z

packages/peregrine/lib/talons/CheckoutPage/ShippingInformation/AddressForm/useCustomerForm.js

@@ -39,6 +47,30 @@ export const useCustomerForm = props => {
        }
    }, [getCustomerError]);

+    useEffect(() => {
+        if (createCustomerAddressError || updateCustomerAddressError) {
+            errorRef.current.scrollIntoView({


I think scrollIntoView() is the right choice here, but I don't think it belongs in the talon.

The only other place we use this, PaymentsFormItems, has a talon of its own as well, but the component is the one to call scrollIntoView because this kind of effect is really just presentational. There's very little logic we put in the component rather than the talon, but I think this qualifies.

Moved this logic to FormError and put scroll and ref logic in component instead of talon (57ab1a7).

# Conflicts: # packages/peregrine/lib/talons/CartPage/ProductListing/EditModal/__tests__/useProductForm.spec.js # packages/peregrine/lib/talons/CartPage/ProductListing/EditModal/useProductForm.js

sirugh

This looks good - I don't see anything worth holding up the PR. Would be nice to have a utility function, and perhaps move the error scrolling to the component but otherwise 👍

sirugh · 2020-06-24T19:49:53Z

packages/peregrine/lib/talons/CartPage/PriceAdjustments/ShippingMethods/useShippingForm.js

+        if (setShippingAddressError.graphQLErrors) {
+            derivedError = setShippingAddressError.graphQLErrors
+                .map(({ message }) => message)
+                .join(', ');
+        } else {
+            derivedError = setShippingAddressError.message;
+        }


I smell a utility function!

We are probably not going to render Apollo errors going forward, since they can't be trusted to be human readable. I have moved this logic to a new FormError component which should reduce the boiler plate though, but you still gotta handroll this logic if you don't like the presentation of that component.

sirugh · 2020-06-24T19:51:54Z

packages/peregrine/lib/talons/CartPage/ProductListing/EditModal/useProductForm.js

    ] = useMutation(updateConfigurableOptionsMutation);

    const isSaving =
        (updateQuantityCalled && updateQuantityLoading) ||
        (updateConfigurableCalled && updateConfigurableLoading);

-    useEffect(() => {
-        if (formApi) {
-            formApi.setValue('quantity', cartItem.quantity);


What was this doing and why is it gone now?

The intention was to make sure quantity was updated between renders, but two things made it obsolete:

I started un-mounting between renders, so this wasn't needed

This was fixed in the Quantity component in the same way, so this was redundant anyway (even if we didn't unmount between renders)

…nd rendering error messages.

jimbo · 2020-06-24T23:27:12Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/ShippingMethods/shippingForm.js

@@ -40,6 +42,7 @@ const ShippingForm = props => {
    return (
        <Fragment>
            <h3 className={classes.formTitle}>Destination</h3>
+            <FormError errors={formErrors} />


You don't have to do this, but consider just rendering the FormError element as the first child of the Form element, rather than wrapping these forms in fragments. Might help if the form wants to have the form error participate in its layout.

sirugh · 2020-06-25T14:44:06Z

packages/peregrine/lib/talons/CartPage/PriceAdjustments/useCouponCode.js

                .map(({ message }) => message)
                .join(', ');
        } else {
            // A non-GraphQL error occurred.
-            applyErrorMessage - applyError.message;
+            derivedErrorMessage = errorTarget.message;


Do we need to handle network errors? They'd be on the target as errorTarget.networkError.

Discussed offline - I am for moving this sort of logic into a utility that is used by useFormError and by any other components that need to turn apollo errors into a string.

I was going to suggest that too, 💯 agree

supernova-at · 2020-06-25T15:04:48Z

...i/lib/components/CartPage/PriceAdjustments/ShippingMethods/__tests__/shippingMethods.spec.js

@@ -23,7 +23,7 @@ jest.mock('../shippingForm', () => 'ShippingForm');
 jest.mock('../shippingRadios', () => 'ShippingRadios');

 test('renders description and confirm link w/o shipping address set', () => {
-    useLazyQuery.mockReturnValue([
+    useQuery.mockReturnValue([


I'm surprised this is working - useQuery just returns an object, not an array like useLazyQuery does.

Very odd. I haven't full covered this with tests yet, so I'll be sure to investigate this and update the mock to accurately reflect the implementation of useQuery.

sirugh · 2020-06-25T15:09:53Z

packages/peregrine/lib/talons/CartPage/PriceAdjustments/useCouponCode.js


-    if (applyError) {
-        if (applyError.graphQLErrors) {
+        if (errorTarget.graphQLErrors) {


OOPS - as we just discovered, graphQLErrors is an array. All these checks will need to be updated to length

Great catch! Added this check to FormError, and we'll address the other areas once we create a utility.

supernova-at

Looks awesome! Great organization and consistency.

I had one question about a unit test mock that I'm surprised doesn't break things.

sirugh

Good - we will push the fixing of the other places missing the length check to a follow up ticket.

dpatil-magento · 2020-06-25T18:25:13Z

QA Approved.

Address error gaps in cart page components

098e2e7

m2-community-project bot added this to Ready for Review in Pull Request Progress Jun 17, 2020

tjwiebell commented Jun 17, 2020

View reviewed changes

packages/peregrine/lib/talons/CartPage/ProductListing/useProduct.js Show resolved Hide resolved

m2-community-project bot assigned tjwiebell Jun 17, 2020

m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Jun 17, 2020

m2-community-project bot added the Progress: review label Jun 17, 2020

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Jun 17, 2020

m2-community-project bot assigned sirugh Jun 18, 2020

tjwiebell added 2 commits June 18, 2020 10:11

Address the simple error cases of checkout and fixup tests

6f0aac5

Remove some obsolete quantity state sync logic that was fixed in the …

5678499

…quantity component.

tjwiebell added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Jun 18, 2020

tjwiebell added 3 commits June 18, 2020 15:59

Add error rendering to the remaining checkout components.

b6c9a5a

Fixup tests so they're back to green

04b213e

Fix lint error

893e080

tjwiebell changed the base branch from develop to release/7.0 June 22, 2020 16:46

jimbo suggested changes Jun 24, 2020

View reviewed changes

m2-community-project bot assigned jimbo Jun 24, 2020

m2-community-project bot moved this from Review in Progress to Changes Requested in Pull Request Progress Jun 24, 2020

m2-community-project bot added Progress: changes requested and removed Progress: review labels Jun 24, 2020

Merge branch 'release/7.0' into tommy/error-handling

142a0ad

# Conflicts: # packages/peregrine/lib/talons/CartPage/ProductListing/EditModal/__tests__/useProductForm.spec.js # packages/peregrine/lib/talons/CartPage/ProductListing/EditModal/useProductForm.js

sirugh previously approved these changes Jun 24, 2020

View reviewed changes

tjwiebell added 2 commits June 24, 2020 15:46

Implement FormError component to hold boiler plate logic of parsing a…

57ab1a7

…nd rendering error messages.

Fixup failing tests

7e5d4a9

tjwiebell dismissed sirugh’s stale review via 7e5d4a9 June 24, 2020 20:59

jimbo previously approved these changes Jun 24, 2020

View reviewed changes

sirugh reviewed Jun 25, 2020

View reviewed changes

supernova-at reviewed Jun 25, 2020

View reviewed changes

m2-community-project bot assigned supernova-at Jun 25, 2020

m2-community-project bot moved this from Changes Requested to Review in Progress in Pull Request Progress Jun 25, 2020

m2-community-project bot added Progress: review and removed Progress: changes requested labels Jun 25, 2020

sirugh reviewed Jun 25, 2020

View reviewed changes

supernova-at previously approved these changes Jun 25, 2020

View reviewed changes

Add length check so message fall through actually works

49cc995

tjwiebell dismissed stale reviews from supernova-at and jimbo via 49cc995 June 25, 2020 15:24

sirugh approved these changes Jun 25, 2020

View reviewed changes

jimbo approved these changes Jun 25, 2020

View reviewed changes

dpatil-magento merged commit 7f67037 into release/7.0 Jun 25, 2020

m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress Jun 25, 2020

m2-community-project bot removed the Progress: review label Jun 25, 2020

tjwiebell mentioned this pull request Jun 26, 2020

[PWA-720] Error Case Tests #2517

Merged

1 task

dpatil-magento mentioned this pull request Jul 14, 2020

[Doc] Release note 7.0.0 #2542

Merged

1 task

tjwiebell deleted the tommy/error-handling branch October 2, 2020 18:42

sirugh restored the tommy/error-handling branch April 26, 2021 16:29

sirugh deleted the tommy/error-handling branch April 26, 2021 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PWA-336] Cart/Checkout Input Errors UI cases #2495

[PWA-336] Cart/Checkout Input Errors UI cases #2495

tjwiebell commented Jun 17, 2020 •

edited

PWAStudioBot commented Jun 17, 2020 •

edited

devops-pwa-codebuild commented Jun 17, 2020 •

edited

jimbo Jun 24, 2020

sirugh Jun 24, 2020

tjwiebell Jun 24, 2020

jimbo Jun 24, 2020

tjwiebell Jun 24, 2020

tjwiebell Jun 24, 2020

jimbo Jun 24, 2020

jimbo Jun 24, 2020

tjwiebell Jun 24, 2020

jimbo Jun 24, 2020

jimbo Jun 24, 2020

tjwiebell Jun 24, 2020

sirugh left a comment

sirugh Jun 24, 2020

tjwiebell Jun 24, 2020

sirugh Jun 24, 2020

tjwiebell Jun 24, 2020

jimbo Jun 24, 2020

sirugh Jun 25, 2020

sirugh Jun 25, 2020

supernova-at Jun 25, 2020

supernova-at Jun 25, 2020

tjwiebell Jun 25, 2020

sirugh Jun 25, 2020

tjwiebell Jun 25, 2020

supernova-at left a comment

sirugh left a comment

dpatil-magento commented Jun 25, 2020

	color: rgb(var(--venia-error));
	color: rgb(var(--venia-global-color-error));

[PWA-336] Cart/Checkout Input Errors UI cases #2495

[PWA-336] Cart/Checkout Input Errors UI cases #2495

Conversation

tjwiebell commented Jun 17, 2020 • edited

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jun 17, 2020 • edited

devops-pwa-codebuild commented Jun 17, 2020 • edited

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

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

sirugh left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at left a comment

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

dpatil-magento commented Jun 25, 2020

tjwiebell commented Jun 17, 2020 •

edited

PWAStudioBot commented Jun 17, 2020 •

edited

devops-pwa-codebuild commented Jun 17, 2020 •

edited