Fix for product meta lookup table incorrectly marking a product as onsale Closes #43008

[pintend]

Determine if a product is on sale based on regular price, which is how its done in the WC_Product class

woocommerce/plugins/woocommerce/includes/abstracts/abstract-wc-product.php

Line 1634 in 7122669

if ( '' !== (string) $this->get_sale_price( $context ) && $this->get_regular_price( $context ) > $this->get_sale_price( $context ) ) {

Currently $product->is_on_sale() == false when _regular_price and _sale_price are the same, but the product lookup table ignores the _regular_price and only checks that _price and _sale_price are the same.

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Closes #43008 .

How to test the changes in this Pull Request:

1. Create a simple test product in WC > Products. Make sure it's not "on sale" and take note of its ID.
2. Force an update of the meta lookup table by running wp eval "wc_update_product_lookup_tables_column( 'onsale' );".
3. Confirm the product is not on sale in the lookup table:

   > wp db query "SELECT onsale FROM $(wp db prefix)wc_product_meta_lookup WHERE product_id = <product_id>;"
   +--------+
   | onsale |
   +--------+
   |      0 |
   +--------+
   

4. Now go back and edit the product and set a sale price.
5. Confirm that the product appears on sale in the lookup table:

   > wp db query "SELECT onsale FROM $(wp db prefix)wc_product_meta_lookup WHERE product_id = <product_id>;"
   +--------+
   | onsale |
   +--------+
   |      1 |
   +--------+
   

6. To simulate the product having regular_price, sale_price and price all set to the same value run the following command:

   wp post meta update <product_id> _sale_price `wp post meta get <product_id> _regular_price`
   wp post meta update <product_id> _price `wp post meta get <product_id> _regular_price`

   Not that this is only to expedite testing, but this is in fact possible via the admin UI if one disables frontend validation (for example, by disabling JS).
7. Force (again) an update of the meta lookup table by running wp eval "wc_update_product_lookup_tables_column( 'onsale' );".
8. Confirm that the product is not marked as being on sale in the lookup tables:

   > wp db query "SELECT onsale FROM $(wp db prefix)wc_product_meta_lookup WHERE product_id = <product_id>;"
   +--------+
   | onsale |
   +--------+
   |      0 |
   +--------+
   

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

Use regular_price to determine if product is not sale and don't rely only on price for product_meta_lookup

Comment

[github-actions]

Hi ,

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

[jorgeatorres]

Hi @pintend!

Thanks for your contribution 💯. I left some feedback regarding a tiny change that we would need to make in order for this to work as intended.

Let me know what you think!

[pintend]

did i miss that in the commit? i had it in my test query AND CAST(meta3.meta_value AS DECIMAL(10, 2)) > CAST(meta2.meta_value AS DECIMAL(10, 2))

[jorgeatorres]

@pintend: You did use DECIMAL(10, 2) in the testing instructions, but the code itself uses a placeholder for the decimal places (%d) which is correct, but then you need to provide the actual value.

woocommerce/plugins/woocommerce/includes/wc-product-functions.php

Lines 1571 to 1578 in 0fd0c8e

	CAST( meta1.meta_value AS DECIMAL ) >= 0
	AND CAST( meta2.meta_value AS CHAR ) != ''
	AND CAST( meta1.meta_value AS DECIMAL( 10, %d ) ) = CAST( meta2.meta_value AS DECIMAL( 10, %d ) )
	AND CAST( meta3.meta_value AS DECIMAL( 10, %d ) ) > CAST( meta2.meta_value AS DECIMAL( 10, %d ) )
	, 1, 0 )
	",
	$decimals,
	$decimals

[jorgeatorres]

Hey @pintend,

Thanks for making the changes. I was testing this again and now I'm wondering where the original bug came from. Using the set_price(), set_regular_price() and set_sale_price() functions I can't set a sale price that is equal to the price/regular price. In all cases, the price property updates to the proper amount, which is what the lookup table uses (as you mentioned).

We recently merged fixes related to product sales, so something might've changed.

Is there a way to reproduce this that doesn't involve changing the metadata directly in the database? This is not necessarily supported. Hardening the lookup update might still make sense, but I'd like to understand if there's something else we're missing.

[pintend]

@jorgeatorres: The issue started when a 3rd party plugin created and updated product records directly in the db, although the issue should really be fixed in that plugin (which I don't have control over) I don't see the harm "fixing" it here as well, especially when in the application this is the method it uses to determine if its on sale... might as well keep the logic consistent.

Although the functions dont allow a developer to create this issue, some developers might and do go around those helper functions. adding this extra check should be a safe change that protects from that even if its "useless" for most cases

if the sale_price is not blank and regular_price is more than the sale_price

woocommerce/plugins/woocommerce/includes/abstracts/abstract-wc-product.php

Line 1634 in 7122669

if ( '' !== (string) $this->get_sale_price( $context ) && $this->get_regular_price( $context ) > $this->get_sale_price( $context ) ) {

if the sale_price is not blank and the price and sale_price are equal

woocommerce/plugins/woocommerce/includes/wc-product-functions.php

Line 1572 in 7122669

AND CAST( meta1.meta_value AS DECIMAL( 10, %d ) ) = CAST( meta2.meta_value AS DECIMAL( 10, %d ) )

my change:

if the sale_price is not blank and regular_price is more than the sale_price and the price and sale_price are equal

woocommerce/plugins/woocommerce/includes/wc-product-functions.php

Line 1574 in 7263053

AND CAST( meta3.meta_value AS DECIMAL( 10, %d ) ) > CAST( meta2.meta_value AS DECIMAL( 10, %d ) )

it make it more consistent and prevents the issue from other plugins

[jorgeatorres]

LGTM. Thanks for your patience here @pintend and for this fix. It works great.

FYI I edited the first comment by adding some testing instructions to make it easier for our team to confirm this is working correctly.