-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Update biling and shipping address indexes. #35121
Conversation
Test Results SummaryCommit SHA: 99436b3
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. |
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 👍
I did notice a problem while testing—however it is not the result of your change, it was an oversight in the original PR that added this system to HPOS, so let me know if you prefer we follow-up via a separate issue/PR.
What I observed was that instead of updating the _<TYPE>_address_index
fields (or creating that entry if it does not already exist), what we actually seem to be doing is add a new meta record each time.
You can see this if you repeat point 2 in your testing instructions a few times. In the database, we end up with something like:
id | order_id | meta_key | meta_value |
---|---|---|---|
12345 | 100 | _billing_address_index | 123 My Street, London |
12346 | 100 | _billing_address_index | 123 My Street, Birmingham |
12347 | 100 | _billing_address_index | 345 Some Avenue, Glasgow |
Apart from the fact this is adding unnecessary rows, it arguably reduces the accuracy of search (using the above example, I can still surface order 100 by searching for London, even though that has since been corrected to Glasgow).
The problem, I believe, is that calls to WC_Data::update_meta_data()
always add unless the existing meta ID is also specified, which we aren't currently doing.
Hope the above makes sense. We can go ahead and merge (and one of us can fix the above separately), or we can address here if that's expedient.
I see, I am going to merge this, and open another issue to track preventing duplicates on updates. |
Hi @vedanshujain, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
Follow up PR (WIP) #35192 |
* Update biling and shipping address indexes. * Add changelog. * Code standard fixes. * Add unit test for search after update. * PHPCS fixes.
* Update biling and shipping address indexes. (#35121) * Update biling and shipping address indexes. * Add changelog. * Code standard fixes. * Add unit test for search after update. * PHPCS fixes. * Prep for cherry pick 35121 Co-authored-by: Vedanshu Jain <vedanshu.jain.2012@gmail.com> Co-authored-by: WooCommerce Bot <no-reply@woocommerce.com>
All Submissions:
Changes proposed in this Pull Request:
Looks like we were not saving _billing_address_index and _shipping_address_index metadata on update (we were doing it on create, though). This PR addresses the issue by updating the index metadata on updates as well.
Closes #34989 .
How to test the changes in this Pull Request:
Note that after #35150 , these meta fields won't be needed anymore.
Other information:
pnpm --filter=<project> run changelog add
?FOR PR REVIEWER ONLY: