Skip to content

Isolate Jetpack Onboarding from Jetpack Connect#8414

Merged
oskosk merged 2 commits intomasterfrom
fix/isolate-jpo-from-jpc
Dec 26, 2017
Merged

Isolate Jetpack Onboarding from Jetpack Connect#8414
oskosk merged 2 commits intomasterfrom
fix/isolate-jpo-from-jpc

Conversation

@tyxla
Copy link
Copy Markdown
Member

@tyxla tyxla commented Dec 23, 2017

The problem

As found independently by @ockham and @oskosk and discussed in p1513787687000060-slack-jetpack-onboarding and p1513970123000089-slack-luna, if a user has an onboarding token, this would cause the connection links to contain the onboarding=TOKEN parameter, which would then lead users to a broken JPC flow (because JPO in Calypso is not yet in production).

To reproduce the bug:

  • Checkout the latest Jetpack master on your unconnected Jetpack sandbox.
  • While logged in as an admin, load /wp-admin/admin.php?page=jetpack&action=onboard
  • Go to /wp-admin/admin.php?page=jetpack#/
  • Inspect the connect button link - it contains the onboarding=TOKEN parameter.
  • Attempt to connect - you will be dropped at an empty page in WP.com.

The reason

Seems like this is caused by some code from the previous JPOalypso attempt, where connection URL was directly used to start the onboarding flow. This is no longer the case, so adding the token parameter must be decoupled from the connection URL method - then users with onboarding tokens will be able to use the connection flow like they did before. This is what this PR does, essentially.

Testing instructions

  • Checkout this PR on your unconnected Jetpack sandbox.
  • While logged in as an admin, load /wp-admin/admin.php?page=jetpack&action=onboard
  • Go to /wp-admin/admin.php?page=jetpack#/
  • Inspect the connect button link - it should NOT contain the onboarding=TOKEN parameter.
  • Attempt to connect - you should be able to connect successfully.

@tyxla tyxla self-assigned this Dec 23, 2017
@tyxla tyxla requested review from AnnaMag, ockham and oskosk December 23, 2017 00:44
@tyxla tyxla requested a review from a team as a code owner December 23, 2017 00:44
@oskosk oskosk added this to the 5.7 milestone Dec 26, 2017
oskosk
oskosk approved these changes Dec 26, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests well for me, thanks!

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 26, 2017
@oskosk oskosk merged commit 0f91008 into master Dec 26, 2017
@oskosk oskosk deleted the fix/isolate-jpo-from-jpc branch December 26, 2017 14:11
@tyxla
Copy link
Copy Markdown
Member Author

tyxla commented Dec 28, 2017

Thank you @oskosk!

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Dec 29, 2017 •
edited
Loading

@tyxla as discussed on Slack, fresh sites are reaching the authorize screen instead of the onboarding screen with current code in master. To reproduce.

Either test on a really fresh site, or do the following on an unconnected site before following the numbered steps:

wp jetpack options delete id
wp jetpack options delete blog_token
wp jetpack options delete onboarding
  1. Try to reach wp-admin/admin.php?page=jetpack&action=onboard&calypso_env=development on your site.
    What ends up happening here, is that when you arrive for the first time to the 'onboard': case, build_connect_url() returns a local url like this:
    /wp-admin/admin.php?page=jetpack&action=register&_wpnonce=40b8392343.
    (That is, a local URL for the register action. This action register populates the blog id and the blog_token among the options.
  2. Expect to arrive to the authorize screen not the onboarding screen.
  3. Try to reach again wp-admin/admin.php?page=jetpack&action=onboard&calypso_env=development on your site.
    When you try again,, this time build_connect_url() returns a non-local URL (jetpack.wordpress.com) and then the calypso_env and onboarding parameters are added ending with a redirect to something like this:
    https://jetpack.wordpress.com/jetpack.authorize/1/?etcetcetc&calypso_env=development&onboarding=QYV3O56n5Y2OTbrXCnlOBve45JINjJP7
  4. Expect to arrive to the onboarding page.

@tyxla tyxla mentioned this pull request Jan 3, 2018
Merged
@tyxla
Copy link
Copy Markdown
Member Author

tyxla commented Jan 3, 2018

Thanks so much for diagnosing this one! Fix for it is here: #8449

@tyxla tyxla removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ockham ockham Awaiting requested review from ockham

@AnnaMag AnnaMag Awaiting requested review from AnnaMag

1 more reviewer

@oskosk oskosk oskosk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tyxla tyxla

Labels

General

Projects

None yet

Milestone

5.7

Development

Successfully merging this pull request may close these issues.

2 participants

@tyxla @oskosk