Recalculate order total cost value when it transitions to "complete" #61432
Conversation
Also, add a "cost values are provisional" text in the order details page in admin when the order status is "processing", "pending payment" or "on hold".
github-actions
bot
added
the
plugin: woocommerce
woocommercebot
requested review from
a team and
joshuatf
and removed request for
a team
Testing GuidelinesHi @joshuatf , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
📝 WalkthroughWalkthroughAdds WC_Abstract_Order::costs_are_provisional(), a provisional-costs admin UI notice, recalculates/persists COGS when an order transitions to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin/User
participant Order as WC_Order
participant COGS as COGS Calculator
participant Store as Persistence
Admin->>Order: Change status -> completed
Note over Order: status_transition handler
Order->>Order: Detect transition to "completed"
alt COGS enabled & present && filter allows
Order->>COGS: calculate_cogs_total_value()
COGS-->>Order: updated COGS total
Order->>Store: save()
Store-->>Order: persisted
else No recalculation
Note over Order: No COGS recalculation
end
sequenceDiagram
autonumber
participant UI as Admin Order Items UI
participant AOrder as WC_Abstract_Order
participant Filter as woocommerce_order_costs_are_provisional
UI->>AOrder: costs_are_provisional()
AOrder->>AOrder: Check COGS enabled & has COGS
AOrder->>AOrder: Check status ∈ {pending, processing, on-hold}
AOrder->>Filter: apply_filters(...)
Filter-->>AOrder: boolean
AOrder-->>UI: return provisional? (true/false)
alt provisional
UI->>UI: Render provisional cost notice
else not provisional
UI->>UI: No notice
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
plugins/woocommerce/changelog/pr-61432 (1)
1-4: Changelog phrasing nitConsider mentioning COGS explicitly for clarity and match project terminology.
-Recalculate order total cost value when it transitions to 'completed' +Recalculate order Cost of Goods Sold (COGS) total when an order transitions to 'completed'.plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php (1)
750-758: Silence PHPMD unused parameter in filter closureRename the unused parameter to underscore-prefixed to satisfy PHPMD without suppressions.
- function ( $costs_are_provisional, $order ) { + function ( $_costs_are_provisional, $order ) { return true; // Force all orders to have provisional costs. },Also, add declare(strict_types=1); at the file top for consistency with test guidelines. As per coding guidelines
plugins/woocommerce/includes/admin/meta-boxes/views/html-order-items.php (1)
311-315: Minor UI polish: avoid inline styles, reuse classesReplace inline style with a small class to keep markup clean and easier to theme/override.
- <div style="margin-top: 8px; text-align: right;"> + <div class="wc-order-cogs-provisional"> <span class="description"><?php echo wc_help_tip( __( 'Definitive cost values will be calculated when the order is completed', 'woocommerce' ) ); ?> <?php esc_html_e( 'Cost values are provisional.', 'woocommerce' ); ?></span> </div>And add CSS (in admin stylesheet) to right-align and add top margin for
.wc-order-cogs-provisional.plugins/woocommerce/includes/abstracts/abstract-wc-order.php (1)
2647-2664: Typos and constants for statuses
- Typo in local var name:
*_cosas→*_costs.- Prefer OrderStatus constants over raw strings for readability and IDE support.
- $statuses_with_provisional_cosas = array( - 'pending', - 'processing', - 'on-hold', - ); - - $costs_are_provisional = in_array( $this->get_status(), $statuses_with_provisional_cosas, true ); + $statuses_with_provisional_costs = array( + OrderStatus::PENDING, + OrderStatus::PROCESSING, + OrderStatus::ON_HOLD, + ); + + $costs_are_provisional = in_array( $this->get_status(), $statuses_with_provisional_costs, true );plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php (1)
222-235: Remove unused initial order creationThese lines create and save an order that is never used; drop for clarity and speed.
- $order = new WC_Order(); - $order->set_status( 'pending' ); - $order->save(); - $product = WC_Helper_Product::create_simple_product(); $product->set_cogs_value( 50 ); $product->save(); $order = new WC_Order(); $order->set_status( 'pending' ); $order->add_product( $product, 1 ); $order->calculate_cogs_total_value(); $order->save();
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
plugins/woocommerce/changelog/pr-61432(1 hunks)plugins/woocommerce/includes/abstracts/abstract-wc-order.php(1 hunks)plugins/woocommerce/includes/admin/meta-boxes/views/html-order-items.php(1 hunks)plugins/woocommerce/includes/class-wc-order.php(1 hunks)plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php(1 hunks)plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{php,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-quality.mdc)
**/*.{php,js,jsx,ts,tsx}: Guard against unexpected inputs
Sanitize and validate any potentially dangerous inputs
Ensure code is backwards compatible
Write code that is readable and intuitive
Ensure code has unit or E2E tests where applicable
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.phpplugins/woocommerce/includes/admin/meta-boxes/views/html-order-items.phpplugins/woocommerce/includes/class-wc-order.phpplugins/woocommerce/includes/abstracts/abstract-wc-order.phpplugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
plugins/woocommerce/tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/woo-phpunit.mdc)
plugins/woocommerce/tests/**/*.php: Ensure test classes define public setUp() and tearDown() methods (not protected)
All PHPUnit test methods must be public, not protected
Add declare(strict_types=1); at the top of each test file
Test classes should extend WC_Unit_Test_Case
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.phpplugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
**/*.{php,js,jsx,tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/avoid-regex.mdc)
**/*.{php,js,jsx,tsx,ts}: Avoid using regular expressions unless absolutely necessary to favor readability and maintainability
Only consider regex when no built-in language alternative (string/array APIs) fits the need
Only use regex when performance is critical and faster than alternatives, with measurements to justify
Use regex for complex pattern matching only if the pattern is well-documented and thoroughly tested
Allow regex when maintaining legacy code where an existing, correct pattern is being modified
If regex is necessary, document the pattern extensively to explain what it matches
If regex is necessary, add comprehensive tests covering edge cases and potential security issues
Use named capture groups in regex to improve readability when supported
Validate input before applying regex to ensure it is safe
Assess and mitigate security risks when using regex, including ReDoS and injection vulnerabilities
Avoid regex patterns that can cause catastrophic backtracking (ReDoS)
Do not construct regex from untrusted input to prevent injection attacks
Ensure regex patterns do not overmatch and unintentionally capture unexpected inputs
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.phpplugins/woocommerce/includes/admin/meta-boxes/views/html-order-items.phpplugins/woocommerce/includes/class-wc-order.phpplugins/woocommerce/includes/abstracts/abstract-wc-order.phpplugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
**/*.{php,js,ts,jsx,tsx}
⚙️ CodeRabbit configuration file
**/*.{php,js,ts,jsx,tsx}: Don't trust that extension developers will follow the best practices, make sure the code:
- Guards against unexpected inputs.
- Sanitizes and validates any potentially dangerous inputs.
- Is backwards compatible.
- Is readable and intuitive.
- Has unit or E2E tests where applicable.
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.phpplugins/woocommerce/includes/admin/meta-boxes/views/html-order-items.phpplugins/woocommerce/includes/class-wc-order.phpplugins/woocommerce/includes/abstracts/abstract-wc-order.phpplugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
plugins/woocommerce/tests/php/**
📄 CodeRabbit inference engine (plugins/woocommerce/CLAUDE.md)
Store PHP test data and fixtures under tests/php/
Files:
plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
🧬 Code graph analysis (5)
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php (2)
plugins/woocommerce/includes/class-wc-order.php (2)
set_status(306-326)save(271-277)plugins/woocommerce/tests/legacy/framework/helpers/class-wc-helper-product.php (1)
create_simple_product(46-72)
plugins/woocommerce/includes/admin/meta-boxes/views/html-order-items.php (2)
plugins/woocommerce/includes/abstracts/abstract-wc-order.php (1)
costs_are_provisional(2642-2664)plugins/woocommerce/includes/wc-core-functions.php (1)
wc_help_tip(1675-1697)
plugins/woocommerce/includes/class-wc-order.php (1)
plugins/woocommerce/includes/abstracts/abstract-wc-order.php (3)
has_cogs(2526-2528)calculate_cogs_total_value(2537-2557)save(207-252)
plugins/woocommerce/includes/abstracts/abstract-wc-order.php (4)
plugins/woocommerce/includes/class-wc-order.php (1)
has_cogs(2528-2530)plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php (1)
has_cogs(466-468)plugins/woocommerce/includes/class-wc-order-item.php (1)
has_cogs(467-469)plugins/woocommerce/tests/php/src/Internal/DataStores/Orders/OrdersTableDataStoreTests.php (2)
has_cogs(3590-3592)has_cogs(3670-3672)
plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php (1)
plugins/woocommerce/includes/abstracts/abstract-wc-order.php (1)
costs_are_provisional(2642-2664)
🪛 PHPMD (2.15.0)
plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
753-753: Avoid unused parameters such as '$costs_are_provisional'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. 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. |
This way, clients of the action will already see the recalculated order.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php (1)
1-7: Add strict types declaration.Per coding guidelines, test files must include
declare(strict_types=1);at the top of the file.Apply this diff to add the required declaration:
<?php + +declare(strict_types=1); + /** * Class WC_Tests_Order file. * * @package WooCommerce\Tests\Order */
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/woocommerce/includes/class-wc-order.php(1 hunks)plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/woocommerce/includes/class-wc-order.php
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{php,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-quality.mdc)
**/*.{php,js,jsx,ts,tsx}: Guard against unexpected inputs
Sanitize and validate any potentially dangerous inputs
Ensure code is backwards compatible
Write code that is readable and intuitive
Ensure code has unit or E2E tests where applicable
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php
plugins/woocommerce/tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/woo-phpunit.mdc)
plugins/woocommerce/tests/**/*.php: Ensure test classes define public setUp() and tearDown() methods (not protected)
All PHPUnit test methods must be public, not protected
Add declare(strict_types=1); at the top of each test file
Test classes should extend WC_Unit_Test_Case
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php
**/*.{php,js,jsx,tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/avoid-regex.mdc)
**/*.{php,js,jsx,tsx,ts}: Avoid using regular expressions unless absolutely necessary to favor readability and maintainability
Only consider regex when no built-in language alternative (string/array APIs) fits the need
Only use regex when performance is critical and faster than alternatives, with measurements to justify
Use regex for complex pattern matching only if the pattern is well-documented and thoroughly tested
Allow regex when maintaining legacy code where an existing, correct pattern is being modified
If regex is necessary, document the pattern extensively to explain what it matches
If regex is necessary, add comprehensive tests covering edge cases and potential security issues
Use named capture groups in regex to improve readability when supported
Validate input before applying regex to ensure it is safe
Assess and mitigate security risks when using regex, including ReDoS and injection vulnerabilities
Avoid regex patterns that can cause catastrophic backtracking (ReDoS)
Do not construct regex from untrusted input to prevent injection attacks
Ensure regex patterns do not overmatch and unintentionally capture unexpected inputs
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php
**/*.{php,js,ts,jsx,tsx}
⚙️ CodeRabbit configuration file
**/*.{php,js,ts,jsx,tsx}: Don't trust that extension developers will follow the best practices, make sure the code:
- Guards against unexpected inputs.
- Sanitizes and validates any potentially dangerous inputs.
- Is backwards compatible.
- Is readable and intuitive.
- Has unit or E2E tests where applicable.
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php
🧬 Code graph analysis (1)
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php (2)
plugins/woocommerce/tests/legacy/framework/helpers/class-wc-helper-product.php (1)
create_simple_product(46-72)plugins/woocommerce/includes/class-wc-order.php (3)
save(271-277)WC_Order(21-2575)set_status(306-326)
🪛 PHPMD (2.15.0)
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php
279-279: Avoid unused parameters such as '$recalculate'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests - Legacy MiniCart 3/3 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 10/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests - Legacy MiniCart 1/3 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 9/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 8/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests - Legacy MiniCart 2/3 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 6/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 5/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests - Legacy MiniCart - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest (HPOS:off) [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Blocks e2e tests 4/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest (HPOS:off) [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.4] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest - Legacy MiniCart [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest - Legacy MiniCart [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.4] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (3)
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php (3)
216-251: LGTM! Well-structured test coverage.The test correctly verifies:
- COGS remains unchanged when transitioning to non-completed statuses (processing)
- COGS is recalculated when transitioning to completed status
- The recalculation picks up the updated product cost
The test flow through pending → processing → completed provides good coverage of the feature behavior.
277-287: LGTM! Filter parameter validation is thorough.The test correctly validates that:
- The filter receives the correct from/to status values
- The filter receives the order instance
- Returning false prevents COGS recalculation
Note: The PHPMD warning about the unused
$recalculateparameter is a false positive. It's intentional and common in test code to accept callback parameters that aren't needed for the specific test scenario.
292-296: Comprehensive assertions.The assertions provide excellent validation:
- COGS value unchanged when filter returns false
- Filter parameters (from/to status) are correctly passed
- Order instance identity is preserved
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php (3)
1-1: Add strict types declaration at file top.Add strict types to align with our test guidelines.
As per coding guidelines.
<?php +declare(strict_types=1);
216-251: LGTM; add a quick sanity assertion for clarity.Test logic is solid. Consider asserting the initial COGS equals the original product cost to make intent explicit.
$initial_cogs = $order->get_cogs_total_value(); + // Sanity check: initial COGS reflects the original product cost. + $this->assertEquals( 50.00, $initial_cogs );
253-299: Harden filter cleanup and silence PHPMD unused parameter.
- Ensure the filter is always removed even if an assertion fails (try/finally).
- Address PHPMD UnusedFormalParameter for
$recalculateby unsetting it.$from_status = null; $to_status = null; $transitioned_order = null; - add_filter( - 'woocommerce_recalculate_cogs_on_order_status_transition', - function ( $recalculate, $from, $to, $order ) use ( &$from_status, &$to_status, &$transitioned_order ) { - $from_status = $from; - $to_status = $to; - $transitioned_order = $order; - return false; - }, - 10, - 4 - ); - - $order->set_status( 'completed' ); - $order->save(); - - $cogs_after_completion = $order->get_cogs_total_value(); - $this->assertEquals( $initial_cogs, $cogs_after_completion, 'COGS should not be recalculated when filter returns false' ); - $this->assertEquals( 'pending', $from_status ); - $this->assertEquals( 'completed', $to_status ); - $this->assertSame( $order, $transitioned_order ); - - remove_all_filters( 'woocommerce_recalculate_cogs_on_order_status_transition' ); + try { + add_filter( + 'woocommerce_recalculate_cogs_on_order_status_transition', + function ( $recalculate, $from, $to, $order ) use ( &$from_status, &$to_status, &$transitioned_order ) { + unset( $recalculate ); // Silence PHPMD.UnusedFormalParameter. + $from_status = $from; + $to_status = $to; + $transitioned_order = $order; + return false; + }, + 10, + 4 + ); + + $order->set_status( 'completed' ); + $order->save(); + + $cogs_after_completion = $order->get_cogs_total_value(); + $this->assertEquals( $initial_cogs, $cogs_after_completion, 'COGS should not be recalculated when filter returns false' ); + $this->assertEquals( 'pending', $from_status ); + $this->assertEquals( 'completed', $to_status ); + $this->assertSame( $order, $transitioned_order ); + } finally { + remove_all_filters( 'woocommerce_recalculate_cogs_on_order_status_transition' ); + }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/woocommerce/includes/class-wc-order.php(1 hunks)plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php(1 hunks)plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/woocommerce/includes/class-wc-order.php
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{php,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/code-quality.mdc)
**/*.{php,js,jsx,ts,tsx}: Guard against unexpected inputs
Sanitize and validate any potentially dangerous inputs
Ensure code is backwards compatible
Write code that is readable and intuitive
Ensure code has unit or E2E tests where applicable
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.phpplugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
plugins/woocommerce/tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/woo-phpunit.mdc)
plugins/woocommerce/tests/**/*.php: Ensure test classes define public setUp() and tearDown() methods (not protected)
All PHPUnit test methods must be public, not protected
Add declare(strict_types=1); at the top of each test file
Test classes should extend WC_Unit_Test_Case
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.phpplugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
**/*.{php,js,jsx,tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/avoid-regex.mdc)
**/*.{php,js,jsx,tsx,ts}: Avoid using regular expressions unless absolutely necessary to favor readability and maintainability
Only consider regex when no built-in language alternative (string/array APIs) fits the need
Only use regex when performance is critical and faster than alternatives, with measurements to justify
Use regex for complex pattern matching only if the pattern is well-documented and thoroughly tested
Allow regex when maintaining legacy code where an existing, correct pattern is being modified
If regex is necessary, document the pattern extensively to explain what it matches
If regex is necessary, add comprehensive tests covering edge cases and potential security issues
Use named capture groups in regex to improve readability when supported
Validate input before applying regex to ensure it is safe
Assess and mitigate security risks when using regex, including ReDoS and injection vulnerabilities
Avoid regex patterns that can cause catastrophic backtracking (ReDoS)
Do not construct regex from untrusted input to prevent injection attacks
Ensure regex patterns do not overmatch and unintentionally capture unexpected inputs
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.phpplugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
**/*.{php,js,ts,jsx,tsx}
⚙️ CodeRabbit configuration file
**/*.{php,js,ts,jsx,tsx}: Don't trust that extension developers will follow the best practices, make sure the code:
- Guards against unexpected inputs.
- Sanitizes and validates any potentially dangerous inputs.
- Is backwards compatible.
- Is readable and intuitive.
- Has unit or E2E tests where applicable.
Files:
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.phpplugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
plugins/woocommerce/tests/php/**
📄 CodeRabbit inference engine (plugins/woocommerce/CLAUDE.md)
Store PHP test data and fixtures under tests/php/
Files:
plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
🧬 Code graph analysis (2)
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php (2)
plugins/woocommerce/tests/legacy/framework/helpers/class-wc-helper-product.php (1)
create_simple_product(46-72)plugins/woocommerce/includes/class-wc-order.php (3)
save(271-277)WC_Order(21-2567)set_status(306-326)
plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php (1)
plugins/woocommerce/includes/abstracts/abstract-wc-order.php (1)
costs_are_provisional(2642-2664)
🪛 PHPMD (2.15.0)
plugins/woocommerce/tests/legacy/unit-tests/order/class-wc-tests-orders.php
279-279: Avoid unused parameters such as '$recalculate'. (undefined)
(UnusedFormalParameter)
plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
654-654: Avoid unused parameters such as '$costs_are_provisional'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: Blocks e2e tests 8/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 10/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 4/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 5/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.4] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest (HPOS:off) [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest (HPOS:off) [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests - Legacy MiniCart - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest - Legacy MiniCart [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: PHP: 8.4 WP: latest - Legacy MiniCart [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (1)
plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php (1)
639-664: LGTM! Test correctly verifies filter behavior.The test properly validates that the
woocommerce_order_costs_are_provisionalfilter can override the default provisional-cost logic. The test structure is sound:
- Correctly enables COGS feature
- Creates a completed order (where costs should not be provisional by default)
- Verifies the filter can force provisional status
- Properly cleans up filters
The unused parameter
$costs_are_provisionalin the closure (line 654) is intentional and already acknowledged with the phpcs:ignore comment, which is appropriate for test code that needs to match the filter signature.
joshuatf
left a comment
joshuatf
left a comment
There was a problem hiding this comment.
Left a question about custom statuses, but also as a quick sanity check: is completed always the status that we want to mark the final COGS? I believe the answer is "yes" but I could also seem some edge cases where a custom status of "shipped" is the final status.
I think it might be okay that completed is the canonical last step for provisional costs, but just want to call that out in case this makes you think of anything.
| * | ||
| * @return bool True if the order costs are provisional. | ||
| */ | ||
| public function costs_are_provisional(): bool { |
There was a problem hiding this comment.
I know that this is the standard pattern for new order functionality, but this order abstraction file is getting unwieldy.
We may want to consider how we add new order functionality. It seems like there's already a CogsAwareTrait, though I'm not sure if it's appropriate for this specific change or not.
There was a problem hiding this comment.
Indeed, the problem is that both the abstract order class and WC_Order itself are also quite big already.
CogsAwareTrait is for general COGS functionality and not specific to order. Maybe we could move all the order COGS functionality to a new OrderCogsTrait, but that would be a refactoring to be done separately.
There was a problem hiding this comment.
Thanks for the explanation! A new trait makes sense.
We have a little bit of a bystander effect happening here by not taking the opportunity to clean this up and this is how technical debt begins to accumulate.
I'd love to see at least the new changes implemented in OrderCogsTrait so that we don't continue piling on. The other changes can be migrated there in a later clean-up.
| return false; | ||
| } | ||
|
|
||
| $statuses_with_provisional_cost = array( |
There was a problem hiding this comment.
What about custom statuses? It seems possible they could be affected by this change as well.
There was a problem hiding this comment.
I added woocommerce_order_costs_are_provisional filter precisely to cover this scenario, since in principle we can't know which order statuses should show that "costs are provisional" message. Or should we assume that all of them do?
There was a problem hiding this comment.
Since you know more about this feature, I'll defer to you. I think they are potentially affected by this, but the only question is whether we want to default to on or off for these.
If the default for new order statuses should be off, then this is good as is. Otherwise we might consider updating this to an array diff with the "finished" statuses removing from all order statuses.
This actually prevents the recalculation of order costs on order save. To be rethought and maybe readded later if necessary.
That's why I added the |
joshuatf
left a comment
joshuatf
left a comment
There was a problem hiding this comment.
Sorry about the delay in reviewing this. I left a couple of responses. I don't want to continue blocking you on this further, but one thing I think functionally that we might need to update is the provisional cost of an order when it's not complete.
I would expect to see the latest (provisional) cost for an order's COGS when the COGS for that product is updated. Steps to reproduce:
- Set COGS for a product and purchase
- Update the product COGS
- Navigate to the order in the admin
- Note the old COGS is still shown on the order
Before order place:
After order placed:
| return false; | ||
| } | ||
|
|
||
| $statuses_with_provisional_cost = array( |
There was a problem hiding this comment.
Since you know more about this feature, I'll defer to you. I think they are potentially affected by this, but the only question is whether we want to default to on or off for these.
If the default for new order statuses should be off, then this is good as is. Otherwise we might consider updating this to an array diff with the "finished" statuses removing from all order statuses.
| * | ||
| * @return bool True if the order costs are provisional. | ||
| */ | ||
| public function costs_are_provisional(): bool { |
There was a problem hiding this comment.
Thanks for the explanation! A new trait makes sense.
We have a little bit of a bystander effect happening here by not taking the opportunity to clean this up and this is how technical debt begins to accumulate.
I'd love to see at least the new changes implemented in OrderCogsTrait so that we don't continue piling on. The other changes can be migrated there in a later clean-up.
Changes proposed in this Pull Request:
Recalculate the total value of Cost of Goods Sold for an order whenever it transitions to the "completed" state.
Show a "Cost values are provisional" text in the order details page in admin (under the total cost value) when the order is in "processing", "pending payment" or "on-hold" state.
woocommerce_order_costs_are_provisionalandwoocommerce_recalculate_cogs_on_order_status_transitionfilters to customize the new behavior.Closes #61391.
How to test the changes in this Pull Request:
add_filter('woocommerce_order_costs_are_provisional', '__return_false');and reload the product details page, the message should no loner appear.To continue testing we need to switch to the command line since changing the state of the order from the admin page would trigger an explicit recalculation. Take note of the order id, run
wp shelland then these commands:Now you can reload the order details page and you should also see the new order cost value (and the "provisional cost" message should have disappeared since the order is now completed).
add_filter('woocommerce_recalculate_cogs_on_order_status_transition', '__return_false');- now the previous step should not recalculate the order costs.Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment