Domain Management: Finish replacing CartData with ShoppingCartProvider

[sirbrillig]

Changes proposed in this Pull Request

In #47605 we updated PopoverCart to replace the CartData wrapper with the ShoppingCartProvider within the domains management components (this will help us eventually remove CartData; see #24019), but there were some things that were missed.

First, although HeaderCart (a parent of PopoverCart) was changed to use withShoppingCart to provide its cart prop, that prop was still being explicitly passed in by the parents of that component. This PR removes those props in case they might cause confusion or bugs.

Second, the DomainSearch component uses cart actions to add and remove products from the cart, but those actions still use CartStore. This PR converts that component to use the ShoppingCartProvider functions for manipulating the cart.

Third, the DomainManagementData wrapper has a needsCart property that fetches the cart from the CartStore. This PR removes that property. Many of the pages under the domain management section had that property enabled, but from what I can tell, almost none of them used it, so this does not add withShoppingCart to any of the children (but if any of them need the cart, that would be an easy solution).

This also updates GSuiteUpgrade to use useShoppingCart from @automattic/shopping-cart in place of CartData. This is similar to the upgrade to GSuiteNudge from #48438. We had to include this here because otherwise either the domain that was added by the domain search page will not have finished updating the cart by the time we hit the G Suite page, causing the G Suite submission to overwrite it (which will probably fail since it will be in a cart without a domain), or the G Suite page will redirect back to the domain search page because it can't see the domain in the cart.

Testing instructions

• Click through to the domains management page using the sidebar links "Manage" > "Domains".
• Add a domain to your cart and click through to checkout.
• Leave checkout without completing the purchase by clicking the "X" in the upper-left.
• Click through to the domains management page again using the sidebar links "Manage" > "Domains".
• Verify that the cart icon appears in the upper-right of the page after a moment and that it shows a "1" dot.
• Verify that clicking on the cart icon shows the domain you added.

We should also verify that the other domain management pages still work as expected since the cart data has been removed from them. I'm not 100% sure how to test each of these but it may be enough to look at each one and verify that they don't use the cart prop.

• DomainManagement.Edit
• DomainManagement.SiteRedirect
• DomainManagement.TransferIn
• DomainManagement.ManageConsent

Testing instructions for the G Suite nudge

• Add a domain to your cart.
• Verify that you see the G Suite upsell before you reach checkout.
• Add a G Suite product to your cart from the upsell.
• Verify that you arrive at checkout and that the cart contains both your domain and the G Suite product.

[matticbot]

Test live: https://calypso.live/?branch=remove/cart-store-from-domain-management

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~166 bytes added 📈 [gzipped])

