-
Notifications
You must be signed in to change notification settings - Fork 808
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
REST API: Enhance author details by including additional data when either user_id or site_id are available. #41159
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 🤖 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. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Please note that #41254 which was merged last week, adds two optional fields to the 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.
This tests well for me. I have some notes / questions, but no blockers.
$profile_url = 'https://gravatar.com/' . md5( strtolower( trim( $email ) ) ); | ||
$nice = ''; | ||
$site_id = -1; | ||
$id = ( isset( $author->user_id ) && $author->user_id ) ? $author->user_id : 0; |
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 here, maybe we can simplify this a bit? It may be an opportunity to cast the return user ID. I don't know if that's necessary, but it may avoid any issues since we check 0 < $id
later.
$id = ( isset( $author->user_id ) && $author->user_id ) ? $author->user_id : 0; | |
$id = ! empty( $author->user_id ) ? (int) $author->user_id : 0; |
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.
$login = $user->user_login; | ||
$first_name = $user->first_name; | ||
$last_name = $user->last_name; | ||
$nice = $user->user_nicename; |
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.
Is there any chance that some of those values return an empty string or are not defined at some point?
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.
Thank you for pointing that out. I naively assumed these class properties were declared on the WP_User
class — I should have checked. It turns out they are dynamic.
I've added the necessary checks in 9deae9b.
As for whether they can be empty, I think it's fine since this code doesn't replace any existing data but simply augments what's available in the wp_comments
table.
Code Coverage SummaryCoverage changed in 1 file.
|
c192526
to
c63354d
Compare
Fixes #9984
Proposed changes:
wp_comments
table as the source of truth if the$author
parameter passed to theWPCOM_JSON_API_Endpoint::get_author
method is an instance ofWP_Comment
;WP_Comment
instance has a validuser_id
, include thefirst_name
,last_name
,nice_name
, andlogin
fields in the response;site_id
field to the response when aWP_Comment
instance is passed to theWPCOM_JSON_API_Endpoint::get_author
method.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
public-api.wordpress.com
comments endpoint should return complete data about the commenter, including thelogin
,first_name
,last_name
, andnice_name
fields, if theuser_id
value in thewp_comments
table is defined.Refer to the screenshots below for comparison:
Before the patch:
After the patch: