-
Notifications
You must be signed in to change notification settings - Fork 834
jetpack-mu-wpcom: Limit comment like requests to edit-comment and gate by module #44205
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
jetpack-mu-wpcom: Limit comment like requests to edit-comment and gate by module #44205
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
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.
Since we’re now caching requests, the likes feature isn’t working as expected. From the wp-admin comments screen, I can like a comment, but after reloading it appears as if I didn’t. Here’s a video demonstrating the issue.
likes-broken.mp4
99833a3
to
7867065
Compare
Thanks for the review @rcrdortiz. I've just pushed a fix that address the cache busting. |
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.
Sorry, I didn't do the complete review on Saturday. Just wanted to mark this PR so that it didn't get merged.
The feature should be enabled regardless of whether the module is enabled. This code was created when we untangled the comments screen. To achieve feature parity with Calypso, we need to always enable the "Liked by me" functionality on all WPCom Atomic sites.
projects/packages/jetpack-mu-wpcom/src/features/wpcom-comments/wpcom-comments.php
Show resolved
Hide resolved
There’s an endpoint Calypso used to use that already returns this information, but it wasn’t accessible through wp-admin and was out of scope when we implemented feature parity. Now that Calypso comments aren’t available to our customers, maybe we could revisit and update the API. |
projects/packages/jetpack-mu-wpcom/src/features/wpcom-comments/class-wp-rest-comment-like.php
Outdated
Show resolved
Hide resolved
Thanks. I've changed the expiration to 900 seconds, or 15 minutes, which I think is a reasonable cache timeout for this scenario. |
if ( class_exists( 'Jetpack' ) && method_exists( 'Jetpack', 'is_module_active' ) ) { | ||
// @phan-suppress-next-line PhanUndeclaredClassMethod | ||
return Jetpack::is_module_active( 'comment-likes' ); | ||
} |
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 need to remove this. Any user on WPCom should be able to like comments from wp-admin regardless of whether Jetpack comment-likes
is enabled or not.
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 don't think that I agree with this. But, I'll consider it a bit further. 😄
Consider the scenario where a user goes to wp-admin → Comments, opens a post, likes a comment, then returns to wp-admin and sees their like hasn’t been registered. This issue will recur regardless of cache duration unless we actively invalidate the cache after a like action. Could you help me understand how introducing a 15-minute cache window would mitigate this problem? |
It doesn't directly mitigate this issue. It does definitely minimize the impact. Not having caching of the requests is not an option in my opinion. Otherwise, we have greater than 1 second of http requests on every comments page load. |
Hey @ebinnion , thanks again for iterating on this! I wanted to recap where we landed and propose the next steps, so we’re all on the same page: 1. Preserve Calypso-style comment likes in wp-admin
2. No front-end caching in wp-admin
3. Batching via the WP.com comments endpoint
|
ec1e5d1
to
ec49f68
Compare
I've thought about it some bit, and I think that there's a decent stopping point for now by just removing the transient caching. Only making the requests on the edit-comments.php screen and allowing disabling the requests by checking for the comment-likes module provides a significant improvement. |
Problem statement
This functionality was initially added for simple sites in #41475 and later for WoW sites in #42300. When the functionality was added for WoW sites it resulted in many API calls that occurred on each page load that includes comments. I initially noticed this on the
wp-admin
of my site.But, after investigating the code, the API calls were also present on the frontend of the site.
Proposed changes:
The solution that I am proposing is:
comment-likes
module is enabled.comment_class
if we're in the admin to minimize frontend requests.Potential future changes
I'm not a fan of making $x number of requests, one for each comment. I think the way that we'll solve this in the future is probably by hooking in when the comments table is being rendered and making a batch API request. If we take this approach, we'd request early, store in an array in the singleton class, and then pull from that array as we render.
Further, we could consider expanding the
is_comment_likes_enabled
method to include WPCOM simple sites. Deferring for now because of the minimal performance implications.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
/wp-admin
and/wp-admin/edit-comments.php
continues to function./wp-admin
and/wp-admin/edit-comments.php
as well.wp transient delete --all && wp jetpack module deactivate comment-likes
over SSH and then loading the same URLs above.