I've been working on the commerce_userpoints module to improve its UX rather drastically. The code is published here: http://drupal.org/sandbox/gdarends/1787934

Right now what that basically does is add a slider to use userpoints as credit to pay for the order. The slider does an AJAX request, submitting the amount of points to use, and refreshes the cart_contents and checkout_review panes to reflect the change to the order.
Now it may be possible that a user has enough points to make the order total reach €0 in which case I'd like to also enable the "No payment" payment method. To accommodate this workflow, I've made the AJAX callback also refresh the commerce_payment checkout pane. I've also changed the payment method rule for "No payment" to only be enabled when the order total is €0

Unfortunately, nothing I do (except for a full pageload) makes the No payment method show up! The JSON data that is returned to the browser contains the exact same payment methods as the HTML request of the page. What's worse, is that it's possible to fool the system. I make sure the order total is €0, reload the page, select the "No payment" payment method, return the slider to its original state (not using any userpoints), and submit the order. The order goes through!

Trying to do a debug print from commerce_payment_pane_checkout_form() reveals that apparently, the code isn't even executed during AJAX requests. How is that possible? In any case, I can work around this problem by putting the slider on a different checkout page, but this appears to be a bug from my perspective.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

a.ross’s picture

Correction, the code does appear to be run, since a log entry is created when I use watchdog. A dsm() for some reason didn't work, even though other debug outputs did work -_-'
Anyway, now I'm really at a loss :/

rszrama’s picture

Category: bug » support
Status: Active » Postponed (maintainer needs more info)

Sounds like something isn't being saved in the order during your AJAX request; I doubt there's something Commerce specific going on in here. The only real Commerce related issue is the trapping of Drupal messages; they can be orphaned unfortunately, but there's another issue for addressing that (or it might already be fixed in -dev, can't remember off the top of my head). I would just use watchdog().

Not sure if there's really a problem here, but I'll move this to postponed unless you find a reproducible issue.

a.ross’s picture

Thanks, debugging reveals that the $order variable in commerce_payment_pane_checkout_form() is incorrect. Do I need to call some explicit entity_save or something? I'll have another look next week.

rszrama’s picture

Not sure off the top of my head; sounds like it's just using a cached version of the order in the $form_state or simply hasn't done the final save on the item yet.

a.ross’s picture

Title: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. » Problems with payment methods with AJAX form submission in checkout.
Version: 7.x-1.x-dev » 7.x-1.3
Status: Needs review » Postponed (maintainer needs more info)

It's pretty strange. I've checked and the commerce_userpoints_checkout commerce_userpoints_discount module already instantly saves the order object, so that cannot be it.
After more testing it turns out that all the checkout panes show the old/incorrect $order object. The other checkout panes I refresh apparently work because the relevant part of those is a view.

Now, even if I rebuild the form in my callback function like this:

  dsm($new_order = commerce_order_load($form_state['order']->order_id));
  $form = drupal_get_form($form['#form_id'] /* commerce_checkout_form_$ID */, $new_order, $form_state['checkout_page']);

... the commerce_payment_pane_checkout_form() function has the old $order object, while the debug print proves the new one I'm passing is correct. So it's getting the old version for some reason, instead of the new one I'm passing when calling drupal_get_form().

The following also doesn't change the situation, as expected:

  dsm($new_order = commerce_order_load($form_state['order']->order_id));
  $form = commerce_checkout_router($new_order, $form_state['checkout_page']);

Do you have any idea where that old version may be cached?

a.ross’s picture

Another snippet that doesn't work:

  $form_state['order'] = commerce_order_load($form_state['order']->order_id);
  $new_form = drupal_rebuild_form($form['#form_id'], $form_state, $form);
rszrama’s picture

Hmm, you shouldn't be rebuilding the form that way, but it should be happening automatically thanks to the #ajax system. No new lead ideas from my end just yet.

a.ross’s picture

Title: Problems with payment methods with AJAX form submission in checkout. » During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect.
Status: Postponed (maintainer needs more info) » Needs review
FileSize
846 bytes

Well, the reason I have to rebuild the form is that I make changes to the order after the AJAX system has rebuilt it. If I then just return the relevant checkout panes as-is to the client, the order will be displayed one step behind, if you get what I mean.

For example (assuming 100 points == €1):
Step 0: Order has no userpoints_discount line item
Step 1: Slide the userpoints_discount slider halfway (for example 100 points) => Will show the order as in step 0, with order total unchanged
Step 2: Slide the userpoints_discount slider to full (for example 200 points) => Will show the order as in step 1, with order total reduced by €1
Step 3: Slide the userpoints_discount slider back to naught => Will show the order as in step 2, with order total reduced by €2

So I have to rebuild the form to overcome this. It might be better to use drupal_rebuild_form(), but that's not the issue here.
After some more debugging, it's apparent that the faulty $order object comes somewhere from the internals of the Form API, because even if I pass the correct $order object in a drupal_get_form(), the commerce_checkout_form() function gets the old one. I've attached a patch to commerce-1.3 to fix this, although I'm not sure if it's the right way to go, so I'll leave this at support request. Also, changing title.

rszrama’s picture

Hmm, my guess is it's loading it from the form cache, but I would've expected it to be updated on each request that resulted in a save operation. Maybe not, since it'd be stuffed in the arguments array. : ?

Have you tried $form_state['rebuild'] = TRUE; instead of directly rebuilding the form?

Status: Needs review » Needs work

The last submitted patch, commerce_ajax_checkout_form-1804592-8.patch, failed testing.

a.ross’s picture

Status: Needs work » Needs review

Exactly my thoughts.

Trying to force a rebuild with $form_state['rebuild'] = TRUE doesn't work. using drupal_rebuild_form works fine, and according to the docs that is used in Ajax callbacks.

Back on topic, what would be the correct way to go; somehow prevent the form from caching in the first place, even if that requires a hook_form_alter() for third-party modules that need it?

Otherwise, I've found a (probably very evil and hackish) solution:

  $form_state['build_info']['args'][0] = commerce_order_load($form_state['order']->order_id);
  $form = drupal_rebuild_form($form['#form_id'], $form_state, $form);

It works, but something tells me there must be a more elegant way :/

a.ross’s picture

Status: Needs review » Active

Crosspost... are all those test failures caused by my patch? ^^

rszrama’s picture

hehe Looks like it. ; )

rszrama’s picture

Version: 7.x-1.3 » 7.x-1.x-dev
Status: Active » Needs review

Oh, but wait - the problem is most likely that this is pegged to 7.x-1.3. Tests should always be run against dev. We'll let test bot have a bite, but I'm still not sure it's the way to go.

a.ross’s picture

Ah, I was wondering if that would work. Patch was made from 7.x-1.3, so I wasn't sure if it would apply to 7.x-1.x.

Go ahead testbot

Edit: patch is the same, I missed the "re-test" link, my bad.

Status: Needs review » Needs work

The last submitted patch, commerce_ajax_checkout_form-1804592-15.patch, failed testing.

a.ross’s picture

Status: Needs work » Needs review
FileSize
807 bytes

Well that was stupid... I should read next time before I copy-paste code :)

a.ross’s picture

Title: Problems with payment methods with AJAX form submission in checkout. » During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect.
Version: 7.x-1.3 » 7.x-1.x-dev
Status: Postponed (maintainer needs more info) » Needs review

Since the full $order object is now stored in the form's $form_state['build_info']['args'] array, and in various parts of Commerce's Form API code the order object is loaded anyway, doesn't it make more sense to just use the order ID as an argument?
Otherwise I think my patch is a much better solution than the evil, hackish snippet given above. This would also prevent such ugly hacks for other third-party modules doing AJAX magic in checkout. Commerce Coupon comes to mind: #1329600: Provide ajax support to reload form after coupon has been added

a.ross’s picture

Boink. Any new thoughts on this?

rszrama’s picture

Category: support » bug

This seems to be popping up in other issues, but I think I lost track of it after I made it a support request but didn't come back to the category later. Recategorizing and have linked a few other people here to review the patch to see if it solves AJAX related issues in checkout.

rszrama’s picture

Issue summary: View changes

Clarify what "second time" means

roderik’s picture

Issue summary: View changes
roderik’s picture

rockaholiciam’s picture

Not sure if it belongs here but just in case it's related and helps. I am using commerce for our shop and our flow is something like shop (with minicart in sidebar) > checkout page (with shipping on same page and lots of ajax) > order review page (with commerce_cart_summary view).

Now if I change the country, the shipping is supposed to get updated and it does. I check the order object dumps pre and post updating and everything is in order. I do a custom reload of the order object to confirm it has been saved and it mentions a revision id with an old revision id. From here, If I go back to the shop page with minicart on, everything is in order and accurate, however if I proceed and go to review page, order total is inaccurate with outdated shipping line item price.

Now if I try loading and dumping the order object in hook_view_pre_render of commerce_cart_summary, I get the order with outdated shipping line item and the order revision id itself is not the updated one but the one being referred to by the old revision id in the previous pages order object dump. So somehow the order reverts to the outdated version?

Also, if after changing the country on the checkout page and having the cart updated, if I refresh the page once and then proceed to review, everything is in order. Same is the case if I hack form_state and update it with latest instances of order object and then do a form_cache_reset but that has its own issues.

So any help or pointers here would be much appreciated.

a.ross’s picture

Maybe you could try my patch in #17, if that still applies?

rockaholiciam’s picture

it didn't. on the checkout page, everything is fine, its only when I move to the review page that the shipping messes up.

a.ross’s picture

Hm, sounds like a different issue, as this is only about AJAX form submissions. In my case, after a full page load (F5), things would be sorted out.

rockaholiciam’s picture

Well I would say the issue is still arising because of me using ajax and manually manipulating the shipping line item for the order. It could be that I am missing a method call somewhere but for now, I have ruled out revisioning as a potential issue by turning it off. Then I noticed there were multiple shipping items against an order in my line item table so I removed all but the one recent shipping line item. At this point I expected things to work out for now there is only one revision against an order and only one shipping line item but it keeps looking for the old shipping line item apparently and doesn't display shipping altogether on review page.

Just need to figure out how shipping line items are linked to an order now and if someones got any pointers, it would be much appreciated.

rockaholiciam’s picture

As a last resort solution, I tried updating the shipping via code in hook_page_build. Still outdated. So I moved the code to hook_views_pre_render instead and viola, all is in order. Maybe this information helps someone figure out the actual problem.

thedavidmeister’s picture

I'm guessing here because I don't have access to @rockaholiciam's setup, but is it possible that caching is somehow an issue (maybe even due to another contrib module like Entity Cache or similar).

If we take #17 a bit further, to force entity_load to not return a cached object, could that help?

#17 does this:

$order = commerce_order_load($order->order_id);

What if we try this:

$order = reset(entity_load('commerce_order', array($order->order_id), array(), TRUE));

roderik’s picture

@a.ross/#18:

Since the full $order object is now stored in the form's $form_state['build_info']['args'] array, and in various parts of Commerce's Form API code the order object is loaded anyway, doesn't it make more sense to just use the order ID as an argument?

So the attached patch (yours + some extra) would be enough?

I didn't encounter your issue (having to do a form rebuild myself after updating the order on AJAX), but it reads like a good idea to me: prevents obscure bugs, and has no real drawbacks... I think we can safely assume noone's using
$form_state['build_info']['args'][0] in their own modules directly, since they already have $form_state['order'] for the same purpose...

--

(Myself, I also had problems after implementig order updates on AJAX... but they relate to the question
"should you follow up your commerce_order_save($order); command with a $form_state['order'] = $order;?"
There's no real guidelines for that.

It took me a while to realize that my issue is probably unrelated to this one... I guess I should open up another issue for questions on that. But in the meantime maybe this patch might refocus the discussion to the original topic...)

a.ross’s picture

@Roderik, sounds like the exact same issue to me.

roderik’s picture

@a.ross: ...yeah, I guess they're related. The exact same issue? I failed to find that out.

With the above patch, I can think of only one way that this could cause a regression: if a module has validate function that changes $order and expects those changes to not persist in the drupal_rebuild_form() of an AJAX request. Which just sounds silly.

So I think the patch is a good idea.

---

I also think that if this patch was everywhere, we could tell people they wouldn't need to do commerce_order_load() calls in their form builders. But I'm not 100% sure - there could be situations where another pane-form callback that has just run, has messed with $order so that you really want a fresh one.

(Semi useless blabbering:

The commerce_order_load() calls in commerce_coupon v2.0, plus the comment in #2132865-3: [2.x] Update $form_state['order'] on ajax submit that setting $form_state['order'] = $order right after saving the order "seems to cause conflicts with other modules that use this variable during checkout", seems to suggest that there actully is a reason to reload, sometimes. But I'm not sure what this reason would be. Hence "I failed to find that out."

I thought I knew, because commerce_shipping v2.0 actually uses $order (specifically $order->shipping_rates) to store temporary calculation data which doesn't need to be saved into the database... though saving it does no harm. Not reloading the order in your own function before saving, but after commerce_shipping has run, would save this temporary data too.
...but then I saw that the rules action in commerce_shipping_rate_apply() makes no effort to clean out $order->shipping_rates either. And I gave up trying to see a pattern.
)

thedavidmeister’s picture

Priority: Normal » Major
Status: Needs review » Needs work

I think we can safely assume noone's using
$form_state['build_info']['args'][0] in their own modules directly, since they already have $form_state['order'] for the same purpose...

You can't safely assume something like that unless you document it heavily and prominently (even then, it's bad DX to have buggy behaviour like this lying around like a land mine). The provided patch under review provides zero documentation explaining that using $form_state['build_info']['args'][0] can cause difficult to detect/debug bugs that can be quite damaging for an eCommerce site so I'm marking this as CNW.

Since this bug leads to incorrect payment/shipping/coupon data for production sites with no known workaround I'm bumping this to "major" as the definition of that priority is:

Issues which have significant repercussions but do not render the whole system unusable are marked major. An example would be a PHP error which is only triggered under rare circumstances or which affects only a small percentage of all users. These issues are prioritized in the current development release and backported to stable releases where applicable. Major issues do not block point releases.

In Drupal 7 this also applies to test failures in secondary environments.

Major priority is also used for tasks or features which consensus has agreed are important (such as improving performance or code refactoring), but which are not functional bugs.

roderik’s picture

Component: Payment » Checkout
Status: Needs work » Needs review

You can't safely assume something like that unless you document it heavily and prominently (even then, it's bad DX to have buggy behaviour like this lying around like a land mine).

1) see below
2) this is why the patch removes the land mine.

The provided patch under review provides zero documentation explaining that using $form_state['build_info']['args'][0] can cause difficult to detect/debug bugs that can be quite damaging for an eCommerce site so I'm marking this as CNW.

The patch provides zero documentation on using $form_state['build_info']['args'][0] in creepy ways, because it eliminates the possibility of using $form_state['build_info']['args'][0] in creepy ways. Code does not usually document removed behavior of previous versions of the code.

Maybe you meant that you want to see this in a Change Notice, so people who might be using $form_state['build_info']['args'][0], will be notified that they can't do that anymore?
Or you glanced over the latest patch in #30 because somehow the test robot ignored it (so it wasn't green)?

Setting back to Needs Review for clarification.

thedavidmeister’s picture

I think I just misunderstood what was being said. Apologies and carry on :)

Status: Needs review » Needs work

The last submitted patch, 30: commerce_ajax_checkout_form-1804592-30.patch, failed testing.

The last submitted patch, 30: commerce_ajax_checkout_form-1804592-30.patch, failed testing.

rszrama’s picture

Any ideas on what's causing the failing tests?

Additionally, I'm not sure we should move forward with a change to this form's signature. It's quite possible that sites exist that directly invoke this function. Changing the parameter from $order to $order_id will break any such site. In that case, we'd just do a $order = commerce_order_load($order->order_id); instead of $order = commerce_order_load($order_id);.

I saw the back and forth above about people directly accessing the form arguments array, but I'm not sure why anyone would need to do that given that we've long been storing the order in a separate form state variable. I'd rather provide documentation that $form_state['order'] should be used instead of the argument and not break existing direct builds of this form.

a.ross’s picture

The only explanation I can think of is that the order object stored in the form_state didn't have an order ID... don't know if that's possible.

As mentioned in my case, I'm making changes to the order after the form has been built. If I return the original form to the browser as response to the AJAX call, it will not have up-to-date checkout panes. I don't know what would be the right way to solve it, but as mentioned I tried various things and the only thing that worked was the ugly hack in #11. The patch in #17 did fix my problem.

andyg5000’s picture

Updating patch to simply reload the $form_state['order'] object from the $order->order_id

andyg5000’s picture

Status: Needs work » Needs review
andyg5000’s picture

Status: Needs review » Needs work

Last patch doesn't work because $pane_form = $callback($form, $form_state, $checkout_pane, $order); is called later and at that point $form_state['order'] and $order are two separate objects.

a.ross’s picture

@andyg5000 doesn't the patch in #17 work for you?

andyg5000’s picture

@a.ross that patch would be better than the one I posted :). However, I'm not sure it's necessary. The commerce_checkout_form is being invoked by commerce_checkout_router with is being directly invoked by the menu system. I don't see how the $order object would be different so early in the game since the menu_execute_active_handler() is basically doing the same thing. I think the issue is that other modules were calling commerce_order_save($form_state['order']) which easily falls out of sync with the current $order object. See my post in https://drupal.org/node/2009326#comment-8807237

If you agree, let's close this out :) If not, I'd love to hear more info and how else to reproduce and validate the #17 patch.

a.ross’s picture

Well, the point is that when calling drupal_rebuild_form() the order object is refreshed. And there is no way around having to call drupal_rebuild_form() because multiple checkout panes could alter the order object, influencing eachother. Thus a possible race condition. The only way to prevent that would be to preclude race conditions altogether, which means having to break up the entire checkout in one checkout page per checkout pane. In other words: not possible. You just need drupal_rebuild_form() fixed, or some alternative API function offered by Drupal Commerce.

rszrama’s picture

Issue tags: +sprint

Tagging for http://contribkanban.com/#/board/commerce/7.x-1.x.

Might check with Andy to see if he has any more thoughts here.

torgosPizza’s picture

Seeing this issue as well with trying to Ajaxify some parts of the Checkout process.

Happy to help test any patches that come down the line.

mglaman’s picture

It seems the biggest issue here is that modules aren't always grabbing up to date data and might be getting something stale from the form state. I wonder if this is a works as designed and there just needs to be some good old elbow work done in the contributed project space.

torgosPizza’s picture

@mglaman: That's what the issue opened up at #2533396: Convention for updating the order during checkout is attempting to ascertain. It does seem like a lot of contribs rely on the form state to retrieve the order, which may not be a best practice. I don't recall seeing any documentation on this specific topic, so it'd be great if we could create something definitive (or at least get us pointed in the right direction).

mglaman’s picture

So accessing the entity from $form_state seems to be the pretty standard thing to do in core -- albeit it isn't even standardized. Node sends $form['node'] and $form_state['node'] for instance. I think it falls "what the hell system am I in, where do I get the entity?!" and having API docs strictly state "Load a fresh copy. Always, especially in Commerce where fresh data is crucial."

The latter is why I always load fresh, because you never know, even though objects are passed by reference.

ramil g’s picture

The issue here https://www.drupal.org/node/2065083 is also related to this. Patch #17 seems to be the best fix so far.

torgosPizza’s picture

I'm thinking that the approach in #40 is in fact the best. Since the $order object essentially bubbles up to all of the forms that get built at checkout (even during an Ajax refresh the commerce_checkout_form() function gets fired for all commerce pane forms) it makes sense to handle it there. This way each form will always have the same version of the order, even if a form changes it in its "local" form_state['order'] item (assuming of course that the pane form in question is correctly using the Ajax API).

It does seem odd to me though that $order is an argument being passed in, and then having it immediately get overloaded. Is there some kind of condition / sanity check that should go in there? Maybe loading the $order and comparing its revision status? (Alternatively, should we be "refreshing" the $order being passed in, instead of simply loading it again?)

GoZ’s picture

I'm here because i have a different issue and that patch seems to fix it.

If have an issue with Commerce coupon (+ commerce_discount and commerce_discount_usage). I create several order discounts fix or percent discount with a coupon for each one. No value in usage fields.
When i try to redeem between 2 and 4 discounts in checkout, some time, an ajax error returned (500). debugging, i see commerce_discount_usage try to access to a commerce_unit_price of a line item which doesn't exists anymore.
The strange thing is : commerce_coupon.checkout_pane.inc launch commerce_coupon_redeem_coupon_code(), which make commerce_order_save() (with old line items, at this point, only coupons are added in coupon list), which load entity_load_unchanged() which trigger commerce_cart_commerce_order_load() which launch commerce_cart_order_refresh() which save my new order with new line items (the good one).
At this point, i'm still in entity_load_unchanged function, so first commerce_order_save has old entity as entity to save and new entity as original...

I apply patch #40 and so far, so good.

torgosPizza’s picture

That's a great result. Can you tell us anything more about your scenario? It would be great if we could reliably duplicate and test for an outdated order. The issue is obviously outdated line items overwriting a saved current order, and the only downside I can see is the extra call to load the order.

As long as we can ensure that is performant, I really think this needs to make it in to Commerce.

GoZ’s picture

My error is finally solved thanks to #2369899 and #2258373, so i'm not sure this issue solves it.

torgosPizza’s picture

Sorry, I thought the patch in #40 helped?

GoZ’s picture

Sorry, i thought too. It was a false positive.

torgosPizza’s picture

I did discover an issue just now with this one-line patch: it breaks compatibility with Commerce Addressbook.

Before the patch, selecting a new address at Checkout from the address book dropdown would result in the NEW values populating the address fields.

After the patch, selecting a new address loads the data but the fields in the address form are not populated with the new address.

So it does appear that because of how the order is stored in the form and then reused at least for the address book, this patch may not be fully formed. Either that, or we need to patch Commerce Addressbook to save an order whenever an address is selected.

rooby’s picture

I recently ran into this problem and am currently doing this to work around it:

$form_state['build_info']['args'][0] = $form_state['order'] = $my_updated_order;

Just looking at the code in some of the patches I like the idea behind #30 but I think #17 would also be acceptable and less risky.

Based on #38 it seems that #17 is the preferred approach, along with some documentation to describe the correct way to update the order during checkout.

#17 still applies with fuzz but here's a fresh reroll.

rooby’s picture

Status: Needs work » Needs review
torgosPizza’s picture

@rooby: My main issue is that this approach breaks compatibility in other modules (in my case, Commerce Addressbook). See my above comment.

I suppose this could probably be fixed at the Addressbook level, as the way things should work is the form should update the order before the order gets reloaded by commerce_checkout.pages.inc. But TBH I'm not sure yet how feasible this is, and we haven't heard much from the Commerce Guys on this particular topic.

rooby’s picture

@torgosPizza:

Was that comment referring to the patch in #40 or the patch in #17?

The patch in #40 won't work properly.

The patch in #17 should cause minimal disruption to existing code. It should only break things that were manually putting changes into $form_state['build_info']['args'][0] without saving their changes to the order in the DB, which is not a good thing to do in the first place really.

[edit]
Oh yeah actually it breaks things that don't specifically save the order. That could be a reasonable amount of modules.
[/edit]

Since there has never really been any real standard for how to make changes to these order objects any possible solution to this problem will risk breaking some third party code but I think the approach in #17 will have the least impact.

At some point we're going to have to bite the bullet and make a change and then fix any third party modules that break as a result.

andyg5000’s picture

I would vote that any module manipulating the order object in $form_state must be responsible for updating the form state once their updates are made.

Dealing with this issue now in #2026321: Avoid overwriting an already updated order

andyg5000’s picture

Ok ok ok! This patch does things! The problem with all other approaches is that the order object may get altered by a checkout panes form logic outside of the passed reference. If we force refresh the order object after each callback it makes sure each pane is provided the updated object.

Not sure if we need to do the same thing in the validate/submit handlers of commerce_checkout, but this is fixing all 99 of my problems!

torgosPizza’s picture

The patch in #64 seems to work with Addressbook, so that's a really nice and elegant approach.

Thanks for digging into this, Andy!

mglaman’s picture

Makes sense to me. This patch essentially ensures the $form_state is fresh for each form generation, as one would expect with other forms. The only alternative I could see is checking what Inline Entity Form does. However that's a very difference use case. I give my +1 to this.

torgosPizza’s picture

Actually, I wonder if the call to commerce_order_load() should be moved to above the foreach() loop. That way we can call it once and not n times (depending on the number of panes in a checkout config).

a.ross’s picture

@torgosPizza, that sounds like what the patches in #17 and #40 do.

