Skip to content

Conversation

@leogermani
Copy link
Contributor

@leogermani leogermani commented Mar 23, 2021

Stop using is_active to determine what XMLRPC endpoints should be available for the requests.

This is part of the cleanup to slowly deprecate the is_active method and open the path to the user-less project.

Changes proposed in this Pull Request:

  • Changes the criteria used to register XML-RPC methods

Jetpack product discussion

1191179647901802-as-1200087466875449

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

No

Testing instructions:

In order to test this PR, there is this handy script -> 28d59-pb/#php

It will make 3 request to system.listMethods in your site. Save this script in your sandbox public_html folder and edit it to add your test site ID.

  • One with an unauthenticated request
  • One authenticated with a blog token
  • One authenticated with a user token

So the test flow is:

  • Disconnect your site
  • Run the script on your sandbox
  • Connect only at a site level
  • Run the script
  • Connect a user
  • Run the script

This will give you 3 outputs. Each output with the three types of request.

Look at the table here -> p9dueE-1Wm-p2

Make sure the exposed methods match the table. Each of the script runs will represent one column in the table.

Also, please read the code, follow the logic and check the criteria are not altered.

Finally, make sure the Jetpack Debugger passes all the tests and test the connection flows.

NOTE For the Debugger tests to pass you'll need #19301 and D59309-code (both deployed/merged already)

Copy link

@test-case-reminder test-case-reminder bot left a comment

Choose a reason for hiding this comment

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

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

Verify that the changes are compatible with the plugins that use the connection package.

  • WooCommerce Payments
  • Jetpack Boost
  • Previous versions of Jetpack

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.

@github-actions github-actions bot added [Package] Connection [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Mar 23, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 23, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: April 6, 2021.
  • Scheduled code freeze: March 29, 2021.

@github-actions github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 23, 2021
Copy link
Contributor

@fgiannar fgiannar left a comment

Choose a reason for hiding this comment

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

Hey Leo!

Great work here! Thanks for providing a handy script to test with!

Code looks good, logic is solid I believe.
I've found a typo (see inline comments).

I'm having a bit of trouble testing with the provided script. I seem to get NULL for unauthorized requests to a non connected site.
I've tried it via Postman though and works as expected.

@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Mar 24, 2021
@leogermani
Copy link
Contributor Author

@fgiannar Thanks for spotting the typo.

I've fixed it and also updating the broken tests. As soon as #18952 is merged, I'll move these tests into the Connection package, where they belong.

As for the testing script, I understand you managed to test it with Postman, right?

@leogermani leogermani added [Status] Needs Review This PR is ready for review. 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 Mar 24, 2021
@github-actions github-actions bot added the Actions GitHub actions used to automate some of the work around releases and repository management label Mar 24, 2021
@leogermani
Copy link
Contributor Author

Updated the testing script with one that should work for all sites: 28d59-pb/#php

(also updated in the PR description)

Copy link
Contributor

@fgiannar fgiannar left a comment

Choose a reason for hiding this comment

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

Looks great, Leo! I tested using the provided instructions and everything works as expected! 💯

@leogermani leogermani merged commit 51cc4df into master Mar 26, 2021
@leogermani leogermani deleted the update/xmlrpc_methods_visibility branch March 26, 2021 16:41
@matticbot matticbot removed the [Status] Needs Review This PR is ready for review. label Mar 26, 2021
@github-actions github-actions bot added this to the jetpack/9.6 milestone Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Actions GitHub actions used to automate some of the work around releases and repository management [Package] Connection [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Status] Needs Package Release This PR made changes to a package. Let's update that package now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants