Update/split site user connection mgt my jetpack #41520

jboland88 · 2025-02-03T22:04:49Z

Proposed changes:

This PR allows users to break user connections separately from the connection management window in My Jetpack
This allows non-admin users to manage their own user connection from My Jetpack
Some messaging improvements are also made in the connection messages shown to non-admins

Does this pull request change what data or activity we track or use?

Yes, this PR adds one new click event for when someone clicks to disconnect their user account: jetpack_manage_connection_dialog_disconnect_user_click

Testing instructions:

To test this PR fully, you will need a test site with a non admin user and an admin user along with two separate WPCOM accounts that you can use to connect these users individually.
Start by connecting both the admin and non-admin users with their respective WPCOM accounts
As the admin user, navigate to My Jetpack (Jetpack in the admin left nav)
Your connection area should look like below, showing both your wpcom username and email:
Now, check My Jetpack as the non-admin user. You should see your wpcom username and email as well as the local username of the connected site owner
As the non-admin user, click "Manage" next to the connection icons. You should get the manage connection dialog with only one option - disconnecting your user account:
Click on "Disconnect My User account". The modal should show in a "pending" state while the user disconnects and then close itself. The page will also reload after the modal closes
Now that the non-admin user has been disconnected, the connection area should no longer show the "Manage" link next to the connection icons. In place of the user information, there should be a prompt to sign back in.
Test that you can sign in again as the non-admin user. Leave the non-admin user connected for further testing later on.
Now, return to the site as the admin user
Click on "Manage" next to the connection icons to open the connection management modal
The admin user should have three options (assuming they are also the connection owner) - transfer ownership, disconnect user account, and Disconnect Jetpack
Go ahead and disconnect the admin user. The page will reload after disconnection. After reload, the connection area should look something like this (the message can differ depending on whether you had products that were previously relying on the user connection to work)
If you click "Manage" again as the admin user, there should only be one option, to Disconnect Jetpack. This will disconnect the site from Jetpack (removes the site token)
Go ahead and disconnect the site from Jetpack. The connection area should prompt for re-connection of the site after disconnection:
Re-connect the site by clicking "Connect"
After the site connects, before re-connecting the admin user, visit the site as the non-admin user (who we should have left in a connected state in our previous test)
For the connected non-admin, now that the admin user and connection owner has disconnected, the connection area should no longer show the connection owner as being "also connected"
The option to "Manage" and disconnect the non-admin user is still present - make sure that you can still disconnect the non-admin user here with no admin connection owner.
After the non-admin user disconnects, without the presence of a connection owner, the connection area should now indicate that an admin needs to connect before they can sign in again
Fiddle around with the different users and connection states and confirm that the copy and messaging makes sense for different states and roles.

Testing with an older version of the Jetpack plugin

Because this PR moves the /connection/user API endpoint from the Jetpack plugin to the connection package, there are scenarios where both the Jetpack plugin and the connection package will register the same endpoint. To mitigate this, this PR makes sure the API endpoint registration for the connection package runs later than the registration for the Jetpack plugin by running on priority 11 for the rest_api_init hook.

To test that the new endpoint it still called when an older version of Jetpack is active:

Get a new test site with Jetpack Beta (works better if it's not your local dev environment)
Checkout this branch on the Jetpack Boost plugin using the Jetpack Beta plugin
In the Jetpack Beta plugin, set the version of the main Jetpack plugin to 14.2
Confirm that you are still able to disconnect your user account from My Jetpack.

github-actions · 2025-02-03T22:05:38Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the update/split-site-user-connection-mgt-my-jetpack branch.
- For jetpack-mu-wpcom changes, also add define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true ); to your wp-config.php file.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack update/split-site-user-connection-mgt-my-jetpack

bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/split-site-user-connection-mgt-my-jetpack

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

jboland88 · 2025-02-04T22:26:03Z

Here's an open question @jeherve and @bindlegirl

This PR moves an API endpoint from the Jetpack plugin to the connection package (connection/user). The endpoint in the package has a version check so that it won't double-register the endpoint if an older version of Jetpack has already registered it.

But this raises the issue - if a standalone plugin using the newer version of My Jetpack and newer connection package is installed on a site with an older version of the Jetpack plugin, the way My Jetpack is going to expect the connection/user endpoint to respond is not going to align with what the old version of the Jetpack plugin will return.

I've tested and can verify this behavior with the Jetpack Beta plugin.

An alternative could be to register the endpoint in the connection package as a "new" endpoint with a different name - but that feels counterproductive. Any ideas for the best way forward here?

bindlegirl · 2025-02-05T10:00:34Z

Any ideas for the best way forward here?

Would skipping the version check and setting the override parameter to true help solve the problem?

I haven't checked the differences in the updated endpoint just yet so I am throwing out that suggestion.

jboland88 · 2025-02-05T13:49:37Z

Would skipping the version check and setting the override parameter to true help solve the problem?

@bindlegirl - that sounds like it would be a good option. I've overlooked the override parameter on register_rest_route until now - TIL! As long as the route for the connection package is registered later than Jetpack's that would allow us to replace it 👍

The new endpoint should be compatible if called from the older version of the Jetpack plugin, the old behavior and arguments are still present, the updated version just adds a new optional parameter and uses a new mapped capability to allow for unlinking of non-admins w/o a connection owner.

jeherve · 2025-02-05T18:26:05Z

This PR moves an API endpoint from the Jetpack plugin to the connection package (connection/user). The endpoint in the package has a version check so that it won't double-register the endpoint if an older version of Jetpack has already registered it.

What if the new endpoint were to have a different slug? We'd keep the old Jetpack-only endpoint around, but stop using it in new versions of My Jetpack. Then in a few versions, we could get rid of the Jetpack-only version entirely.

CodeyGuyDylan

I tested the changes thoroughly with both the Jetpack main plugin and then again with a standalone plugin (I did Protect) and it all works as expected. I couldn't find any issues!

I do have one question about the user disconnection. Is a page reload required for this? Or could we update all the data on the screen async? You obviously know more about this flow than me so I assume there probably needs to be one for some reason, but wanted to bring it up just in case 😄

I left some comments. None major, but there are enough I think it warrants requesting changes 😄

CodeyGuyDylan · 2025-02-05T18:11:53Z

projects/js-packages/connection/components/manage-connection-dialog/index.jsx

+							{ isCurrentUserAdmin &&
+								connectedUser.currentUser?.isConnected &&
+								connectedUser.currentUser?.isMaster && (
+									<ManageConnectionActionCard


I know this is the same functionality as before, but I was wondering if we should open a new window when the user clicks this link? The "External link" icon generally means a new window will open as well in my experience

Good thought, yes I think that makes sense

CodeyGuyDylan · 2025-02-05T18:15:07Z

projects/js-packages/connection/components/manage-connection-dialog/index.jsx

+	onClick = () => null,
+	link = '#',
+	action,
+	disabled,


Should we have disabled be false by default? I guess an omission leaving it null would most likely have the same behavior, I guess I just prefer explicit defaults 😅

yes, I agree adding false is more clear 👍

CodeyGuyDylan · 2025-02-05T18:30:01Z

projects/packages/connection/src/class-rest-connector.php

+	 */
+	public static function unlink_user( $request ) {
+
+		if ( ! isset( $request['linked'] ) || false !== $request['linked'] ) {


I'm not running into issues while testing, so I assume this works correctly. I'm just having a hard time understanding why the correct thing to do is return an error if the user is linked in this case 🤔

Wouldn't false !== $request['linked'] be true if the user is linked? Or am I misunderstanding the linked parameter?

It's kind of an odd parameter - if it's false it means we want the user to be unlinked from what I understand. At least that's the effect. It doesn't have to do with checking that the current user is connected already or not.

Thank you for explaining! It is a very confusingly named param 😅 But it makes sense why it works correctly

CodeyGuyDylan · 2025-02-05T18:35:21Z

projects/packages/connection/src/class-rest-connector.php

+
+		// Allow admins to force a disconnect by passing the "force" parameter
+		// This allows an admin to disconnect themselves
+		if ( isset( $request['force'] ) && false !== $request['force'] && current_user_can( 'manage_options' ) && ( new Manager( 'jetpack' ) )->disconnect_user_force( get_current_user_id() ) ) {


I'm finding it a bit weird to have the actual disconnect function be invoked in the if statement checks. I guess it works correctly, and I can see the benefit of a simpler hierarchy of if statements but I don't think it's best practices and, imo, it makes it harder to read and understand what's happening 🤔

I disagree on this one - since the function returns a boolean, I think It's OK to run in as part of the if statement, given that the return value of the function is intended to be a rest_response object or WP_Error.

The first couple of checks in the if statement are used to bail early before trying the disconnect.

The second else if condition follows the same pattern to determine if the disconnect operation worked or failed.

I don't think this is a big enough issue to push back too hard, so I won't block approval for it again 😄 However I just wanted to share an article I found that explains, in better words than I could, why I am on the other side of this debate: https://www.teamten.com/lawrence/programming/keep-if-clauses-side-effect-free.html

CodeyGuyDylan · 2025-02-05T18:39:24Z

projects/packages/my-jetpack/_inc/components/connection-status-card/index.tsx

@@ -63,7 +63,7 @@ const ConnectionListItem: ConnectionListItemType = ( {
 		<div className={ styles[ 'list-item' ] }>
 			<Text className={ clsx( styles[ 'list-item-text' ], statusStyles ) }>
 				{ icon && <Icon icon={ icon } /> }
-				{ text }
+				<span>{ text }</span>


Was this for some sort of styling change? I don't see any CSS changes related to this so I just wanted to make sure it wasn't leftover from a previous implementation

It was to appease the tests 😅 They were having trouble locating an element with the specified text on the screen, adding a wrapping element helped the matching

Makes sense 😆

CodeyGuyDylan · 2025-02-05T18:44:28Z

projects/packages/my-jetpack/_inc/components/connection-status-card/index.tsx

-					__( 'Connected as %1$s (Owner).', 'jetpack-my-jetpack' ),
-					userConnectionData.currentUser?.wpcomUser?.display_name
+					/* translators: %1$s is user name, %2$s is the user email */
+					__( 'Connected as %1$s (Owner) ( %2$s ).', 'jetpack-my-jetpack' ),


I noticed the email here has spaces around it whereas the connected owner does not. Was this intentional?

Not intentional, I'll remove those spaces!

CodeyGuyDylan · 2025-02-05T18:49:03Z

In addition to the comments in my review, I just noticed it looks like a few of the connection tests are not passing

bindlegirl · 2025-02-06T09:39:40Z

@jeherve @jboland88

I discussed this case with @fgiannar, and she shared some previous PRs where endpoints were moved to connection package #16384 and #20440

I think it would be best to follow the same procedure we used there (moving the endpoint, deprecating methods). The workings of the override parameter are explained in #20440

It seems the safest route would be to make sure the UI can support the edge case where the old endpoint is registered last.

bindlegirl · 2025-02-06T09:48:06Z

Another suggestion is to do the endpoint changes in a separate PR.
Hat tip @fgiannar

jboland88 · 2025-02-07T16:08:10Z

Thanks for the review @CodeyGuyDylan

I do have one question about the user disconnection. Is a page reload required for this? Or could we update all the data on the screen async? You obviously know more about this flow than me so I assume there probably needs to be one for some reason, but wanted to bring it up just in case 😄

Sort of. After the user is disconnected, we can update the connection state on the front end but the red bubble alerts and the evaluation of "broken" modules that required a user connection are currently pulled from the initial react state - which is loaded in from PHP.

The page "works" without reloading, the connection area still shows the user as disconnected, but there are a few things that are not fully up to date - card statuses, alert messages, etc. I do like the experience better without the reload, but I am in favor of accepting the tradeoff of reloading in order to update the product and alert information for now. Once we re-fetch that data async and aren't pulling it from the initial react state then the reload could be removed here.

…d error handling in case the user disconnect fails

CodeyGuyDylan · 2025-02-07T20:04:19Z

The page "works" without reloading, the connection area still shows the user as disconnected, but there are a few things that are not fully up to date - card statuses, alert messages, etc. I do like the experience better without the reload, but I am in favor of accepting the tradeoff of reloading in order to update the product and alert information for now. Once we re-fetch that data async and aren't pulling it from the initial react state then the reload could be removed here.

That makes sense, I guess it'd be quite a bit more work to make sure everything is in sync after that (if it's even possible to get all of it). I think it's okay in this case 😄

CodeyGuyDylan

Looks like you have one linting issue that's causing a test to fail, but other than that I think the changes look great, LGTM

CodeyGuyDylan · 2025-02-07T20:17:51Z

projects/js-packages/connection/components/manage-connection-dialog/index.jsx

+						'jetpack-connection-js'
+					);
+				}
+				setUnlinkError( errorMessage );


Nice addition with the error message 😄

bindlegirl

Tested it all and it works well. Nice job!

jboland88 added 12 commits February 3, 2025 17:40

Try to fix optimization bug

c5b4499

small improvements and bug fixes

d434968

Fixing some tests

d457ef8

More test repair

7d28020

more test repair

ad41d7b

Add new tests

89a9fb8

Fix error in new unit tests

66b1f6c

update js tests for my jetpack

1cccd8e

Actually fix my jetpack js tests

ccc18b7

A few small adjustments

072df8e

Add click tracking for user disconnect

960a43a

Fix jetpack versioning in conditional API registration

c4c4d62

jboland88 marked this pull request as ready for review February 4, 2025 21:36

jboland88 mentioned this pull request Feb 4, 2025

[Deprecate Dashboard] React error and blank page when trying to disconnect (as non-admin) #34535

Open

Set the new connection/user endpoint to override and register later

9c4f5b0

CodeyGuyDylan requested changes Feb 5, 2025

View reviewed changes

bindlegirl mentioned this pull request Feb 6, 2025

Jetpack: add connection owner info to dashboard #41606

Merged

3 tasks

jboland88 added 2 commits February 7, 2025 12:28

Fix some connection test, add back deprecated endpoint in Jetpack, ad…

e8b54c7

…d error handling in case the user disconnect fails

A few small adjustments

67f4292

CodeyGuyDylan approved these changes Feb 7, 2025

View reviewed changes

bindlegirl approved these changes Feb 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/split site user connection mgt my jetpack #41520

Update/split site user connection mgt my jetpack #41520

jboland88 commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

jboland88 commented Feb 4, 2025

bindlegirl commented Feb 5, 2025

jboland88 commented Feb 5, 2025 •

edited

Loading

jeherve commented Feb 5, 2025

CodeyGuyDylan left a comment

CodeyGuyDylan Feb 5, 2025

jboland88 Feb 7, 2025

CodeyGuyDylan Feb 5, 2025

jboland88 Feb 7, 2025

CodeyGuyDylan Feb 5, 2025

jboland88 Feb 7, 2025

CodeyGuyDylan Feb 7, 2025

CodeyGuyDylan Feb 5, 2025

jboland88 Feb 7, 2025

CodeyGuyDylan Feb 7, 2025

CodeyGuyDylan Feb 5, 2025

jboland88 Feb 7, 2025 •

edited

Loading

CodeyGuyDylan Feb 7, 2025

CodeyGuyDylan Feb 5, 2025

jboland88 Feb 7, 2025

CodeyGuyDylan commented Feb 5, 2025

bindlegirl commented Feb 6, 2025 •

edited

Loading

bindlegirl commented Feb 6, 2025

jboland88 commented Feb 7, 2025

CodeyGuyDylan commented Feb 7, 2025

CodeyGuyDylan left a comment

CodeyGuyDylan Feb 7, 2025

bindlegirl left a comment

Update/split site user connection mgt my jetpack #41520

Are you sure you want to change the base?

Update/split site user connection mgt my jetpack #41520

Conversation

jboland88 commented Feb 3, 2025 • edited Loading

Proposed changes:

Does this pull request change what data or activity we track or use?

Testing instructions:

Testing with an older version of the Jetpack plugin

github-actions bot commented Feb 3, 2025 • edited Loading

jboland88 commented Feb 4, 2025

bindlegirl commented Feb 5, 2025

jboland88 commented Feb 5, 2025 • edited Loading

jeherve commented Feb 5, 2025

CodeyGuyDylan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jboland88 Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeyGuyDylan commented Feb 5, 2025

bindlegirl commented Feb 6, 2025 • edited Loading

bindlegirl commented Feb 6, 2025

jboland88 commented Feb 7, 2025

CodeyGuyDylan commented Feb 7, 2025

CodeyGuyDylan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bindlegirl left a comment

Choose a reason for hiding this comment

jboland88 commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

jboland88 commented Feb 5, 2025 •

edited

Loading

jboland88 Feb 7, 2025 •

edited

Loading

bindlegirl commented Feb 6, 2025 •

edited

Loading