Order item modifications does not recalculate shipping total

This is not implemented yet.

Here is a patch that recalculate shipping rates. The only limitation for the moment is that in case of multiple rates, we don't know which one was used, so we take the first one.

To be able to deal with many rates recalculation rates, some huge efforts should be done

Patch is missing

A line has been removed, here is the fixed patch

And here is a patch for beta4 version of module. Do not use this for development.

Fix #7 for beta-4 version which embed code from -dev.
DO NOT use this for development.

Here is a fix for both which occur when shipment has no method yet.

DO NOT use -beta4 for development.

I'm actually facing the same issue here, and I have a feeling its rather something with the shouldRepack method. Sometimes it gets executed sometimes it doesn't. Maybe a flag should be set if only items quantity has changed. This patch seems to work half of the time. Can you test the following: Add a product, go through the checkout process until you reach the review page... then go in the cart and edit the quantity and checkout once again. On my end, the shipping cost never gets reCalculated and I think its the way shouldRepack is coded.

@webadpro Your steps looks like what is described in main post and is the same as my tests, and shouldRepack return TRUE as expected. Can you provide more information ? Do you try with patch 2957513-10-shipping-recalculation.patch in #10 ?

@GoZ, it seems if you are in the last step right before the payment, then the shouldRepack always returns false which doesn't make it recalculate. And yes I did applied your beta4 patch.

Right, there is something about this case in comments. When we are on cart, previous steps are both null and we cannot know if quantity has changed.

In ShipmentOrderProcessor::shouldRepack():

    // Ideally repacking would happen only if the order items changed.
    // However, it is not possible to detect order item quantity changes,
    // because the order items are saved before the order itself.
    // Therefore, repacking runs on every refresh, but as a minimal
    // optimization, this processor ignores refreshes caused by moving
    // through checkout, unless an order item was added/removed along the way.
    if (isset($order->original) && $order->hasField('checkout_step')) {
      $previous_step = $order->original->checkout_step->value;
      $current_step = $order->checkout_step->value;
      $previous_order_item_ids = array_map(function ($value) {
        return $value['target_id'];
      }, $order->original->get('order_items')->getValue());
      $current_order_item_ids = array_map(function ($value) {
        return $value['target_id'];
      }, $order->get('order_items')->getValue());
      if ($previous_step != $current_step && $previous_order_item_ids == $current_order_item_ids) {
        return FALSE;
      }
    }

I suggest to add a flag when quantity is updated so we know we need repack. See patch.

DO NOT use -beta4 for development.

Shame on me, i miss a missing ;

Fix and relaunch tests

DO NOT use -beta4 for development.

There was a (quite hidden) typo in #18, which you can see in the failed test results. Here's the fixed version

Would this be compatible with the recent Beta 5 release or only dev?

I have applied the patch against beta5 successfully

Thank you, applied cleanly for me too

fixing the CS violation

Hi, the bug is still present and the patch doesn't work on current -dev version.

Changed to 'Needs Work' based on FiNeX's comment.

rerolled against latest -dev

This patch successfully solves the problem of updating the shipping amount.
The FAIL patch has the EventSubscriber commented out in the services.yml. This should be equivalent to running the new test the clean 8.x-2.x-dev branch.

This patch successfully solves the problem of updating the shipping amount.

What did you do to fix that problem? The pass interdiff contains only a new test.

The fail patch with commented out code is unexpected. And it is a service so there is code in the fail patch not related to the failure. Is the test sufficient to demonstrate the failure? If so, can we have a patch with just that?

Hi, the "shipping rates" widget works fine, but the cart summary still display the previous shipping rate.

Customers still needs to go to the review page in order to view the correct total.

Now that https://www.drupal.org/node/2710999 has been solved (panes now should support ajax refreshing) the behaviour should be fixed.

Until then the shipping module is unusable because users doesn't see correct values :-(

@FiNeX

Could you send a screenshot of the error you are seeing? The test I wrote focuses on the /cart page and the summary that is shown when you change quantities and 'Update Cart'.

Perhaps we are addressing different errors?

@quiteone
I have not done anything to fix this issue, the solution was already in patch 28 and previous. I wrote a test to try prove that the issue is fixed.

Are these patches more helpful?

@FiNeX

This is the page I was focusing on: /cart

This correction is necessary, customers get confused when they return and add more products to the shopping cart and the shipping method is not updated.

I've recorded a short video which clearly show the bug. Using commerce -dev, shipping -dev and your latest patch (without your patch nothing change): https://www.youtube.com/watch?v=8ZnaxSLMoQw

Thank you for the video @FiNeX.

You have found a rather complex set of problems. Let me try separate each issue.

There are three problems that you pointed out:

1. The first time the cart page (/cart) is loaded, the shipping total is not displayed.

Explanation: This is because the shipping method has not been applied to the order. This is the default behavior because there are many shipping types than cannot be calculated until the shipping address is known.

You raise a good point that when a 'Flat Rate' or 'Flat Rate Per Item' shipping method is selected, we should not need to wait to see the shipping address.

Solution: Apply shipping method to order when 'Add to Cart' is clicked, if applicable.
I have created this new issue to make this automatic: #3032265: Default shipping method for Order based on conditions
Also, this existing issue would would resolve this problem and problem 2a in the Cart form.#2937403: Allow cart to have shipping selection

2a. The first time the 'Order Information' page (/checkout/{order_id}/order_information) is loaded, the shipping total is not displayed in the 'Order Summary'.

Explanation: This is because the shipping method has not been applied to the order.

Solution: If the shipping method is applied to the order before the 'Order Information' page loads, this problem will be solved. The solution is connected to the new issue created for problem 1.

2b. The shipping method radiobutton is listed within the 'Shipping Information' container, but the shipping total is not updated/listed in the Order Summary.

Explanation: This is because the shipping total is not recalculated until 'Continue to review' form submit button is clicked.

Solution: #2898118: Update the order summary via AJAX when a shipping rate is selected

3. When you went back to your cart and updated the Item Quantity in your order, the new total price should have set failed your condition of shipping method 'Flat Rate' only applies to orders LESS THAN 80€.

3a. On /cart page, 'Update Cart' does not recalculate shipping total.

Solution: Patch 36 and previous address this problem. This issue should be limited to solving only this issue. I will update the description.

3b. On /cart page, 'Update Cart' does not re-assess shipping method conditions.

Solution: New Issue: Assess shipping method conditions when 'Update Order' is clicked.

Thanks @nlz, great analysis :-)

To avoid customers from seeing an outdated shipping cost in their cart, I've applied the patch from this related issue which does exactly what it says.

#43 I think it is the right solution.

This needs to be reviewed alongside #2994345: Remove shipping when cart is emptied. This would technically cause duplicate efforts. And we cannot always assume the cart module is available.

We have two problems:

- Cart manipulation
- Orders without cart module having order items updated.

In #2994345: Remove shipping when cart is emptied I am proposing a new flag that can be set on orders to force repacking

+  public function onCartItemUpdate(CartOrderItemUpdateEvent $event) {
+    $cart = $event->getCart();
+    $cart->setData('shipping_force_repacking', TRUE);
+  }

This addresses my concern of duplicate processing in #45. Possibly.

I believe a flag on the order is better than a flag on the processor

+   * Flag shipments needs to be repacked.
+   *
+   * @var bool
+   */
+  protected $needsRepack = FALSE;
+

An order item save operation should correlate to a leading order save. So hopefully we can set the flag in this event subscriber and have it persist. Tests will tell.

Postponing for #2994345: Remove shipping when cart is emptied. Both are trying to solve the same problem. This issue has a patch which fixes recalculation. I think we should work on the other issue first, and then we can use this to ensure recalculation is available.

Discussed with bojanz. We will handle order item CRUD in this issue for updating shipments

On #47 comment, the issue reference #2994345: Remove shipping when cart is emptied change subject. Talking about empty cart make more sense than removing shipment each time cart is refreshed.

Reading #47 the first time made me think shipping we'll be removed each time cart is updated. That's not the case, and it's fine !

Here is a rerolled patch which takes @nlz's work in #37 for \Drupal\commerce_shipping\Entity\Shipment::populateFromProposedShipment to recalculate costs.

