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

Social | Add wpcom/v3/publicize/connections endpoint #40540

Closed
wants to merge 11 commits into from

Conversation

manzoorwanijk
Copy link
Member

@manzoorwanijk manzoorwanijk commented Dec 10, 2024

Extracted out of #40536

Proposed changes:

  • Add wpcom/v3/publicize/connections endpoint that can be used in every scenario the publicize module is used
    • WPCOM sites
    • Jetpack sites - both Jetpack and Social plugins

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

  • Apply this PR to your sandbox and point public API to the sandbox
  • Goto Developer console
  • Hit WP REST API - wpcom/v3/sites/<site_id>/publicize/connections
  • Confirm that the site connections are displayed correctly as per the schema in this PR
  • Break a connection by removing Jetpack from the service, e.g. here for Facebook.
  • Add ?test_connections=1 to the endpoint above
  • Verify that the tests are run for connections
  • Verify that status is set to 'broken' for the broken connection
  • Point your Jetpack site to the sandbox
  • Goto any page which uses wp.apiFetch e.g. Block Editor
  • In console, run this
await wp.apiFetch({
    path: '/wpcom/v3/publicize/connections'
});
  • Confirm that the site connections are displayed correctly as per the schema in this PR
Screenshot 2024-12-10 at 7 21 25 PM Screenshot 2024-12-11 at 10 53 51 AM Screenshot 2024-12-10 at 7 21 09 PM

@manzoorwanijk manzoorwanijk added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Dec 10, 2024
@manzoorwanijk manzoorwanijk requested a review from a team December 10, 2024 13:52
@manzoorwanijk manzoorwanijk self-assigned this Dec 10, 2024
Copy link
Contributor

github-actions bot commented Dec 10, 2024

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the add/social-unitied-connections-endpoint branch.

  • To test on Simple, run the following command on your sandbox:

    bin/jetpack-downloader test jetpack add/social-unitied-connections-endpoint
    

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

Copy link
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Choose a review path based on your changes:
    • A. Team Review: add the "[Status] Needs Team Review" label
      • For most changes, including minor cross-team impacts.
      • Example: Updating a team-specific component or a small change to a shared library.
    • B. Crew Review: add the "[Status] Needs Review" label
      • For significant changes to core functionality.
      • Example: Major updates to a shared library or complex features.
    • C. Both: Start with Team, then request Crew
      • For complex changes or when you need extra confidence.
      • Example: Refactor affecting multiple systems.
  3. Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

/**
* Publicize Connection Fields class.
*/
class Connection_Fields {
Copy link
Member Author

Choose a reason for hiding this comment

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

The methods in this class are copied from Publicize_Base class which we can later on clean up to these methods as fallback after deprecating them.

@manzoorwanijk manzoorwanijk marked this pull request as ready for review December 10, 2024 14:03
Copy link
Contributor

@pablinos pablinos left a comment

Choose a reason for hiding this comment

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

I think we might be doing too much in one go here. We should definitely be doing some refactoring of the Publicize_Base class, but I'm not sure if we want to tackle that at the same time.

It would be good to do the smallest change that fixes our problem.

'title' => 'jetpack-publicize-connection',
'type' => 'object',
'properties' => array(
'connection_id' => array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this, the endpoint exists because we need to rename a couple of fields and add some more. Would it be possible to extend the current connection test results endpoint, and create a version 2.1 from there?

We're copying quite a bit of code and creating a third set of endpoints, which we should probably avoid if we can.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be possible to extend the current connection test results endpoint, and create a version 2.1 from there?

I initially tried to update that endpoint to enhance it but it resulted in complications for its use on all the sites - Simple and Jetpack. It also cannot be used in Standalone plugins as is.

We're copying quite a bit of code and creating a third set of endpoints, which we should probably avoid if we can.

Yes, existing endpoints are spread all over and the logic is not consolidated, which actually resulted in the schema mess we are trying to solve. I did think about keeping the changes to minimal but it felt like creating more such mess.

The approach here tries to consolidate the endpoint which can be used everywhere - WPCOM and Jetpack sites, both in Jetpack and Social plugin.

@manzoorwanijk
Copy link
Member Author

I think we might be doing too much in one go here.

I agree but I am confident about these changes that they will be better in the long run.

We should definitely be doing some refactoring of the Publicize_Base class, but I'm not sure if we want to tackle that at the same time.

We are not touching Publicize_Base to leave that refactor for a later time. I just copied the useful bits from there which are needed for the endpoint. We can afterwards deprecate those methods in that class.

It would be good to do the smallest change that fixes our problem.

I am afraid that may lead to further complicated logic of the Publicize classes on Jetpack and WPCOM, making it harder for someone else to follow the business logic.

Comment on lines -500 to +501
if ( 'threads' === $service_name && isset( $connection['external_name'] ) ) {
return 'https://www.threads.net/@' . $connection['external_name'];
if ( 'threads' === $service_name && isset( $cmeta['external_name'] ) ) {
return 'https://www.threads.net/@' . $cmeta['external_name'];
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is to ensure it works on WPCOM sites where connection is not an array.

@@ -527,7 +527,7 @@ public function get_profile_link( $service_name, $connection ) {
}

$profile_url_query = wp_parse_url( $cmeta['connection_data']['meta']['profile_url'], PHP_URL_QUERY );
$profile_url_query_args = null;
$profile_url_query_args = array();
Copy link
Member Author

Choose a reason for hiding this comment

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

Phan complained about this.

@@ -608,7 +608,7 @@ public function get_username( $service_name, $connection ) {
* @param object|array $connection The Connection object (WordPress.com) or array (Jetpack).
* @return string
*/
private function get_profile_picture( $connection ) {
public function get_profile_picture( $connection ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes it available externally for facade.

@manzoorwanijk manzoorwanijk force-pushed the add/social-unitied-connections-endpoint branch from 53a4d87 to b0190a0 Compare December 12, 2024 04:25
@manzoorwanijk
Copy link
Member Author

Closing in favour of #40589 as we no longer need the connections endpoint. We can remove the wpcom/v2/publicize/connections endpoint in the future and use the publicize class directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Publicize Now Jetpack Social, auto-sharing [Package] Publicize [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants