Wp-calypso: Jetpack: Broken upgrade flow from plugin

[beaulebens]

I'm filing this here because I believe the most immediate possible fix will be within Calypso/JPC.

Repro

1. install/activate/connect Jetpack
2. go to the Plans page within the plugin (/wp-admin/admin.php?page=jetpack#/plans)
3. click the "Compare All Plans" button in the main graphic area (the URL on the button looks like https://jetpack.com/redirect/?source=plans-compare-free&site={site}; note that we don't know the protocol of the site, which is not ideal)
4. land on a URL something like https://wordpress.com/jetpack/connect/store/?site={site} after redirection

Note at this point that if your site was installed in a subdirectory, there will be an additional layer of encoding which starts messing it up, so the :: that we'd normally see in a site slug is shown as %253A%253A

1. now click on a plan with the intent to purchase it; you'll be pushed into JPC, with the URL pre-filled (https://wordpress.com/jetpack/connect?url={site}).

Because the protocol is missing (and the sub-directory is extra-encoded), this will fail, and you'll see an error saying "That's not a valid url.".

I think we need to;

1. include the protocol on outbound links from the Jetpack plugin
2. not double-encode things
3. preferably in JPC, handle cases where the protocol is missing, or encoding is wonky, and just recover and continue on

[sirreal]

see pMz3w-7wI-p2

[sirreal]

Here's a dump of my findings:

From the "Compare Plans" button, there are two 302 redirects before landing on the WordPress.com plans page, each of which seem to apply url encoding to the GET parameters, resulting in : -> %3A%3A -> %253A%253A. This is not good as it results in an invalid URL error when we get to the connection page after attempting to purchase a plan.

Additionally, this is "slug" encoding, which translates / to ::, but https://wordpress.com/jetpack/connect?url={url} expects a _url_ and not a _slug_, so we'd need to attempt to translate back to a URL when we go to the connect page (replace :: with /). A single-encoded site slug is recognized at the connection page, but it does not recognize that it's a connected site because it tries to match the slug as a URL.

[oskosk]

@richardmuscat besides the encoding issue Jon mentions, looks like the code deciding where to redirect is not taking in account the value "plan-compare-free" as $source in order to calculate which cta and target to follow with the redirection.

[withinboredom]

It's a problem of the default cta/target page not properly decoding the url before going on to the connect/etc. The real place is appearing to work correctly. Deployed in r10958-a8c Can someone give it a sanity check? I'll hunt down the problem with the default cta/target page.

[oskosk]

Thanks a lot @withinboredom ! I'd say that the changeset (r10958) is fine and fixes the broken flow for sites in a subdir. But I have a few doubts that may be clarified by @beaulebens or @johnHackworth .

With this change, after I click Compare All Plans in my Jetpack dashboard, I'm taken to this page (jetpack.com/pricing)

Clicking the Choose Plan link takes me to /jetpack/connect/store and this one redirects me to /jetpack/connect/plans/:site

Subdirs are properly carried from one end to the other, but

1. What I'm not sure about is if https://jetpack.com/pricing/plans-compare-free/ is the right page to land coming from the Compare all plans link in Jetpack's Dashboard. But if its is, I have a few other extra doubts:
   
   
   
   1. Should Clicking the Choose Plan take the user of an already connected site to /jetpack/connect/plans/:site(Connect Plans) or to /plans/:site (regular plans page) ?
   
   
   2. If the user is to be taken to the Connect Plans version, why am I instantly redirected to the regular plans version after being flashed with the Connect Plans version? (See gif). This was already happening before @withinboredom's changeset was introduced.
   
   

[beaulebens]

@oskosk I _think_ some of your observations there are coming from us sending users through this flow in an attempt to handle the case where they're logged out of WP.com. Maybe @richardmuscat can confirm that though.

[oskosk]

Closing this as it looks like fixed now. My observations were fixed in #18627. @beaulebens please reopen if it's not fixed.