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.

Sign up for GitHub

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

Jump to bottom

Prevent adding global_unique_id to old schema versions #49472

Merged
merged 6 commits into from
Jul 18, 2024
Merged

Prevent adding global_unique_id to old schema versions #49472

merged 6 commits into from
Jul 18, 2024

Conversation

nathanss
Copy link
Contributor

@nathanss nathanss commented Jul 12, 2024 •
edited
Loading

Submission Review Guidelines:

Changes proposed in this Pull Request:

This adds a guard to ensure the checkout continues to run successfully in case the database update fails for some reason, during WooCommerce update.

How to test the changes in this Pull Request:

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

I couldn't think of a way of testing this without database access, as it's a very specific scenario to reproduce.

  1. Go to WooCommerce > Status > Tools > Verify base database tables and click on 'Verify database'
  2. Execute a checkout for a product successfully
  3. Using WP cli, run the command wp option update woocommerce_schema_version 430 to update the option to 430.
  4. Using a database tool, delete the global_unique_id column from the wp_wc_product_meta_lookup table
  5. Execute a checkout for a product successfully again

At the end of testing, make sure to run Verify database again.

Changelog entry

  • Automatically create a changelog entry from the details below.
  • This Pull Request does not require a changelog entry. (Comment required below)
Changelog Entry Details

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

Changelog Entry Comment

Comment

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jul 12, 2024
@nathanss nathanss self-assigned this Jul 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 12, 2024 •
edited
Loading

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

@PanosSynetos PanosSynetos self-requested a review July 16, 2024 06:03
@github-actions GitHub Actions
Copy link
Contributor

Hi @PanosSynetos,

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

PanosSynetos
PanosSynetos approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@PanosSynetos PanosSynetos left a comment

Choose a reason for hiding this comment

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

Manually tested and we're good - No fatal errors and I can checkout.

Approving.

plugins/woocommerce/src/Admin/API/Options.php Outdated
@@ -223,6 +223,7 @@ public static function get_default_option_permissions() {
'woocommerce_private_link',
'woocommerce_share_key',
'woocommerce_show_lys_tour',
'woocommerce_schema_version',
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why do we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this here in order for it to have edit permission through WC Test Helper, so that the testing instructions can be executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, then we could remove this and not commit it - instead we could update the value with wp cli (that's how I actually did it)

wp option update woocommerce_schema_version 430

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I'll update the testing instructions.

vedanshujain
vedanshujain approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

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

LGTM! Tested by switching to branch with WC version already set to 9.2 so that update routine won't run. Verified that schema is still at 430, and was able to complete the checkout.

Repeated the same test with deleting wc version first so that update routine is triggered. Verified that update routine was triggered and new column was added on next request. Was able to complete a checkout.

@vedanshujain vedanshujain merged commit f7153da into trunk Jul 18, 2024
24 checks passed
@vedanshujain vedanshujain deleted the fix/product-data-store-guard branch July 18, 2024 13:41
@github-actions github-actions bot added this to the 9.2.0 milestone Jul 18, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Jul 18, 2024
thealexandrelara pushed a commit that referenced this pull request Jul 18, 2024
@nathanss @thealexandrelara
Prevent adding global_unique_id to old schema versions (#49472)
7b094c8 
* Update changelog from PR that added new field to product lookup table

* Update db_version variable and use it to prevent adding global_unique_id when the lookup table was not yet updated

* Add woocommerce_schema_version to get_default_option_permissions

* Add changelog

* Remove schema version from default_option_permissions
chihsuan pushed a commit that referenced this pull request Jul 19, 2024
@nathanss @chihsuan
Prevent adding global_unique_id to old schema versions (#49472)
be259c0 
* Update changelog from PR that added new field to product lookup table

* Update db_version variable and use it to prevent adding global_unique_id when the lookup table was not yet updated

* Add woocommerce_schema_version to get_default_option_permissions

* Add changelog

* Remove schema version from default_option_permissions
@rodelgc rodelgc added needs: internal testing Indicates if the PR requires further testing conducted by Solaris 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 Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PanosSynetos PanosSynetos PanosSynetos approved these changes

@vedanshujain vedanshujain vedanshujain approved these changes

Assignees

@nathanss nathanss

Labels
needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Milestone
9.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@nathanss @PanosSynetos @vedanshujain @rodelgc