Comments: Add Bulk Update/Delete Endpoint #9990
Conversation
Copons
added
[Type] Enhancement
|
Caution: This PR has changes that must be merged to WordPress.com |
@Automattic/jetpack-crew any suggestions for this? I assume this is why the wpcom check is failing here? |
Correct - the tests on D16965-code are failing because of the "missing" file. Fusion saw that this PR touched WP.com files and decided to build that new differential. It's not that smart:
I'm not sure where those matticbot/wpcom tests come from or if there's a way to connect this PR to the correct differential. Perhaps @abidhahmed or @brbrr knows? Alternatively, there's a script for building a GitHub PR from a differential: Whether that method of creating a PR would suffer this same fate under Fusion, I don't know :) I've never used the script and don't know what all happens behind the scenes. @dereksmart? |
|
Thanks @Copons 💖 I did some manual testing with the API console + Automattic/wp-calypso#25076 for bulk status changes. Looks like this is working pretty well! I ran out of time but we should also check the case when trash is disabled from a site. (eg in this case make sure the first delete will perma-delete items, vs needing 2 calls). Could I also get a +1 👀 from @Automattic/lannister to make sure I didn't miss anything? |
|
@mdawaffe I'm a bit lost: should I deploy the .com diff before this PR, or should I wait for this PR to be merged (and released in prod?) first? Just to be clear: I'm talking about the "manual" diff I created (D13678-code), not the one Fusion automatically opened, and that it has been abandoned (D16965-code). |
|
@Copons In general: whatever works best for you. For this change specifically, I think endpoints need to be launched on WordPress.com first before they endpoints are accessible in a Jetpack site. That certainly used to be the case with the WP.com REST API endpoints. I suspect it's still the case with WP-API endpoints. (Do we have better terminology to differentiate those two things?) |
|
If you've already considered this idea, no need to rehash the discussion here. If you've not already considered this idea, it's probably (again :)) too little too late. Instead of these bulk endpoints, have you considered adding a more generic batch endpoint? One that could accept multiple requests all at once and forward each to the correct controller. |
|
Thanks @mdawaffe, Regarding the batch endpoint: I think we talked about it (the .com diff is from May, so my memories are a bit cloudy now 😄) but we decided to not pursue it because this takes care of a few very specific tasks that aren't a good fit for a batch endpoint. For example, with this endpoint we can empty the trash by setting a simple parameter. |
There was a problem hiding this comment.
I ran out of time but we should also check the case when trash is disabled from a site. (eg in this case make sure the first delete will perma-delete items, vs needing 2 calls).
@gwwar, @Copons, Did we ever double-check this?
Could I also get a +1 👀 from @Automattic/lannister to make sure I didn't miss anything?
I'm assuming the WP.com merge was the +1? :)
@mdawaffe I totally forgot about that! I've tested it right now (disabled the trash by adding
I'll try to loop somebody else into testing this :) |
There was a problem hiding this comment.
Nice work @Copons! I tested the various cases and everything worked fine for me. Left some minor comments regarding the code that could be fixed before merging. This is optional, because the code mostly works fine even without them. :)
| */ | ||
| function bulk_update_comments_status( $comment_ids, $status ) { | ||
| if ( count( $comment_ids ) < 1 ) { | ||
| return new WP_Error( 'empty_comment_ids', 'The request must include comment_ids' ); |
There was a problem hiding this comment.
We should return the status code here too (400).
| */ | ||
| function bulk_delete_comments( $comment_ids ) { | ||
| if ( count( $comment_ids ) < 1 ) { | ||
| return new WP_Error( 'empty_comment_ids', 'The request must include comment_ids' ); |
There was a problem hiding this comment.
Same as above regarding the status code.
| 'response_format' => array( | ||
| 'results' => '(array) An array of updated Comment IDs.' | ||
| ), | ||
| 'example_request' => 'https://public-api.wordpress.com/rest/v1.1/sites/82974409/comments/status', |
There was a problem hiding this comment.
The example URL should contain .../v1/... in this case
| 'response_format' => array( | ||
| 'results' => '(array) An array of deleted or trashed Comment IDs.' | ||
| ), | ||
| 'example_request' => 'https://public-api.wordpress.com/rest/v1.1/sites/82974409/comments/delete', |
| $input = $this->input(); | ||
|
|
||
| if ( is_array( $input['comment_ids'] ) ) { | ||
| $comment_ids = (array) $input['comment_ids']; |
There was a problem hiding this comment.
If we already established that it's an array on the line above, why do we need to explicitly cast to it to array here again?
|
|
||
| if ( $this->api->ends_with( $path, '/delete' ) ) { | ||
| if ( ! empty( $input['empty_status'] ) ) { | ||
| $empty_status = $input['empty_status']; |
There was a problem hiding this comment.
It seems like we don't really need to create this variable since it's not used anywhere else in the function. We could pass the $input['empty_status'] directly to the function.
| wp_defer_comment_counting( true ); | ||
|
|
||
| if ( $this->api->ends_with( $path, '/delete' ) ) { | ||
| if ( ! empty( $input['empty_status'] ) ) { |
There was a problem hiding this comment.
Maybe it would be more readable if we performed validate_empty_status_param here instead of hiding it inside the delete_all function. We could also avoid calling that function for invalid values.
| $result['results'] = $this->bulk_delete_comments( $comment_ids ); | ||
| } | ||
| } else { | ||
| $status = $input['status']; |
There was a problem hiding this comment.
This also looks like an unnecessary assignment. :)
| * @return boolean | ||
| */ | ||
| function validate_status_param( $status ) { | ||
| return in_array( $status, array( 'approved', 'unapproved', 'pending', 'spam', 'trash' ) ); |
There was a problem hiding this comment.
By default in_array will also do type conversion before comparison if necessary, which could sometimes lead to unexpected results. For example, in_array( true, array( 'approved', 'unapproved', 'pending', 'spam', 'trash' ) ) will evaluate to true. That's why I prefer passing true as third argument to force the strict comparison.
| * @return boolean | ||
| */ | ||
| function validate_empty_status_param( $empty_status ) { | ||
| return in_array( $empty_status, array( 'spam', 'trash' ) ); |
This is automated check which relies on Generated by 🚫 dangerJS |
mdawaffe
added
[Status] Ready to Merge
There was a problem hiding this comment.
I just tested the branch, and I'm getting a few PHP notices in some cases:
Undefined index: empty_status
Type: PHP Notice Line: 88
File: wp-content/plugins/jetpack-dev/json-endpoints/class.wpcom-json-api-bulk-update-comments-endpoint.php
Undefined index: status
Type: PHP Notice Line: 94
File: wp-content/plugins/jetpack-dev/json-endpoints/class.wpcom-json-api-bulk-update-comments-endpoint.php
Undefined index: comment_ids
Type: PHP Notice Line: 73
File: wp-content/plugins/jetpack-dev/json-endpoints/class.wpcom-json-api-bulk-update-comments-endpoint.php
It may be nice to avoid those when some of the requests do not include all params.
jeherve
added
[Status] Needs Author Reply
|
@Copons Did you have the chance to look at this? Would you still like this to be shipped with Jetpack 6.6 (the beta ships next Tuesday)? |
|
@jeherve I'm so sorry, I got caught up with more urgent team tasks and had to put this on the back-burner. |
|
@jeherve The notices should be fixed now. Thanks for pointing them out, I totally forgot to check the error log 🙁 |
kraftbj
added
[Status] Needs Review
jeherve
added
[Status] Ready to Merge
ghost
removed
[Status] Needs Changelog
[Status] Ready to Merge
|
Thank you @jeherve! ✨ |
Contributes to fixing Automattic/wp-calypso#20299
WPCOM diff: D13678-code
I'm not exactly sure how to do this from a Fusion standpoint.
I've (accidentally) created the WPCOM diff first, which has been thoroughly reviewed and approved, and then realized that it contains: 1 file already synced, and 1 completely new file.
What would be the best practice here?
Changes proposed in this Pull Request:
This diff proposes the introduction of several new bulk endpoints:
POST /sites/{site}/comments/status: update multiple comments statuses (also useful because currently we do this with a frontend loop).POST /sites/{site}/comments/delete: delete multiple comments or, in alternative, all spam or trash comments.Most of the logic here is inspired by how Core handles all of this: https://github.com/WordPress/WordPress/blob/master/wp-admin/edit-comments.php
Testing instructions:
Notes: a comment cannot be permanently deleted unless it's
spamortrash, but it's moved totrashinstead.