It also takes work from my initial patches in #2994345: Remove shipping when cart is emptied for tests and flagging an order to be repacked.

Besides being confusing without the patch it is possible to complete the checkout with completely wrong shipping amount.

Steps to reproduce:
- Create shipping method with "Flat rate per item" plugin.
- Add product to cart and go to checkout
- Fill in Shipping information and click "Recalculate shipping"
- Go back to cart and change item quantity
- Complete checkout

With current -dev version of the module and patch from #50 everything works great.

Thank you!

*DO NOT use patch from #50 with -beta6.

Applied patch #50 to the latest dev. It needs work.

When modifying the cart, the shipping rate does not update on the cart form. Fine.

But when proceeding to the Order Information step, the shipping rate in the Shipping Information pane is updated, while the old rate continues to be displayed in the Order Checkout Summary of the sidebar. Only when clicking Continue to Review does the Order Checkout Summary get the new rate.

If a customer sees this discrepancy on the Order Information step, he is likely to:

1. click the recalculate shipping
2. refresh the page
3. click the recalculate shipping again
4. think there is something wrong with the shop
5. hesitate to click on to the next step
6. get annoyed
7. think of contacting the store
8. give up and maybe try again later

This is a UX no-go.

Reroll patch from #49 against beta8 (without the kernel test, as this is WIP anyway and -dev already has some changes inside).. just trying to get this work with beta8

sorry, here it is

next try

Thanks for the update @agoradesign

Attaching a new patch against the latest dev with some additional changes.
We could cover also updating properly shipping amounts on a cart and do the recalculation of these in the early order processor.

Added tests for that case also. So now should be possible to have:
Add to cart -> order information -> choose shipping -> review -> cart -> update items -> recalculate and apply proper amount on cart

Note that when the shipping method needs to be changed, it is selected as they are sorted by amount. So with the lowest amount should be automatically chosen on the cart. And displayed proper shipping adjustment amount

The test is covering three scenarios for cart/checkout
1. Update order -> the shipping methods/rates stay the same
2. Update order -> shipping rates / methods needs update
3. Update order -> clear shipments while there are not available rates.

About placement of those, think it is viable to have an additional method in \Drupal\commerce_shipping\EarlyOrderProcessor which handles these cases (\Drupal\commerce_shipping\EarlyOrderProcessor::recalculateShippingRates)

Please test it.

Some coding standards cleanups.

Drupal\Tests\commerce_shipping\FunctionalJavascript\CheckoutPaneTest is failing on all tests, regardless of this patch.

But this one is failing seems only with a patch here Drupal\Tests\commerce_shipping\Kernel\ShipmentManagerTest

Committed #3116985: CheckoutPaneTest are failing because credit card in tests are using past dates for expiration.

Let's rerun this!

It looks like when we reset the shipments and pick the shipping method we don't ensure it is translated.

1. +++ b/src/EarlyOrderProcessor.php
   @@ -33,6 +33,11 @@ class EarlyOrderProcessor implements OrderProcessorInterface {
   +  protected $shippingManager;
   
   @@ -40,10 +45,13 @@ class EarlyOrderProcessor implements OrderProcessorInterface {
   +   * @param \Drupal\commerce_shipping\ShipmentManagerInterface $shipping_manager
   +   *   The shipping manager.
   ...
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ShippingOrderManagerInterface $shipping_order_manager, ShipmentManagerInterface $shipping_manager) {
   ...
   +    $this->shippingManager = $shipping_manager;
   

   s/shippingManager/shipmentManager

2. +++ b/src/EarlyOrderProcessor.php
   @@ -66,13 +74,69 @@ class EarlyOrderProcessor implements OrderProcessorInterface {
   +          $shipment->setShippingMethodId($shipping_rate->getShippingMethodId());
   +          $shipment->setAmount($shipping_rate->getAmount());
   +          $shipment->setOriginalAmount($shipping_rate->getAmount());
   +          $shipment->setShippingService($shipping_rate->getService()->getId());
   

   This is the same logic as

   public function selectRate(ShipmentInterface $shipment, ShippingRate $rate) {
   

   We should get the shipment method plugin and use selectRate on the given rate for the shipment.

3. +++ b/src/EarlyOrderProcessor.php
   @@ -66,13 +74,69 @@ class EarlyOrderProcessor implements OrderProcessorInterface {
   +        // Assign new shipping rate only if existing is not among
   +        // listed available rates for shipment.
   +        if (!isset($shipment_rates[$shipping_rate])) {
   

   If it is set, shouldn't we still run selectRate on the shipping method plugin.

   So we get the rate, then

   $shipping_method = $shipping_method_storage->load($rate->getShippingMethodId());
   $shipping_method_plugin = $shipping_method->getPlugin();
   $shipping_method_plugin->selectRate($shipment, $rate);
   

We should replace it with a proper select rate when does not exist.
But when rates exist, we should not alter anything. They are valid.

Funny thing is, that during tests which fail: ShipmentManagerTest,
it's never triggered on \Drupal\commerce_shipping\EventSubscriber\OrderItemSubscriber
$order->setData('shipping_force_repacking', TRUE);

So any underlying logic which lies on that, it is not executed during those tests, \Drupal\commerce_shipping\EarlyOrderProcessor::recalculateShippingRates then returns everything as they should be without the patch.

The crazy thing is If I remove that event subscriber \Drupal\commerce_shipping\EventSubscriber\OrderItemSubscriber then tests pass.

Even during test condition to set $order->setData('shipping_force_repacking', TRUE);
is never executed

New patch.

Replaced as per #65 with selectRate, seems cleaner.

About failing tests, it is related to container states or something during tests that are executed.
When removed shipment order manager from an event subscriber, and using from #55 method to determine if the order is shippable, all tests should be green

And additional comment to address one of the issues in the description

However (The problem) I still see a shipping cost of 10euro even when my order total is already way beyond 200euros.

Basing shipping methods condition on Order Total Price condition is not going to work properly in a lot of cases.
Because shipping adjustment amount is mostly included inside Order total price when that condition is executed.

Best would be to make a fork of Order total price condition without shipping adjustments included, or basing condition on Order Subtotal amount.

Or the best scenario maybe would be to extended Order Total Price in commerce core with the ability to include/exclude specific adjustments from the calculation

Best would be to make a fork of Order total price condition without shipping adjustments included, or basing condition on Order Subtotal amount.

Or the best scenario maybe would be to extended Order Total Price in commerce core with the ability to include/exclude specific adjustments from the calculation

That's being worked on here #2938729: Condition based on order subtotal instead of order total

"ability to include/exclude specific adjustments from the calculation" sounds better than just picking subtotal -> when I calculate shipping costs based on a total amount, obviously I do not want the previously calculated shipping costs included, but on the other hand I still want promotions to be applied

Going to build off of #67. For the order total comparison, I would love some effort on the core Commerce module issue.

1. +++ b/src/Entity/Shipment.php
   @@ -104,7 +105,15 @@ class Shipment extends ContentEntityBase implements ShipmentInterface {
   +    if ($this->getShippingMethod() !== NULL && !$original_items->equals($this->get('items'))) {
   +      $rates = $this->getShippingMethod()->getPlugin()->calculateRates($this);
   

   I'm going to throw some debugging here and test this. I think this will always be false (which is probably fine.) The FieldItemList::equals does a strict check at first, which will fail if we didn't persist the same objects (I doubt we do.)

   It then later does a loose check against objects which might pass.

2. +++ b/src/EventSubscriber/OrderItemSubscriber.php
   @@ -0,0 +1,60 @@
   +    if ($order !== NULL && $this->isOrderShippable($order) && $order_item->getQuantity() !== $order_item->original->getQuantity()) {
   +      $order->setData('shipping_force_repacking', TRUE);
   +    }
   

   Should we just always force recalculation if the order item updated? in 90% of use cases it will update because of the quantity. The other 10% would be for custom fields modified somehow, later. Might prevent follow ups to this line of code.

3. +++ b/src/EventSubscriber/OrderItemSubscriber.php
   @@ -0,0 +1,60 @@
   +  private function isOrderShippable(OrderInterface $order) {
   +    return $order->hasField('shipments') && !$order->get('shipments')->isEmpty();
   +  }
   

   Weird that we have to provide this instead of using the shipping order manager. But I have seen goofier things in test state.

   EDIT I think it is due to the fact this has completely different logic. \Drupal\commerce_shipping\ShippingOrderManager::isShippable checks each purchasable entity whereas this just checks if shipments have been calculated yet.

Okay, here is an update. Also did some discussion with @valic. We renamed the order data key to shipping_force_refresh because it forces a repack and recalculation. It was also moved to a constant.

Also: interdiff is kind of wrong, had it as FORCE_PROCESSING not FORCE_REFRESH.

+++ b/src/Entity/Shipment.php
@@ -90,6 +90,7 @@ class Shipment extends ContentEntityBase implements ShipmentInterface {
+    $original_items = clone $this->get('items');

This isn't used anymore. Forgot to delete.

I have pinged a few folks in Slack to review the patch. Here's a patch with #73 addressed. I'll be committing this within the next day unless someone objects.

I have some questions and proposals, upon reading the patch (not have tested it):

+  public function onOrderItemUpdate(OrderItemEvent $order_item_event) {
+    $order_item = $order_item_event->getOrderItem();
+    $order = $order_item->getOrder();
+    if ($order !== NULL && $this->orderHasShipments($order) && $order_item->getQuantity() !== $order_item->original->getQuantity()) {
+      $order->setData(EarlyOrderProcessor::FORCE_REFRESH, TRUE);
+    }
+  }

I'm not sure, if asking if the order has shipments is right here (on item delete it makes sense of course). What if the updated quantity is higher than the original and now an order (sub) total condition is met that wasn't met before? I personally always calculate a rate, even a zero one, but I guess there might be value-based configurations too. Maybe we should check, if the order item is a shippable purchasable entity, and react on every quantity change?

+        $shipping_rate = $shipment->getShippingMethodId() . '--' . $shipment->getShippingService();
+        $shipment_rates = $this->shipmentManager->calculateRates($shipment);

$shipment_rates is actually an array of \Drupal\commerce_shipping\ShippingRate objects, so shouldn't we call that $shipping_rates instead? And $shipping_rate is a string used to identify a rate, but the name suggests that it's rather a ShippingRate object too. So shouldn't we call it $shipping_rate_id or $shipping_rate_identifier instead?

@agoradesign there is test coverage for these cases. (increasing quantity on a cart, and calculating new rates)

If you increase quantity, and your current rate needs to be recalculated, new is going to be assigned.

You need to see the entire block of code why decided to go with this string
$shipping_rate = $shipment->getShippingMethodId() . '--' . $shipment->getShippingService();

calculateRates returns rates keyed by the example above, as $shipping_rate. So for easier / faster logic with isset check.

Let say we have two shipping methods:
- 5 products - shipping rate is 10$
- more than 5 products - free shipping

So we have 2 products in cart - and shipping cost 10$.
If I add an increase to 4 products in the cart, shipping is the same

calculateRates fetch all available rates, and one among them is $shipping_rate -> so we don't change anything. Current rates are valid (even if we have more valid rates, but why changed it to something else, while we quessing here)

If I add an increase to 10 products in the cart, shipping should be updated.
We don't have any more inside calculateRates our shipping rate
if (!isset($shipment_rates[$shipping_rate]))

so we select a new rate and update it

        // On order item update we may need to use different shipping methods.
        $shipping_rate = $shipment->getShippingMethodId() . '--' . $shipment->getShippingService();
        $shipment_rates = $this->shippingManager->calculateRates($shipment);

        // There is no rates for shipping. Clear that shipment.
        if (empty($shipment_rates)) {
          unset($shipments[$key]);
          $shipment->delete();
          continue;
        }

        // Assign new shipping rate only if existing is not among
        // listed available rates for shipment.
        if (!isset($shipment_rates[$shipping_rate])) {
          $rate = $this->shippingManager->selectDefaultRate($shipment, $shipment_rates);
          $shipping_method_storage = $this->entityTypeManager->getStorage('commerce_shipping_method');
          /** @var \Drupal\commerce_shipping\Entity\ShippingMethodInterface $shipping_method */
          $shipping_method = $shipping_method_storage->load($rate->getShippingMethodId());
          $shipping_method_plugin = $shipping_method->getPlugin();
          $shipping_method_plugin->selectRate($shipment, $rate);
        }

@valic thanks for the explanation. I just wanted to be sure that this works.

Regarding the variable naming - I do have seen, why there is this string, and that's fine. But I recommend a different naming, like $shipping_rate_identifier instead of $shipping_rate, as well as $shipping_rates instead of $shipment_rates. This would improve readability a lot

@agoradesign agree, naming then would be more self-explanatory.

Let me reroll the patch. Also, conditions with order subtotal or any similar are going to play well with this patch. (except order total price because of stil of #68 #69.

Planned to do test coverage based on amounts, but would require plugin which has shipping adjustments excluded

Updated naming. Plus corrected one old comment

-   * Constructs a new LateOrderProcessor object.
+   * Constructs a new EarlyOrderProcessor object.

I've been reviewing this and planning on posting an updated patch later today (Probably minor changes)...

I've been making some changes to the patch, but still not 100% satisfied with it...

First of all, I'm wondering if the rates recalculation code is necessary in populateFromProposedShipment() (we by the way don't call selectDefaultRate and manually use the first rate available()...).

Also, I'm suspecting the rate to be applied, even if no rates were already applied.
I think our test coverage is insufficient...

Also, wondering about the following:

1. What happens when adding a new order item? Since we react to update/delete (Are we not recalculating there?)
2. I'm pretty convinced we should be used cart events instead, this way we target cart operations more cleanly, and we have direct access to the order object (instead of getting it from the order item). If we do use cart events, we'll have to make sure Commerce API is using the cart manager to update order items.
3. I moved the FORCE_REFRESH constant to the ShippingOrderManagerInterface
4. Related to 2) Our functional test focuses on testing cart/checkout, so we don't even test order item CRUD.
5. Is there any risk of updating the rates without the customer noticing?

I'll submit a different patch which will use the cart events instead so we can choose.

So we need to:

• Use CartEvents (discussed with @jsacksick and @valic and I believe we're on agreement here)
• Expand test coverage to also add an item to the cart.

Yeah?

Following populateFromProposedShipment, let's check when it is used. Maybe that `@todo` was moot.

I expanded test coverage to cover adding a new item to the cart (This wasn't recalculating the rates, it is now).

I also managed to remove the duplicated logic in populateFromProposedShipment() and I'm pretty sure we don't need it anymore after the change I made in EarlyOrderProcessor.

I discovered a "bug" while running the tests, since we were deleting the shipments when no rates were returned we were loosing data (mainly the shipping profile), I fixed that by setting the amount, original_amount to NULL, as well as the shipping method (perhaps we should move that to a method on the ShipmentManager).

I'm fine with keeping the order item subscriber (instead of cart events, the result would be the same anyway).... And we implement the cart event for adding an order item to the cart...

+++ b/src/EarlyOrderProcessor.php
@@ -86,23 +86,20 @@
+        $shipment->set('shipping_method', NULL);
+        continue;

We also need to nullify shipping_service

As discussed on Slack, we should open a follow-up to move this logic in a resetRate() method on something on the shipment itself.

Committed! Thanks everyone!

Great work! I will try soon the update, thanks :-)

This is not working well for me:
I have shipping method in value of 5CHF and with condition "Current order total Less than 50CHF"
I'm testing with cart item in value of 42CHF so only 1 should trigger shipping costs and more than 2 shouldn't.
Situation I have actually is that calculation should shipping be applied or not is made based on previous cart state. So if I had 1 that item before cart update shipping is added. And if I had more than 1 before cart was updated shipping is not added. Shipping costs are always 1 cart update behind somehow.

Another issue I'm facing is that I do not see any shipping costs (conditional or not) until I go to offsite payment and return (by canceling the process of payment). After returning shipping is shown. Before that there is no shipping information of any kind. "Update cart" on cart page or "Recalculate shipping" order information page are not helping.

- So there is some action which is triggered when user is redirected to offsite payment only and should be executed earlier, most likely every time cart page is displayed.
- Cart update is done in some wrong order - first items number should be updated, total sum calculated and then shipment calculations called. From my point of view it's done opposite way so shipping is calculated before cart is updated?