Purchases: Individual purchases not loading on sites with Site Redirect #536

umurkontaci · 2015-11-23T18:01:46Z

This fixes the issue of individual purchases not loading on site with Site Redirect record set as primary domain. (Fixes #122)

It changes the use of domain property from purchases data to getting the site slug from SitesList. This will the URLs will behave the same as in store.

Testing

Assert that a site with a site redirect appears properly in /purchases.
Assert that two sites that redirect to the same site appear properly in /purchases.
QA review
Code review

mikeshelton1503 · 2015-11-24T01:13:38Z

👍 This fixes my issue from #122

fabianapsimoes · 2015-11-24T16:20:43Z

This seems to solve the issues with Site Redirect, but is failing some tests. Moving back to Awaiting Author Response.

umurkontaci · 2015-11-24T16:39:32Z

Yes, due to some twisted dependencies. I've added react-test-env-setup to cart-synchronizer-test and the tests should be up and running now.

drewblaisdell · 2015-11-24T16:56:43Z

client/lib/purchases/index.js

@@ -27,6 +30,7 @@ function getPurchasesBySite( purchases ) {
 				domain: currentValue.domain,
 				id: currentValue.siteId,
 				name: currentValue.siteName,
+				slug: sites.getSite( currentValue.siteId ).slug,


Is there a guarantee that sites has been fetched at this point?

It's only called in a single place under client/me/purchases/list/index.jsx, and I think that page is only loaded after we have the sites. But apart from that, there is no guarantee. I have come to think that it might be a better idea to move the function to somewhere else, because a dependency to sites-list breaks many tests since sites-list depends on DOM and we need to add react-test-env-setup to many tests to get them running.

What do you think about removing this call to SitesList and updating this function to accept an array of sites as a parameter, like getPurchasesBySite( purchases, sites )? It would keep the function pure, make this easier to test, and would be clearer that information from SitesList is necessary in modules that use getPurchasesBySite.

That's a great idea, I've changed the code to implement it as such.

drewblaisdell · 2015-11-24T19:37:32Z

@umurkontaci A potentially unresolved blocker for using the primary domain in the purchases/checkout URLs is that Jetpack sites are represented with two site objects, each with separate blog_ids:

The Jetpack site (e.g. foobar.com)
A .com site that relates to the Jetpack site (e.g. foobar.wordpress.com)

Purchases are sometimes (possibly always, I'm unsure) associated with the .com site, but if we redirect users from /purchases to the checkout through /checkout/foobar.com, the cart will be created for the Jetpack site, and the purchases will not appear as renewals, because the subscriptions exist only for foobar.wordpress.com.

@dzver or @matthusby: Do either of you know whether subscriptions for Jetpack sites are always associated with the .com site, in this case?

dzver · 2015-11-25T09:14:18Z

I'm having trouble confirming that a Jetpack site has two blog_ids. My Jetpack site seems to only have one. Do you mind giving me an example?

drewblaisdell · 2015-11-25T18:56:07Z

@dzver Apparently, the example I was using was the following, non-standard case of a user with:

A Jetpack site with a URL property of http://foobar.com.
A .com site, foobar.wordpress.com, with a primary domain, http://foobar.com, that has its DNS record point to the Jetpack site.

SitesList and Site use a property (hasConflict) to resolve collisions between sites that have the same slug, by setting the slug of the .com site to the unmapped URL (foobar.wordpress.com, in the previous example).

@umurkontaci's approach of setting the slug property in the purchases assembler to that of the slug property of the site from SitesList should fix this problem correctly, so we don't need to worry about the issue with which blog_id is associated with a purchase.

umurkontaci · 2015-11-26T13:55:35Z

This has been updated with @drewblaisdell suggestion about sending the SitesList as an argument.

scruffian · 2015-11-26T16:25:38Z

I tested this with .com sites and it works well.

I think we should test it with a Jetpack site before we merge.

The code looks good

umurkontaci · 2015-11-26T16:31:26Z

@scruffian I've tested with an account on a single Jetpack site and confirm that it's using the correct URL.

scruffian · 2015-11-27T11:25:18Z

@jeherve @v18, my concern is that this will reintroduce a problem with domain mapping for Jetpack site that we saw last week:

The problem occours when a user has a Jetpack-connected self-hosted site and a domain registered at WordPress.com, mapped to the self-hosted site. In this case the user couldn’t renew the domain through the Jetpack-connected site using Calypso – this makes some sense as the domain is registered in connection with her .com site, but it is definitely confusing.

In this case users were able to see the domain and attempt to renew it, but they got an error about requiring domain registration details. (p2MSmN-4aE-p2)

I have tried setting up my Jetpack site to test this, but I couldn't connect it. Could one of you test it?

jeherve · 2015-11-27T13:56:51Z

I tested the branch with a domain of mine, registered on WordPress.com, and currently pointed to a Jetpack site connected to the same WordPress.com account.

I was able to go through the renewal.

this makes some sense as the domain is registered in connection with her .com site, but it is definitely confusing.

I might not be the perfect test subject for this, but I must say that it was less confusing than I expected. The domain name is displayed in a much bigger font size than the site title on /purchases, so you naturally click on it without thinking twice.

I did, however ran into another site during the renewal. The following js error appeared when clicking the "Purchase subscription" button:

I didn't click on anything else before on that page, my stored credit card was automatically selected for me. When entering a new credit card, payment went through.

I couldn't reproduce when trying the same steps on WordPress.com, and could reproduce again with another subscription when in that branch.

scruffian · 2015-12-02T15:15:19Z

I was getting an error if a site was deleted and still had purchases - since it won't be in sites. This shouldn't happen, since we shouldn't let users delete sites with upgrades, but there are probably ways round it at the moment.

scruffian · 2015-12-02T15:31:22Z

I tried mocking @jeherve's credit card data to see if I could reproduce it, and I was unable to, so the problem must lie elsewhere. I'm not sure the best way to proceed now...

drewblaisdell · 2015-12-03T02:04:22Z

client/lib/purchases/index.js

+
+			// If a site has been deleted then there will be no site with this ID in site.
+			if ( site ) {
+				slug = site.domain;


@scruffian I believe this should be site.slug.

drewblaisdell · 2015-12-03T02:21:36Z

@scruffian I updated your commit to use site.slug instead of site.domain and made some minor style tweaks to avoid assigning to different types of site objects to the same identifier.

I'm fairly confident that the credit card issues above are unrelated to this change and that this is safe to merge. I'm going to try and pair with @jeherve soon to make sure we figure out what is causing that error.

gziolo · 2015-12-03T07:24:42Z

client/me/purchases/list/index.jsx


-		if ( ! this.props.purchases.isFetching && ! this.props.purchases.hasLoadedFromServer ) {
-			return null;
+		if ( ! this.props.sites.fetched ) {


What about checking against sites.initialized? It looks like it works the same way as sites.fetched, but also works for data cached in local storage.

Nice catch - fixed.

jeherve · 2015-12-03T19:32:20Z

After another test, I can't reproduce these js errors anymore. Renewal went well, no issues at all.

Not sure what happened before, and what fixed it...

drewblaisdell · 2015-12-03T20:21:53Z

@scruffian and @gziolo checked the code and QA box for this, and I reviewed @scruffian's last commit (making only minor style changes), so this is ready to merge.

gziolo · 2015-12-03T20:42:56Z

👍

…ct (Fixes #122) This fixes the issue of individual purchases not loading on site with Site Redirect record set as primary domain. It changes the use of `domain` property from purchases data to getting the site slug from `SitesList`. This will the URLs will behave the same as in store.

…esList`

This removes the `SitesList` dependency from this function, and makes the behavior a bit more explicit.

…sites list - it might be deleted

…edirect Purchases: Individual purchases not loading on sites with Site Redirect

scruffian · 2015-12-03T22:36:51Z

\o/

umurkontaci changed the title ~~Purchases: Individual purchases not loading on sites with Site Redirect (Fixes #122)~~ Purchases: Individual purchases not loading on sites with Site Redirect Nov 23, 2015

umurkontaci added [Feature] Purchase Management Related to managing purchases such as subscriptions, plans, history, auto-renew, cancellation, etc. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 23, 2015

umurkontaci added this to the Purchases: v1 milestone Nov 23, 2015

umurkontaci self-assigned this Nov 23, 2015

fabianapsimoes added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 24, 2015

umurkontaci force-pushed the fix/122-purchases-with-site-redirect branch from 1f21c28 to 7df2ac6 Compare November 24, 2015 16:21

umurkontaci added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Nov 24, 2015

umurkontaci mentioned this pull request Nov 24, 2015

Purchases: Use the correct site address in the URL #627

Closed

umurkontaci force-pushed the fix/122-purchases-with-site-redirect branch from e6227b5 to c0a113c Compare November 24, 2015 16:43

drewblaisdell reviewed Nov 24, 2015
View reviewed changes

umurkontaci added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 24, 2015

umurkontaci force-pushed the fix/122-purchases-with-site-redirect branch from c0a113c to 0cdd3b6 Compare November 26, 2015 13:37

umurkontaci added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Nov 26, 2015

fabianapsimoes added the [Status] Needs Author Reply label Nov 27, 2015

drewblaisdell reviewed Dec 3, 2015
View reviewed changes

drewblaisdell force-pushed the fix/122-purchases-with-site-redirect branch 2 times, most recently from 74b46d2 to ceeb93b Compare December 3, 2015 02:12

gziolo reviewed Dec 3, 2015
View reviewed changes

drewblaisdell force-pushed the fix/122-purchases-with-site-redirect branch 2 times, most recently from 895d34b to cd7a33b Compare December 3, 2015 17:54

drewblaisdell added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 3, 2015

umurkontaci and others added 4 commits December 3, 2015 13:49

Purchases: Ensure that the sites are loaded before rendering `Purchas…

2face5f

…esList`

Purchases: Accept an array of sites in getPurchasesBySite

47564c6

This removes the `SitesList` dependency from this function, and makes the behavior a bit more explicit.

Purchases: Don't assume that a site we have a purchase for is in the …

f76af43

…sites list - it might be deleted

drewblaisdell force-pushed the fix/122-purchases-with-site-redirect branch from cd7a33b to f76af43 Compare December 3, 2015 21:49

drewblaisdell added a commit that referenced this pull request Dec 3, 2015

Merge pull request #536 from Automattic/fix/122-purchases-with-site-r…

4810232

…edirect Purchases: Individual purchases not loading on sites with Site Redirect

drewblaisdell merged commit 4810232 into master Dec 3, 2015

drewblaisdell removed the [Status] Ready to Merge label Dec 3, 2015

drewblaisdell deleted the fix/122-purchases-with-site-redirect branch December 3, 2015 22:03

fabianapsimoes modified the milestone: Purchases: Future Feb 25, 2016

jessie-ross mentioned this pull request Dec 17, 2020

WIP ExPlat: Self-Contained Client #48475

Closed

9 tasks

mayuba mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities mayuba/wp-calypso#80

Open

mdno mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities mdno/wp-calypso#123

Open

anarh mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities anarh/wp-calypso#111

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Purchases: Individual purchases not loading on sites with Site Redirect #536

Purchases: Individual purchases not loading on sites with Site Redirect #536

umurkontaci commented Nov 23, 2015

mikeshelton1503 commented Nov 24, 2015

fabianapsimoes commented Nov 24, 2015

umurkontaci commented Nov 24, 2015

drewblaisdell Nov 24, 2015

umurkontaci Nov 24, 2015

drewblaisdell Nov 25, 2015

umurkontaci Nov 26, 2015

drewblaisdell commented Nov 24, 2015

dzver commented Nov 25, 2015

drewblaisdell commented Nov 25, 2015

umurkontaci commented Nov 26, 2015

scruffian commented Nov 26, 2015

umurkontaci commented Nov 26, 2015

scruffian commented Nov 27, 2015

jeherve commented Nov 27, 2015

scruffian commented Dec 2, 2015

scruffian commented Dec 2, 2015

drewblaisdell Dec 3, 2015

drewblaisdell commented Dec 3, 2015

gziolo Dec 3, 2015

drewblaisdell Dec 3, 2015

jeherve commented Dec 3, 2015

drewblaisdell commented Dec 3, 2015

gziolo commented Dec 3, 2015

scruffian commented Dec 3, 2015

Purchases: Individual purchases not loading on sites with Site Redirect #536

Purchases: Individual purchases not loading on sites with Site Redirect #536

Conversation

umurkontaci commented Nov 23, 2015

mikeshelton1503 commented Nov 24, 2015

fabianapsimoes commented Nov 24, 2015

umurkontaci commented Nov 24, 2015

drewblaisdell Nov 24, 2015

Choose a reason for hiding this comment

umurkontaci Nov 24, 2015

Choose a reason for hiding this comment

drewblaisdell Nov 25, 2015

Choose a reason for hiding this comment

umurkontaci Nov 26, 2015

Choose a reason for hiding this comment

drewblaisdell commented Nov 24, 2015

dzver commented Nov 25, 2015

drewblaisdell commented Nov 25, 2015

umurkontaci commented Nov 26, 2015

scruffian commented Nov 26, 2015

umurkontaci commented Nov 26, 2015

scruffian commented Nov 27, 2015

jeherve commented Nov 27, 2015

scruffian commented Dec 2, 2015

scruffian commented Dec 2, 2015

drewblaisdell Dec 3, 2015

Choose a reason for hiding this comment

drewblaisdell commented Dec 3, 2015

gziolo Dec 3, 2015

Choose a reason for hiding this comment

drewblaisdell Dec 3, 2015

Choose a reason for hiding this comment

jeherve commented Dec 3, 2015

drewblaisdell commented Dec 3, 2015

gziolo commented Dec 3, 2015

scruffian commented Dec 3, 2015