It seems Order total summary formatter for empty order total price could break the order view.

To reproduce, create new order and check the View order without saving the order edit form provided.

So we need to make sure that this formatter know what to do: return empty array if the field is empty, before other any other formatting processes.

CommentFileSizeAuthor
#7 711-updated.diff5.38 KBreplicaobscura

Comments

vasike created an issue. See original summary.

vasike’s picture

vasike
Location Ramnicu Valcea
commented

There is a new PR on this
https://github.com/drupalcommerce/commerce/pull/711

Extra:
- Added "No results behavior" for the "Order items" view.
- Update hook to revert the view
- Update the OrderAdminTest::testAdminOrderView() to test the changes made.

p.s. hmm, i made "by default" Needs review ;)

bojanz’s picture

bojanz commented
Title: Order total summary formatter for empty total price » Order total summary formatter does not handle empty orders
finne’s picture

finne
Primary language Dutch
Location Amsterdam
commented

What is the preferred place to work on commerce code: DO or github?

finne’s picture

finne
Primary language Dutch
Location Amsterdam
commented

Review:

This patch fixes the problem, but destroys any customisation that might have been done to the commerce_order_item_table view. Does it need to warn the user beforehand?

Other options:
a) test for values in the twig template, and print 0 / print no line if no value found.
b) test for value in PriceTwigExtension.php and assume 0 if no value.
c) OrderTotalSummary.php could return 0 if no prices found.
d) Order::getTotalPrice() could return 0 if no price found.

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented
Status: Needs review » Needs work

Current patch conflicts with existing commerce_order_post_update_6() function introduced in recent commerce version.

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented
Status: Needs work » Needs review
StatusFileSize
new 711-updated.diff5.38 KB

Since the patch still applies otherwise, I just changed it from update hook 6 to 7 in the patch. Here's a new version to test.

vasike’s picture

vasike
Location Ramnicu Valcea
commented

@bmcclure : Thanks for the support
I updated the PR to fix the merge conflict

Is there anything else?

bojanz’s picture

bojanz commented
Assigned: vasike » bojanz

Would be nice to get this into the next release.

bojanz’s picture

bojanz commented
Title: Order total summary formatter does not handle empty orders » Add empty text to the order items view
Category: Bug report » Task

The crash got fixed in #2871569: OrderTotalSummary and OrderItemTable formatters can crash when used inside Views, which leaves the view and tests in this issue.

  • bojanz committed 40c724a on 8.x-2.x authored by vasike 
    Issue #2867430 by bmcclure, vasike, bojanz, finne: Add empty text to the...
bojanz’s picture

bojanz commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.