Fix schedule sales error #42700

octaedro · 2023-12-12T13:45:33Z

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:

This PR fixes an error when setting a scheduled sale. Before this fix, the product price didn't change after the sale started.

Before

After

Closes #41898.

UPDATED

This PR only fixes the scheduled sales error for simple products. I also opened #42971 to tackle the error we face with scheduled sales in variable products.

How to test the changes in this Pull Request:

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

Create a WooCommerce store with this branch and enable the product editor using WooCommerce > Settings > Advanced > Features.
Go to Products > Add New and create a product simple with a scheduled sale after your current time (one minute or two). Set the end of the scheduled sale one minute after the sale starts.
Press Preview quickly and verify the sale hasn't started yet.
Wait until the scheduled sale starts and verify that the sale price is correct.
Wait until the scheduled sale ends and verify that the price is the regular price again.

Changelog entry

Automatically create a changelog entry from the details below.

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

Comment

github-actions · 2023-12-12T14:06:20Z

Hi @louwie17,

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

github-actions · 2023-12-12T14:12:58Z

Test Results Summary

Commit SHA: 61f3b7a

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 37s
E2E Tests	258	0	0	3	0	261	14m 14s

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.

louwie17

This did fix the error I was seeing previously, but not sure if I like adding a specific condition for variable products just to accommodate for that.
I left an alternative suggestion.
Btw it might be worth checking in with someone from Proton to get their intake as well.

plugins/woocommerce/includes/data-stores/class-wc-product-data-store-cpt.php

octaedro · 2023-12-19T22:02:03Z

Hey @louwie17, I removed the code to clean the cache from this PR (to fix the error only for scheduled sales in simple products) and created this issue containing the information I gathered for variable products.
Could you take another look at this PR?

louwie17

Hmm I guess maybe the get_price_html isn't the only place and we want to change it in the get_price method?

Maybe reverting this change and instead add:

if ( $this->is_on_sale() ) {
			return $this->get_prop( 'sale_price', $context );
		}

here would solve it:

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

Line 269 in 01fb8f2

public function get_price( $context = 'view' ) {

louwie17 · 2023-12-20T14:35:22Z

plugins/woocommerce/tests/legacy/unit-tests/product/data.php

+	/**
+	 * A test ensuring get_price_html works well for scheduled sales for issue #41898.
+	 */
+	public function test_get_price_html_for_scheduled_sales() {


Thanks for adding a test for this :)

octaedro · 2023-12-20T22:19:55Z

Good eyes @louwie17! I like the get_price approach! I added it in the commit 61f3b7a.

louwie17

LGTM 🚀 Tested well, thanks for working on this @octaedro, it was a tricky one.

nerrad · 2023-12-22T20:08:34Z

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

+		if ( $this->is_on_sale() ) {
+			return $this->get_prop( 'sale_price', $context );
+		}


This is a pretty significant change to existing behaviour - especially considering there is an explicit get_sale_price method already available. The public contract for this method has now changed and I suspect could have a ripple effect across all code, referencing it for products that have a sale price.

Please review and ensure this won't impact negatively existing usages.

On the surface, this does look like the right fix, but looking at code like this the price field for the product is supposed to be updated to the sale price in the db. I'd be wary about shipping this change as it looks like it might just be a bandaid over the real problem (and might introduce problems of its own).

To sum up my previous two comments:

The code changed in this PR has been present for 7 years. Bugs can be in code that old, but $product->get_price is a pretty hot code path so we'd want to make sure any changes are not affecting the expected return value.

The code added is an attempt to make sure get_price is returning the sale price (overriding what is in the db) if the product is on sale (to fix the apparent bug with scheduled sales not updating price).

I'm concerned it's a bandaid over the actual problem, given the sale price is supposed to be updated in the DB on schedule.

This fix on the surface looks like a good fix, but still changes the behaviour for the getter which may already be a bug accounted for in code in the wild. So the fix may have some unexpected side-effects if released as is (besides not addressing what may potentially be the real bug).

I'm concerned this fix is a shallow fix without giving appropriate thought to the ramifications of the change nor accounting for what the real bug might be (for instance, why isn't the price updated in the DB?).

We've created a #43094 as we are considering reverting the change.

This reverts commit 9b800aa.

