-
Notifications
You must be signed in to change notification settings - Fork 798
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
Connection: properly add GET-parameters for the fetchAuthorizationUrl
API call
#21750
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. 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. Jetpack plugin:
Backup plugin:
|
Don't you think it's time to deprecate the
This would ensure the iframe flow will never be returned in any case. |
Hi @leogermani.
Deprecate - yes, good idea, I'll do that. |
if you deprecate it and force it to false, no need to enforce the |
@leogermani, If I'm missing something, please let me know. |
a965f5c
to
e740fb5
Compare
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.
Looks good, after patching I no longer saw the jetpack.authorize_iframe
Great news! One last step: head over to your WordPress.com diff, D70131-code, and commit it. Thank you! |
r235541-wpcom |
# By Jeremy Herve (5) and others # Via GitHub * master: (26 commits) Colors: update Primary green reference to match latest brand book (#21741) JS Connection: Moves registerSite logic to the store (#21732) Search: Add essential scaffolding for package (#21814) Search: avoid registering the widget when the module is not active (#21588) Add Video Tracks Support to VideoPress Block (#21578) Add deprecated to VideoPress block (#21802) Admin Menu: Make API tests compatible with WPCOM (#21850) External Media: Short-circuit requests earlier in the stack (#21854) Add Busy State to License Activation Flow Button (#21861) Fixed an issue with screen resolutions of under 783px that caused the content to not be properly displayed when the nav-unification is expanded on wp-admin. (#21869) E2E tests: migrate from Jest to Playwright runner (#21848) Update reCAPTCHA constants to match Google's Verbage (#12289) JITM: Sideload Jetpack Boost functionality (#21860) Connection: properly add GET-parameters for the `fetchAuthorizationUrl` API call (#21750) License Flow: Assorted Styling Improvements (#21859) JITM: Sideload Jetpack Backup (#21719) Widget Visibility: Apply widget filtering to customizer preview (#21505) jetpack: Avoid generating unused JS for static-site-generator assets (#21789) Nav Unification: Support absolute URLs in upsell nudges (#21821) RePublicize: Enable the block editor UI by default (#21855) ... # Conflicts: # projects/plugins/boost/tests/e2e/lib/env/prerequisites.js # projects/plugins/boost/tests/e2e/lib/setupTests.js
Changes proposed in this Pull Request:
Bug:
fetchAuthorizationUrl
API method (RNA API package) did not include theno_iframe
parameter in "plain permalinks" mode.Therefore, site-connected users are unable to connect their user accounts to WP.com if the site has "plain permalinks" enabled.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
First, reproduce the issue without checking out the branch.
jetpack.authorize_iframe
page with minimal UI.plugins/jetpack
if needed, and try to connect the user one more time. Confirm that everything went well.