name            parsed_size           gzip_size
domains              +875 B  (+0.1%)     +238 B  (+0.1%)
woocommerce          -117 B  (-0.0%)      -20 B  (-0.0%)
site-purchases       -117 B  (-0.0%)      -24 B  (-0.0%)
signup               -117 B  (-0.0%)      -24 B  (-0.0%)
purchases            -117 B  (-0.0%)      -28 B  (-0.0%)
plans                -117 B  (-0.0%)      -24 B  (-0.0%)
export               -117 B  (-0.0%)      -24 B  (-0.0%)
email                -117 B  (-0.0%)      -24 B  (-0.0%)
customize            -117 B  (-0.1%)      -24 B  (-0.0%)
checkout             -117 B  (-0.0%)      -24 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~35 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-domains                       +343 B  (+0.1%)      +59 B  (+0.1%)
async-load-calypso-blocks-editor-checkout-modal       -117 B  (-0.0%)      -24 B  (-0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[sirbrillig]

From what I can tell, none of these actually use a cart prop apart from DomainManagement.List, which only used it for HeaderCart and that has been converted to use withShoppingCart already.

[sirbrillig]

Since this potentially touches a lot of files and I'm not sure how to test them all just yet, I've extracted the bug fix itself into #48285

[jsnajdr]

Looks good. The only thing that I was unable to verify is if addProductsToCart( [ fillInSingleCartItemAttributes( item ) ] ) is really the right replacement of addItem( item ) 🙂

[sirbrillig]

Looks good. The only thing that I was unable to verify is if addProductsToCart( [ fillInSingleCartItemAttributes( item ) ] ) is really the right replacement of addItem( item )

The API may not be as elegant 😅 (and it can be improved in the future), but it is the right replacement. The differences are:

• addItem adds one product at a time, addProductsToCart adds multiple, so you have to pass an array.
• addItem is directly connected to the Redux store so it has access to the list of products and can fill in the product_id which is dynamic and not already included by the functions that create cart items, but which is required by the shopping-cart endpoint. addProductsToCart is in a package outside Calypso and so does not have access to the Redux store, so we have to use fillInSingleCartItemAttributes to add in the missing product_id.

Eventually it would be nice to improve the process of getting the product_id for each product, but that's outside the scope of this PR.

[nbloomf]

I verified that the Manage > Domains page works as expected, but I'm also not sure how to test the other domain pages.

[klimeryk]

Thanks for the PR! Love to see the flux CartStore go away ❤️

However, I've noticed on this branch a possible regression:

Screen.Recording.2020-12-21.at.16.02.22.mov

First of all, after you select a domain (press the Select button), we put it in busy mode (as expected). We're double-checking the availability of the domain in the background - however, once that is done, there seems to be a noticeable pause where the Select button becomes active again and nothing seemingly is happening. Then the G Suite upsell finally shows up.

However, a bigger issue is that if you come back to the domain management page, remove the domain, select another (or the same), it goes to the cart, but we're no longer taken to the upsell/checkout. Note also the whole list refresh - that's not me, that was automatic (I'm guessing a page navigation was triggered?).
I have not tested if this is specific to this order of events, but it seemed quite "unstable". I'm also not sure if one of the other pending PRs is not perhaps addressing this already, but wanted to mention it in case it's a new issue 😅 🙇‍♂️ (I've checked and cannot reproduce it on production)

[sirbrillig]

however, once that is done, there seems to be a noticeable pause where the Select button becomes active again and nothing seemingly is happening. Then the G Suite upsell finally shows up.

Good point! This delay is because, unlike the previous version, the code here waits for the cart update to finish before redirecting to the next page. This is to prevent race conditions where the next page might fetch the cart before the update completes.

From what I can tell, the "busy" state is actually only triggered by the call to preCheckDomainAvailability, so while the actual cart adding is processing, we just see the buttons as normal. We can probably resolve the issue here by tying the busy indicator to the isPendingUpdate property of the shopping cart manager.

I've pushed a commit to do that by adding a isCartPendingUpdate prop to each button and their parents. The only downside is that it currently only works for buttons under DomainSearch. It would be better to attach the cart directly to the DomainRegistrationSuggestion, but we can't do that until all the parents of that component are wrapped in a ShoppingCartProvider, and this PR does not yet tackle routes like TransferDomainStep which still relies on CartData. To that end, I also provided isCartPendingUpdate in those other components using the metadata provided by CartData.

remove the domain, select another (or the same), it goes to the cart, but we're no longer taken to the upsell/checkout. Note also the whole list refresh - that's not me, that was automatic (I'm guessing a page navigation was triggered?).

Wow, that's very interesting... I was able to reproduce pretty easily. It's not obvious from the video, but the URL does change to the google apps upsell correctly. The issue appears to be that that URL is loading the domain search page instead. I didn't change anything in this PR about how the redirect works (page( '/domains/add/' + domain + '/google-apps/' + this.props.selectedSiteSlug );), so it's odd that this isn't also occurring in production. I'll do some more research to see what's going on.

[sirbrillig]

Ah hah. The issue is that the GSuiteUpgrade redirects back to the domains page if the cart doesn't contain a domain. Since GSuiteUpgrade is still using CartData, it doesn't have an updated copy of the cart and doesn't know the domain is there. This would be fixed by #48516 which converts GSuiteUpgrade, but that PR depends on this one because of the opposite problem: if this PR is not merged, then an updated GSuiteUpgrade will still not be able to find the domain.

Because of this strong dependency that both components use the same cart data, it's probably best to include that PR here. I've pulled those commits into this PR and updated the testing instructions.

[klimeryk]

Other than the UX quirk (which hopefully should be an easy fix), this LGTM. Tested a few cases, including existing shopping cart items, upgrading with a plan and everyone's favourite: /domains ;)

[klimeryk]

I think this check should go higher, to line 170 and simply be } else if ( pendingCheckSuggestion || this.props.isCartPendingUpdate ) {. Otherwise, this causes a weird UX where suddenly the suggestions Select buttons go from disabled to suddenly busy and clickable. So, you can actually select another domain while you're waiting to be redirected to the cart. Which is not an error in itself, but I can imagine could cause some confusion (and possibly unwanted domains being bought).

[sirbrillig]

That's a good suggestion; I didn't realize that if busy is set, the button remains clickable!

In that case, we'll need to decide what to do about the existing if ( pendingCheckSuggestion ) clause. With your suggestion, that clause will never activate, which makes me think that you're saying we should replace line 170 with the condition. But if we do that, then the button will not display as busy when adding the product to the cart, it will be disabled instead because pendingCheckSuggestion will not be set. I wonder if you mean removing the nested if entirely like:

		} else if ( pendingCheckSuggestion || this.props.isCartPendingUpdate ) {
			buttonStyles = { ...buttonStyles, busy: true, disabled: true };
		}

[sirbrillig]

Rebased and hopefully fixed the UX issue where the button jumps from disabled to busy.