Revert "Fix schedule sales error (#42700)" This reverts commit 9b800aa.

…s documentation (#43055) * Update woocommerce.com URLs in documentation and code files * Add changelog * Fix github repository link in extend-rest-api-add-custom-fields.md Co-authored-by: Albert Juhé Lluveras <[email protected]> * Add github reporter (#42974) * Add github reporter * Add changelog --------- Co-authored-by: Jon Lane <[email protected]> * Fix product task redirect to support grouped and external products (#43051) * Rearrange product redirection logic to better accept grouped and external produc types * Add changelog * Modify feedback modal actions (#43005) * Adapt feedback modal actions * Add changelogs * Modify comments * Fix tests * Fix test * Update class-wc-gateway-bacs.php (#43054) * Update class-wc-gateway-bacs.php Fix typo in textdomain * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <[email protected]> * [Product Block Editor]: Add `Linked product` tab (#43009) * add linked-products to group IDs * add Linked Products tab * tweak hideConditions condition * changelog * fix typo in doc comment * Introduce a product type selection within the new experience (#41823) * Create a relation between the product type and the product block template * Add 'patterns' to name the kind of products that can be created for a specific template * Resolve template using its id as a template query param * Rename ProductEditPattern to ProductTemplate * Rename get_patterns hook to woocommerce_product_editor_get_product_templates * Return the list of templates to the client * Set layout template events as array * Register the layout template based on the product template or the post type in case of product variations * Registering non supported product types * Create and register the woocommerce/product-details-section-description block * Add the product type to the section description * Create product type selector * Fix menu item style * Highlight selected menu item * Set the selected product template * Set product template title to lowercase in the content description * Rename blocks by blockTemplates under the AbstractBlockTemplate class * Rename to woocommerce_product_editor_product_templates filter * Remove product_template_ prefix from the supported_product_types map * Rename get_formatted to to_JSON and convert the props to client side like * Refactor get_product_templates * Fix icon resolution * Add a confirmation modal for unsupported product templates * Add changelog files * Remove product types using for testing * Fix redirection when changing to a non supported product template * Set the change button state to busy when it is saving the product * Fix php linter errors * Fix rebase conflict * Move ProductTemplate to Automattic\WooCommerce\Admin\Features\ProductBlockEditor namespace * Add the to_json definition to the BlockTemplateInterface * Create default product template by custom product type if it does not have a template associated yet * Fix some comments and product template creation validation * Add support to load the product template icon from an external resource * Fix php linter * Fix the changelog description * [Experimental] Interactivity Dropdown multi-select mode, ratings filter and introduce each directive (#42981) --------- Co-authored-by: David Arenas <[email protected]> * Introduce the transient files engine (#42877) Co-authored-by: Corey McKrill <[email protected]> * Change marketplace install API request to POST instead of GET (#43033) * Change marketplace install API to using POST instead of GET * Fix linting error * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <[email protected]> * Prep trunk for 8.6 cycle (#43021) Prep trunk for 8.6 cycle with version bump to 8.6.0-dev Co-authored-by: WooCommerce Bot <[email protected]> * Add Playwright tests for All Reviews, Reviews by Product and Reviews by Category blocks (#42903) * Remove Reviews blocks Puppeteer tests * Minor code cleanup * Typos * Create publishAndVisitPost() editor util * Fix subcategories when importing products in Playwright and add reviews * Add Reviews blocks tests in Playwright * More typos * Add changefile(s) from automation for the following project(s): woocommerce-blocks * Create a 'reviews' object in data.ts so we can store reviews data in one single place * Update test so instead of creating a new post in each test, we go to the already-created post * Add source comments to reviews data to match it with the script --------- Co-authored-by: github-actions <[email protected]> * Release: Remove 8.5 change files (#43022) Delete changelog files from 8.5 release Co-authored-by: WooCommerce Bot <[email protected]> Co-authored-by: Alex López <[email protected]> * Delete changelog files based on PR 43033 (#43079) Delete changelog files for 43033 Co-authored-by: WooCommerce Bot <[email protected]> * Delete changelog files based on PR 43051 (#43081) Delete changelog files for 43051 Co-authored-by: WooCommerce Bot <[email protected]> * Interactive Price Filter: use `context` instead of `state` (#42980) * feat: use context instead of state * fix: temporary move the context to inner element for diffing to work * fix: update context before navigation for optimistic UI * Load google analytics gtag script asynchronously in WooCommerce Blocks (#43040) Co-authored-by: github-actions <[email protected]> * set WOOCOMMERCE_BLOCKS_PHASE to 1 for the production build (#43074) * set WOOCOMMERCE_BLOCKS_PHASE to 1 for the production build * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <[email protected]> * Revert "Fix schedule sales error" (#43094) Revert "Fix schedule sales error (#42700)" This reverts commit 9b800aa. * [Product Block Editor]: add Linked product sections. First approach. (#43013) * add Linked products, Upsell section * changelog * add Cross-lens section * add links to the Upsell sections * changelog * fix lint issues * fix lint issus * fix linting issue :-| * check whether the linked product group is defined * [Product Block Editor]: introduce ShoppingBags component (#43042) * add ShoppingBags component * Add ShoppingBag story * changelog * Fix: Collection data being leaked between Collection Filters blocks (#43044) * fix: CYS - change heading color (#43076) * fix: CYS - change heading color * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <[email protected]> Co-authored-by: Patricia Hillebrandt <[email protected]> * Delete changelog files based on PR 43074 (#43118) Delete changelog files for 43074 Co-authored-by: WooCommerce Bot <[email protected]> * [Product Block Editor]: fix feature flag to hide the Linked products (#43119) * fix flag to hide/show product editor * changelog * Add changelog --------- Co-authored-by: Albert Juhé Lluveras <[email protected]> Co-authored-by: Jonathan Lane <[email protected]> Co-authored-by: Jon Lane <[email protected]> Co-authored-by: louwie17 <[email protected]> Co-authored-by: Fernando Marichal <[email protected]> Co-authored-by: Marc Guay <[email protected]> Co-authored-by: github-actions <[email protected]> Co-authored-by: Damián Suárez <[email protected]> Co-authored-by: Maikel David Pérez Gómez <[email protected]> Co-authored-by: Sam Seay <[email protected]> Co-authored-by: Néstor Soriano <[email protected]> Co-authored-by: Corey McKrill <[email protected]> Co-authored-by: Kyle Nel <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: WooCommerce Bot <[email protected]> Co-authored-by: Alex López <[email protected]> Co-authored-by: Tung Du <[email protected]> Co-authored-by: Thomas Roberts <[email protected]> Co-authored-by: Luigi Teschio <[email protected]> Co-authored-by: Patricia Hillebrandt <[email protected]>

Revert "Fix schedule sales error (#42700)" This reverts commit 9b800aa.

…s documentation (#43055) * Update woocommerce.com URLs in documentation and code files * Add changelog * Fix github repository link in extend-rest-api-add-custom-fields.md Co-authored-by: Albert Juhé Lluveras <[email protected]> * Add github reporter (#42974) * Add github reporter * Add changelog --------- Co-authored-by: Jon Lane <[email protected]> * Fix product task redirect to support grouped and external products (#43051) * Rearrange product redirection logic to better accept grouped and external produc types * Add changelog * Modify feedback modal actions (#43005) * Adapt feedback modal actions * Add changelogs * Modify comments * Fix tests * Fix test * Update class-wc-gateway-bacs.php (#43054) * Update class-wc-gateway-bacs.php Fix typo in textdomain * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <[email protected]> * [Product Block Editor]: Add `Linked product` tab (#43009) * add linked-products to group IDs * add Linked Products tab * tweak hideConditions condition * changelog * fix typo in doc comment * Introduce a product type selection within the new experience (#41823) * Create a relation between the product type and the product block template * Add 'patterns' to name the kind of products that can be created for a specific template * Resolve template using its id as a template query param * Rename ProductEditPattern to ProductTemplate * Rename get_patterns hook to woocommerce_product_editor_get_product_templates * Return the list of templates to the client * Set layout template events as array * Register the layout template based on the product template or the post type in case of product variations * Registering non supported product types * Create and register the woocommerce/product-details-section-description block * Add the product type to the section description * Create product type selector * Fix menu item style * Highlight selected menu item * Set the selected product template * Set product template title to lowercase in the content description * Rename blocks by blockTemplates under the AbstractBlockTemplate class * Rename to woocommerce_product_editor_product_templates filter * Remove product_template_ prefix from the supported_product_types map * Rename get_formatted to to_JSON and convert the props to client side like * Refactor get_product_templates * Fix icon resolution * Add a confirmation modal for unsupported product templates * Add changelog files * Remove product types using for testing * Fix redirection when changing to a non supported product template * Set the change button state to busy when it is saving the product * Fix php linter errors * Fix rebase conflict * Move ProductTemplate to Automattic\WooCommerce\Admin\Features\ProductBlockEditor namespace * Add the to_json definition to the BlockTemplateInterface * Create default product template by custom product type if it does not have a template associated yet * Fix some comments and product template creation validation * Add support to load the product template icon from an external resource * Fix php linter * Fix the changelog description * [Experimental] Interactivity Dropdown multi-select mode, ratings filter and introduce each directive (#42981) --------- Co-authored-by: David Arenas <[email protected]> * Introduce the transient files engine (#42877) Co-authored-by: Corey McKrill <[email protected]> * Change marketplace install API request to POST instead of GET (#43033) * Change marketplace install API to using POST instead of GET * Fix linting error * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <[email protected]> * Prep trunk for 8.6 cycle (#43021) Prep trunk for 8.6 cycle with version bump to 8.6.0-dev Co-authored-by: WooCommerce Bot <[email protected]> * Add Playwright tests for All Reviews, Reviews by Product and Reviews by Category blocks (#42903) * Remove Reviews blocks Puppeteer tests * Minor code cleanup * Typos * Create publishAndVisitPost() editor util * Fix subcategories when importing products in Playwright and add reviews * Add Reviews blocks tests in Playwright * More typos * Add changefile(s) from automation for the following project(s): woocommerce-blocks * Create a 'reviews' object in data.ts so we can store reviews data in one single place * Update test so instead of creating a new post in each test, we go to the already-created post * Add source comments to reviews data to match it with the script --------- Co-authored-by: github-actions <[email protected]> * Release: Remove 8.5 change files (#43022) Delete changelog files from 8.5 release Co-authored-by: WooCommerce Bot <[email protected]> Co-authored-by: Alex López <[email protected]> * Delete changelog files based on PR 43033 (#43079) Delete changelog files for 43033 Co-authored-by: WooCommerce Bot <[email protected]> * Delete changelog files based on PR 43051 (#43081) Delete changelog files for 43051 Co-authored-by: WooCommerce Bot <[email protected]> * Interactive Price Filter: use `context` instead of `state` (#42980) * feat: use context instead of state * fix: temporary move the context to inner element for diffing to work * fix: update context before navigation for optimistic UI * Load google analytics gtag script asynchronously in WooCommerce Blocks (#43040) Co-authored-by: github-actions <[email protected]> * set WOOCOMMERCE_BLOCKS_PHASE to 1 for the production build (#43074) * set WOOCOMMERCE_BLOCKS_PHASE to 1 for the production build * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <[email protected]> * Revert "Fix schedule sales error" (#43094) Revert "Fix schedule sales error (#42700)" This reverts commit 9b800aa. * [Product Block Editor]: add Linked product sections. First approach. (#43013) * add Linked products, Upsell section * changelog * add Cross-lens section * add links to the Upsell sections * changelog * fix lint issues * fix lint issus * fix linting issue :-| * check whether the linked product group is defined * [Product Block Editor]: introduce ShoppingBags component (#43042) * add ShoppingBags component * Add ShoppingBag story * changelog * Fix: Collection data being leaked between Collection Filters blocks (#43044) * fix: CYS - change heading color (#43076) * fix: CYS - change heading color * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <[email protected]> Co-authored-by: Patricia Hillebrandt <[email protected]> * Delete changelog files based on PR 43074 (#43118) Delete changelog files for 43074 Co-authored-by: WooCommerce Bot <[email protected]> * [Product Block Editor]: fix feature flag to hide the Linked products (#43119) * fix flag to hide/show product editor * changelog * Add changelog --------- Co-authored-by: Albert Juhé Lluveras <[email protected]> Co-authored-by: Jonathan Lane <[email protected]> Co-authored-by: Jon Lane <[email protected]> Co-authored-by: louwie17 <[email protected]> Co-authored-by: Fernando Marichal <[email protected]> Co-authored-by: Marc Guay <[email protected]> Co-authored-by: github-actions <[email protected]> Co-authored-by: Damián Suárez <[email protected]> Co-authored-by: Maikel David Pérez Gómez <[email protected]> Co-authored-by: Sam Seay <[email protected]> Co-authored-by: Néstor Soriano <[email protected]> Co-authored-by: Corey McKrill <[email protected]> Co-authored-by: Kyle Nel <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: WooCommerce Bot <[email protected]> Co-authored-by: Alex López <[email protected]> Co-authored-by: Tung Du <[email protected]> Co-authored-by: Thomas Roberts <[email protected]> Co-authored-by: Luigi Teschio <[email protected]> Co-authored-by: Patricia Hillebrandt <[email protected]>

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Dec 12, 2023

octaedro self-assigned this Dec 12, 2023

octaedro added Bug The issue is a confirmed bug. priority: high Product/Inventory Management Issues related to product or product page. labels Dec 12, 2023

octaedro requested a review from a team December 12, 2023 14:05

octaedro marked this pull request as ready for review December 12, 2023 16:34

octaedro force-pushed the fix/41898_schedule_sale_error branch from 268ccfe to 2793117 Compare December 13, 2023 13:46

octaedro added 3 commits December 14, 2023 07:40

Fix schedule sales error

bd0a96e

Add changelog

fb6651b

Fix lint

e22cce9

octaedro force-pushed the fix/41898_schedule_sale_error branch from 7c91d8a to e22cce9 Compare December 14, 2023 10:42

octaedro removed the request for review from a team December 15, 2023 12:47

Fix variable product error

8a1d228

louwie17 requested changes Dec 18, 2023

View reviewed changes

plugins/woocommerce/includes/data-stores/class-wc-product-data-store-cpt.php Outdated Show resolved Hide resolved

octaedro added 5 commits December 18, 2023 15:06

Add clear cache

de03fd1

Remove sale price fix from data-store-cpt

37d4943

Fix comment

fc0ea87

Remove cache cleaning

01fb8f2

Add unit tests

97b11f6

octaedro mentioned this pull request Dec 19, 2023

Scheduled sale error in variable products #42971

Open

octaedro requested a review from louwie17 December 19, 2023 22:02

Fix lint

63d3cfa

louwie17 requested changes Dec 20, 2023

View reviewed changes

Modify get_price

61f3b7a

octaedro requested a review from louwie17 December 20, 2023 22:19

louwie17 approved these changes Dec 21, 2023

View reviewed changes

octaedro merged commit 9b800aa into trunk Dec 21, 2023

octaedro deleted the fix/41898_schedule_sale_error branch December 21, 2023 13:10

github-actions bot added this to the 8.6.0 milestone Dec 21, 2023

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Dec 21, 2023

nerrad reviewed Dec 22, 2023

View reviewed changes

retrofox added a commit that referenced this pull request Dec 22, 2023

Revert "Fix schedule sales error (#42700)"

9ab3233

This reverts commit 9b800aa.

retrofox mentioned this pull request Dec 22, 2023

Revert "Fix schedule sales error" #43094

Merged

retrofox added a commit that referenced this pull request Dec 22, 2023

Revert "Fix schedule sales error" (#43094)

7f2b1c4

Revert "Fix schedule sales error (#42700)" This reverts commit 9b800aa.

thealexandrelara pushed a commit that referenced this pull request Dec 26, 2023

Revert "Fix schedule sales error" (#43094)

bd3589c

Revert "Fix schedule sales error (#42700)" This reverts commit 9b800aa.

opr pushed a commit that referenced this pull request Jan 3, 2024

Revert "Fix schedule sales error" (#43094)

a96964d

Revert "Fix schedule sales error (#42700)" This reverts commit 9b800aa.

octaedro mentioned this pull request Jul 19, 2024

Schedule sale error #41898

Closed

5 tasks

Fix schedule sales error #42700

Fix schedule sales error #42700

Uh oh!

Conversation

octaedro commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submission Review Guidelines:

Changes proposed in this Pull Request:

UPDATED

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

Uh oh!

github-actions bot commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

octaedro commented Dec 19, 2023

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

louwie17 Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

octaedro commented Dec 20, 2023

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

nerrad Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

nerrad Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

nerrad Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

retrofox Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

octaedro commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading