Connection: move the connection owner deletion notice to the Connection package #17136

kbrown9 · 2020-09-10T20:26:27Z

A notice displays when a user attempts to delete the connection owner. Move this notice and the endpoint for setting the connection owner to the connection package. With this change, the connection owner deletion notice will be displayed for all consumers of the connection package.

Fixes #15568

Changes proposed in this Pull Request:

Move the delete_user_update_connection_owner_notice method from the JITM package to the Connection package.
Move the /connection/owner endpoint, including the callback and permission callback, to the Connection package.

Jetpack product discussion

n/a

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

no

Testing instructions:

Attempt to delete the connection owner with a logged-out admin.

Install, activate, and connect Jetpack. This user is the primary admin and the current connection owner.
Checkout this PR's branch.
Navigate to Users -> Add New and create another administrator user. This user is the secondary admin. Don't connect this user yet.
While logged in as the secondary admin, navigate to the Users page and attempt to delete the primary user (connection owner). You should see a notice that warns you that you're attempting to delete the connection owner.
Click the Connect to WordPress.com button to connect the secondary admin to WPCOM.
Refresh the user deletion page from step 4 and click the Set new connection owner button to change the connection owner. (Don't delete the primary admin user, because we'll switch the connection owner back to that user in the next test.)
The secondary admin is now the connection owner. You can verify this by navigating to Tools -> Site Health, selecting the Info tab, and opening the Jetpack drop-down. The Jetpack Master User (aka connection owner) should be the secondary admin.

Attempt to delete the connection owner with a logged-in admin.

Log in as the primary admin from step 1 above. This user should already be connected to WordPress.com
Navigate to Users and attempt to delete the connection owner, the secondary admin.
Since the primary admin is already connected to WordPress.com, you should see the Set new connection owner button in the notice. Click that button.
The primary admin is now the connection owner. You can verify this by navigating to Tools -> Site Health, selecting the Info tab, and opening the Jetpack drop-down. The Jetpack Master User (aka connection owner) should be the primary admin.

After conducting these tests, verify that no errors were generated in the debug.log. You can also verify that the jetpack_connection_owner_notice_view and jetpack_set_connection_owner_success tracks events were recorded.

Verify that the `connection/owner` endpoint checks user permissions properly.

Send a POST request to the jetpack/v4/connection/owner endpoint while not logged in.
Verify that a 401 response is received with the code invalid_user_permission_set_connection_owner.

Proposed changelog entry for your changes:

tbd

…on package A notice displays when a user attempts to delete the connection owner. Move this notice and the endpoint for setting the connection owner to the connection package. With this change, the connection owner deletion notice will be displayed for all consumers of the connection package.

test-case-reminder

Here are some suggested test cases for this PR.

Connection

In-place connection with free plan
In-place connection with paid plan
In-place connection with product purchase
Classic connection. Use Safari, or set a constant JETPACK_SHOULD_NOT_USE_CONNECTION_IFRAME to true
Disconnect/reconnect connection
Secondary user connection
Connection on multisite

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

jetpackbot · 2020-09-10T20:39:41Z

Scheduled Jetpack release: October 6, 2020.
Scheduled code freeze: September 29, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17136

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against e6540b8

jeherve

This tested well for me. 🚢

anomiex

Asking a few questions to try to understand this better.

packages/connection/src/class-owner.php

anomiex · 2020-09-17T16:51:04Z

packages/connection/src/class-owner.php

+						$.ajax( {
+							type        : "POST",
+							url         : "<?php echo esc_url( get_rest_url() . 'jetpack/v4/connection/owner' ); ?>",
+							data        : formData,
+							headers     : {
+								'X-WP-Nonce': "<?php echo esc_js( wp_create_nonce( 'wp_rest' ) ); ?>",
+							},
+							success: function() {
+								results.innerHTML = "<?php esc_html_e( 'Success!', 'jetpack' ); ?>";
+								setTimeout( function() {
+									$( '#jetpack-notice-switch-connection-owner' ).hide( 'slow' );
+								}, 1000 );
+							},
+						} ).done( function() {
+							submitBtn.disabled = false;
+						} );


I don't see any error handling in this ajax call, it looks like if it fails it'll just sit there forever with a disabled button, never informing the user. Am I missing something?

In commit e6540b8, I added error handling to the Ajax call. If the call fails, an error message will be displayed and the button will be enabled.

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

anomiex · 2020-09-17T17:01:06Z

tests/php/_inc/lib/test_class.rest-api-endpoints.php

@@ -960,40 +960,78 @@ public function test_fetch_not_registered_widget_data() {
 	}

 	/**
-	 * Test changing the master user.
+	 * Test changing the master user without the owner parameter.


Do these tests still belong here, or should they be moved along with the implementation of the endpoint they're testing?

I don't think it's currently possible to implement these tests in the Connection package tests. These tests require creating users and saving them in the database. The Connection package tests do not run in a full WordPress environment with a database. Instead, they use the WorDBless package, which provides WordPress core functions without a database.

So, I left the tests the require a WordPress environment in the Jetpack plugin's tests and added a few tests that do not require the WordPress environment to the Connection package.

Hmm. I see test_change_owner_with_permission() is able to have a current user that has the necessary capability to get into set_connection_owner().

Looks like test_change_owner_with_invalid_param() tests the same thing as test_change_owner_with_permission(). It's just calling into the code differently.

It looks like test_change_owner_with_bad_user() is trying to change to a nonexistent ID, which seems like it should work the same there too.

test_change_owner_to_same_user() is trying to change to the same user, which again should work the same there.

The last test, test_change_owner_to_valid_user(), does seem to need an actual second user, which wordbless doesn't seem to support yet other than by low-level hacking up of the DB queries.

- Remove the `method_exists` check for Tracking:::record_user_event. - Add error handling for the Ajax request. - Use Tracking::class insstead of the full class name. - Remove a duplicate unit test and update the unit test comments.

anomiex · 2020-09-22T14:39:08Z

tests/php/_inc/lib/test_class.rest-api-endpoints.php

@@ -960,40 +960,78 @@ public function test_fetch_not_registered_widget_data() {
 	}

 	/**
-	 * Test changing the master user.
+	 * Test changing the master user without the owner parameter.


Hmm. I see test_change_owner_with_permission() is able to have a current user that has the necessary capability to get into set_connection_owner().

Looks like test_change_owner_with_invalid_param() tests the same thing as test_change_owner_with_permission(). It's just calling into the code differently.

It looks like test_change_owner_with_bad_user() is trying to change to a nonexistent ID, which seems like it should work the same there too.

test_change_owner_to_same_user() is trying to change to the same user, which again should work the same there.

The last test, test_change_owner_to_valid_user(), does seem to need an actual second user, which wordbless doesn't seem to support yet other than by low-level hacking up of the DB queries.

anomiex · 2020-09-22T15:07:54Z

packages/connection/src/class-owner.php

+							submitBtn.disabled = false;
+						} ).fail( function() {
+							results.classList.add( 'error-message' );
+							results.innerHTML = "<?php esc_html_e( 'Something went wrong. Please try again.', 'jetpack' ); ?>";


Honest question: Is this the preferred behavior for error reporting? My inclination would be to try to relay a more specific message from the endpoint. But maybe that would be more confusing for a typical user?

stale · 2020-12-25T13:03:46Z

This PR has been marked as stale. This happened because:

It has been inactive in the past 3 months.
It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

jeherve · 2022-03-02T08:47:05Z

I'll close this PR for now because of the lack of activity on this. We can always reopen in the future if needed, but it will need a rebase, so it may be easier to start a new PR at this point.

kbrown9 added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Pri] Normal [Focus] Jetpack DNA [Package] Connection labels Sep 10, 2020

kbrown9 added this to the 9.0 milestone Sep 10, 2020

kbrown9 self-assigned this Sep 10, 2020

matticbot added the [Status] Needs Tracks Review Added/removed/modified a tracks event. label Sep 10, 2020

test-case-reminder bot reviewed Sep 10, 2020

View reviewed changes

github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Sep 10, 2020

kbrown9 added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Sep 11, 2020

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 17, 2020

jeherve previously approved these changes Sep 17, 2020

View reviewed changes

anomiex reviewed Sep 17, 2020

View reviewed changes

kbrown9 added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 18, 2020

kbrown9 dismissed jeherve’s stale review via e6540b8 September 21, 2020 19:25

kbrown9 added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 21, 2020

anomiex reviewed Sep 22, 2020

View reviewed changes

anomiex added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 22, 2020

kbrown9 removed this from the 9.0 milestone Sep 25, 2020

stale bot added the [Status] Stale label Dec 25, 2020

jeherve closed this Mar 2, 2022

github-actions bot removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Mar 2, 2022

fgiannar mentioned this pull request Feb 2, 2023

Connection: move the notice for deleting connection owner #28516

Merged

3 tasks

anomiex removed the [Status] Stale label Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection: move the connection owner deletion notice to the Connection package #17136

Connection: move the connection owner deletion notice to the Connection package #17136

kbrown9 commented Sep 10, 2020 •

edited

Loading

test-case-reminder bot left a comment

jetpackbot commented Sep 10, 2020 •

edited

Loading

jeherve left a comment

anomiex left a comment

anomiex Sep 17, 2020

kbrown9 Sep 21, 2020

anomiex Sep 17, 2020

kbrown9 Sep 21, 2020

anomiex Sep 22, 2020

anomiex Sep 22, 2020

anomiex Sep 22, 2020

stale bot commented Dec 25, 2020

jeherve commented Mar 2, 2022

Connection: move the connection owner deletion notice to the Connection package #17136

Connection: move the connection owner deletion notice to the Connection package #17136

Conversation

kbrown9 commented Sep 10, 2020 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

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

Testing instructions:

Attempt to delete the connection owner with a logged-out admin.

Attempt to delete the connection owner with a logged-in admin.

Verify that the connection/owner endpoint checks user permissions properly.

Proposed changelog entry for your changes:

test-case-reminder bot left a comment

Choose a reason for hiding this comment

Connection

jetpackbot commented Sep 10, 2020 • edited Loading

jeherve left a comment

Choose a reason for hiding this comment

anomiex left a comment

Choose a reason for hiding this comment

anomiex Sep 17, 2020

Choose a reason for hiding this comment

kbrown9 Sep 21, 2020

Choose a reason for hiding this comment

anomiex Sep 17, 2020

Choose a reason for hiding this comment

kbrown9 Sep 21, 2020

Choose a reason for hiding this comment

anomiex Sep 22, 2020

Choose a reason for hiding this comment

anomiex Sep 22, 2020

Choose a reason for hiding this comment

anomiex Sep 22, 2020

Choose a reason for hiding this comment

stale bot commented Dec 25, 2020

jeherve commented Mar 2, 2022

kbrown9 commented Sep 10, 2020 •

edited

Loading

Verify that the `connection/owner` endpoint checks user permissions properly.

jetpackbot commented Sep 10, 2020 •

edited

Loading