-
Notifications
You must be signed in to change notification settings - Fork 847
Donations: Save fallback for non-frontend contexts #17381
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
|
Scheduled Jetpack release: November 10, 2020. E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17381 Thank you for the great PR description! When this PR is ready for review, please apply the |
|
Is there anything to look at here at this point? (It's listed in the "Needs Review" column but it doesn't look like there's anything testing-wise here right now.) |
Nope, that's why I added the "[Status] In Progress" label 🙂. I usually add all the PRs I work on to our board as per our Team Board guidelines (PaYKcK-w0-p2).
|
64f402f to
14d3f50
Compare
|
Caution: This PR has changes that must be merged to WordPress.com |
| array( | ||
| 'render_callback' => __NAMESPACE__ . '\render_block', | ||
| 'plan_check' => true, | ||
| 'attributes' => array( |
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.
Block attributes need to be defined server-side because default values of attributes defined client-side are not available in render_callback.
class-jetpack-currencies.php
Outdated
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.
For folks reviewing, this has been moved from Jetpack_Simple_Payments: https://github.com/Automattic/jetpack/pull/17381/files#diff-b73125af4e0fe93b3939f29ff9c7e6fc33d17e564a32fb7ea8d082d1009e3401
class-jetpack-currencies.php
Outdated
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.
This one comes from Jetpack_Simple_Payments too: https://github.com/Automattic/jetpack/pull/17381/files#diff-b73125af4e0fe93b3939f29ff9c7e6fc33d17e564a32fb7ea8d082d1009e3401
|
@krymson24 I double checked the instructions, followed them once more and they are working for me. It seems your site is missing a paid plan. What do you get when you run |
208317e to
fcff2c5
Compare
I ran into this problem too @krymson24. I found I had to purchase a more specific Jetpack Complete plan (I think I'd had Jetpack Security initially) for the Donations block to appear. Double check which plan you're on? @mmtr I got partway through testing this yesterday but couldn't get the Jetpack patch to fully apply on my sandbox. I ran I then ran But the output still looks broken for email, notifications, and Reader: I've confirmed I'm sandboxing the API and the test site(s), so I think it's the patch not applying cleanly. Any ideas? |
@sixhours did you publish the post before applying the patch? Seems you applied the patch correctly, but maybe you're testing with a post that was already published? The fallback is only used when saving posts from now on. Contexts where the fallback output is going to be rendered are usually timely streamers, so we don't need to worry about using the fallback in existing posts. |
Ahhhh, I bet that's it. I'll give that a try. :D |
I tried a Jetpack Security and Complete plan, and available is still Confirming I have Jetpack Complete: See below for screenshot that shows no Donations block as an option. I thought maybe that I hard coded the availability as 😢 |
fcff2c5 to
533ea29
Compare
Co-authored-by: Jeremy Herve <[email protected]>
459a9f5 to
4030797
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.
This tests well for me in Jetpack.
| /** | ||
| * Class for testing the Jetpack_Currencies class. | ||
| */ | ||
| class WP_Test_Jetpack_Currencies extends WP_UnitTestCase { |
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.
<3
|
Internal reference, part 1: r215962-wpcom |
|
Part 2: r216557-wpcom |
I don't see anything in #17381 that discusses this behavior. It's possible it was an accidental bug that wasn't noticed because of the undefined array key access producing null which produces the empty string, and that the test was a copy-paste of the previous test that didn't notice `false` being passed. I see no other uses of `Jetpack_Currencies` with `$symbol = false` in the monorepo or opengrok or wpdirectory, so this should be safe to fix.











Fixes #17358
Fixes 241-gh-dotcom-manage
Fixes #17359
Changes proposed in this Pull Request:
This PR continues the work initiated in #17256 and adapts the Donations block so the
savefunction is used as a proper fallback for non-frontend contexts whilerender_callbackis used for augmenting the content dynamically for frontend contexts.The block saves now a fallback output into the post content. That fallback includes a textual call-to-action linking to the post so folks can donate from the website.
The block also sets a render callback that augments the UI with working donate buttons.
Jetpack product discussion
N/A.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Web
master./etc/hostsfile and pointpublic-api.wordpress.comandsubscribe.wordpress.comto your WP.com sandbox IP.JETPACK__SANDBOX_DOMAIN./wp-content/mu-plugins/0-sandbox.phpin your WP.com sandbox:update/donations-fallback).API
GETrequest to/sites/:site/posts/:post-id(where:post-idis the ID of a post that contains a Donations block).Email, notifications, and reader (only testable in WP.com)
php ./bin/subscriptions/send.php <BLOG_ID> post <POST_ID> <YOUR_EMAIL>.Proposed changelog entry for your changes:
Donations: Save fallback for non-frontend contexts