Problem:

Adjustments (in my test case, I have a 'shipping' type order item adjustments) which are correctly displayed in the adjustments property from OrderTotalSummary::buildTotals, are not included in the calculation of the order total (Order::getTotalPrice()) until after the first cart item quantity is updated or when the second order item is added.

Note: The reason the OrderTotalSummaryTest explicitly skips the order refresh process is because we haven't implemented a way to lock adjustments ... so if a refresh happens it blows away whatever you added (with the exception of custom adjustments).

Steps to reproduce:

  1. Implement an commerce_order.order_processor service to add order item adjustments (like this).
  2. Add an item with adjustment(s) to the cart.
  3. Notice the adjustments are included in the summary, but the subtotal and total are the same.

    Total not correct

Fix/hack/workaround:

Skip order refresh and save order before getting total in OrderTotalSummary::buildTotals:

<?php
  ...
  public function buildTotals(OrderInterface $order) {
    $order->setRefreshState(OrderInterface::REFRESH_SKIP);
    $order->save();
    ...
    return [
      'subtotal' => $order->getSubtotalPrice(),
      'adjustments' => $adjustments,
      'total' => $order->getTotalPrice(),
    ];
  }

Proposed resolution:

(Not sure yet).

CommentFileSizeAuthor
#5 order_total_summary-2860646-5.patch2.19 KBmglaman
Screen Shot 2017-03-14 at 4.35.41 PM.png21.06 KBsteveoliver
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steveoliver created an issue. See original summary.

steveoliver’s picture

steveoliver CreditAttribution: steveoliver at Circatree commented

Put up a WIP PR at: https://github.com/drupalcommerce/commerce/pull/671

steveoliver’s picture

steveoliver CreditAttribution: steveoliver at Circatree commented
Issue summary: View changes
Related issues: +#2805355: Add the ability to "Lock" an Adjustment so it cannot be removed during refresh.

Linking related issue and adding a note in the issue summary about why we skip refresh in test.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro for LivePerson, Inc commented

Subbing, as I want to add test coverage for order item adjustments.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro for LivePerson, Inc commented
Status: Active » Needs work
FileSize
order_total_summary-2860646-5.patch2.19 KB

Here's a patch which fixes the issue. It's due to the order total not be recalculated after the refresh. Which the method is protected. So we either move the preSave storage logic into the entity (which the storage invokes anyways) or we make recalculation a public method.

EDIT: Putting to needs work. First things first is we need to do some test coverage for this. A test for OrderTotalSummaryTest could benefit from registering a one-off order processor into the container which adds the adjustments.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro for LivePerson, Inc commented
Assigned: Unassigned » mglaman

First things first. We need to make a test module which will provide those adjustments, I think so that they persist and apply at refresh. And we need to have the test fail to show order item adjustments get lost in the order total calculation.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro for LivePerson, Inc commented

Okay, I closed original PR and opened this one: https://github.com/drupalcommerce/commerce/pull/700

This adds a order refresh processor in commerce_order_test and allows for applying adjustments during refresh via some data options. The problem is.. it works. Tests pass. Completely. I cannot replicate the original issue which I had happen in a client site.

So I pushed the updated test and passing result to at least track improvement and incremental steps to getting a failing test. The goal is to break our test.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro for LivePerson, Inc commented

Okay, the first commit passed https://travis-ci.org/drupalcommerce/commerce/builds/217013153

Here's the second which should fail (it did locally): https://travis-ci.org/drupalcommerce/commerce/builds/217016647

Tests

  • Order adjustment only: PASS
  • Order item adjustment only: FAIL!
  • Order & item adjustments: PASS

I don't know why or what, but for some reason, it's like the order does not recalculate itself when only an order item is adjusted. I don't know if it's because, for some reason, the order wasn't modified?

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro for LivePerson, Inc commented
Status: Needs work » Needs review

Okay, fixed the tests and got it to pass by making sure the order total is explicitly refreshed in the refresh process. When the order has an adjustment added we force calculation. Our order adjustment test happens to add those last which caused false positive pass.

Nothing causes an order's total to recalculate, currently, when order items have adjustments. The entity class preSave is invoked before the storage's, which is when refresh happens.

https://travis-ci.org/drupalcommerce/commerce/builds/217019305

Yay!

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro for LivePerson, Inc commented

:( It failed because we just uncovered a long lasting bug. The order admin test fails because we've assumed the admin entered price will be correct.

    // @todo Admin specified pricing is overridden due to order refresh.
    // This should equal 5.33. Instead it's (999.00 * 3) + 2
    $this->assertEquals(new Price('2999.00', 'USD'), $order->getTotalPrice());

We need to open a follow up discussing admin order edit/create price overrides. Right now we expose the input but any input will get destroyed on save and reset.

This build should do it https://travis-ci.org/drupalcommerce/commerce/builds/217023461

steveoliver’s picture

steveoliver CreditAttribution: steveoliver at Circatree commented
Related issues: +#2833249: Allow providing a custom order item price

@mglaman, that follow-up issue you mention, I believe already exists as #2833249: Allow providing a custom order item price.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Title: Order total summary (total amount) not including adjustments » The order total doesn't always include order item adjustments
Priority: Normal » Major

  • bojanz committed 109bcae on 8.x-2.x authored by mglaman 
    Issue #2860646: The order total doesn't always include order item...
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Fixed

Thanks, guys.

Status: Fixed » Closed (fixed)

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