-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Update shipping partner suggestions to use data from API #38457
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
Conversation
| '$schema' => 'http://json-schema.org/draft-04/schema#', | ||
| 'title' => 'payment-gateway-suggestions', | ||
| 'type' => 'array', | ||
| 'type' => 'object', |
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.
Random fix: array in schema was wrong, it's supposed to be an object
Test Results SummaryCommit SHA: 40a31fc
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
|
Hi @moon0326, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
moon0326
left a comment
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 working on this 👍
I have one minor comment regarding the onboarding store changes.
Do you think it's worth implementing shipping method codes in a separate package, similar to payment gateways? It seems that the logic related to shipping methods does not fit into the onboarding package since it is not used during the onboarding process. However, I also noticed payment suggestion logic included in the onboarding package.
Other than that, everything LGTM 👍
rjchow
left a comment
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.
Tests well for Mexico! Wondering why we should set remote suggestions to false to test this though? (tested both true and false and they are both ok)
|
Thanks for the suggestion, @moon0326! I've added the new data module in this commit 4ea2c65 and removed the old changes here 13386af. Please take another look! 🙏
Thanks for testing it out @rjchow! Reasons why:
|
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #38456.
Adds consumption of shipping partner suggestions from WCCOM API.
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Testing row layout
WooCommerce > Settings > Advanced > WooCommerce.com(/wp-admin/admin.php?page=wc-settings&tab=advanced§ion=woocommerce_com)Display suggestions within WooCommerceoption and saveItalycountry and savewoocommerce_admin_created_default_shipping_zones.Add shipping costsSave shipping optionsTesting column layout
Canadacountry and savewoocommerce_admin_created_default_shipping_zones.Add shipping costsSave shipping optionsOptional further testing
7. Optionally, repeat testing for any countries from this list
'CA', 'AU', 'GB', 'ES', 'IT', 'DE', 'FR', 'MX', 'CO', 'CL', 'AR', 'PE', 'BR', 'UY', 'GT', 'NL', 'AT', 'BE'and ensure it follows the rules in DefaultShippingPartners.php