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

Return settings prop as object in the REST API #2560

Merged
merged 6 commits into from
Aug 27, 2024
Merged

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Aug 23, 2024

Changes proposed in this Pull Request:

Ref: p1724235667349019/1722414955.111429-slack-C02BB3F30TG

Reverts #2519

This PR fixes an error introduced in #2519

At first, I understood that it was expected to get all empty props as an empty object. However, what was expected was only to get the Settings prop as an object. Rest of the props should be as before.

This PR reverts #2519 and makes only settings to be returned as an object.

Screenshots:

Screenshot 2024-08-23 at 11 34 23 Screenshot 2024-08-23 at 11 34 46 Screenshot 2024-08-23 at 11 35 45 Screenshot 2024-08-23 at 11 35 31

Detailed test instructions:

  1. Checkout this PR and
  2. Request wp-json/wc/v3/products?gla_syncable=1
  3. attributes field is shown as an array (when is empty and when is filled)
  4. gla_attributes field is shown as an object (when is empty and when is filled)
  5. Request wp-json/wc/v3/settings/general?gla_syncable=1
  6. settings field is shown as an object (when is empty and when is filled)
  7. Add the following filter which will return empty settings for the flat shipping rate:
add_filter(
	'woocommerce_shipping_instance_form_fields_flat_rate',
	function ( $defaults ) {
		return [];
	},
	10,
	1
);
  1. Make a request to v3/shipping/zones/YOUR_SHIPPING_ZONE/methods?gla_syncable=1 which includes a Flat Rate method.
  2. See the response:

Before:

image

After

image

Additional details:

Changelog entry

@puntope puntope self-assigned this Aug 23, 2024
@puntope puntope requested a review from jorgemd24 August 23, 2024 07:40
@github-actions github-actions bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Aug 23, 2024
@puntope puntope marked this pull request as ready for review August 23, 2024 07:41
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.0%. Comparing base (4c96cd7) to head (9981934).
Report is 19 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             develop   #2560   +/-   ##
=========================================
  Coverage       65.0%   65.0%           
- Complexity      4588    4591    +3     
=========================================
  Files            475     475           
  Lines          17900   17907    +7     
=========================================
+ Hits           11640   11646    +6     
- Misses          6260    6261    +1     
Flag Coverage Δ
php-unit-tests 65.0% <100.0%> (+<0.1%) ⬆️

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

Files Coverage Δ
src/Integration/WPCOMProxy.php 74.1% <100.0%> (ø)

... and 2 files with indirect coverage changes

@jorgemd24
Copy link
Contributor

jorgemd24 commented Aug 27, 2024

Thanks, @puntope, for fixing this! However, I noticed there was some confusion—the issue wasn’t actually with the wp-json/wc/v3/settings/general endpoint but with the settings in shipping/zones/X/methods. See this comment: p1724749602301079/1722414955.111429-slack-C02BB3F30TG. I just realized the testing steps hadn’t been updated and still mentioned the settings endpoint, so I’ve updated them. I’ve updated the PR to fix the issue specifically for the shipping zone method endpoint, rather than for all settings in every response. We’re not sure if other endpoints with the setting key will return an object.

Before:

image

After:

image

I’ll go ahead and merge this PR for you.

@jorgemd24 jorgemd24 merged commit 7882202 into develop Aug 27, 2024
12 checks passed
@jorgemd24 jorgemd24 deleted the fix/settings-format branch August 27, 2024 10:04
@tomalec tomalec mentioned this pull request Aug 27, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants