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
[HPOS CLI] Add support for backfilling specific properties or metadata #45171
Conversation
Test Results SummaryCommit SHA: ba3bac2
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 @lsinger, 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: |
$order_hpos = $this->sut->get_order_from_datastore( $order->get_id(), 'hpos' ); | ||
$this->assertEquals( $order_hpos->get_billing_first_name(), $order_cpt->get_billing_first_name() ); | ||
$this->assertEquals( $order_hpos->get_status(), $order_cpt->get_status() ); | ||
} |
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 wonder if it could be beneficial here to add a last scenario where we re-enable sync and see that even if partial backfills happened in the meantime, sync still behaves as intended. What do you think? 🤔
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.
Added in c7fa5fe.
* Updates an order (in this datastore) from another order object. | ||
* | ||
* @param \WC_Abstract_Order $order Source order. | ||
* @return bool Whether the order was updated. |
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.
Is there ever a situation where we don't return true
? If not -- should there be? Or will there be one in the future? If we only ever return true
, having a return value feels a bit misleading, as a caller of the method might believe based on the return value that it can fail.
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.
So this docblock was just copied over from the parent class, which does return a value based on whether the post was updated or not. That said, it's a bit misleading too since the metadata update could've failed (which in that case is actually all order data except status/date) and that's not taken into account.
Similarly, save_meta_data()
doesn't tell us whether the meta was updated, so I'd say that right now we can't really tell whether the order was correctly updated or not, so the bool seems useless but we might eventually provided we make some adjustments elsewhere.
* @param bool $should_save Whether to trigger a full save after metadata is changed. | ||
*/ | ||
return apply_filters( | ||
'woocommerce_orders_table_datastore_should_save_after_meta_change', |
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.
For improving readability a bit, could we pull out the boolean value into its own variable (e.g., $should_save
) that we then pass to apply_filters
?
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.
Done in 70a77f2.
plugins/woocommerce/src/Internal/DataStores/Orders/LegacyDataHandler.php
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/DataStores/Orders/LegacyDataHandler.php
Outdated
Show resolved
Hide resolved
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 tests as described and looks good to me -- very nice job.
I did leave a few comments, but I'd consider none of them blocking. Please handle them as you see fit and feel free to re-request a review if you prefer.
…_save_after_meta_change’ filter
Thank you for such detailed testing instructions 🙌 |
Submission Review Guidelines:
Changes proposed in this Pull Request:
In #44281 we introduced the
wp wc hpos backfill
command which allows backfilling an order from/to the HPOS or posts datastore, regardless of which datastore is currently active. This allows merchants to fix one off errors where some data might've been modified outside of the CRUD layer.While this works ok, sometimes there might be a need to sync just some specific metadata or properties from/to either datastore instead of the whole order. This PR introduces
--meta-keys
and--props
arguments towp wc hpos backfill
which do precisely that.While ideally orders would be backfilled in their entirety, this at least provides a last resort for some possible tricky cases.
Closes #41910.
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Note: It shouldn't really matter which datastore is set and we'll test syncing both ways, but this will make testing instructions more straightforward.
wp wc cot sync
to ensure that things are 100% synced between datastores.wp wc hpos diff <order_id>
and confirm that the changes made above are listed as differences.wp wc hpos backfill <order_id> --from=hpos --to=posts --props=billing_first_name
to backfill the billing first name alone.wp wc hpos diff <order_id>
again and confirm that the status is no longer listed as being different. This proves that the--props
arg limited the backfill to only that prop (leaving "status" and meta alone).wp post meta update <order_id> my_posts_meta my_meta_value
.wp wc hpos diff <order_id>
to confirm that the new meta is listed as a difference (with a value only on the 'post' side).my_posts_meta
from the post to the HPOS order by runningwp wc hpos backfill <order_id> --from=posts --to=hpos --meta_keys=my_posts_meta
. Runwp wc hpos diff <order_id>
to confirm that this difference has now disappeared.wp wc hpos backfill <order_id> --from=posts --to=hpos --meta_keys=<meta_key>
. Again, verify that the difference is gone withwp wc hpos diff <order_id>
.wp wc hpos backfill <order_id> --from=posts --to=hpos --props=status
. Verify once more withwp wc hpos diff <order_id>
.Changelog entry
Significance
Type
Message
Comment