-
Notifications
You must be signed in to change notification settings - Fork 798
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
Jetpack Sync: Ensure HPOS order status is prefixed with 'wc-' before sending it to WPCOM #38258
Conversation
…sending it to WPCOM
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. |
*/ | ||
protected function get_all_possible_order_status_keys() { | ||
$order_statuses = array( 'checkout-draft', 'auto-draft', 'trash' ); | ||
$order_statuses = array( 'auto-draft', 'trash' ); |
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.
@vedanshujain Just a confirmation if we should include draft
here. In our discussion you mentioned that draft
is a potential order status, however based on the Woo check upon setting the status, it looks like it's not.
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.
yeah, looks like we are actively changing draft to pending in case it is set. so this implementation looks good,
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! 👍
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
Jetpack Sync: Ensure HPOS order status is prefixed with
wc-
before sending it to WPCOMThis is needed so that when we insert custom order statuses in the corresponding
wc_orders
table on WPCOM, we get the properwc-
prefix. We are handling the issue in the Sync package end sincewc_get_order_statuses
is not available when the corresponding Sync action is processed.Proposed changes:
Automattic\Jetpack\Sync\Modules\WooCommerce_HPOS_Orders
: Introduce a wrapper forwc_get_order_statuses
in case it's not present, but also to handle a bug in the WooCommerce codebase, sincewc-checkout-draft
is missingAutomattic\Jetpack\Sync\Modules\WooCommerce_HPOS_Orders
: Updatefilter_order_data
to prefix the order status withwc-
when applicable before sending the order to WPCOMOther information:
Jetpack product discussion
p9dueE-8ec-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
get_objects_by_id
is also affected it would be nice to repeat the test instructions from Jetpack Sync HPOS: Ensure get_objects_by_id will return all relevant orders #38251exclude_from_search
set totrue
and ensure the corresponding order will be synced with the status prefixed withwc-
.wc-
prefix forauto-draft
andtrash
orders