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

[API PULL] Backbone for SYNC endpoint #2788

Open
wants to merge 6 commits into
base: feature/api-pull-sync-endpoint
Choose a base branch
from

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Feb 10, 2025

Changes proposed in this Pull Request:

This is a preparatory PR for the work related to creating a WPCOM Proxied endpoint allowing to GET and UPDATE the enabled Sync mode.

In this PR we create a backbone for the endpoint getting a default value or the value saved in wp_options.gla_api_pull_sync_mode

PT: pcTzPl-2yG-p2

Screenshots:

Screenshot 2025-02-10 at 23 14 10

Detailed test instructions:

  1. Request GET wp-json/wc/gla/sync
  2. See you get the default response
'products' => [
			'pull' => false,
			'push' => false,
		],
		'coupons'  => [
			'pull' => false,
			'push' => false,
		],
		'shipping' => [
			'pull' => false,
			'push' => false,
		],
		'settings' => [
			'pull' => false,
			'push' => false,
		],
  1. Update wp_options.gla_api_pull_sync_mode and save some data for example this with all true:
    a:4:{s:8:"products";a:2:{s:4:"push";b:1;s:4:"pull";b:1;}s:7:"coupons";a:2:{s:4:"push";b:1;s:4:"pull";b:1;}s:8:"shipping";a:2:{s:4:"push";b:1;s:4:"pull";b:1;}s:8:"settings";a:2:{s:4:"push";b:1;s:4:"pull";b:1;}}
  2. Perform the request in step 1 again. And see the expected result

Additional details:

Changelog entry

@github-actions github-actions bot added type: enhancement The issue is a request for an enhancement. changelog: add A new feature, function, or functionality was added. labels Feb 10, 2025
@puntope puntope self-assigned this Feb 10, 2025
@puntope puntope requested a review from a team February 10, 2025 19:19
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 33.92857% with 37 lines in your changes missing coverage. Please review.

Project coverage is 67.1%. Comparing base (b04c648) to head (641e06c).

Files with missing lines Patch % Lines
...rc/API/Site/Controllers/RestAPI/SyncController.php 32.7% 37 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                         Coverage Diff                         @@
##             feature/api-pull-sync-endpoint   #2788      +/-   ##
===================================================================
+ Coverage                              66.7%   67.1%    +0.4%     
- Complexity                                0    4684    +4684     
===================================================================
  Files                                   316     481     +165     
  Lines                                  4921   19625   +14704     
  Branches                               1204       0    -1204     
===================================================================
+ Hits                                   3282   13174    +9892     
- Misses                                 1502    6451    +4949     
+ Partials                                137       0     -137     
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 67.1% <33.9%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ernal/DependencyManagement/RESTServiceProvider.php 100.0% <100.0%> (ø)
...rc/API/Site/Controllers/RestAPI/SyncController.php 32.7% <32.7%> (ø)

... and 795 files with indirect coverage changes

Copy link
Contributor

@mikkamp mikkamp 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 the changes, they are working as expected and I'm getting the response either from the defaults or the option.

I just left a couple comments, let me know if you think that would be worthwhile to do.

src/API/Site/Controllers/RestAPI/SyncController.php Outdated Show resolved Hide resolved
[
'methods' => TransportMethods::READABLE,
'callback' => $this->get_sync_callback(),
'permission_callback' => $this->get_permission_callback(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that we only want these endpoints accessible by the API Pull implementation. In the WC endpoints we check for the gla_syncable parameter being set to 1 in the request before we modify the responses.

Should we have a custom permissions callback to also check if this is set? Not very difficult to get around, but it might provide better separation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I will update that in future PRs if that's ok

@puntope puntope requested a review from mikkamp February 13, 2025 13:26
Copy link
Contributor

@mikkamp mikkamp 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 the changes. I retested and it's return a hybrid of the option and defaults. So everything is as expected:

{
    "products": {
        "push": true,
        "pull": true
    },
    "coupons": {
        "push": false,
        "pull": false
    },
    "shipping": {
        "push": true,
        "pull": true
    },
    "settings": {
        "pull": false,
        "push": false
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants