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
Overwrite clone method to prevent duplicate data when saving a clone. #37313
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## trunk #37313 +/- ##
==========================================
- Coverage 45.8% 45.8% -0.0%
Complexity 17196 17196
==========================================
Files 429 429
Lines 64911 64911
==========================================
- Hits 29703 29702 -1
- Misses 35208 35209 +1
|
Test Results SummaryCommit SHA: 3a25942
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. |
b78afe7
to
56d4bbc
Compare
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 looks good to me. Thank you @vedanshujain!
I'm approving, but I can't merge as the e2e tests are failing (though seems unrelated). I even tried updating this branch with the latest trunk
and that didn't help. Something is broken with our e2e tests atm 😬.
56d4bbc
to
3a25942
Compare
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.
…#37313) * Add unit test to simulate duplicate meta insert. * Overwrite clone method to prevent duplicate datq when saving a clone. * Add changelog. * Coding standard fixes. * Fix phpcs --------- Co-authored-by: Jorge A. Torres <jorge.torres@automattic.com>
…#37313) * Add unit test to simulate duplicate meta insert. * Overwrite clone method to prevent duplicate datq when saving a clone. * Add changelog. * Coding standard fixes. * Fix phpcs --------- Co-authored-by: Jorge A. Torres <jorge.torres@automattic.com>
* Overwrite clone method to prevent duplicate data when saving a clone. (#37313) * Add unit test to simulate duplicate meta insert. * Overwrite clone method to prevent duplicate datq when saving a clone. * Add changelog. * Coding standard fixes. * Fix phpcs --------- Co-authored-by: Jorge A. Torres <jorge.torres@automattic.com> * Prep for cherry pick 37313 --------- Co-authored-by: Vedanshu Jain <vedanshu.jain.2012@gmail.com> Co-authored-by: Jorge A. Torres <jorge.torres@automattic.com> Co-authored-by: WooCommerce Bot <no-reply@woocommerce.com>
All Submissions:
Changes proposed in this Pull Request:
This method overwrites the base class's clone method to make it a no-op. In the base class
WC_Data
, we are unsetting the meta_id to clone. It seems like this was done to avoid conflicting the metadata when duplicating products (see #14118). However, doing that does not seem necessary for orders.In fact, when we do that for orders, we lose the capability to clone orders with custom metadata by caching plugins. This is because, when we clone an order object for caching, it will clone the metadata without the ID. Unfortunately, when this cached object with nulled meta ID is retrieved, WC_Data will consider it as a new meta and will insert it as a new meta-data causing duplicates if the object is saved.
Eventually, we should move away from overwriting the
__clone
method in the base class itself, since it's easily possible to still duplicate the product without having to hook into the__clone
method.How to test the changes in this Pull Request:
Unfortunately, this is not easy to test from UI since this needs cache setting unsettling. On a WP shell, run these commands:
For an order with ID XX, with HPOS enabled:
Other information:
pnpm --filter=<project> changelog add
?FOR PR REVIEWER ONLY: