Preselect lowest cost shipping method for authed users without one #2402

supernova-at · 2020-05-18T15:45:08Z

Description

This PR pre-selects the lowest cost shipping method for authenticated (signed in) users who don't yet have a saved / preferred shipping method.

I also added a new INITIALIZING value to the displayState enumeration to remove a visual hiccup that occurred during auto-selection of a shipping method. The ShippingMethod component should be INITIALIZING before fetching its data and while the auto-selection is in progress.

Adding this INITIALIZING state allowed us to greatly simplify downstream logic as well 🎉 .

Related Issue

Closes PWA-573.

Acceptance

Verification Stakeholders

@soumya-ashok

Specification

Verification Steps

Sign in
Add an item to your cart
Set a shipping address (via mini-cart if [PWA-245] Shipping Information (Authenticated) #2380 isn't merged yet)
Go to the /checkout page
Verify that you're never actually presented with the shipping radios to select; the lowest cost shipping option (FREE) is selected for you (after some loading) and you are taken directly to the payment section
Fill out the payment section
Verify that you can successfully place your order

Screenshots / Screen Captures (if appropriate)

Checklist

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

… for them

PWAStudioBot · 2020-05-18T15:59:50Z

	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-573.

Generated by 🚫 dangerJS against 8ba5c8f

devops-pwa-codebuild · 2020-05-18T16:04:53Z

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-2402.pwa-venia.com : LH Performance Expected 0.85 Actual 0.58, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 90 Actual 88
https://pr-2402.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.35, LH Best Practices Expected 1 Actual 0.92
https://pr-2402.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

visual hiccup and erroneous error messages

supernova-at · 2020-05-18T18:50:13Z

packages/venia-ui/lib/components/CheckoutPage/ShippingMethod/shippingMethod.js

+        const bodyContents =
+            displayState === displayStates.INITIALIZING
+                ? initializingContents
+                : editingContents;


The only logic that actually changed is the addition of what to do when displayState is INITIALIZING. The other changes here are just for readability.

supernova-at · 2020-05-18T18:51:20Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

+
+    // If an authenticated user does not have a preferred shipping method,
+    // auto-select the least expensive one for them.
+    useEffect(() => {


This is the meat of the PR, everything else is minor changes to support a seamless experience for the user.

supernova-at · 2020-05-19T15:31:59Z

I should note that the Update Modal also has a new behavior:

The call to get all available shipping methods is cache-and-network, and previously this modal was showing a loading spinner until the network responded. Now, the modal shows the data that is in the cache (if present) immediately - it no longer waits for the network to respond. This results in a better, faster experience for the user.

In the case where the network data actually differs from what is in the cache, the modal still disables the "submit" button until the network responds.

In other words, you can't actually make a different selection until we know we have the most up-to-date data, but we no longer show a loading spinner until that happens. This does introduce the potential for the selection list to change while the user is looking at it.

soumya-ashok · 2020-05-19T21:34:16Z

@supernova-at I followed the steps, and the shipping method behaves as you described. That part is UX approved.

The use-case I wanted to test but can't until Tommy's work is merged is that very first time someone comes through auth checkout and doesn't have a saved shipping address. Once they enter the shipping address in the new checkout flow, I assume the shipping method behavior will stay consistent?

supernova-at · 2020-05-20T17:20:04Z

Once they enter the shipping address in the new checkout flow, I assume the shipping method behavior will stay consistent?

Yes, it should behave as it does here 👍

tjwiebell

Nice work, all the logic is here and this functions perfectly 👌 I've added some suggestions that should clean up the talon quite a bit, but I'd be fine to save the big changes for another task after we've had a chance to discuss as a team.

tjwiebell · 2020-05-20T20:30:05Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

+    const [displayState, setDisplayState] = useState(
+        displayStates.INITIALIZING
+    );
+    const [isBackgroundAutoSelecting, setIsBackgroundAutoSelecting] = useState(


You shouldn't need this, its just duplicating state you already have access to. You should track called and loading state of setShippingMethodCall, and just have a derived isLoading that looks at both. Will add a comment by nextStateDisplay with what I'm talking about.

tjwiebell · 2020-05-20T20:33:10Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

@@ -159,9 +167,67 @@ export const useShippingMethod = props => {
        // Determine the component's display state.
        const nextDisplayState = selectedMethod
            ? displayStates.DONE
+            : loading || isBackgroundAutoSelecting


Example of using state you already have, with some renames. In some of my recent code I've found tracking called isn't needed, but it reads better in this example; consider it optional.

Suggested change

: loading || isBackgroundAutoSelecting

: fetchShippingMethodLoading || (setShippingMethodCalled && setShippingMethodLoading)

tjwiebell · 2020-05-20T20:36:54Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

+        // Functions passed to useEffect should be synchronous.
+        // Set this helper function up as async so we can wait on the mutation
+        // before re-querying.
+        const autoSelectShippingMethod = async shippingMethod => {


Shouldn't need this after isBackgroundAutoSelecting state is removed.

tjwiebell · 2020-05-20T20:38:51Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

+            setIsBackgroundAutoSelecting(true);
+
+            // Perform the operation on the backend.
+            await setShippingMethodCall({


Use the same fragment as fetchShippingMethodInfo and you won't need to do your re-fetch below. This also should mean you can worry less about the execution flow.

If you absolutely must refetch this way, you can still do that without this async function. Use the refetchQueries option of the mutation.

Tommy is right - if the data can be returned by a mutation and assuming the response data is correct (as in no bug in GQL) we should opt to query for that data in the response rather than firing a second request/query. If you're able to get all you need in the response you can get rid of the extra loading state variable too. Seems like a win-win!

Edit: I'll add that we did see that loading prop returned from the Apollo hooks does not reflect state of in-flight refetchQueries.

tjwiebell · 2020-05-20T20:42:22Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

+            });
+        };
+
+        const primaryAddress = data.cart.shipping_addresses[0];


This should be unreachable without an address set, but still good to guard against shipping_addresses being null.

Should be a possibility once virtual products are supported.

tjwiebell · 2020-05-20T20:43:22Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

+            const leastExpensiveShippingMethod = shippingMethodsByPrice[0];
+
+            if (leastExpensiveShippingMethod) {
+                autoSelectShippingMethod(leastExpensiveShippingMethod);


This is where you would move the setShippingMethodCall.

tjwiebell · 2020-05-20T21:13:59Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

    const [isUpdateMode, setIsUpdateMode] = useState(false);
+    const [selectedShippingMethod, setSelectedShippingMethod] = useState(null);
+    const [shippingMethods, setShippingMethods] = useState([]);


I know this pattern existing prior to this PR, but wanted to discuss why I think it might be an anti-pattern, and something we should avoid. Apollo's hooks are effects themselves, so you should never need to wrap that returned state in an effect, or duplicate it into its own state. I'm a bit unique in that I didn't write much React before hooks, but I think the pattern you're going for here from previous paradigms is derivedState.

Two use cases for derived state:

Data needs manipulated - useMemo if complex, but option 2 also works, wrap logic in a function:

const memoizedShippingMethods = useMemo(() => { let mutatedShippingMethods = []; if (data) { const primaryShippingAddress = data.cart.shipping_addresses[0]; // Shape the list of available shipping methods. // Sort them by price and add a serialized property to each. const rawShippingMethods = primaryShippingAddress.available_shipping_methods; const shippingMethodsByPrice = [...rawShippingMethods].sort( byPrice ); mutatedShippingMethods = shippingMethodsByPrice.map( addSerializedProperty ); } return mutatedShippingMethods; }, [data]);

Data just needs extracted - no special treatment

const derviedSelectedShippingMethod = data && addSerializedProperty( data.cart.shipping_addresses[0].selected_shipping_method ); // you may want a || null default value here

I would like to earmark this to be discussed during tomorrow's team time, as I think we should make sure we're all aligned on this. I'm not 100% I'm right, so worth discussing for my own benefit as well.

We use the derived pattern often throughout the codebase. Here it is in the shipping info talon and here it is in the breadcrumb talon.

Efficiency aside I think it's more readable which is why I'd prefer it :)

tjwiebell · 2020-05-20T21:24:04Z

packages/venia-ui/lib/components/CheckoutPage/ShippingMethod/shippingMethod.js

+        const bodyContents =
+            displayState === displayStates.INITIALIZING
+                ? initializingContents
+                : editingContents;


Recent feedback from Jimmy, very minor. We've already paid the cost of creating the editingContent element and this means we may never render it. Best to only create elements we intend to render.

…studio into supernova/573_ship_shortcut

jimbo · 2020-05-21T18:27:12Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

+        ? displayStates.INITIALIZING
+        : displayStates.EDITING;
+
+    if (nextDisplayState !== displayState) {


You don't need this check; if React detects the new state matches the previous it should skip the update.

jimbo · 2020-05-21T18:42:34Z

packages/venia-ui/lib/components/CheckoutPage/ShippingMethod/__tests__/updateModal.spec.js

@@ -40,7 +40,7 @@ test('it renders correctly', () => {
    expect(instance.toJSON()).toMatchSnapshot();
 });

-test('it renders correctly during loading', () => {
+test('it disables the submit butting while loading', () => {


Butting whom?

…studio into supernova/573_ship_shortcut

supernova-at · 2020-05-21T20:09:39Z

PR updated with derived state!

This greatly simplifies the whole talon thank you so much @tjwiebell !

I am also trying to sneak in the disabling of the input radios when the page is updating 😉 . I think Shipping Methods is in a much better place now 💪

tjwiebell

Re-factor looks amazing, nice work 👍 Definitely copying how you structure your talon sections into my PR, makes it really easy to understand.

tjwiebell · 2020-05-22T17:54:44Z

packages/peregrine/lib/talons/CheckoutPage/useShippingMethod.js

+    const isBackgroundAutoSelecting =
+        isSignedIn &&
+        !derivedSelectedShippingMethod &&
+        Boolean(derivedShippingMethods.length);


dpatil-magento · 2020-05-22T22:15:11Z

Could not test edit address flow for Auth users but other than that it look good.

Authed users without shipping methods get lowest cost one preselected…

8a7167e

… for them

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

PWAStudioBot added the pkg:peregrine label May 18, 2020

Mocks user context in unit test

8247775

supernova-at added the version: Minor This changeset includes functionality added in a backwards compatible manner. label May 18, 2020

supernova-at added 5 commits May 18, 2020 13:24

Adds an initializing display state to remove

63b3032

visual hiccup and erroneous error messages

Runs prettier

3078b10

Fixes continue button regression

eca0864

Unit test changes

d775ae6

Remove change to isLoading logic

2faa5fc

supernova-at commented May 18, 2020

View reviewed changes

m2-community-project bot assigned supernova-at May 18, 2020

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

m2-community-project bot added the Progress: review label May 18, 2020

supernova-at commented May 18, 2020

View reviewed changes

PWAStudioBot added the pkg:venia-ui label May 18, 2020

supernova-at and others added 2 commits May 19, 2020 10:19

Commits fully to the new initialized display state.

f9c24df

Merge branch 'develop' into supernova/573_ship_shortcut

54263cd

Merge branch 'develop' into supernova/573_ship_shortcut

52fcaaf

tjwiebell suggested changes May 20, 2020

View reviewed changes

m2-community-project bot assigned tjwiebell May 20, 2020

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

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

Merge branch 'develop' into supernova/573_ship_shortcut

6e6cf59

m2-community-project bot assigned sirugh May 21, 2020

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

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

supernova-at added 5 commits May 21, 2020 11:13

Simplifies talon logic

fcfd4ed

Use derived state where possible

14b5f3c

Runs prettier

a2d786a

Merge branch 'supernova/573_ship_shortcut' of github.com:magento/pwa-…

464b661

…studio into supernova/573_ship_shortcut

Derive displayState instead of keeping it in local state

40ac220

jimbo reviewed May 21, 2020

View reviewed changes

m2-community-project bot assigned jimbo May 21, 2020

supernova-at and others added 5 commits May 21, 2020 14:08

Derives isBackgroundAutoSelecting instead of tracking it in state

f845da8

Disables input radios while the page is updating

a1cce81

Merge branch 'develop' into supernova/573_ship_shortcut

782ee96

Runs prettier

9780b3e

Merge branch 'supernova/573_ship_shortcut' of github.com:magento/pwa-…

ee3486e

…studio into supernova/573_ship_shortcut

tjwiebell approved these changes May 22, 2020

View reviewed changes

Merge branch 'develop' into supernova/573_ship_shortcut

8ba5c8f

dpatil-magento merged commit ec0b9ca into develop May 26, 2020

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

m2-community-project bot removed the Progress: review label May 26, 2020

dpatil-magento deleted the supernova/573_ship_shortcut branch May 26, 2020 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preselect lowest cost shipping method for authed users without one #2402

Preselect lowest cost shipping method for authed users without one #2402

supernova-at commented May 18, 2020 •

edited

PWAStudioBot commented May 18, 2020 •

edited

devops-pwa-codebuild commented May 18, 2020 •

edited

supernova-at May 18, 2020

supernova-at May 18, 2020 •

edited

supernova-at commented May 19, 2020

soumya-ashok commented May 19, 2020

supernova-at commented May 20, 2020

tjwiebell left a comment

tjwiebell May 20, 2020

tjwiebell May 20, 2020

tjwiebell May 20, 2020

tjwiebell May 20, 2020

sirugh May 21, 2020 •

edited

tjwiebell May 20, 2020

fooman May 21, 2020

tjwiebell May 20, 2020

tjwiebell May 20, 2020

sirugh May 21, 2020

tjwiebell May 20, 2020 •

edited

jimbo May 21, 2020

jimbo May 21, 2020

supernova-at commented May 21, 2020

tjwiebell left a comment

tjwiebell May 22, 2020

dpatil-magento commented May 22, 2020

	: loading \|\| isBackgroundAutoSelecting
	: fetchShippingMethodLoading \|\| (setShippingMethodCalled && setShippingMethodLoading)

Preselect lowest cost shipping method for authed users without one #2402

Preselect lowest cost shipping method for authed users without one #2402

Conversation

supernova-at commented May 18, 2020 • edited

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented May 18, 2020 • edited

devops-pwa-codebuild commented May 18, 2020 • edited

Choose a reason for hiding this comment

supernova-at May 18, 2020 • edited

Choose a reason for hiding this comment

supernova-at commented May 19, 2020

soumya-ashok commented May 19, 2020

supernova-at commented May 20, 2020

tjwiebell 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

sirugh May 21, 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

tjwiebell May 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at commented May 21, 2020

tjwiebell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpatil-magento commented May 22, 2020

supernova-at commented May 18, 2020 •

edited

PWAStudioBot commented May 18, 2020 •

edited

devops-pwa-codebuild commented May 18, 2020 •

edited

supernova-at May 18, 2020 •

edited

sirugh May 21, 2020 •

edited

tjwiebell May 20, 2020 •

edited