-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Add Cost of Goods Sold information in the order editor in admin #55840
Conversation
…o zero and saved.
…hooks New names: 'woocommerce_product_empty_cogs_html' and 'woocommerce_product_get_cogs_html' (to eliminate ambiguity, since orders have cost values too).
Also the existing "Cost" column is renamed to "Price".
Size Change: +1.56 MB (+38.57%) 🚨 Total Size: 5.6 MB
|
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 is for consistency with woocommerce_product_get_cogs_html
Testing GuidelinesHi @jorgeatorres , 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:
|
Thanks for this @Konamiman! So, could we change "Items Subtotal", "Order Total" and "Cost Total" > "Items subtotal", "Order total" and "Cost total". I see there is also "Transaction Fee", but I guess that's added by WooPayments? |
It would be possible but for consistency and better housekeeping that's what I would do:
|
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.
Hey @Konamiman!
This tests well. Thank you! I left some feedback but it's more about "backwards compatibility". Nothing seems broken.
<td class="item_cost" width="1%" data-sort-value="<?php echo esc_attr( $item->get_cogs_value() ); ?>"> | ||
<div class="view"> | ||
<?php | ||
echo $item->get_cogs_value_html(); |
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.
Maybe we should pass this through wp_kses_post()
or similar just in case. Given the value is filterable, it might contain unsafe HTML.
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.
Ok, that makes sense since other parts of the same page are enclosed in wp_kses_post
already. Done.
@@ -47,7 +49,16 @@ | |||
|
|||
<?php do_action( 'woocommerce_admin_order_item_values', $product, $item, absint( $item_id ) ); ?> | |||
|
|||
<td class="item_cost" width="1%" data-sort-value="<?php echo esc_attr( $order->get_item_subtotal( $item, false, true ) ); ?>"> | |||
<?php if ( wc_get_container()->get( CostOfGoodsSoldController::class )->feature_is_enabled() ) : ?> | |||
<td class="item_cost" width="1%" data-sort-value="<?php echo esc_attr( $item->get_cogs_value() ); ?>"> |
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.
I'm not too sure we should hijack the item_cost
CSS class as there might be 3rd party scripts or even just styles targeting what now would be the "Price" column as it has always used the other class. I know it's a bit confusing, but I'd suggest we use a new CSS class for the COGS column and despite the new "Price" name leave item_cost
as the CSS class for the usual price column.
What do you think?
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.
Done, I've reverted the existing class to the original item_cost
name, and renamed the new class to item_cost_of_goods
.
<?php if ( $cogs_is_enabled ) : ?> | ||
<th class="item_cost sortable" data-sort="float"><?php esc_html_e( 'Cost', 'woocommerce' ); ?></th> | ||
<?php endif; ?> | ||
<th class="item_price sortable" data-sort="float"><?php esc_html_e( 'Price', 'woocommerce' ); ?></th> |
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.
Same as above.
The new class to show costs is now "item_cost_of_goods".
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.
LGTM. Thank you @Konamiman!
I've tested that:
- The new column doesn't appear on the order edit screen unless COGS is enabled in WC > Settings > Advanced > Features.
- The new column updates correctly when changing quantities and adding or removing items on the order edit screen.
- The items table on the order edit screen can be sorted as usual by different columns, including this new one.
- CSS styles look correct:
Changes proposed in this Pull Request:
This pull request adds Cost of Goods Sold information in the order details page in admin:
The backend changes here are minimal since all the code API to deal with cost values in orders was already introduced in #52067.
Note that the existing "Cost" column is renamed to "Price" to avoid confussion. This change is permanent, it applies even when the Cost of Goods Sold feature is disabled.
Additional changes:
woocommerce_order_item_no_cogs_html
andwoocommerce_order_item_cogs_html
.woocommerce_empty_cogs_html
andwoocommerce_get_cogs_html
hooks are renamed towoocommerce_product_empty_cogs_html
andwoocommerce_product_get_cogs_html
respectively.How to test the changes in this Pull Request:
There's an additional feature that needs a small code change to be tested. The
get_cogs_value_html
method of theWC_Order_Item
class (and derived classes) will return a dash if thehas_cogs
method of the class returns false (as opposed to rendering a zero ifhas_cogs
returns true but a particular instance has no cost defined), but we are using this method only for product items, for whichhas_cogs
always returns true. So to test this behavior you can do the following temporary change to theWC_Order_Item_Product
class: redefine thehas_cogs
method as follows:Then reload the order details page and verify that the lines corresponding to products with a cost of zero display a dash, instead of zero, in the cost column.
With the previous code modification still in place, you can add the following snippet to test the new hooks:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment