Avoid overwriting an already updated order

Makes sense, comitted to dev.

First off, thank you for finding this. I think this has caused me headaches with every commerce project I've dealt with. In reviewing it though, shouldn't we be comparing <= instead of <.

If you load and order, make changes to the fields or properties and then load another order, the created values will be the same.

I also came across problems because of this issue when using this module in combination with Commerce Addressbook.

In my case every time the shipping checkout page was loaded a duplicate address (commerce customer profile) was created. Somehow, when loading the checkout page initially the by Commerce Addressbook prefilled address was different than the saved address, thus that triggered creating a new address. But that is a different problem. The point is that new addresses kept being created each time the shipping checkout page was reloaded. That was because the order that holds a reference to the commerce customer profile didn't get saved, although the reference changed. The reason for that was the order's "changed" timestamp did NOT change.

The patch in #4 fixes an issue where the order didn't get saved when it was changed, but the "changed" timestamp did not change.
It does not fix the creation of a duplicate address on the initial page load, but, as I said, that is total a different issue. I just used that as an example to illustrate the problem.

Looks RTBC to me.

We have been running into this issue for some time.
Check of the timestamps does help with it, but also it looks like updated shipping rates do not save after the ajax call as this condition: $stored_order->changed < $order->changed is never true.
Changing the condition to <= allows the changes to be saved, but in this case I think a check is required if the order is still on the same checkout step (in case a delayed ajax request comes in straight after the customer went to the next step). Also all these checks can be done much earlier in the process.

Thanks, the validation looks nice.
I kept the latter condition in the submit handler - better safe than sorry.
@tatyana I had to fix some Coding Standard violations (I recommend Code Sniffer) and the patch wasn't really a git patch. It worked but it takes more effort to get stuff done, which sometimes delays things.

Sorry but I think this patch causes problems with discount module, which often makes changes to the order when commerce_shipping_recalculate_services_validate() calls commerce_order_load (because this can trigger a refresh).

I don't see why we need to throw a nasty exception when the validation finds a changed order. I see the point of "Check if the order is still on the same checkout step" for basic integrity purposes, but the part where it checks for changes and then potentially dies with a nasty message to the user seems problematic.

Again, I'm pretty sure this breaks compatibility with Discount module. Can others verify? I was able to reproduce it simply by

- Install Commerce Shipping and Commerce Discount.
- Put shipping service pane on same page as shipping address.
- Write something in the address and press tab (do this twice, it triggers it the second or third time).
- You'll get the "You are not authorized to access this command" error.

While it is true that Discount module currently churns through line items way too frequently, there are times when it needs to be rebasing line items at this exact point (e.g. if conditions of the discount cause it to be removed).

This issue came to my attention via: https://www.drupal.org/node/2412549#comment-10036629

I'm also seeing problems where I can't submit an order because some other code is changing the order and the shipping throws 'You are not authorized to access this command.', which by the way is a very confusing error message considering the actual problem.

I'm sure I've seen other commerce related modules that load a new order object for actions in the submit instead of using the one in form state, presumably to avoid such things.

Really there needs to be a convention around this because all sorts of things could be changing the order dusing checkout submission.

Either you always load a new order then make your changes and save it, or everyone makes sure that they update the order in form state if they are updating it.

Really there needs to be a convention around this because all sorts of things could be changing the order dusing checkout submission.

Either you always load a new order then make your changes and save it, or everyone makes sure that they update the order in form state if they are updating it.

I totally agree with this. However, what's the way to go here. Should we risk to update a stall order, possibly overwriting other changes?
Shall we load the order, and see which one is newer? If the one load is newer replace the one in form state?

I too would like a convention around this. With any pane being able to load an order and affect changes to it, I'm wondering if there should be a new API function introduced to handle this? Something akin to drupal_static()? (Just a guess.) I'm not sure what or the logistics or how, but I am kind of surprised because, due to this type of issue, in my testing a single-page checkout doesn't really seem to work very well out of the box. The creation of the Checkout Router was supposed to give us that type of flexibility but it seems like other issues are preventing this from being as powerful as we'd like.

For us this issue becomes noticeable when I try to ajaxify panes such as Coupons and payment forms. The ajax stuff does not have a very consistent $order stored and that is causing us to be unable to ship this.

I'd love to know more about a potential direction to go with this, and I'm happy to work on patches and other ideas that help get this working as smoothly as it should be.

I created an issue in the commerce queue for discussion of this: #2533396: Convention for updating the order during checkout.

I'm experiencing this conflict between Commerce Discount and Commerce Shipping as described above and I'm trying to alleviate the pain until a systemic approach is settled on.

In comparing the two (possibly out of sync) order objects, it seems like in my use case the timestamp / changed values are different, but everything else is identical, including line items. Is it reasonable to dig a little deeper in order comparison to see if there is anything we're actually overwriting before failing out so hard?

I'm sure it's not quite that simple, but rather than the timestamp comparison, I'm doing this:

if($form_state['order']->changed < $order->changed) {
	  foreach($order as $property => $value) {
			if($order->$property != $form_state['order']->$property) {
				if(($property != 'changed') && ($property != 'timestamp') && ($property != 'revision_timestamp')) {
					 // Clear all errors.
					form_clear_error();
					watchdog('access denied', check_plain($_GET['q']), NULL, WATCHDOG_WARNING);
					drupal_set_message(t('An error has occurred. Something has modified the ' . $property . ' property'), 'error');
					$form_state['rebuild'] = TRUE;
					return FALSE;
				}
			}
	  }
  }

Am I missing something big here?

The patch committed in #8 causes a critical bug because there is no standard on how to handle $form_state['order']. If another module updates the order object (like commerce_coupon) and doesn't refresh the form_state object, this module throws a useless error and prevents checkout causing loss of sale. This kind of harshness does nothing but piss of customers and store owners. There are several other issues on d.o related to this matter and until a standard protocol is developed and all modules have been updated to follow that protocol, we're going to keep running into issues.

I'm providing an updated that makes sure the form state object is fresh, removes the harsh error, and only returns false if the order is no longer in checkout.

Related #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect.

@andyg5000 re #16 that patch doesn't apply as the path is /includes/commerce_shipping.checkout_pane.inc Or is the -dev release not in sync with the repo?

eh.. I created it with phpstorm. Didn't realize it doesn't create a properly formatted patch.

Regardless, that patch needs work 100% and will likely require other modules to update including commerce core :(

So... calling commerce_order_load() in commerce_shipping_recalculate_services_validate() causes commerce_cart_order_refresh() to get called. This means that any order with an ajax enabled shipping pane and a discount on the order will not be able to checkout because they're presented with 'You are not authorized to access this command'.

In other words $form_state['order']->changed < $order->changed will never pass in this scenario because the order is being refreshed in the process :(

@andy: I haven't experienced this error since using your patch at #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect., and commenting out the entire block that was introduced by the patch in #6. Since the updated order is being sent to the pane's form_state, it seems like this would no longer be an issue if that patch lands. I could be way off but what do you think?

Hey @erik. You're right. If you apply that patch and get rid of the extra validation in commerce_shipping_recalculate_services_validate() all is right with the world ;)

If you just apply the #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. patch then commerce_shipping still tells customer to pound sand :)

Going to work up a patch to remove that extraneous load and validation in shipping shortly

I just created #2593983: Create a utility method to compare an orders changed date against the database which if committed, could be used to to check the order changed value without a refresh being called. However, I think we need to go ahead and remove the errors that are preventing customers with discounts from checking out. Working a patch now :)

Ok, I think we need to revert the patch that was committed in #6 (76e2a50c) and push #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. forward to close this out. If orders are still getting updated outside of checkout then we can hopefully get #2593983: Create a utility method to compare an orders changed date against the database in or write our own for commerce_shipping and use that for validation.

The patch attached is the same as git revert 76e2a50c

I will be reviewing Andy's patch against the reproducible errors in Comment #10 by Dan.

I've done the following:

1. installed the latest dev release of commerce_kickstart-1.x, commerce_shipping-2.x and commerce_discount-1.x
2. Set up a product-level discount
3. Set up a flat rate shipping service
4. moved the select a shipping service to the same page as shipping profile pane
5. Checked out

No errors? I need andy or torgospizza or someone to produce a simple reproducible way for the "you are not authorized" error to show up.

Josh

Did you enable "Calculate shipping rates via AJAX as addresses are updated on the checkout form."? Maybe try an order based discount as well. Let me know if you're still not able to reproduce it. The issue is caused by the fact that commerce_order_load() when called via AJAX (system/ajax) causes the commerce cart refresh methods to fire. These methods delete and re-create line items (thanks to commerce_discount) with each refresh and cause the order to be updated between when the form is loaded and the ajax callback fires the shipping recalc on page load.

Try add another currency. I am having error that every time I change currency on check out page.

Confirming #23! By applying these changes now everything works fine for me! +1 for RTBC to these points from me.

Here is #23, which reverts change, but resets entity controller cache to the loaded order has a proper changed timestamp. Since this issue is happening in AJAX probably, REQUEST_TIME should be properly adjusted.

Note: this is similar to what is provided in #2593983: Create a utility method to compare an orders changed date against the database.

This seems reasonable to revert and fix with this.

Commited and push to 2.x-dev, thanks.

Thanks, current dev including this patch solved this issue for me!

Follow up to this issue #2830800: [Follow-up] Avoid overwriting an already updated order