-
Notifications
You must be signed in to change notification settings - Fork 846
Editor: add blogging prompts to posts #26680
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
|
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:
|
|
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run |
roo2
left a comment
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.
hmm it seems strange to me that this is implemented as an experimental block. Maybe a standalone package might make more sense? I'm not sure, I wonder what the jetpack crew will say.
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
roo2
left a comment
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.
One thing I noticed, if I were to start a post, then save it as a draft and then come back to it, I'd no longer see the writing prompt. I don't think it's worth handling that case specially but just something I noticed. Appart from that I just had very minor nit picks.
projects/plugins/jetpack/extensions/blocks/writing-prompts/editor.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/editor.js
Outdated
Show resolved
Hide resolved
roo2
left a comment
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.
Another thing I noticed with this approach is that when I hit the backspace key when the post is empty, the prompt is removed.
Screen.Recording.2022-10-25.at.11.20.18.am.mov
projects/plugins/jetpack/extensions/blocks/writing-prompts/editor.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/writing-prompts/writing-prompts.php
Outdated
Show resolved
Hide resolved
|
Note that this PR should now be compatible with Simple Sites, as well. I've added testing instructions in the PR description. There is a small change required to wpcom to get things working, which you can see in D91157-code. This change will need to be merged when this feature is released to wpcom. |
roo2
left a comment
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.
Tested on simple sites with D91157-code andmy jurassic.tube site with D88262-code. LGTM!
When we merge this we will be seeing the prompt, I think we need to make sure the translation issue is solved at least before we merge.
|
I've added a number of changes today: Breaks out the blogging site checks so they can be used individually
Adds the Change the Moved the extension to |
|
Thanks for the team reviews! I think this is ready for a look by Jetpack Crew. |
bindlegirl
left a comment
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.
Works as advertised.
|
Note that there's a companion PR at #27483 to move this editor extension to experimental. We'd like to make it available to WP.com hosted sites (Simple and Atomic) for next week's release. |
* Blogging Prompts: Move to experimental blocks * Simplifies date generation
| * @return void | ||
| */ | ||
| function jetpack_setup_blogging_prompt_response( $post_id ) { | ||
| // phpcs:ignore WordPress.Security.NonceVerification.Recommended |
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.
Why is nonce verification being ignored here? We try to include a comment explaining it.
See p9dueE-4z8-p2 for more info.
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.
We are generating the link to be clicked from a WP.com notification, so it's possible the nonce would change before the user clicks the link. I'll add a comment.
Not ideal, as we are "setting a field," in the form of a tag, post_meta with the prompt id, and a prefilled pull quote block, though no data is saved until the user publishes. Please let me know if you have other thoughts or a better solution!
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.
As long as it's well-explained in the comment, I think that's a good start. 👍
Do I have this right? There's a link in the notification, and clicking the link winds up calling this function. You can't put a nonce in the link, because it might be a while in between when the link is generated and when it's clicked. The "attack" that a nonce would prevent would be some third party tricking the blog admin's browser into following the link, maybe repeatedly to flood the site with these posts. Ideally the comment should address why that potential attack isn't something to actually worry about.
| */ | ||
| function jetpack_get_daily_blogging_prompts( $time = 0 ) { | ||
| // Default to the time in the site's timezone. | ||
| $timestamp = $time ? $time : current_time( 'timestamp' ); // phpcs:ignore WordPress.DateTime.CurrentTimeTimestamp.Requested |
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.
Why are you ignoring this sniff? Wouldn't it be better to use time() and wp_date() instead?
As it is, you have $time documented as a Unix timestamp on line 57, but then on line 64 you're treating it as a deprecated "WordPress timestamp+offset" value.
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.
I didn't realize that wp_date will default to the site's timezone (which is what's desired here, to respect the site settings). Will refactor.
| } | ||
|
|
||
| $blog_id = \Jetpack_Options::get_option( 'id' ); | ||
| $path = '/sites/' . $blog_id . '/blogging-prompts?from=' . $yesterday . '&number=10&_locale=' . $locale; |
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.
While all this is probably safe, it might be better to wrap the variables with rawurlencode() to guarantee it.
| return null; | ||
| } | ||
|
|
||
| $body = json_decode( wp_remote_retrieve_body( $response ) ); |
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.
Should there be error checking here in case the json_decode() returns null?
| * | ||
| * @return string | ||
| */ | ||
| function jetpack_get_mag16_locale() { |
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 function seems a bit unfortunate to have in Jetpack rather than having the server side do the fix-up. If you were to add fr_BE, for example, you'd have to wait for the monthly Jetpack release (and for people to update to it) for standalone sites to get it. While if this were done on the server side instead, they'd start getting it as soon as you updated it there.
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.
That's a good point. The blogging-prompts endpoint is relying on a global _locale, but I'll see about customizing it just for this endpoint and pulling out this function. FYI, some related conversation: D91251-code#1932120
| if ( should_load_blogging_prompts() ) { | ||
| $daily_prompts = wp_json_encode( jetpack_get_daily_blogging_prompts() ); | ||
|
|
||
| // See p7H4VZ-2cf-p2 for why prompt data is escaped this way. |
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.
As the post itself notes, json_encode() is safe to use here with the right flags, and the problem back in February 2019 was that WordPress still supported PHP 5.2 which didn't have those flags. We've been at a minimum of 5.6 now for a long time, so just use wp_json_encode( ..., JSON_HEX_TAG ).
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.
Thanks for clarifying what's currently considered safe. I'll refactor.
| function should_load_blogging_prompts() { | ||
| return jetpack_has_write_intent() || | ||
| jetpack_has_posts_page() || | ||
| // phpcs:ignore WordPress.Security.NonceVerification.Recommended |
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.
As above, why can we ignore nonce verification here?
|
Hmm, my review came in a few minutes too late. :( Hopefully those comments get addressed in a followup. |
|
@anomiex Thanks so much for the review and feedback! I'll answer comments and create a follow-up PR to address issues later today. |
|
Follow-up PR here: #27557 |

Changes proposed in this Pull Request:
Other information:
Jetpack product discussion
pe7F0s-gI-p2
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Jetpack Site
define( 'JETPACK_BETA_BLOCKS', true )(or Settings > Jetpack Constants if you're using jurassic.ninja)site_intentoption towriteanswer_promptquery param with the prompt id for today as the value (e.g./wp-admin/post-new.php?answer_prompt=2129), and you should see a writing prompt as a pullquote block, the tagdailypromptadded to the post, and the_jetpack_blogging_prompt_keypost meta set to the prompt id.Simple Site
bin/jetpack-downloader test jetpack try/paragraph-block-writing-prompts