Add REST API endpoint to replace AJAX callback #33

johnwatkins0 · 2019-08-11T22:05:24Z

This is intended to resolve #14 . ~~It depends on #29~~ (Merged)

Description of the Change

REST endpoint

The current version of the plugin uses an AJAX handler to save the metadata that determines whether a post will be tweeted and whether the default autotweet content will be overridden.

This update removes the AJAX handler in favor of a custom REST endpoint. The endpoint is simple, accepting a POST request with three required fields (post ID, whether autotweet is enabled, and override content). Permission on the REST endpoint is set to check that the current user can edit the post. After the data is successfully saved, the REST endpoint sends back details about the data that was saved.

I also updated the JS file that handles the autotweet feature in the classic editor. It's now hooked up the REST endpoint instead of the AJAX endpoint.

Post meta updates

While setting up the REST endpoint, I did a minor refactor of some details around how post meta is handled. There was already a get_autotweet_meta function that wraps get_post_meta with a prefix on the meta key, and I created equivalents for updating and deleting meta. I also renamed some of the meta-related constants and used them in several places where the values were hardcoded. And I used the meta keys as the required parameters in the REST endpoint for clarity and consistency.

**Note: ** I changed the meta prefix to use autotweet instead of auto-tweet, and changed one of the meta keys to make it more descriptive. This will break backward-compatibility if the current version of this plugin is being used anywhere. Could that be an issue? If so, I will switch it back to what it was.

What this does not do

Although I modified the existing JS file to use the REST endpoint instead of AJAX -- and some ESLinting was applied to the file -- I stopped there. That JS file could easily be refactored to remove the jQuery dependency and use ES6 (via the Webpack config), but I considered that out of scope.

Alternative designs

I considered adding autotweet-related metadata as REST fields, eliminating the need for the REST endpoint. But with that approach, the autotweet data would be included in all REST responses for the supported posts, which probably isn't useful anywhere outside this plugin's narrow set of features.

Verification Process

Confirm that the integration with the classic editor still works as expected. It now runs through the REST API rather than AJAX.

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests passed.

johnwatkins0 · 2019-08-11T22:06:05Z

assets/js/admin-auto_tweet.js

@@ -3,93 +3,87 @@
 *
 * @todo soooo much dependency :facepalm:


Most of this file's changeset is ESLint.

johnwatkins0 · 2019-08-11T22:06:46Z

assets/js/admin-auto_tweet.js

@@ -3,93 +3,87 @@
 *
 * @todo soooo much dependency :facepalm:
 */
-(function ($) {
+( function( $ ) {


Intentionally not removing the jQuery dependency for this PR.

includes/admin/assets.php

johnwatkins0 · 2019-08-11T22:10:07Z

includes/admin/post-meta.php


 /**
 * The meta prefix that all meta related keys should have
 */
-const META_PREFIX = 'tenup-auto-tweet';
+const META_PREFIX = 'tenup-autotweet';


OK that this would break backward-compatibility?

probably not, leave as is

includes/core.php

includes/rest.php

adamsilverstein · 2019-08-15T20:14:27Z

**Note: ** I changed the meta prefix to use autotweet instead of auto-tweet, and changed one of the meta keys to make it more descriptive. This will break backward-compatibility if the current version of this plugin is being used anywhere. Could that be an issue? If so, I will switch it back to what it was.

This might be an issue, this is already in active use on 9to5.

adamsilverstein

Thanks for your work here @johnwatkins0! I left some feedback, let me know if you have any questions. Will also test locally.

adamsilverstein · 2019-08-15T20:27:11Z

includes/admin/assets.php

+	$post_id = intval( get_the_ID() );
+
+	if ( empty( $post_id ) ) {
+		$post_id = isset( $_GET['post'] ) ? absint( $_GET['post'] ) : 0; // phpcs:disable WordPress.Security.NonceVerification.Recommended


Can you document the reason for ignoring the nonce check here? See adamsilverstein/wp-post-meta-revisions#58 (comment) for format

adamsilverstein · 2019-08-15T20:28:04Z

assets/js/admin-auto_tweet.js

-				'post_id': adminTUAT.postId,
-				'text': $tweetText.val(),
-				'type': event.type
+		$.ajax( adminAutotweet.restUrl, {


should we consider changing this to wp.apiFetch?

That would be nice, but I believe for WP 4.9 we'd have to install it as an NPM dependency and bundle it with this file. Which is not a big lift at all.

Or is there another best practice for using WP5 JS packages in WP4?

it is tiny so we could bundle, however wp externalizes already so we can bundle a standalone wp.apiFetch file for 4.9 (either a copy from core or we can build on the fly from the npm package which is what core/gb and site kit do).

once we include the script in the plugin, we register the script if its not already registered, then add api-fetch as a dependency for our script. (we do this with a slew of scripts for Site Kit - https://github.com/google/site-kit-wp/blob/7d4aac9711a1174dc76acfad601c3fcd50a0baca/includes/Core/Assets/Assets.php#L627)... already registerd check: https://github.com/google/site-kit-wp/blob/develop/includes/Core/Assets/Script.php#L59

Nice, thanks for the links. I'll give that a shot.

adamsilverstein · 2019-08-15T20:29:55Z

includes/admin/post-meta.php

@@ -134,25 +60,32 @@ function save_tweet_meta( $post_id ) {
 	// Check check.
 	if (
 		! isset( $_POST['tenup_auto_tweet_meta_nonce'] ) ||
-		! wp_verify_nonce( sanitize_text_field( wp_unslash( $_POST['tenup_auto_tweet_meta_nonce'] ) ), 'tenup_auto_tweet_meta_fields' ) ||
+		! wp_verify_nonce(


can we drop this nonce check since the rest api is already performing a nonce check?

This nonce check can now be removed.

I have one more commit in progress -- got delayed by client work in the middle of these updates 😄

👍 I will patiently await your updates!

assets/js/admin-auto_tweet.js

adamsilverstein · 2019-08-15T20:34:08Z

assets/js/admin-auto_tweet.js

+		$.ajax( adminAutotweet.restUrl, {
+			beforeSend: function( xhr ) {
+				pendingStatus();
+				xhr.setRequestHeader( 'X-WP-Nonce', adminAutotweet.nonce );


if we do use wp.apiFetch I think it handles the nonce already?

adamsilverstein · 2019-08-15T20:45:23Z

includes/rest.php

+		$id = isset( $request->get_attributes()['id'] ) ? $request->get_attributes()['id'] : null;
+	}
+
+	if ( ! is_int( $id ) || 1 > $id ) {


adamsilverstein · 2019-08-15T20:45:44Z

includes/rest.php

+		return false;
+	}
+
+	return current_user_can( 'edit_post', $id );


sanitization is missing? in sanitize_callback cast this as an int

adamsilverstein · 2019-08-15T20:46:33Z

includes/rest.php

+function update_post_autotweet_meta( $request ) {
+	$post_id = $request['id'] ? $request['id'] : null;
+
+	if ( empty( $post_id ) ) {


move to validate_callback

adamsilverstein · 2019-08-15T20:47:14Z

includes/rest.php

+
+	$params = $request->get_params();
+
+	$sanitized_tweet_body = trim( sanitize_text_field( wp_unslash( $params[ TWEET_BODY_KEY ] ) ) );


sanitize the parameter in sanitize_callback

adamsilverstein · 2019-08-15T20:49:13Z

tests/phpunit/integration/TestRest.php

+ *
+ * @sincd 1.0.0
+ */
+class TestRest extends WP_UnitTestCase {


adamsilverstein

Requested changes :)

johnwatkins0 · 2019-08-15T21:17:04Z

Thanks, @adamsilverstein -- great feedback! I'll follow up within the next few days.

adamsilverstein · 2019-08-21T17:06:02Z

assets/js/admin-auto_tweet.js

+		data[adminAutotweet.enableAutotweetKey] = status;
+		data[adminAutotweet.tweetBodyKey] = $tweetText.val();
+
+		wp.apiFetch(


adamsilverstein · 2019-08-21T17:11:39Z

assets/js/admin-auto_tweet.js

-
-		});
-	};
+		).catch( onRequestFail );


We may want to look for errors in the then handler as well, we should do some testing around different errors: eg network error, blocked request (rest disabled) etc. See https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch

adamsilverstein · 2019-08-21T17:14:04Z

includes/admin/assets.php

+
+	if ( empty( $post_id ) ) {
+		$post_id = intval(
+			filter_input( INPUT_GET, 'post', FILTER_SANITIZE_NUMBER_INT )  // Filter removes all characters except digits.


johnwatkins0 · 2019-08-23T03:25:49Z

@adamsilverstein . I've pushed a few commits, and this is ready for another round of review. Let me know if you see anything else.

Summary of recent commits:

dafbba7 Restored backward-compatible meta keys
a3f1de9 Added more robust validation/sanitization of meta fields saved both via REST and via save_post. Refactored so that REST and save_post share a single function that saves data (previously it was implemented in two places).
a3f1de9 Added a bit of extra error handling within apiFetch for when the response is not ok. I resisted the urge to go deep into this because I'm not certain at this point what errors to account for and how to do so. That could be a later task. For now, in the non-Gutenberg JS when apiFetch is not ok the user will see something like

which feels like a WIP but is probably better than nothing. Let me know if you have suggestions.

eb075f8 Removed a debug REST response mistakenly left in 🤦‍♂

Thank you for all your feedback! I'm looking forward to getting into Gutenberg metabox next.

adamsilverstein · 2019-08-26T18:31:16Z

assets/js/admin-auto_tweet.js

+	function onRequestFail( error ) {
+		var errorText = '';
+		if ( 'statusText' in error && 'status' in error ) {
+			errorText = adminAutotweet.errorText + ' ' + error.status + ': ' + error.statusText;


This can use a template literal:

Suggested change

errorText = adminAutotweet.errorText + ' ' + error.status + ': ' + error.statusText;

errorText = `${ adminAutotweet.errorText } ${ error.status }: ${ error.statusText }`;

@adamsilverstein This JS isn't transpiled -- is browser support an issue if we use a template literal? https://caniuse.com/#feat=template-literals

adamsilverstein · 2019-08-26T18:36:16Z

includes/utils.php

+/**
+ * Updates autotweet-related post metadata by prefixing the passed key.
+ *
+ * @param int    $id Post ID.


parameter descriptions should be aligned

adamsilverstein · 2019-08-26T18:36:35Z

includes/utils.php

+ * @param int    $id Post ID.
+ * @param string $key Autotweet meta key.
+ * @param mixed  $value The meta value to save.
+ * @return mixed meta_id if the meta doesn't exist, otherwise returns true on success and false on failure.


Capitalize "If"

…auto-tweet into feature/rest-endpoint

adamsilverstein · 2019-08-26T21:05:51Z

This looks great @johnwatkins0 ! I left a little bit of final feedback.

One thing I noticed when testing is that images are not sent with tweets - possibly because of the image I am testing with. I know this worked at some point in the past because I see images in my test tweets (https://twitter.com/adamtest110/). This was pulling from the featured image. We can address this in a follow up PR

johnwatkins0 · 2019-08-27T01:23:23Z

Thanks, @adamsilverstein . Addressed those last bits of feedback in those last two commits, and re-requesting review now.

adamsilverstein

Fantastic work @johnwatkins0 - thank you!

johnwatkins0 added 10 commits August 11, 2019 16:37

Add additional autotweet meta util functions

23913eb

Convert metadata functions to autotweet versions

582c3ea

Create REST route and convert WP AJAX to WP REST

0c119bc

Meta key case consistency

be98d1b

Convert consts back to vars in untranspiled JS file

f1c741f

Remove curly brace use syntax for PHP5.6 compatability

1676cd9

Merge branch 'feature/webpack-build-process' into feature/rest-endpoint

3021245

Variable name fix

35b27d8

Function name fix

4a45475

In JS, check for boolean instead of 'true'

f78fe06

johnwatkins0 commented Aug 11, 2019

View reviewed changes

includes/admin/assets.php Show resolved Hide resolved

johnwatkins0 commented Aug 11, 2019

View reviewed changes

includes/core.php Show resolved Hide resolved

johnwatkins0 commented Aug 11, 2019

View reviewed changes

includes/rest.php Outdated Show resolved Hide resolved

jeffpaul assigned johnwatkins0 Aug 12, 2019

jeffpaul added the type:enhancement New feature or request. label Aug 12, 2019

jeffpaul added this to the 1.0.0 milestone Aug 12, 2019

jeffpaul requested a review from adamsilverstein August 12, 2019 16:43

johnwatkins0 changed the base branch from feature/webpack-build-process to develop August 13, 2019 02:12

adamsilverstein approved these changes Aug 15, 2019

View reviewed changes

adamsilverstein self-requested a review August 15, 2019 20:50

adamsilverstein suggested changes Aug 15, 2019

View reviewed changes

johnwatkins0 added 3 commits August 20, 2019 02:15

Legacy JS - eplace jquery ajax with apiFetch

c0510ff

Replace direct access of with filter_input

2e422bb

Restore meta keys to previous forms for backward compatibility

dafbba7

adamsilverstein reviewed Aug 21, 2019

View reviewed changes

johnwatkins0 added 3 commits August 21, 2019 19:34

Add REST sanitize/validate CBs and refactor meta saving

a3f1de9

Improve error handling in legacy JS

16f1e25

Remove debug code

eb075f8

johnwatkins0 requested a review from adamsilverstein August 23, 2019 16:06

adamsilverstein reviewed Aug 26, 2019

View reviewed changes

Adam Silverstein added 2 commits August 26, 2019 13:25

Merge branch 'feature/rest-endpoint' of https://github.com/10up/10up-…

51b7cf0

…auto-tweet into feature/rest-endpoint

improve rest response message

b7d0f8c

johnwatkins0 mentioned this pull request Aug 26, 2019

Include featured image in tweet #36

Closed

johnwatkins0 added 2 commits August 26, 2019 20:18

Switch JS concatenation to template literal

1ee6551

Docblock formatting fixes

f230945

johnwatkins0 requested a review from adamsilverstein August 27, 2019 01:22

johnwatkins0 added 3 commits August 26, 2019 22:34

Cast checked boolean values in comparison

d19badf

Remove no-longer-used nonce field

8248745

Update failing integratino test

d0f0541

johnwatkins0 mentioned this pull request Aug 27, 2019

Feature/tweet image #37

Merged

6 tasks

adamsilverstein approved these changes Sep 12, 2019

View reviewed changes

johnwatkins0 merged commit 0fa2ca6 into develop Sep 12, 2019

johnwatkins0 mentioned this pull request Sep 13, 2019

Replace wp.ajax use with wp.apiRequest [2 hrs.] #15

Closed

jeffpaul deleted the feature/rest-endpoint branch June 30, 2020 19:15


		$params = $request->get_params();

		$sanitized_tweet_body = trim( sanitize_text_field( wp_unslash( $params[ TWEET_BODY_KEY ] ) ) );

	errorText = adminAutotweet.errorText + ' ' + error.status + ': ' + error.statusText;
	errorText = `${ adminAutotweet.errorText } ${ error.status }: ${ error.statusText }`;

Add REST API endpoint to replace AJAX callback #33

Add REST API endpoint to replace AJAX callback #33

Conversation

johnwatkins0 commented Aug 11, 2019 • edited Loading

Description of the Change

REST endpoint

Post meta updates

What this does not do

Alternative designs

Verification Process

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwatkins0 Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein commented Aug 15, 2019

adamsilverstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

johnwatkins0 commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwatkins0 commented Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein commented Aug 26, 2019

johnwatkins0 commented Aug 27, 2019

adamsilverstein left a comment

Choose a reason for hiding this comment

johnwatkins0 commented Aug 11, 2019 •

edited

Loading

johnwatkins0 Aug 11, 2019 •

edited

Loading

johnwatkins0 commented Aug 23, 2019 •

edited

Loading