-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add the Cost of Goods Sold related code and REST APIs for the orders and order items #52067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The previous names were too generic, the renamed hooks are: woocommerce_get_cogs_total_value -> woocommerce_get_product_cogs_total_value woocommerce_load_cogs_value -> woocommerce_load_product_cogs_value woocommerce_save_cogs_value -> woocommerce_save_product_cogs_value And the renamed methods: add_cogs_info_to_returned_data -> add_cogs_info_to_returned_product_data add_cogs_related_schema -> add_cogs_related_product_schema
- WC_Order_Item class gets these methods: - has_cogs - defaulting to false, derived classes must override - calculate_cogs_value - calculate_cogs_value_core - derived classes must override - get_cogs_value - set_cogs_value - for internal usage only - WC_Order_Item_Product overrides calculate_cogs_value_core - Abstract_WC_Order_Item_Type_Data_Store - Stores COGS values in a '_cogs_value' order item meta entry
- WC_Abstract_Order gets these methods: - has_cogs - defaulting to false, derived classes must override - calculate_cogs_total_value - calculate_cogs_total_value_core - derived classes can override - get_cogs_total_value - set_cogs_total_value - for internal usage only - calculate_totals - modified to invoke calculate_cogs_total_value - WC_Order overrides has_cogs to return true - OrdersTableDataStore - Stores COGS values in '_cogs_total_value' order meta keys
- "cost_of_goods_sold" object with "total_value" key for the order - "cost_of_goods_sold" object with "value" key for the product line items
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. |
Also fix one of the tests for WC_Order_Item.
woocommerce_load_cogs_overrides_parent_value_flag --> woocommerce_load_product_cogs_overrides_parent_value_flag woocommerce_save_cogs_overrides_parent_value_flag --> woocommerce_save_product_cogs_overrides_parent_value_flag
|
Hi @barryhughes, Apart from reviewing the code changes, please make sure to review the testing instructions and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. You can follow this guide to find out what good testing instructions should look like: |
barryhughes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good! I do still need to test, but it looks good on initial read. Left some questions and comments, but all are minor.
| throw new Exception( | ||
| sprintf( | ||
| // translators: %1$s = class and method name. | ||
| __( 'Method %1$s is not implemented. Classes overriding has_cogs must override this method too.', 'woocommerce' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not escape (vs disabling the ExceptionNotEscaped rule)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exception signals a "design time" error: if you get it it means that your code is wrong (you did override has_cogs to return true but didn't override calculate_cogs_value_core) and needs fixing. You are never going to get this in production, so it doesn't make much sense to add escaping.
plugins/woocommerce/includes/data-stores/abstract-wc-order-item-type-data-store.php
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php
Outdated
Show resolved
Hide resolved
barryhughes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests great (thanks for the great instructions!). Works as described and, when the feature is disabled, the relevant properties disappear from REST API responses.
I don't have anything else to add: approving but not merging (so you can review some earlier notes I left—though none are blocking, so also feel free to move ahead with a merge) 👍🏼
Co-authored-by: Barry Hughes <[email protected]>
Co-authored-by: Barry Hughes <[email protected]>
I think we might need to poke at this one; some of the other CI fails seem unrelated (the PHP linting check is just failing to run, which I was hitting on some other PRs this week). |
The good news is that what was wrong here was the test itself, not the tested code. It's fixed now. |
|
Re-running checks again; same problem with |
|
This might the wrong place for this comment, but I'll place it anyway. I think parts of this - namely the option to make order line level cogs read-only, combined with #61432 is the wrong approach. Let me give an example:
Now, we have 2 orders which COGS totals to $24, while in reality the COGS should totaling to $22. This happens because ERPs (rightfully) stores averages on product levels and actuals (based on FIFO, most often) on order line level. This means that we need to:
|
Changes proposed in this Pull Request:
Following #51675 this pull request adds programmatic and REST API support for the orders and order items related part of the Cost of Goods Sold feature.
Changes in code API
The following methods are added to
WC_Order_Item:has_cogs(): bool- A boolean that by default returns false. This method must be overridden in derived classes representing line items that manage a COGS value.calculate_cogs_value(): bool- If the calculation succeeds it sets the resulting value withset_cogs_valueand returns true.calculate_cogs_value_core(): ?float- It simply throws an exception. This method must be overridden wheneverhas_cogsreturns true.get_cogs_value(): float- Returns the value that was set withset_cogs_value.set_cogs_value(float): void- Marked as@internal, intended for use only bycalculate_cogs_valueand when loading the object from the database.The following methods are added to
WC_Order_Item_Product(overridding the methods inWC_Order_Item):has_cogs(): bool- Returns true.calculate_cogs_value_core(): ?float- Calculates the COGS value by multiplying the product value by the quantity. If the product no longer exists it returns null and thencalculate_cogs_valuewon't change the current value.The following methods are added to
WC_Abstract_Order:has_cogs(): bool- Same meaning as inWC_Order_Item.calculate_cogs_total_value(): float- Invokescalculate_cogs_total_value_coreand sets the result withset_cogs_total_value.calculate_cogs_total_value_core(): float- Calculates the value by looping through all the order items for whichhas_cogsreturns true, invoking theircalculate_cogs_valuemethods, and adding up the value returned by theirget_cogs_valuemethods.get_cogs_total_value(): float- Returns the value that was set withset_cogs_value.set_cogs_total_value(float): void- Marked as@internal, intended for use only bycalculate_cogs_total_valueand when loading the object from the database.The following methods are added to
WC_Order:has_cogs(): bool- Overridden to always return true.The order's
calculate_cogs_total_valuemethod is invoked in two places: insidecalculate_totalsin the same object, and inWC_Checkout::create_order.This design allows to add new line item types and new order types in the future, with or without COGS management.
Storage
The COGS values are stored as follows:
_cogs_valuemeta entry in thewp_woocommerce_order_itemmetatable._cogs_total_valuemeta entry in thewp_wc_orders_metatable.Important! For now, order meta items are stored only when HPOS is enabled. If support for CPT tables is added (undecided yet), this will be done in a separate pull request.
These values are stored on save (and retrieved on load) only when the Cost of Goods Sold feature is enabled, when the corresponding order or order item's
has_cogsmethod returns true, and when the value to store is not zero.Changes in REST API
The following is added to the REST API schema for the get order(s) endpoint (
/wp-json/wc/v3/ordersand/wp-json/wc/v3/orders/<order_id>,GETonly):{ "cost_of_goods_sold": { "total_value": <float> }, "line_items": [ { "cost_of_goods_sold": { "value": <float> } } ] }These new items appear only when the feature is enabled.
There's no explicit support for altering order and order items COGS values in the REST API, but any request that triggers a order's
calculate_totalswill cause a recalculation of the COGS values as well.Hooks
The following filters are introduced:
woocommerce_calculated_order_cogs_value: to customize the value that gets calculated for an order before it's set.woocommerce_calculated_order_item_cogs_value: to modify the value that gets calculated for an order item before it's set.woocommerce_load_order_item_cogs_value: to modify the value that gets saved to the database for a given order item, or to suppress the value saving altogether.woocommerce_save_order_item_cogs_value: to modify the value that gets saved to the database for a given order item, or to suppress the value saving altogether.woocommerce_load_order_cogs_value: to modify the value that gets saved to the database for a given order, or to suppress the value saving altogether.woocommerce_save_order_cogs_value: to modify the value that gets saved to the database for a given order, or to suppress the value saving altogether.This pull request also modifies the following hook names (all added in #51675) to make them less ambiguous:
woocommerce_load_cogs_value-->woocommerce_load_product_cogs_valuewoocommerce_save_cogs_value-->woocommerce_save_product_cogs_valuewoocommerce_get_cogs_total_value-->woocommerce_get_product_cogs_total_valuewoocommerce_load_cogs_overrides_parent_value_flag-->woocommerce_load_product_cogs_overrides_parent_value_flagwoocommerce_save_cogs_overrides_parent_value_flag-->woocommerce_save_product_cogs_overrides_parent_value_flagHow to test the changes in this Pull Request:
Reference: #51675. As with that other pull request, testing will mostly be done using
wp shell.Preparation
wp wc hpos sync && wp wc hpos enable --ignore-plugin-compatibilityCost calculation and data storage
$product_1_idand$product_2_id.Testing the REST API
After setting the product COGS values to the original values (step 4, minus the products creation) and recalculating the order totals again (second block of commands in step 8), perform a
GETrequests to/wp-json/wc/v3/orders/<order id>and verify that you get the following additions to the returned data:{ "cost_of_goods_sold": { "total_value": 122.76 }, "line_items": [ { "cost_of_goods_sold": { "value": 22.44 } }, { "cost_of_goods_sold": { "value": 100.32 } } ] }Try also an
OPTIONSrequests on the endpoint and verify that the extended schema information returned is accurate.Testing the filters
Here's scaffolding code to test the filters, you can add it at the end of woocommerce.php or as a code snippet. Try changing the return value of one filter at a time, and verifying via code API or REST API that the result is as expected:
Testing with the feature disabled
Disable the feature in the features settings page and verify that:
cost_of_goods_soldkey (and the same for order line items).OPTIONSrequests don't include a cost_of_goods_sold key in the returned schema.Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment