Skip to content
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

Prevent backup post from being deleted when HPOS is authoritative #45330

Merged
merged 5 commits into from Mar 8, 2024

Conversation

jorgeatorres
Copy link
Member

@jorgeatorres jorgeatorres commented Mar 5, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

When HPOS is authoritative, calling wp_delete_post() on a backup post also removes the HPOS order. By default, this shouldn't happen, but it can happen when, for example, sync is temporarily disabled and the backup post is kept in the trash, which will get autoremoved once WP empties the trash (and thus also deleting the HPOS order).

There are other implications as a few caches and stats are updated when an order is removed, and a post with shop_order as type would trigger this even when HPOS is set as datastore and the "true" order hasn't been deleted.

While those are some edge cases, they could happen. This PR short-circuits wp_delete_post() so that when HPOS is authoritative, any attempts to delete the backup post are not successful. This only applies to non placeholder posts, as placeholder posts won't trigger the cascade of operations that might be problematic here.

It's worth mentioning that our sync code doesn't consider missing backup posts as truly missing and will let you switch datastores freely, which can result in an incorrect "view" of the orders on your site. That's addressed in #45332, but this PR will at least make sure that can't happen again.

Closes #42746.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Make sure HPOS is configured as datastore and that compatibility mode is enabled in WC > Settings > Features > Advanced.
  2. Ensure your test site has a few orders. Create as necessary.
  3. Choose an order ID and run wp eval "wp_delete_post( <ORDER_ID> );".
  4. Confirm that the order still exists on the HPOS side and, in particular, in the order stats table (which proves that the post deletion hooks did not run):
     wp db query "SELECT * FROM $(wp db prefix)wc_order_stats WHERE order_id = <ORDER_ID>"
    
  5. Change datastore to posts in WC > Settings > Features > Advanced.
  6. Run wp eval "wp_delete_post( <ORDER_ID> );" again and confirm that the order has been removed, and also from the HPOS tables (and stats table):
     wp db query "SELECT * FROM $(wp db prefix)wc_orders WHERE id = <ORDER_ID>"
     wp db query "SELECT * FROM $(wp db prefix)wc_order_stats WHERE order_id = <ORDER_ID>"
    

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 5, 2024
Copy link
Contributor

github-actions bot commented Mar 5, 2024

Test Results Summary

Commit SHA: 2a39b3b

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 38s
E2E Tests339001203517m 34s

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.

@jorgeatorres jorgeatorres marked this pull request as ready for review March 7, 2024 22:23
@jorgeatorres jorgeatorres requested review from a team and coreymckrill and removed request for a team March 7, 2024 22:24
Copy link
Contributor

github-actions bot commented Mar 7, 2024

Hi @coreymckrill,

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:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

1 similar comment
Copy link
Contributor

github-actions bot commented Mar 7, 2024

Hi @coreymckrill,

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:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Collaborator

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good, tests well. I also tried deleting some non-order posts to make sure this filter doesn't cause disruption for other post types.

@coreymckrill coreymckrill merged commit f229c3d into trunk Mar 8, 2024
41 checks passed
@coreymckrill coreymckrill deleted the fix/42746 branch March 8, 2024 00:24
@github-actions github-actions bot added this to the 8.8.0 milestone Mar 8, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Mar 8, 2024
@Stojdza Stojdza added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Mar 8, 2024
Konamiman pushed a commit that referenced this pull request Mar 13, 2024
…5330)

This PR short-circuits wp_delete_post() so that when HPOS is authoritative, any attempts to delete the backup post are not successful (which could also end up deleting the HPOS order). This only applies to non placeholder posts, as placeholder posts won't trigger the cascade of operations that might be problematic here.

Fixes #42746
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve HPOS sync of deleted or missing orders
3 participants