-
Notifications
You must be signed in to change notification settings - Fork 795
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
Social: Standardize the connection object in the recently added social endpoints #37390
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. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. 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. Backup plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin 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:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Migration plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Inspect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Automattic For agencies client plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
93b333d
to
5bf0671
Compare
212978c
to
ba6ad93
Compare
ee8874d
to
ecad234
Compare
32d26cc
to
f4c2065
Compare
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 think this is a good change to be moving away from setting the option.
For the GET request, this will be the response. Notice that connection_health in Unknown. If you include the query param like this ?params=connection_health, the connection_result will be updated. Possible values are Unknown, Healthy and Unhealthy.
I'm not sure that we should be doing this. There is the _fields global parameter for the REST API which allows the caller to request which fields should be included in the response. We can have the default as not including the connection_states but use this core functionality to allow it to be requested when required.
@@ -443,7 +450,7 @@ public function update_publicize_connection( $request ) { | |||
$body['shared'] = $shared; | |||
} | |||
|
|||
$response = Client::wpcom_json_api_request_as_user( | |||
Client::wpcom_json_api_request_as_user( |
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.
Does it matter if this is unsuccessful? I think we're changing the response so it always returns that it's successful, but tha actual connection may not have been modified. This means that the caller would need to check the connection data that has been returned and can't rely on the status code.
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've added error handling here.
return rest_ensure_response( $this->make_proper_response( $response ) ); | ||
|
||
global $publicize; | ||
return rest_ensure_response( $publicize->get_connections_for_user( (int) $connection_id ) ); |
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.
Should we have some way of forcing this to be uncached? I think what's happening is after the connection is updated an XMLRPC request will get made so the cache is busted and the connections data will be updated, we'll then get the new connection data here.
But, we could get into some race conditions there. If the XMLRPC request is delayed, then we'll be reading the old connection data at this 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.
I have added a param here to force refresh. Is that better? When the page loads initially we can fire a call without the force refresh param. Behind the scenes, should we force refresh after the page loads?
…ic/jetpack into update/social-endpoints
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.
A few thoughts. They probably don't stop us merging this.
|
||
$connections = get_transient( self::JETPACK_SOCIAL_CONNECTIONS_TRANSIENT ); | ||
|
||
if ( $connections === false ) { | ||
$connections = array(); | ||
} | ||
|
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.
Thinking about this some more, if the transient expires and we can't retrieve the connections because of a network error, then it will look the same as if they have no connections, and we won't inform the user of the error.
Currently we would always show a cached version of the connections from the options, which would then show as broken when the test results API call fails. I'm not sure how much of an issue this is, but it could be worth us implementing something so that we can determine the difference between a site having no connections and there being an error preventing us from retrieving them.
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.
Perhaps record an error on local whenever we receive an error? I can create a follow-up task for it.
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.
Yes, I think we should have an error state. There will definitely be errors, so we should cope with that and provide feedback - although I realise we don't cope well with it currently!
I think previously we were using a transient for that, but we could use this same transient and store an error state in it, so that gets sent to the front end and can be displayed.
Then we can have an option to force it to try again.
@@ -262,6 +279,8 @@ public function get_all_connections_for_user() { | |||
'connection_id' => $connection['connection_data']['id'], | |||
'can_disconnect' => current_user_can( 'edit_others_posts' ) || get_current_user_id() === $user_id, | |||
'profile_link' => $this->get_profile_link( $service_name, $connection ), | |||
'shared' => $connection['connection_data']['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.
Should we use this to cater for numeric 0
here and also the Yoda conditional PHPCS error?
'shared' => $connection['connection_data']['user_id'] === '0', | |
'shared' => empty( $connection['connection_data']['user_id'] ), |
Merge #37510 first, and then rebase this PR. |
@@ -252,7 +252,7 @@ public function get_all_connections_for_user( $args = array() ) { | |||
array( | |||
'service_name' => $service_name, | |||
'connection_id' => $connection['connection_data']['id'], | |||
'can_disconnect' => self::can_manage_connection( $connection['connection_data'] ), | |||
'can_disconnect' => current_user_can( 'edit_others_posts' ) || get_current_user_id() === $user_id, |
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 didn't remember making this change, so I didn't add it as a part of #37510
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.
That change was made in #37501
With #37529 getting merged, we can abandon this PR. |
The newly added Jetpack Social endpoints don't return data in the same format as the initial state. This PR standardizes the response for the connection object. I've also updated the
jetpack/v4/publicize/connections
endpoint which isn't used anywhere.We are getting rid of the site option and are going to rely on transient to cache the connections on the Jetpack site.
Proposed changes:
get_connections_for_user
publicize function.get_all_connections_for_user
publicize function.publicize_connections
to a transient to avoid issues with caching of the option.shared
key to the connections object.status
to the connections object.run_test_results=true
to your get request.clear_cache =1
to your get request.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No
Testing instructions:
define( 'JETPACK_SOCIAL_USE_ADMIN_UI_V1', true );
is enabled or add the blog sticker to enable the feature.POST wp-json/jetpack/v4/social/connections
, PUTwp-json/jetpack/v4/social/connections/:id
and GETwp-json/jetpack/v4/publicize/connections
and observe that the API response.For the GET request, this will be the response.
To get connection test results, include these query params
wp-json/jetpack/v4/publicize/connections?run_test_results=true
When you create a connection by calling
https://temporary-lemming.jurassic.ninja/wp-json/jetpack/v4/social/connections
with the body{"keyring_connection_ID" : xxx, "shared" : false }
, you will see a similar response. Likewise for globalizing/unglobalizing as well.