Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deleting the connection owner? Think again (or transfer ownership.) #13322

Merged
merged 15 commits into from
Aug 26, 2019
Merged

Deleting the connection owner? Think again (or transfer ownership.) #13322

merged 15 commits into from
Aug 26, 2019

Conversation

dereksmart
Copy link
Member

Changes proposed in this Pull Request:

  • Adds some new helper methods for getting info about the user connections.
  • Modifies the permission callback for the /connection/owner endpoint.
  • Adds a whole notice UI and logic during the delete user process with information and actions to take if the connection owner is being deleted.

What this PR is intentionally not doing, and will be done in other PRs:

  • Add tracking
  • Better copy/design

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • I wouldn't really call it a feature, more a preventative enhancement.

Testing instructions:

There's a lot.

Test changing connection owner to secondary Admin

  1. Set up a new site. Connect Jetpack to whatever user - let's call it User A.
  2. Add a new admin user to the site - let'a call it User B.
  3. In a separate (incognito?) window, log into the site as User B.
  4. Go to the Jetpack admin and connect User B to wordpress.com.
  5. Still logged in as User B, go to the users page.
  6. Click to delete User A. (Remember, User A should be the connection owner). You should be taken to a confirmation page to delete, where you should see this new notice:
    image
  7. Since you are logged in as a secondary connected user, you should see a dropdown with your name in it.
  8. Click the button to switch the connection to you!
  9. The notice should disappear.

Verify it worked
After you did the above, you should be able to verify it "worked" by doing the following things:

  • In CLI: wp jetpack options get master_user - should be User B's ID returned.
  • In wordpress.com NA: Visit the blog's report card. You should see User B's wpcom connected user as the new owner.
  • You should also see the connection owner has been updated in the blog's audit trail in wpcom.

Test when there are no other users connected
Since we're only able to transfer ownership to another connected user, we needed a case to handle if there were none! So, you should expect to see a lot of text and a button to connect if this is the case when you're trying to delete the connection owner. For this one, please verify:

  • That connection link works, and is done in a new tab.
  • The support links work and points to the right places.
    image

General things to check/verify

  • You should never see an option to transfer ownership to a non-admin
  • You should never see an option to transfer ownership to another user that is not connected currently.

Proposed changelog entry for your changes:

*General: Warn users that deleting the connection owner is going to break things.

@dereksmart dereksmart added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it General [Status] Needs Review This PR is ready for review. labels Aug 25, 2019
@dereksmart dereksmart added this to the 7.7 milestone Aug 25, 2019
@dereksmart dereksmart requested review from mdawaffe, zinigor, jeherve and a team August 25, 2019 21:09
@dereksmart dereksmart force-pushed the add/delete-connection-owner-notice branch from 528adf4 to e6e3f84 Compare August 25, 2019 21:10
dereksmart
dereksmart commented Aug 25, 2019
View reviewed changes
packages/connection/src/Manager.php
* @param string The capability of the user
* @return array Array of WP_User objects if found.
*/
public function get_connected_users( $capability = 'any' ) {
Copy link
Member Author

@dereksmart dereksmart Aug 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Happy to break these new methods out into their own PR if it makes it easier.

@jetpackbot
Copy link
Collaborator

jetpackbot commented Aug 25, 2019 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against c19c357

@dereksmart dereksmart added the [Status] Needs Design Review Design has been added. Needs a review! label Aug 25, 2019
dereksmart
dereksmart commented Aug 25, 2019
View reviewed changes
tests/php/_inc/lib/test_class.rest-api-endpoints.php
@@ -933,7 +933,7 @@ public function test_change_owner() {

// Create a user and set it up as current.
$user = $this->create_and_get_user();
$user->add_cap( 'jetpack_connect_user' );
$user->add_cap( 'jetpack_disconnect' );
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the wrong user cap to allow switching connection owner. Any user can connect_user, but not any user can own the connection.

@dereksmart dereksmart force-pushed the add/delete-connection-owner-notice branch from a573984 to 79da062 Compare August 25, 2019 22:31
dereksmart
dereksmart commented Aug 25, 2019
View reviewed changes
tests/php/_inc/lib/test_class.rest-api-endpoints.php
wp_set_current_user( $user->ID );

// Mock site already registered
Jetpack_Options::update_option( 'user_tokens', array( $user->ID => "honey.badger.$user->ID" ) );

// Attempt change, fail because not master user
$response = $this->create_and_get_request( 'connection/owner', array( 'owner' => 999 ), 'POST' );
Copy link
Member Author

Choose a reason for hiding this comment

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

The method now allows non-master users to use this endpoint.

simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
simison
simison reviewed Aug 26, 2019
View reviewed changes
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

I really like this idea so I went ahead and left a few minor comments as a review, hope this helps!

Not necessarily in this PR but might make sense to hide the notice when user is deleting themself and WP is stopping them:

Screenshot 2019-08-26 at 09 54 43

Otherwise the notice might confuse.

jeherve
jeherve requested changes Aug 26, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I like the idea; I think it'd be a great addition to the Users screen. 👍

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Aug 26, 2019
@dereksmart dereksmart force-pushed the add/delete-connection-owner-notice branch from dc993bb to d265ddb Compare August 26, 2019 14:35
jeherve
jeherve previously requested changes Aug 26, 2019
View reviewed changes
@dereksmart
Copy link
Member Author

@jeherve ready again.

jeherve
jeherve requested changes Aug 26, 2019
View reviewed changes
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Aug 26, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Aug 26, 2019
jeherve
jeherve previously approved these changes Aug 26, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This should be good to go like this! 👍

@jeherve jeherve force-pushed the add/delete-connection-owner-notice branch from e4381bf to c19c357 Compare August 26, 2019 17:16
@jeherve jeherve merged commit 0014f0e into master Aug 26, 2019
@jeherve jeherve deleted the add/delete-connection-owner-notice branch August 26, 2019 17:26
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 26, 2019
jeherve added a commit that referenced this pull request Aug 27, 2019
@jeherve
Changelog: add #13322
aa8b3e4
jeherve added a commit that referenced this pull request Aug 27, 2019
@jeherve
7.7 changelog: initial set of changes (#13190)
190301b 
* 7.7 changelog: initial set of changes

* Changelog: add #13154

* Changelog: add #13134

* Changelog: add #12699 and many others

* Changelog: add #13127

* Changelog: add #13167

* Changelog: add #13225

* Changelog: add #13179

* Changelog: add #13173

* Changelog: add #13232

* Changelog: add #13137

* Changelog: add #13172

* Changelog: add #13182

* Changelog: add #13200

* Changelog: add #13244

* Changelog: add #13267

* Changelog: add #13204

* changelog: add #13205

* Changelog: add #13183

* Changelog: add #13278

* Changelog: add #13162

* Changelog: add #13268

* Changelog: add #13286

* Changelog: add #13273

* Changelog: add #12474

* Changelog: add #13085

* Changelog: add #13266

* Changelog: add #13306

* Changelog: add #13311

* Changelog: add #13302

* Changelog: add #13196

* Changelog: add #12733

* Changelog: add #13261

* Changelog: add #13322

* Changelog: add #13333

* Changelog: add #13335
This was referenced Oct 22, 2019
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simison simison simison left review comments

@jeherve jeherve jeherve approved these changes

@mdawaffe mdawaffe Awaiting requested review from mdawaffe

@zinigor zinigor Awaiting requested review from zinigor

Assignees

@dereksmart dereksmart

Labels
General [Status] Needs Design Review Design has been added. Needs a review! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
7.7
Development

Successfully merging this pull request may close these issues.
5 participants
@dereksmart @jetpackbot @simison @jeherve @matticbot