Add support for storing COGS values for orders when HPOS is disabled #61307
Conversation
github-actions
bot
added
the
plugin: woocommerce
woocommercebot
requested review from
a team and
prettyboymp
and removed request for
a team
Testing GuidelinesHi @prettyboymp , 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:
|
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. |
📝 WalkthroughWalkthroughAdds conditional loading, persistence, and sync of COGS values across CPT and HPOS order data stores; preserves per-item saved COGS for DB-loaded items; guards HPOS migration from overwriting valid COGS with zeros; adds tests and a changelog entry for these COGS behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Order as WC_Order
participant CPT_DS as CPT Data Store
participant Meta as Post Meta (DB)
participant HPOS_DS as HPOS Data Store
Note over CPT_DS,Meta: Read CPT order
CPT_DS->>Meta: get_post_meta(..., '_cogs_total_value')
alt meta exists and COGS enabled
CPT_DS->>Order: set_cogs_total_value(filtered value)
else meta missing
CPT_DS->>Order: do not set COGS (defer calculation)
end
Note over Order,CPT_DS: Save CPT order
Order->>CPT_DS: update_order_meta_from_object(order)
CPT_DS->>CPT_DS: handle_cogs_value_update -> apply save filter
alt filter returns null (don't save)
CPT_DS->>Meta: delete_post_meta('_cogs_total_value')
else value non-zero and enabled
CPT_DS->>Meta: update_post_meta('_cogs_total_value', value)
end
Note over HPOS_DS,CPT_DS: Migrate CPT -> HPOS on read
HPOS_DS->>CPT_DS: migrate_post_record(...)
alt HPOS has cogs > 0 and incoming CPT value == 0
HPOS_DS-->>CPT_DS: skip migrating cogs_total_value (preserve HPOS)
else
HPOS_DS->>Order: set migrated properties
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (5)
plugins/woocommerce/includes/data-stores/abstract-wc-order-item-type-data-store.php (1)
181-201: Leverage item meta cache and avoid unnecessary datastore callUse the item’s meta API (already primed by read_meta_data) and optionally apply changes to avoid leaving the item dirty after read.
- $cogs_metadata = $this->order_item_data_store->get_metadata( $item->get_id(), '_cogs_value', true ); + $cogs_metadata = $item->get_meta( '_cogs_value', true ); @@ - if ( '' !== $cogs_metadata && false !== $cogs_metadata ) { + if ( '' !== $cogs_metadata && false !== $cogs_metadata ) { $cogs_value = (float) $cogs_metadata; @@ - $item->set_cogs_value( (float) $cogs_value ); + $item->set_cogs_value( (float) $cogs_value ); + // Optional: keep the object clean after read. + $item->apply_changes(); }plugins/woocommerce/includes/data-stores/class-wc-order-data-store-cpt.php (2)
273-291: Avoid duplicating COGS save logic; centralize filter + zero-delete handlingThis block and update_order_meta_from_object both apply woocommerce_save_order_cogs_value and perform delete/update. Consider funneling both through a shared helper (e.g., reuse handle_cogs_value_update or extract one method) to keep behavior in one place.
1418-1445: update_order_meta_from_object duplicationSame recommendation: reuse a single helper for filter + zero→delete behavior to ensure consistent semantics and reduce drift.
plugins/woocommerce/tests/php/includes/data-stores/class-wc-order-data-store-cpt-test.php (2)
1-1: Add strict types declaration to test filePer project test guidelines, add declare(strict_types=1); at the top.
+<?php declare(strict_types=1); -<?php +// phpcs:ignoreFile WordPress.Files.FileName.InvalidClassFileName
828-1260: Float assertions robustnessSeveral tests compare floats with assertEquals. Prefer assertEqualsWithDelta to avoid brittle failures from float precision.
Example:
- $this->assertEquals( $expected_total, (float) get_post_meta( $order->get_id(), '_cogs_total_value', true ) ); + $this->assertEqualsWithDelta( $expected_total, (float) get_post_meta( $order->get_id(), '_cogs_total_value', true ), 0.00001 );
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
plugins/woocommerce/changelog/pr-61307(1 hunks)plugins/woocommerce/includes/class-wc-order-item-product.php(1 hunks)plugins/woocommerce/includes/data-stores/abstract-wc-order-item-type-data-store.php(1 hunks)plugins/woocommerce/includes/data-stores/class-wc-order-data-store-cpt.php(7 hunks)plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php(1 hunks)plugins/woocommerce/tests/php/includes/data-stores/class-wc-order-data-store-cpt-test.php(4 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/includes/data-stores/abstract-wc-order-item-type-data-store.phpplugins/woocommerce/includes/class-wc-order-item-product.phpplugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.phpplugins/woocommerce/tests/php/includes/data-stores/class-wc-order-data-store-cpt-test.phpplugins/woocommerce/includes/data-stores/class-wc-order-data-store-cpt.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/includes/data-stores/abstract-wc-order-item-type-data-store.phpplugins/woocommerce/includes/class-wc-order-item-product.phpplugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.phpplugins/woocommerce/tests/php/includes/data-stores/class-wc-order-data-store-cpt-test.phpplugins/woocommerce/includes/data-stores/class-wc-order-data-store-cpt.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/includes/data-stores/abstract-wc-order-item-type-data-store.phpplugins/woocommerce/includes/class-wc-order-item-product.phpplugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.phpplugins/woocommerce/tests/php/includes/data-stores/class-wc-order-data-store-cpt-test.phpplugins/woocommerce/includes/data-stores/class-wc-order-data-store-cpt.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/php/includes/data-stores/class-wc-order-data-store-cpt-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/data-stores/class-wc-order-data-store-cpt-test.php
🧬 Code graph analysis (4)
plugins/woocommerce/includes/class-wc-order-item-product.php (1)
plugins/woocommerce/includes/abstracts/abstract-wc-data.php (1)
get_object_read(771-773)
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php (2)
plugins/woocommerce/includes/class-wc-order-item-product.php (1)
has_cogs(555-557)plugins/woocommerce/includes/class-wc-order.php (1)
has_cogs(2523-2525)
plugins/woocommerce/tests/php/includes/data-stores/class-wc-order-data-store-cpt-test.php (5)
plugins/woocommerce/tests/legacy/framework/helpers/class-wc-helper-product.php (1)
create_simple_product(46-72)plugins/woocommerce/includes/class-wc-order-item-product.php (1)
has_cogs(555-557)plugins/woocommerce/includes/abstracts/abstract-wc-order.php (2)
set_cogs_total_value(2601-2605)calculate_cogs_total_value(2537-2557)plugins/woocommerce/includes/data-stores/class-wc-order-data-store-cpt.php (2)
update_order_meta_from_object(1418-1445)update_post_meta(253-406)plugins/woocommerce/includes/wc-order-functions.php (1)
wc_create_refund(557-751)
plugins/woocommerce/includes/data-stores/class-wc-order-data-store-cpt.php (4)
plugins/woocommerce/tests/php/includes/data-stores/class-wc-order-data-store-cpt-test.php (3)
has_cogs(886-888)has_cogs(953-955)update_order_meta_from_object(858-860)plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php (2)
read_cogs_data(1396-1412)update_post_meta(2898-2900)plugins/woocommerce/includes/data-stores/abstract-wc-order-data-store-cpt.php (2)
update_order_meta_from_object(751-817)update_post_meta(460-500)plugins/woocommerce/includes/abstracts/abstract-wc-order.php (1)
set_cogs_total_value(2601-2605)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Blocks e2e tests 10/10 - @woocommerce/plugin-woocommerce [e2e]
🔇 Additional comments (9)
plugins/woocommerce/includes/class-wc-order-item-product.php (1)
577-585: Fallback calculation logic is correct.The original product-based calculation is preserved as a sensible fallback for new items, refunds, and items without saved COGS data. This maintains backwards compatibility while supporting the new preservation behavior.
plugins/woocommerce/changelog/pr-61307 (1)
1-4: Changelog format checkConfirm if the changelog generator expects a “Comment:” key. If yes, change line 4 to “Comment: Add support for storing COGS values for orders when HPOS is disabled”.
plugins/woocommerce/includes/data-stores/class-wc-order-data-store-cpt.php (5)
12-12: Trait usage looks goodCOGS feature gating via CogsAwareTrait is correctly wired.
Also applies to: 24-24
85-85: Marking _cogs_total_value as internal meta is correctPrevents exposure as a custom field and avoids accidental deletion in sync.
189-191: Reading COGS during CPT readGood conditional load respecting feature and capability.
1453-1469: CPT COGS read pathLGTM. Mirrors HPOS behavior and applies filter before setting.
1482-1510: handle_cogs_value_update is soundCovers feature gating, null→skip, and zero→delete. Minor: given the pre-check at Line 279, the feature check here is redundant; safe to keep for defensive coding.
plugins/woocommerce/tests/php/includes/data-stores/class-wc-order-data-store-cpt-test.php (2)
6-6: Test scaffoldingUsing CogsAwareUnitTestSuiteTrait and disabling feature in tearDown is correct.
Also applies to: 15-16, 41-46
931-960: Consider adding HPOS sync-on-read test for negative COGSTo validate the guard in OrdersTableDataStore (not overwriting non-zero HPOS COGS, including negative refunds), add a counterpart test under HPOS to cover negative values.
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php (1)
1690-1697: Add explicit float cast for type consistency.The logic correctly prevents overwriting valid HPOS COGS with stale CPT zero values. However, line 1693 should explicitly cast
$hpos_cogsto float for type consistency, as recommended in the previous review comment.Apply this diff:
- $hpos_cogs = $order->get_cogs_total_value( 'edit' ); + $hpos_cogs = (float) $order->get_cogs_total_value( 'edit' );This ensures strict type consistency with the float comparison at line 1694 and aligns with the approach used for
$value.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/src/Internal/DataStores/Orders/OrdersTableDataStore.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/src/Internal/DataStores/Orders/OrdersTableDataStore.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/src/Internal/DataStores/Orders/OrdersTableDataStore.php
🧬 Code graph analysis (1)
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php (2)
plugins/woocommerce/includes/class-wc-order.php (1)
has_cogs(2523-2525)plugins/woocommerce/tests/php/src/Internal/DataStores/Orders/OrdersTableDataStoreTests.php (2)
has_cogs(3590-3592)has_cogs(3670-3672)
⏰ 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 10/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests - Legacy MiniCart 1/3 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 5/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests - Legacy MiniCart 2/3 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests - Legacy MiniCart 3/3 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 6/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- 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 (HPOS:off) [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Blocks e2e tests 8/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 9/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 4/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest - Legacy MiniCart [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests - Legacy MiniCart - @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 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/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: 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] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (1)
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php (1)
105-105: LGTM! Internal meta key addition is correct.The addition of
'_cogs_total_value'to theinternal_meta_keysarray correctly marks COGS data as internal metadata, ensuring proper handling during CPT/HPOS synchronization.
prettyboymp
left a comment
prettyboymp
left a comment
There was a problem hiding this comment.
@Konamiman, I'm seeing some calculation issues when running HPOS without syncing enabled, that don't appear to happen otherwise.
To reproduce - note these are the steps I did to create the issue, I'm not 100% sure which cause the problem:
- Create a variable with 2 attributes, each with 2 values - generating 4 variations.
- Give each variation a price and unique cost.
- Go to the shop and add each variation to your cart and checkout.
- Process the order so it is complete.
- Edit the order.
- Click Refund
- Refund a specific item.
- Check the total cost of goods displayed.
This didn't occur when not using HPOS or when syncing was enabled. And it didn't happen when purchasing only 2 variations.. Though I don't know if it was the number of variations or items in the order.
|
@prettyboymp I have followed these steps (both with HPOS enabled and disable, and automatic data sync disabled in both cases) and I get the expected result: 
Also I tested with two products: one with product-specific attributes and one with general term-based attributes, just in case. What's the exact issue you experienced? Refunds not visible in the page? |
prettyboymp
left a comment
prettyboymp
left a comment
There was a problem hiding this comment.
@Konamiman, I previously tested in WordPress Playground because my local dev has too much data in HPOS to easily switch to the other states. I'm not sure why the issue is occurring there, but it must be a build issue.
I retested locally, and the issue wasn't there.
PR #61307 introduced a change: already persisted COGS values for product line items would be preserved to serve as "snapshots" of the COGS value at the time of purchase. However this has the unintended side effect that for orders not yet completed, the COGS values for these line items will not be recalculated even after product COGS change. So this commit reverts the change.
PR #61307 introduced a change: already persisted COGS values for product line items would be preserved to serve as "snapshots" of the COGS value at the time of purchase. However this has the unintended side effect that for orders not yet completed, the COGS values for these line items will not be recalculated even after product COGS change. So this commit reverts the change.
Changes proposed in this Pull Request:
This pull requests implements the storage of total COGS values for orders when HPOS is disabled.
Closes #61292.
How to test the changes in this Pull Request:
Repeat the testing instructions of Add the Cost of Goods Sold related code and REST APIs for the orders and order items #52067, Add Cost of Goods Sold information in the order editor in admin #55840 and Take in account refunds when calculating Cost of Goods Sold for an order #57838 but with HPOS disabled.
With HPOS enabled and compatibility mode enabled (in WooCommerce - Settings - Advanced - Features), create an order from the store, load it in admin and verify that the total cost value is correct. Then switch off HPOS (
wp wc hpos disable), reload the order details page, and verify that the cost data is still correct.Repeat 2, but in reverse (start with HPOS disabled this time).
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment