Skip to content

Conversation

@zinigor
Copy link
Contributor

@zinigor zinigor commented Jun 14, 2019

Changes proposed in this Pull Request:

[x] Adds the register method to the Connection package.
[x] Hooks the register verification method for registration verification.
[x] Adds multisite registration capability.
[x] Makes sure the methods are moved with only the needed dependencies.
[x] Makes sure the example plugin is able to connect using only the dependent packages and minimal boilerplate code.

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

  • This PR aims to make the Connection package usable without the Jetpack plugin itself.

Testing instructions:

  • Use the example client plugin on a clean WordPress site. It's bare bones, it has one administration page where it displays some debugging information, as well as lets you press a button to run the registration process. You should see that the site properly generates secrets, but can't proceed with user registration because handlers are not yet in place.

Proposed changelog entry for your changes:

  • N/A

@zinigor zinigor added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Focus] Jetpack DNA labels Jun 14, 2019
@zinigor zinigor added this to the 7.5 milestone Jun 14, 2019
@zinigor zinigor requested review from a team, gititon and tyxla June 14, 2019 23:22
@jetpackbot
Copy link
Collaborator

jetpackbot commented Jun 14, 2019

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 c86a4b8

$args['body'] = $this->apply_activation_source_to_args( $args['body'] );

// TODO: fix URLs for bad hosts
$response = \Jetpack_Client::_wp_remote_request(
Copy link
Member

Choose a reason for hiding this comment

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

What about this one? Are we planning to move it to a package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm hoping to merge Michael's #12717 into this PR once it gets merged.

jetpack.php Outdated
// @todo: Abstract out the admin functions, and only include them if is_admin()
require_once( JETPACK__PLUGIN_DIR . 'class.jetpack.php' );
require_once( JETPACK__PLUGIN_DIR . 'class.jetpack-network.php' );
require_once( JETPACK__PLUGIN_DIR . 'class.jetpack-client.php' );
Copy link
Contributor

Choose a reason for hiding this comment

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

This line disappearing is my favourite thing about this PR

@mdawaffe
Copy link
Member

Example client plugin worked for me, and what I understand about the changes make sense :)

@mdawaffe
Copy link
Member

491016a3f344e92d3f25bb28046d4dc52a51db70 moved the $network_wpcom_blog_id = Jetpack_Options::get_option( 'id' ); to after the switch_to_blog(), so we're sending the subsite's wpcom_blog_id (which doesn't exist yet) in the subsite registration request. We should be sending the main site's wpcom_blog_id.

3ecdad75cf619f60dbb8c64294f7e133e7fb046f provides a sort of hacky fix (by unswitching/reswitching the blog).

@zinigor zinigor force-pushed the add/user-authentication branch from 3ecdad7 to 117a651 Compare July 19, 2019 13:12
@zinigor zinigor requested a review from a team as a code owner July 19, 2019 16:23
@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "record_user_event" function.

cc: @pesieminski

@tyxla
Copy link
Member

tyxla commented Aug 8, 2019

I think this is ready for another review!

More info on how to test it in p1HpG7-7lI-p2 and its comments.

@tyxla tyxla requested review from a team and gravityrail August 8, 2019 11:42
@tyxla tyxla added the Connect Flow Connection banners, buttons, ... label Aug 8, 2019
lezama
lezama previously approved these changes Aug 8, 2019
Copy link
Contributor

@lezama lezama left a comment

Choose a reason for hiding this comment

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

Things look to still connect and work, let's merge early and keep iterating and testing around this through the entire current cycle.

Great work @zinigor and @tyxla!

Copy link
Contributor

@lezama lezama left a comment

Choose a reason for hiding this comment

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

re approving after my last commit :)

@lezama lezama merged commit 0118cad into master Aug 8, 2019
@lezama lezama deleted the add/user-authentication branch August 8, 2019 17:19
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review This PR is ready for review. labels Aug 8, 2019
jeherve added a commit that referenced this pull request Aug 23, 2019
jeherve added a commit that referenced this pull request Aug 27, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Connect Flow Connection banners, buttons, ... [Focus] Jetpack DNA [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants