Jetpack Connect: Redirect Pressable partner plan connection after authorize #20025

ebinnion · 2017-11-20T20:46:53Z

As part of activity log and rewind, we need users to approve credential collection after authorizing the connection to their new site. This PR will redirect all Pressable sites that are connecting for a partner plan to a separate authorize screen to approve the rewind connection.

To test:

Checkout branch locally
Run npm start.
- If you already have Calypso running, stop it and restart so that you get the updated config flags
Create a new Pressable site
Connect your site
This is janky, but grab the full connection URL once you're at WP.com, before the authorization details are taken out.
Replace wordpress.com with calypso.localhost:3000 and then go to that URL 🤦‍♂️
On the authorization page, ensure you see a co-branded form
After authorizing, ensure you're redirected to proper URL (at time of this PR being created, it's redirecting to /me. We need the updated URL from the Activity Log folks

matticbot · 2017-11-20T20:46:54Z

Test live: https://calypso.live/?branch=update/jpc-redirect-pressable-to-url

johnHackworth · 2017-11-21T08:46:59Z

client/jetpack-connect/auth-logged-in-form.jsx

+
+		// Redirect sites hosted on Pressable with a partner plan to some URL.
+		// 51652 is a testing partner ID.
+		if ( 49640 === partnerId || 51652 === partnerId ) {


Can we move those magic numbers to a constant somewhere ( for example,

wp-calypso/client/jetpack-connect/auth-logged-in-form.jsx

Line 61 in 09997d9

* Constants

) labeling as what they are? 😺

rcoll · 2017-11-24T20:58:56Z

I added the correct URL in 71e9c16.

Please remember to wall off that flow from non-a12s until we ship to customers.

ebinnion · 2017-11-27T17:26:36Z

I've updated the PR per your feedback @johnHackworth. This should be ready for review now.

sirreal · 2017-11-28T12:11:32Z

This is janky, but grab the full connection URL once you're at WP.com, before the authorization details are taken out.

You can append &calypso_env=development to the URL on the connect button to hit calypso.localhost directly.

sirreal

I've left some questions and suggestions.

Testing works well for me, I see a branded connect page and I'm sent to a NUX flow I've never seen before after connect.

Connecting a "normal" site continues to work… normally 🙂

sirreal · 2017-11-28T11:58:06Z

client/jetpack-connect/auth-logged-in-form.jsx

@@ -185,6 +197,18 @@ class LoggedInForm extends Component {
 		return startsWith( get( props, [ 'authorizationData', 'queryObject', 'from' ] ), 'jpo' );
 	}

+	shouldRedirectJetpackStart( props ) {
+		const partnerId = parseInt( get( props, 'partnerId' ), 10 );


props should always be an object, get(...) shouldn't add any value, we can access directly, often we'll use destructuring assignment:

shouldRedirectJetpackStart( props ) { const { partnerId } = props; // … } // or even shouldRedirectJetpackStart( { partnerId } ) { // partnerId is already assigned! // … }

parseInt should be redundant, it's in the selector:

wp-calypso/client/state/selectors/get-jetpack-connect-partner-id.js

Lines 18 to 20 in 875df39

export default function getJetpackConnectPartnerId( state ) {

return parseInt( get( getAuthorizationRemoteQueryData( state ), 'partner_id', 0 ), 10 );

}

sirreal · 2017-11-28T11:58:25Z

client/jetpack-connect/auth-logged-in-form.jsx

+		// Redirect sites hosted on Pressable with a partner plan to some URL.
+		if (
+			config.isEnabled( 'jetpack/connect-redirect-pressable-credential-approval' ) &&
+			PRESSABLE_CLIENT_ID === parseInt( partnerId, 10 )


parseInt is redundant, it's in the selector:

wp-calypso/client/state/selectors/get-jetpack-connect-partner-id.js

Lines 18 to 20 in 875df39

export default function getJetpackConnectPartnerId( state ) {

return parseInt( get( getAuthorizationRemoteQueryData( state ), 'partner_id', 0 ), 10 );

}

sirreal · 2017-11-28T12:05:35Z

client/jetpack-connect/auth-logged-in-form.jsx


 /**
 * Constants
 */
 const MAX_AUTH_ATTEMPTS = 3;
 const PLANS_PAGE = '/jetpack/connect/plans/';
 const debug = debugModule( 'calypso:jetpack-connect:authorize-form' );
+const PRESSABLE_CLIENT_ID = 49640;


Should this be PRESSABLE_PARTNER_ID?

sirreal · 2017-11-28T12:06:53Z

config/wpcalypso.json

@@ -41,6 +41,7 @@
 		"help/courses": true,
 		"jetpack/activity-log": true,
 		"jetpack/api-cache": true,
+		"jetpack/connect-redirect-pressable-credential-approval": true,


This is a public environment. Is that OK?

sirreal · 2017-11-28T12:06:58Z

config/horizon.json

@@ -39,6 +39,7 @@
 		"help/courses": true,
 		"jetpack/activity-log": true,
 		"jetpack/api-cache": true,
+		"jetpack/connect-redirect-pressable-credential-approval": true,


This is a public environment. Is that OK?

…horize

… config

…nments

ebinnion · 2017-11-28T16:56:53Z

Thanks for the review @sirreal. I've updated the PR per your feedback and have rebased against master to get rid of a merge conflict.

sirreal

I followed the testing instructions and this works well. I left a comment regarding a block that was tricky for me to decipher, feel free to do with that as you wish.

Ready to 🚢 at will, thanks!

sirreal · 2017-11-29T12:59:44Z

client/jetpack-connect/auth-logged-in-form.jsx

+		// If the redirect flag is set, then we conditionally redirect the Pressable client to
+		// a credential approval screen. Otherwise, we need to redirect all other partners back
+		// to wp-admin.
+		return partnerRedirectFlag ? partnerId && PRESSABLE_PARTNER_ID !== partnerId : partnerId;


It's taken me several readings to appreciate what's going on here. This should return true if we have a partnerId and it's not the PRESSABLE_PARTNER_ID, otherwise false. Is that right?

Do you think something like the following better expresses the intent?

shouldRedirectJetpackStart( { partnerId } ) { if ( ! partnerId ) { return false; } // Pressable sites have a special NUX behind a flag if ( config.isEnabled( 'jetpack/connect-redirect-pressable-credential-approval' ) ) { return PRESSABLE_PARTNER_ID !== partnerId } return true; }

Thanks for that. I'd prefer to merge for now, but will be glad to address in a follow-up PR.

ebinnion added Jetpack Rewind [Status] In Progress labels Nov 20, 2017

ebinnion requested review from sirreal and johnHackworth November 20, 2017 20:46

johnHackworth reviewed Nov 21, 2017

View reviewed changes

ebinnion force-pushed the update/jpc-redirect-pressable-to-url branch from 71e9c16 to 739ca22 Compare November 27, 2017 16:58

ebinnion added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Nov 27, 2017

sirreal reviewed Nov 28, 2017

View reviewed changes

ebinnion and others added 8 commits November 28, 2017 10:47

Jetpack Connect: Redirect Pressable partner plan connection after aut…

acbc71c

…horize

Return the correct URL that leads to the Pressable NUX flow

1c01e81

JPC: Use constant for Pressable client ID

fb09263

JPC: Conditionally redirect to Pressable credential approval based on…

472cb07

… config

JPC: Conditionally redirect JPS back to wp-admin after r165953-wpcom

5c3a11b

JPC: Change wording of Pressable client ID

cb7e7c8

JPC: Update how partnerId is accessed for Pressable redirect

10c0f37

JPC: Remove Pressable redirect flag from horizon and wpcalypso enviro…

52d2bfd

…nments

ebinnion force-pushed the update/jpc-redirect-pressable-to-url branch from e75c6c1 to 52d2bfd Compare November 28, 2017 16:48

ebinnion self-assigned this Nov 28, 2017

sirreal self-requested a review November 28, 2017 17:33

sirreal approved these changes Nov 29, 2017

View reviewed changes

ebinnion merged commit cce731d into master Nov 29, 2017

ebinnion deleted the update/jpc-redirect-pressable-to-url branch November 29, 2017 15:55

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 29, 2017

sirreal mentioned this pull request Dec 14, 2017

Jetpack connect: Rip out queryObject state #20406

Merged

7 tasks

ebinnion mentioned this pull request Feb 23, 2018

Config: Jetpack Connection: Redirect Pressable to credential approval in production #22754

Merged

JoshuaGoode mentioned this pull request Feb 16, 2023

Feature Request / Jetpack Backup : Add additional credential auto-config opportunities #73451

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetpack Connect: Redirect Pressable partner plan connection after authorize #20025

Jetpack Connect: Redirect Pressable partner plan connection after authorize #20025

ebinnion commented Nov 20, 2017 •

edited

Loading

matticbot commented Nov 20, 2017

johnHackworth Nov 21, 2017 •

edited

Loading

rcoll commented Nov 24, 2017

ebinnion commented Nov 27, 2017

sirreal commented Nov 28, 2017

sirreal left a comment

sirreal Nov 28, 2017

sirreal Nov 28, 2017

sirreal Nov 28, 2017

sirreal Nov 28, 2017

sirreal Nov 28, 2017

ebinnion commented Nov 28, 2017

sirreal left a comment

sirreal Nov 29, 2017

ebinnion Nov 29, 2017

	export default function getJetpackConnectPartnerId( state ) {
	return parseInt( get( getAuthorizationRemoteQueryData( state ), 'partner_id', 0 ), 10 );
	}

Jetpack Connect: Redirect Pressable partner plan connection after authorize #20025

Jetpack Connect: Redirect Pressable partner plan connection after authorize #20025

Conversation

ebinnion commented Nov 20, 2017 • edited Loading

matticbot commented Nov 20, 2017

johnHackworth Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

rcoll commented Nov 24, 2017

ebinnion commented Nov 27, 2017

sirreal commented Nov 28, 2017

sirreal left a comment

Choose a reason for hiding this comment

sirreal Nov 28, 2017

Choose a reason for hiding this comment

sirreal Nov 28, 2017

Choose a reason for hiding this comment

sirreal Nov 28, 2017

Choose a reason for hiding this comment

sirreal Nov 28, 2017

Choose a reason for hiding this comment

sirreal Nov 28, 2017

Choose a reason for hiding this comment

ebinnion commented Nov 28, 2017

sirreal left a comment

Choose a reason for hiding this comment

sirreal Nov 29, 2017

Choose a reason for hiding this comment

ebinnion Nov 29, 2017

Choose a reason for hiding this comment

ebinnion commented Nov 20, 2017 •

edited

Loading

johnHackworth Nov 21, 2017 •

edited

Loading