Problem/Motivation

Currently on drupal/commerce 2.26 and commerce_shipping 2.1

When ShippingInformation.php gets rebuilt, it checks if there is an 'implicit' shipping method selected and attempts to save the order--I believe this was added so that the Order Summary gets updated via ajax. However, the check for this are inadequate and causes the order to be saved multiple times on a single request, causing an OrderVersionMismatchException. One way this manifests is through the CouponRedemption form. Attempting to add a coupon without an address entered in (thus allowing you to select a shipping method) causes the coupon redemption form to save the order, triggers a rebuild, and then ShippingInformation.php attempts to save the order since there is no rate selected. This is common behavior with anonymous shoppers who want to enter their coupon in before they start anything else.

I would imagine any action taken before selecting a rate would cause this issue if that action uses ajax to rebuild the form. Since ShippingInformation.php reruns the buildPaneForm() method, it always causes an extra order save if there is no rate selected.

I cannot get the version mismatch exception to occur locally. Yet if I follow these steps on production, it ALWAYS creates a version exception (currently my site is set to ignore them but it still causes an issue with the Coupon form). Not sure why that would be happening but the issues with the coupon form happen nonetheless, whether the exception gets thrown or not.

I posted a similar comment in another drupal/commerce issue: https://www.drupal.org/project/commerce/issues/3227507

Steps to reproduce

  1. For checkout flow, make sure OrderInformation and CouponRedemption are on the same page and enable the setting 'Hide shipping costs until an address is entered' on the Shipping information configuration. Ensure order type is shippable. Create promotion for any amount and setup so that it should apply to your order with a coupon.
  2. Shop as anonymous user. Add item to cart. Go to Order information page
  3. Do not add shipping address yet. Attempt to add a coupon and it will fail. It may add an Exception into your log for the version exception
  4. A second attempt to add the coupon will work for some reason, which I'm not sure why
  5. If you don't add an address and you remove the coupon (if you try a second time), you can refresh the page and recreate the issue over and over

This snippet appears to be the issue:

    if (!$this->hasRateSelected($pane_form, $form_state)) {
      array_map(function (ShipmentInterface $shipment) {
        if (!$shipment->isNew()) {
          $shipment->save();
        }
      }, $shipments);
      $this->order->set('shipments', $shipments);
      $this->order->save();
    }

Proposed resolution

Patch included. I add a check for empty($shipments) and skip the save if $shipments is empty. This prevents $this->order->save() from running unnecessarily.

if (!$this->hasRateSelected($pane_form, $form_state) && !empty($shipments)) {

Remaining tasks

TBD

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#13 3231138-13.patch877 bytesjsacksick
#3 commerce-shipping-3231138-3.patch854 bytescchoe1
#2 commerce-shipping-3231138-1.patch995 bytescchoe1

Comments

cchoe1 created an issue. See original summary.

cchoe1’s picture

cchoe1 commented
StatusFileSize
new commerce-shipping-3231138-1.patch995 bytes

Patch attached

cchoe1’s picture

cchoe1 commented
StatusFileSize
new commerce-shipping-3231138-3.patch854 bytes

Sorry, I think I messed that patch up. Rerolling against 8.x-2.x, fingers crossed

cchoe1’s picture

cchoe1 commented
Issue summary: View changes
agoradesign’s picture

agoradesign commented
Parent issue: » #3227507: Exception\OrderVersionMismatchException

marking #3227507: Exception\OrderVersionMismatchException as parent

jsacksick’s picture

jsacksick commented

The thing is, I'm not sure we can commit this, as I'm pretty sure the current code ensures the shipping adjustment is removed from the order summary, if the order couldn't be "packed" (i.e no shipments were returned).

Otherwise, the shipping adjustment would stick... (needs to be confirmed).

I anticipated this code to be problematic, but it turns out that refreshing the order summary properly was more challenging than anticipated.

cchoe1’s picture

cchoe1 commented

@jsacksick, thank you for reviewing this, I'm definitely not privy to the edge cases that might arise from making a change like this. Seems like there could be another issue from this change if a rate is already selected and then something happens that causes the rates to change or disappear?

I forgot we also have 'Hide shipping costs until an address is entered' enabled for our checkout flow. I will update the steps to recreate with this information but I believe this setting is also integral for this bug to crop up so that rates aren't shown by default for anonymous shoppers. If you have rates shown by default like a flat rate, I think that would be selected by default when you land on the Order Information page thus causing the extra $order->save to skip within the buildPaneForm()

cchoe1’s picture

cchoe1 commented
Issue summary: View changes
JeremyFrench’s picture

JeremyFrench commented

Just noting here that the patch from #3 did not resolve the issue for me.

JeremyFrench’s picture

JeremyFrench commented

So I have looked into this, and think I know the issue. I'll write up then write a patch.

The commerce checkout panes by default have an order that is injected when they are created then the panes are processed in turn. Some of these may only take an order_id and then save the order, this means that the order that is in the checkout panes is not up to date.

Any of these panes which save the order will cause the exception (and or) overwrite the updated data.

This has been looked at in commerce as this https://www.drupal.org/project/commerce/issues/3096966

The solution there was to use the order ID and load the order. Rather than relying on a serialised version. This means that the checkout pane should always have the correct version.

JeremyFrench’s picture

JeremyFrench commented

I've not managed to make a nice patch for this.

I've worked around by extending the ShippingInformation checkout pane and putting this at the top

public function buildPaneForm(array $pane_form, FormStateInterface $form_state, array &$complete_form) {

    // Update the order to be the latest version.
    $order_id = $this->order->id();
    $order_storage = \Drupal::entityTypeManager()->getStorage('commerce_order');
    /** @var \Drupal\commerce_order\Entity\OrderInterface $order */
    $this->order = $order_storage->load($order_id);

This means that the correct version of the order is used in the pane and if it is saved it won't overwrite information.

jsacksick’s picture

jsacksick commented

-

jsacksick’s picture

jsacksick commented
Status: Active » Needs review
StatusFileSize
new 3231138-13.patch877 bytes

I managed to reproduce the issue and realized we're probably saving the order too often.

Also, there's still one remaining "bug" with the order summary refresh. If basically the order already references shipments, and the address is changed, if no shipping rates are returned, the shipping adjustment sticks, where it shouldn't, we probably need a followup issue for that.

@JeremyFrench: Could you check with the attached patch? While the patch doesn't address the order being potentially stale, it skips the order save if shipping wasn't recalculated.

@cchoe1: Could you test the attached patch as well?

Status: Needs review » Needs work

The last submitted patch, 13: 3231138-13.patch, failed testing. View results

jsacksick’s picture

jsacksick commented

I need to investigate the tests failures, but have you guys had a chance to test the patch?

cchoe1’s picture

cchoe1 commented

Sorry @jsacksick I will test by Monday or so, I've been a little bit tied up on my end. I will edit this post once I try it out

cchoe1’s picture

cchoe1 commented

@jsacksick I removed my version of the patch, followed my original steps to reproduce (which did reproduce the error) and then applied your patch. I tested the checkout page by hand a few times and didn't run into any issues, yay!

I'm not too sure about the automated tests, my work station isn't currently setup for testing so unfortunately I don't know if I can be much help on that end. My patch also generated the same issue with the tests

travis-bradbury’s picture

travis-bradbury commented
Status: Needs work » Reviewed & tested by the community

I can replicate the bug as described in the issue summary. With the patch, the problem goes away.

The tests are also passing now. I think that #3241178: Shipping methods change automatically when a promotion with display_inclusive is active fixed that. The test was failing because it expected "Shipment #1 $9.99", but it actually had "$16.99", the amount of overnight shipping, rather than the amount of standard shipping. That sounds just like #3241178, which would explain why it's passing since that issue was committed.

I re-ran the tests on the patch in #13 and it's passing now.

  • jsacksick committed 37546ad on 8.x-2.x 
    Issue #3231138 by cchoe1, jsacksick, tbradbury: Prevent order version...
jsacksick’s picture

jsacksick commented
Status: Reviewed & tested by the community » Fixed

Committed! Thanks everyone!

Status: Fixed » Closed (fixed)

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

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: +#3438654: Drupal\commerce_order\Exception\OrderVersionMismatchException: Attempted to save order 125081 with version 8. Current version is 9. in /app/web/modules/contrib/commerce/modules/order/src/Entity/Order.php:682

Similar issue still seems to exist, see #3438654: Drupal\commerce_order\Exception\OrderVersionMismatchException: Attempted to save order 125081 with version 8. Current version is 9. in /app/web/modules/contrib/commerce/modules/order/src/Entity/Order.php:682