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
Add shipping partner suggestions api #37155
Conversation
Test Results SummaryCommit SHA: 9ff33fe
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. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## trunk #37155 +/- ##
==========================================
- Coverage 46.7% 46.7% -0.0%
- Complexity 17183 17197 +14
==========================================
Files 429 429
Lines 64799 64893 +94
==========================================
+ Hits 30251 30287 +36
- Misses 34548 34606 +58
|
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.
Thanks for working on it! @moon0326
I just left some comments and I think we should replace DefaultPaymentGateways
with DefaultShippingPartners
.
'is_visible' => array( | ||
self::get_rules_for_countries( array( 'MX', 'CO' ) ), | ||
), | ||
'available_layouts' => array( 'single' ), |
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 it be "row" layout?
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.
@chihsuan Thank you for catching 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.
Updated in 302ffe0
...ins/woocommerce/src/Admin/Features/ShippingPartnerSuggestions/ShippingPartnerSuggestions.php
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Admin/API/ShippingPartnerSuggestions.php
Outdated
Show resolved
Hide resolved
'args' => array( | ||
'force_default_suggestions' => array( | ||
'type' => 'boolean', | ||
'description' => __( 'Return the default shipping partner suggestions when woocommerce_show_shipping_partner_suggestions and woocommerce_setting_payments_recommendations_hidden options are set to no', 'woocommerce' ), |
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.
Do you know where we set woocommerce_show_shipping_partner_suggestions
options? And I guess we don't need woocommerce_setting_payments_recommendations_hidden
?
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 description needs an update 😄 It only works when woocommerce_show_marketplace_suggestions
is set to false and force_default_suggestions
is set to true
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.
Co-authored-by: Chi-Hsuan Huang <chihsuan.tw@gmail.com>
…ons/ShippingPartnerSuggestions.php Co-authored-by: Chi-Hsuan Huang <chihsuan.tw@gmail.com>
'type' => 'array', | ||
'items' => array( | ||
'type' => 'string', | ||
'enum' => array( 'row', 'column' ), |
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.
FYI, I borrowed row
and column
concept from CSS flex-direction
.
Layout row
represents a smaller banner (?) that can be placed with other smaller banners in a single row.
Layout column
represents a banner that takes the whole row width.
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 i'd go with 'narrow' and 'wide' but no objections to this way either 😀
), | ||
'is_visible' => array( | ||
'description' => __( 'Suggestion visibility.', 'woocommerce' ), | ||
'type' => 'boolean', |
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.
@moon0326 I just noticed that is_visible
is not a boolean value in the API response. Should we change it to boolean?
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.
Good catch!
It should be set to boolean after rule evaluation. Updated in 88bf071
BTW, looks like PHP linting is failing (also WCCOM side). |
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.
Nice work! Looks good to me and tested well. 💯
* @return array Default specs. | ||
*/ | ||
public static function get_all() { | ||
$asset_base_url = WC()->plugin_url() . 'assets/images/shipping_partners/'; |
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.
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.
Left two comments but lgtm overall, thanks! We can iterate on this with the UI work next 😄
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.
Looks good!
All Submissions:
Changes proposed in this Pull Request:
Closes #36281
This PR adds
/wp-json/wp-admin/shipping-partner-suggestions
. The endpoint is designed to be used with the shipping cost task.Default shipping list test
When
woocommerce_show_marketplace_suggestions
option is set tono
, the endpoint should use the default shipping partner list_transient_woocommerce_admin_shipping_partner_suggestions_specs
optoin does not exist.woocommerce_show_marketplace_suggestions
option tono
/wp-json/wp-admin/shipping-partner-suggestions
_transient_woocommerce_admin_shipping_partner_suggestions_specs
option still does not exist.Getting the list from WCCOM
Setup your local WCCOM env with https://github.com/Automattic/woocommerce.com/pull/16291 branch.
Open this file and change the DATA_SOURCES to your local WCCOM.
Make sure
woocommerce_show_marketplace_suggestions
is set toyes
Make sure
_transient_woocommerce_admin_shipping_partner_suggestions_specs
does not exist.Acces
/wp-json/wp-admin/shipping-partner-suggestions
Confirm the endpoint returns a valid JSON
Confirm
_transient_woocommerce_admin_shipping_partner_suggestions_specs
exists.*Depending on your local setup, you might need to set WP_PROXY_HOST to access your local WCCOM env from your WooCommerce dev.
Other information:
pnpm --filter=<project> changelog add
?FOR PR REVIEWER ONLY: