Skip to content
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

Add/rest api for block commenting #65181

Open
wants to merge 27 commits into
base: trunk
Choose a base branch
from

Conversation

poojabhimani12
Copy link
Contributor

@poojabhimani12 poojabhimani12 commented Sep 10, 2024

What?

Add Rest API for Block Level Comment

Why?

#59445
#60622 (comment)

This PR updates the REST API to support our custom comment type, 'block_comment.' Previously, only the default 'comment' type was supported. We've developed the comment API endpoint in Gutenberg to enable block-based commenting, providing greater flexibility for managing comments within the block editor.

Copy link

github-actions bot commented Sep 10, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: MD-sunilprajapati <spmultidots@git.wordpress.org>
Co-authored-by: poojabhimani12 <poojabhimani@git.wordpress.org>
Co-authored-by: rishishah-multidots <rishishah@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: TimothyBJacobs <timothyblynjacobs@git.wordpress.org>
Co-authored-by: spacedmonkey <spacedmonkey@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 10, 2024
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @poojabhimani12! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@youknowriad
Copy link
Contributor

Thanks for splitting the PRs, that's really helpful.

I'm adding @TimothyBJacobs here for his expertise on the REST API. I know @tyxla had some comments on the other thread as well about the REST API.

It would be cool to add some php unit tests to clarify what these changes are about, and maybe consider adding some context to the PR description.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting this one out to a separate PR 🙌

From my perspective, this mostly looks good and is close enough!

Passing the latest feedback over to this PR, and adding some more let me know what you think.


return $response;
}
add_filter( 'rest_prepare_comment', 'add_user_avatar_urls_in_rest_response_6_7', 10, 1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As also mentioned in #60622:

This will only work when retrieving comments via the REST API. If we want to retrieve them on the backend, it won't work.

I'm trying to understand why get_comment will set the avatar URL to null (as mentioned here). A few possibilities come to mind:

  • is it because the show_avatars option is not enabled?
  • is it because the author_avatar_urls field is not enabled when performing the REST API request?

Can you speak more about where this assignment to null happens?

In any case, to achieve what you're doing it should be enough to just use the rest_avatar_sizes filter and the rest_get_avatar_urls() function would build the avatar URLs properly. If we're just doing it for the REST API response, we're doing it in an inconsistent way which is likely to introduce unexpected bugs and missing data in unexpected places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue has been resolved. We found that the REST API was returning a null avatar URL because it only supported the default 'comment' type while we were using a custom type.

To fix this, we applied the get_avatar_comment_types filter to include our custom comment type, 'block_comment'. This allowed us to remove the rest_prepare_comment filter, and the issue is now resolved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks 🙌


return $prepared_comment;
}
add_filter( 'rest_pre_insert_comment', 'update_comment_type_in_rest_api_6_7', 10, 2 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reconsidering the previous approach of updating the comment during a REST request 🙌

Using rest_pre_insert_comment works OK if you always want to do it only through the REST API.

I acknowledge it doesn't make sense to use rest_preprocess_comment because the comment_type and comment_approved fields will be overridden automatically after that.

However, if you want this to work through any interface or flow that updates the comments, you might want to use the wp_update_comment_data filter instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a comment is submitted, whether it's a custom comment type or the default, comments from registered users will automatically be marked as "approved." For comments that need moderation, set their status to "on hold" initially. After review, you can update the status from "on hold" to "approved" as needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying that part @poojabhimani12.

I don't understand why we're doing it only when inserting a comment via the REST API. Wouldn't it be a better idea to do it consistently when a comment is inserted/updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Comment REST API currently only allows inserting default comment types. If you try to use a custom comment type, it returns an incorrect comment type error.

To resolve this, a filter must be applied to include custom comment types in the REST API. However, no filter is needed when using the comment insert function, as it already supports the insertion of custom comment types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense; thanks a lot for elaborating 🙌

if ( ! function_exists( 'update_comment_type_in_rest_api_6_7' ) && gutenberg_is_experiment_enabled( 'gutenberg-block-comment' ) ) {
function update_comment_type_in_rest_api_6_7( $prepared_comment, $request ) {
if ( ! empty( $request['comment_type'] ) && 'block_comment' === $request['comment_type'] ) {
$prepared_comment['comment_type'] = $request['comment_type'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to extend the admin_comment_types_dropdown filter to allow users to filter in the admin interface by the new comment_type that you're introducing. Unless you want to intentionally hide those comments from the interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not always allow there properties to be set and avoid hardcoding the comment type?

'gutenberg-experiments',
'gutenberg_experiments_section',
array(
'label' => __( 'Enable multi-user commenting on blocks in post editor', 'gutenberg' ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to consider this prior feedback:

#60622 (comment)

Perhaps it can be reflected in the experiment title?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’ve made this update based on the feedback provided here: #60622 (comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be adding this setting here, and hardcode the comment type. We should generalise it to work either with all comment types, or add an (experimental) option/setting to the comment type registration and use that setting (instead of hardcoding anything).


return $response;
}
add_filter( 'rest_prepare_comment', 'add_user_avatar_urls_in_rest_response_6_7', 10, 1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Priority 10 and 1 accepted arguments are the defaults for add_filter(), so they can be omitted.

Suggested change
add_filter( 'rest_prepare_comment', 'add_user_avatar_urls_in_rest_response_6_7', 10, 1 );
add_filter( 'rest_prepare_comment', 'add_user_avatar_urls_in_rest_response_6_7' );

return $comment_type;

}
add_filter( 'get_avatar_comment_types', 'update_get_avatar_comment_type', 10, 1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the spacing issues which I'm assuming you're addressing separately, the trailing arguments aren't necessary because they're the defaults:

Suggested change
add_filter( 'get_avatar_comment_types', 'update_get_avatar_comment_type', 10, 1 );
add_filter( 'get_avatar_comment_types', 'update_get_avatar_comment_type' );

@tyxla
Copy link
Member

tyxla commented Sep 10, 2024

It would be cool to add some php unit tests to clarify what these changes are about, and maybe consider adding some context to the PR description.

I also think some basic unit tests would be useful to have here. The logic is straightforward, so I don't assume the tests to be complicated.

@TimothyBJacobs
Copy link
Member

Using the existing comments controller like this is I think sufficient for an experiment. But we most likely are going to want to treat this more like post types, where different comment types are at different routes.

@spacedmonkey
Copy link
Member

Using the existing comments controller like this is I think sufficient for an experiment. But we most likely are going to want to treat this more like post types, where different comment types are at different routes.

I am not sure if we need a new endpoint here. Adding support for this comment type for the existing endpoint makes sense to me.

@ellatrix ellatrix added the [Type] Enhancement A suggestion for improvement. label Sep 23, 2024
@@ -28,6 +28,9 @@ function gutenberg_enable_experiments() {
if ( gutenberg_is_experiment_enabled( 'gutenberg-full-page-client-side-navigation' ) ) {
wp_add_inline_script( 'wp-block-library', 'window.__experimentalFullPageClientSideNavigation = true', 'before' );
}
if ( $gutenberg_experiments && array_key_exists( 'gutenberg-block-comment', $gutenberg_experiments ) ) {
wp_add_inline_script( 'wp-block-editor', 'window.__experimentalEnableBlockComment = true', 'before' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this flag in this PR?

'block_comment' => __( 'Block Comments' ),
);
}
add_filter( 'admin_comment_types_dropdown', 'update_comment_type_filter_dropdown' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to add this comment type to the comment management list? Comments are managed in the editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have incorporated the comment type into the backend comment page filter based on the feedback received: #65181 (comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this related to the REST API?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants