-
Notifications
You must be signed in to change notification settings - Fork 73
Use Jetpack Connection to authenticate #131
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.
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
Conversation
composer.json
Outdated
| "repositories": [ | ||
| { | ||
| "type": "path", | ||
| "url": "/Users/daniel/Projects/jetpack/packages/*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously this won't be needed when automattic/jetpack-* packages are published. There's a way to tell composer to resolve a package from a GitHub repo, but it doesn't work with monorepos like jetpack/jetpack.
| 'headers' => $headers, | ||
| 'blog_id' => Jetpack_Options::get_option( 'id' ), | ||
| 'user_id' => JETPACK_MASTER_USER, | ||
| 'user_id' => true, // Automattic\Jetpack\Constants::get_constant( 'JETPACK_MASTER_USER' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zinigor How is the jetpack-constants package supposed to be used? I would expect that the package itself would define the JETPACK_MASTER_USER constant (and other constants), but it doesn't (yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I see that the Connection\Manager class has the JETPACK_MASTER_USER constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would still be a problem if we wanted to use this method for example:
https://github.com/Automattic/jetpack/blob/ca4cdd56658a0028374d180c2fb8d3eb695e5a28/packages/connection/src/Client.php#L388
190be50 to
b5a0342
Compare
…symlink and Docker conflicts.
…symlink and Docker conflicts.
…ayments into try/jetpack-dna
|
@DanReyLop wrote:
Is this still true? Does @zinigor know? cc @marcinbot - let's clarify what sort of URL or ajax call we'd like for connecting / disconnecting |
Honestly I haven't tried, but by looking at the |
# Conflicts: # includes/wc-payment-api/class-wc-payments-api-client.php
|
Sorry, I was AFK, couldn't reply earlier. Yes, this is still true AFAIK: currently the connection package expects the plugin that uses it to do most of the legwork. Plus there's no corresponding connection flow in WordPress.com yet. |
@zinigor Could you clarify what is the expected legwork? Is there maybe a P2 that summarizes it? Which parts of Jetpack would we have to implement ourselves? We'd like to have a flow similar if not the same as Jetpack (subject to woo design). If Connect was able to provide this, we'd be happy for now. |
|
@marcinbot the main thing the plugin currently needs to do is:
This all you can see in the last three commits to the client-example repo: https://github.com/Automattic/client-example/commits/master Following these steps will make sure that your site has a blog token and a client ID, everything else is still not ready - user authentication part where the user logs in to WordPress.com and accepts the ToS is the biggest chunk of what still needs to be done. |
Fixes #111
Quick & dirty
trybranch, let's not merge it by accident :)Setup:
wp-content/pluginsfolder and delete thejetpackfolder. You can also just move it to someplace else.jetpackfolder there), update it to thetry/connection-package-fixbranch.composer.jsonso it points to the path where you clonedjetpack.composer install, it will say that it created a bunch of symlinks to thejetpack/packages/[...]folders.npm run watch.Paymentssection on WP-Admin, you should see a list of transactions correctly fetched all the way from WPCOM :)See Automattic/jetpack@ca4cdd5 for the changes I had to make in
jetpackjust to make it work. I based that commit off the Automattic/jetpack#12699 PR.This just works if there's already a Jetpack token in the database, so it will only work if you had the full fat Jetpack plugin correctly connected and you deactivated it as said in the instructions.
Sometimes, the Connection package would just work if
jetpackthe plugin was deactivated but still inwp-content/plugins, because the code would still be there. That's why I recommend you completely remove it, to eliminate false positives.Communication from WPCOM to the site doesn't work, so forget about managing your WCPay site from Calypso.
Edit:
I based this branch offNow the list transactions code is inadd/link-to-order-in-transactionbecause right now there's no place inmasterto test that a connection works as easily as this: just go to the transactions list, see if it loads a bunch of data, you're done.master, so I rebased this branch.cc/ @zinigor @marcinbot @v18 @allendav @RadoslavGeorgiev