-
Notifications
You must be signed in to change notification settings - Fork 21
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
Adjust auto-sync shipping option #2526
Adjust auto-sync shipping option #2526
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## update/shippings-settings-phase-1 #2526 +/- ##
=====================================================================
+ Coverage 65.0% 65.2% +0.3%
- Complexity 4585 4588 +3
=====================================================================
Files 476 803 +327
Lines 17898 23012 +5114
Branches 0 1234 +1234
=====================================================================
+ Hits 11626 15012 +3386
- Misses 6272 7833 +1561
- Partials 0 167 +167
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -71,6 +90,9 @@ protected function get_settings_endpoint_edit_callback(): callable { | |||
} | |||
|
|||
foreach ( $schema as $key => $property ) { | |||
if ( ! in_array( 'edit', $property['context'] ?? [], 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.
I added this so it skips only view properties; otherwise, it would save them as options, like the shipping_rates_count
property.
mcSetup?.status === 'incomplete' | ||
? true | ||
: false; |
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.
This can be simplified as just mcSetup?.status === 'incomplete'
{ ...inputProps } | ||
label={ createInterpolateElement( | ||
__( | ||
'Automatically sync my store’s shipping settings to Google.', | ||
'google-listings-and-ads' | ||
), | ||
{ | ||
strong: <strong></strong>, | ||
} | ||
) } |
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.
Since we dont use anymore tag we can omit createInterpolateElement and just use
label={
__(
'Automatically sync my store’s shipping settings to Google.',
'google-listings-and-ads'
)
}
@jorgemd24 I set a shipping method (flat) but still, I don't see the auto-sync option. If I add free shipping yes it works. Screen.Recording.2024-08-16.at.13.27.02.mov |
Thanks @puntope for the review and your suggestions!
Could it be that the issue is related to using a comma instead of a dot for decimal pricing? I encountered a similar problem while working on this #2527, Could you share a screenshot of how the flat rate is set up? Also, could you provide a screenshot of the response from the settings endpoint? Thanks! |
Hi @jorgemd24 I tested it again. Seems like it's because the price field was empty. If I add a value (0, -1, 0.0, 0,0) it works
|
Hi @puntope, you need to have valid shipping methods for the auto sync option to be displayed; otherwise, it won't show up. For example, something like this should work: The original idea was to show the |
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.
LGTM @jorgemd24 Thanks for clarifying the issue with the empty shipping method.
Changes proposed in this Pull Request:
Part of pcTzPl-2qP-p2
This PR includes the following changes:
Onboarding:
t6I53r.mp4
Editing Page (the auto-sync option is always displayed, regardless of shipping settings):
Cu84V6.mp4
Detailed test instructions:
auto-sync
option is not available.auto-sync
option is available.My shipping settings are simple. I can manually estimate flat shipping rates
is checked.auto-sync
option is always displayed, regardless of shipping settingsAdditional details:
is_numeric
will return false. I'll create the issue.google-listings-and-ads/src/Shipping/ZoneMethodsParser.php
Lines 128 to 131 in fe41b5b
Changelog entry