@andyg5000, could you explain why you moved the order reloading somewhere else within the function (compared to #17 and #40)? It seems to me to be in the wrong place now. I would think it's a problem that a checkout pane would alter an order just when loading the form for it.

andyg5000’s picture

@a.ross and @torgosPizza, The reason that I've called this in the each loop and after the pane callback is that panes are manipulating the order object and not updating form state, so they fall out of sync. If you want to see this in action:

Use commerce_coupon 2.x-dev and commerce_shipping-2.x dev
Set up a coupon for $ off an order
Put billing, shipping and shipping service all on one checkout page (with ajax refresh)
Create a cart
Add the coupon
Go to the billing/shipping page
You should see the dreaded "You are not authorized to access this command." because $order has been updated in the refresh cycle and $form_state['order'] is stale

I call it after the pane form callback and not before because the order is already fresh in the 1st iteration, but may not be after the form callback was executed.

I've attempted to make sure the form_state['order'] object is refreshed elsewhere in commerce_checkout.pages.inc, but this is the only place/method that solves the issue.

a.ross’s picture

I understand all but one thing: why would a checkout pane update the order upon returning the pane content (as opposed to the submit callback)?

torgosPizza’s picture

@a.ross: I think this is more of a Commerce question. But basically if you have a checkout pane with an ajax handler, it fires the hook_form() callbacks since that is how the Checkout page is recreated. For me this is the fundamental flaw with how Commerce Checkout works, and I think this line is what makes it clearer (for me anyway):

$pane_form = $callback($form, $form_state, $checkout_pane, $order);

Since $order is being passed into the callback function, any checkout pane is able to alter the $order object. For example when getting shipping rates, Ajax callbacks are fired after a shipping rate is found; however there becomes a discrepancy between the $order object that is held in the $form_state (and which is saved to by Commerce Shipping) versus the original $order that was used to build the Checkout panes/forms in the first place.

In practice, this leads to line items being orphaned, totals being miscalculated, and more.

If you open up commerce_checkout.pages.inc in your editor and place a breakpoint within the function commerce_checkout_form() you'll see how this function is called whenever a checkout pane is interacted with.

EDIT: Just to be clear, I think that what you have described is what should happen, but from what I can tell there does not seem to be a convention or standard way to do this, and Commerce checkout has had to work around this architectural wonkiness by checking for a $triggering_element. An example of this can be found in Commerce Coupon:

/**
 * Checkout pane callback: coupon checkout form.
 */
function commerce_coupon_pane_checkout_form($form, &$form_state, $checkout_pane, $order) {
  // Allow to replace pane content with ajax calls.
  $pane_form = array(
    '#prefix' => '<div id="commerce-checkout-coupon-ajax-wrapper">',
    '#suffix' => '</div>',
  );

  // Store the payment methods in the form for validation purposes.
  $pane_form['coupon_code'] = array(
    '#type' => 'textfield',
    '#title' => t('Coupon Code'),
    '#description' => t('Enter your coupon code here.'),
  );

  $pane_form['coupon_add'] = array(
    '#type' => 'button',
    '#value' => t('Add coupon'),
    '#name' => 'coupon_add',
    '#limit_validation_errors' => array(),
    '#ajax' => array(
      'callback' => 'commerce_coupon_add_coupon_callback',
      'wrapper' => 'commerce-checkout-coupon-ajax-wrapper',
    ),
  );

  $error = '';
  if (isset($form_state['triggering_element']) && $form_state['triggering_element']['#name'] == 'coupon_add') {
    $code = $form_state['input']['commerce_coupon']['coupon_code'];
    if (!empty($code)) {
      $coupon = commerce_coupon_redeem_coupon_code($code, $order, $error);

      if ($coupon) {
        // Clear the field value so that the coupon code does not get
        // resubmitted causing an error when user uses main "Continue to next
        // step" submit.
        $pane_form['coupon_code']['#value'] = '';

        // Reload the order so it is not out of date.
        $order = commerce_order_load($order->order_id);

        // Recalculate discounts.
        commerce_cart_order_refresh($order);
      }
    }
    else {
      ...
  }

  // Extract the View and display keys from the cart contents pane setting.
  $pane_view = variable_get('commerce_coupon_checkout_pane_view', 'order_coupon_list|checkout');
  if ($pane_view != 'none') {
    list($view_id, $display_id) = explode('|', $pane_view);
    if (!empty($view_id) && !empty($display_id) && views_get_view($view_id)) {
      $pane_form['redeemed_coupons'] = array(
        '#type' => 'markup',
        '#markup' => commerce_embed_view($view_id, $display_id, array($order->order_id)),
      );
    }
  }
 ...
  return $pane_form;
}

I'm not sure if this is the best example (or one that should even be followed by other Contrib modules) but Coupon has to work around the paradigm of "every checkout pane is a form" by including both the form itself and the form action functionality into the checkout form callback. Note the line if (isset($form_state['triggering_element']) && $form_state['triggering_element']['#name'] == 'coupon_add') { in the example above.

Hopefully this helps clarify the issue we are attempting to solve :)

To answer your question, which was "Why would a checkout pane update the order upon returning the pane content (as opposed to the submit callback)?"

This is not the issue, the issue is - as the title of this issue states - that the order object returned by submit handlers is not always correct or up to date when an Ajax submit takes place. And that I believe is partially due to how commerce_order_refresh() behaves as well.

JMTorres’s picture

Won't #64 disregard any unsaved changes that may have been made to the form_state['order']? Especially considering @rszarma suggests modifying form_state['order'] directly, in #38.

Any chance of @rszrama chiming back in on this issue? It seems to be going back and forth. #2533396: Convention for updating the order during checkout should be addressed, and then everyone can start patching affected modules.

mglaman’s picture

Issue tags: +commerce-sprint
torgosPizza’s picture

Especially considering @rszrama suggests modifying form_state['order'] directly, in #38.

The issue is that, because the checkout page is a collection of nothing but forms, each checkout pane being its own form, containing its own form_state, in the instances like the OP when there is an Ajax event we can't always ensure that the returned order is correct. The panes do not talk to each other very well in my experience, and whether this is due to some panes not correctly saving to form_state['order'] is at the moment unclear to me.

In any case this is what #2533396: Convention for updating the order during checkout had been intended for: to create a standard way of going about this, because we even found examples in Commerce core that were inconsistent. But given the way many Commerce/Commerce contrib modules don't save the order when a pane is interacted with (or they save form_state['order'] which the other panes know nothing about), I suspect this might not be the easiest issue to solve.

GoZ’s picture

#64 fix a same issue.
My issue: when i add a coupon in checkout with ajax and then uncheck commerce_customer_profile_copy checkbox, customer profile doesn't display. $form_state['build_info'] has old order values. I used #40 patch which fixed anything else, but #64 fix everything.

torgosPizza’s picture

andyg5000 seems to confirm that this fixes the issue with Shipping described in #2026321: Avoid overwriting an already updated order, which introduced new code as a workaround to deal with just the symptoms, whereas this patch addresses the root cause. There is now a new patch in that issue to revert that workaround, in favor of #64. I recommend we consider committing this, as long as no other side-effects are found.

Andy has also opened up #2593983: Create a utility method to compare an orders changed date against the database which may make this much easier - I would propose wrapping that function around our original patch #64 - as well as perhaps some method of static caching - to potentially avoid multiple unnecessary commerce_order_load()s.

torgosPizza’s picture

I wanted to add some more anecdotal evidence as to why this fix is good.

Using Commerce Stripe and its new "Checkout" feature requires an "amount" setting/variable to be set via drupal_add_js. This is needed not only for display purposes but so that the amount in Bitcoin can be calculated and displayed in the checkout widget that pops up (it is a Javascript iframe embed).

Previously, when using this integration, there was an issue when using ajax-enabled panes such as Shipping and Coupons in a single-page checkout configuration. Before this patch, interacting with these panes would adjust the order in form_state, but the widget would not update with the correct, adjusted order total.

With this patch, though, adding a coupon works with Stripe Checkout now because the order is saved by the coupon pane's functions, and then the updated $order is sent via commerce_checkout_panes() to the payment pane form, which is essentially re-rendered, and the Drupal.settings.stripe.checkout array with the newly adjusted total are updated along with it.

I believe this will also work with Shipping, when and if my patch in #1287126: Update the cart contents pane's order totals when shipping is selected lands. (I am working on a new patch that includes some of the pane-updating (via Ajax) commands that were previously integrated into the Ajaxcart sandbox module. EDIT: Confirmed, it solves all the issues I had, and my new patch will render this sandbox module unnecessary. The one thing we will need to enforce in Contrib is that any ajax updates handled by panes should save the order. That chain reaction will result in the $order updating across all panes.

This change is very promising, and aside from any performance issues (we should try to limit the number of times we must call commerce_order_load()) I really think this should be reviewed and if possible committed very soon. I think it will solve many issues and make tons of our kludgy workarounds no longer necessary.

andyg5000’s picture

Here's an update to #64 that includes the utility in #2593983: Create a utility method to compare an orders changed date against the database. We still need more people to test this, preferably with modules like commerce address book enabled. I had issues with #64 when selecting random options in address book, copy billing > shipping, and updating the address fields. For example, switching from domestic to international didn't update the shipping options when the billing address was an address book, but shipping was the basic input. This updated patch appears to help with that, but it may mean that addressbook module needs to be updated after this fix is put in place. ¯\_(ツ)_/¯

Status: Needs review » Needs work
torgosPizza’s picture

@andy: Yeah I think that is true. It may be another case where the order is not saved? I checked out the commerce addressbook ajax callback and it does not call commerce_order_save() anywhere. It does do an $order_wrapper->save() in the element validate callback commerce_addressbook_saved_addresses_validate() but my guess is that wasn't triggered in your test scenario?

Looks like your patch didn't include the definition of that function. Might need to include that in the re-roll?

Anybody’s picture

Same problem here.

torgosPizza’s picture

#81, @Anybody:

Same problem here

Can you be a bit more specific?

torgosPizza’s picture

BLOK-Man’s picture

Patch in#64 appears to have worked for me.
Thanks AndyG5000

Anybody’s picture

@torgosPizza: I'm running into the issues problems. #64 works great for me. Sorry for the too short comment!

I believe we have to care for race conditions hardly because for example off-site-payment providers trigger rules and lead back to checkout panes. There's danger of highly concurrent threads because of payment API calls!
Examples for this can be found in the commerce_paypal and commerce_sofortbanking module.

Anybody’s picture

#64 solved my issues completely. I did not test #78 because it is missing the implementation from #2593983: Create a utility method to compare an orders changed date against the database. These two issues should be combined into one patch as soon as possible, be tested together and be commited.

Is there an active DC maintainer willing to do this to bring this bad problem forward? I'm running into side effects based on these problems in nearly every DC project now...

torgosPizza’s picture

@Anybody: That's great! If these patches in this and other issues solved your problem, you should update the issue to RTBC, to let the maintainers know that you have tested them and that they work for you.

mglaman’s picture

torgosPizza’s picture

@mglaman: Awesome work on this, as always. I would be interested to see some performance stats for this, too, to avoid having a bunch of threads spawn as @Anybody mentioned. Looking forward to trying these updated patches!

torgosPizza’s picture

Just out of curiosity: do you think we need to commerce_order_load() before checking if the order has changed? Is there a way we can check that the order has changed without loading the order again first?

andyg5000’s picture

As a side note, I created https://www.drupal.org/sandbox/andyg5000/2354317 a while back to alert me/store owners if orders didn't make it out of checkout, but were paid. This helps prevent a lot of customer grief :)

torgosPizza’s picture

Anybody’s picture

I've been using #64 since over a month on two production sites now and everything works fine.
As soon as #2593983: Create a utility method to compare an orders changed date against the database is solved or even earlier we should also fix this issue ASAP.

Anybody’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 88: during_ajax_form-1804592-88.patch, failed testing.

torgosPizza’s picture

Thanks Anybody. I know @joelpittet had major reservations about this approach. I would love to have him and Ryan possibly chime in to talk about whether there is a better way forward, or if this is the only option.

I would also request that others take a look at #2646338: Potential data loss during payment checkout pane redirect. which may be related. From what I can recall the router is called before any checkout form callbacks are fired, meaning this might be a potential fix even further upstream. If that method does in fact solve the issue as well then I would vote for that one to be committed instead of this one.

Please check that issue/patch out and let me know if it makes a difference or not.

czigor’s picture

We need to load the order in the beginning of commerce_checkout_form() as well. My use case is described in #2813691: commerce_datatrans_submit_form() might save an obsolete order but tl;dr: The order might be changed outside the checkout process (in another browser tab for example) so cache_form might include a stale version. If in this case the first checkout pane modifies the order, it will save the stale version which is bad. (The other checkout panes are already guaranteed to get the freshest order by the patch in #64.)

This is just the combination of #17 and #64.

arunkumark’s picture

Status: Needs review » Reviewed & tested by the community

The patch #97 works. It will preserve order line items on Ajax calls.

Am facing the issue on discount line item disappears when shipping information changes. After applying the patch the issue has been resolved.

jsacksick’s picture

Issue tags: +release-1.14
andyg5000’s picture

torgosPizza’s picture

I think this is a relatively minor change, but an important fix, and would love to see it committed.

  • rszrama committed c21be87 on 7.x-1.x authored by andyg5000
    Issue #1804592 by a.ross, andyg5000, mglaman, czigor, roderik, rooby,...
rszrama’s picture

Status: Reviewed & tested by the community » Fixed

I gotta be honest here ... the logic change makes sense, but I'm concerned about the performance impact. Unfortunately, try as I might, with multiple payment methods and shipping service Ajax, I couldn't reproduce the problems. I only had one instance where I lost my customer profile reference, and I have no clue how that happened. : P

That said, I understand the need for the fix and put a lot of weight behind the folks supporting the change in this thread. Let's keep an eye out for the performance impact, and in the meantime I'll work with Matt to give us a Locust.io based test suite we can use to test this kinda stuff in the future.

I think it may be a non-issue since subsequent loads should be hitting the static cache in the order controller, and in the rare case it doesn't, we'd want to populate it any use it anyways. (Because if there is no order in there, that means we got an order from the form state that was loaded through normal channels.)

Committed! Thanks everyone. : )

Status: Fixed » Closed (fixed)

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

jsacksick’s picture

Status: Closed (fixed) » Active

I'm reopening this one as it seems to be causing an issue on one of my DC site which causes no shipping rates to be returned when this patch is present....

I have conditions on the rule component that checks if the shipping profile isn't empty since some of the shipping services are shown only for some countries...

I'll try to dig this more tomorrow.

jsacksick’s picture

Status: Active » Closed (works as designed)

I think this can be closed actually... It turns out the issue was coming from the Commerce shipping module (that project was on Commerce shipping 2.2, upgrading to 2.3 did the trick)...

Most probably due to #2830800: [Follow-up] Avoid overwriting an already updated order.