Problem/Motivation

When an user changes their country at checkout, stores may want several things to happen, such as:

  • Updating the zone/state dropdown
  • Updating shipping quotes
  • Applying of payment conditions

Other modules may want to extend this further. At this point, no mechanism exists for doing so, with the exception of a crude extension in the uc_quote module (which allows quotes to be retrieved while updating the zone/state dropdown).

Proposed resolution

Implement a system or API with which ajax events on checkout can be controlled. Adapt the modules uc_quote, uc_cart and uc_payment in such a way that they make use of the new system or API. Make sure that other modules are able to extend the system or API.

Remaining tasks

  1. longwave's "hopefully final" review. See #101. DONE
  2. Commit the appropiate patch. DONE, see #111
  3. Create follow-ups for subissues.

User interface changes

  • A new module called "Ubercart Ajax Administration" is added with which ajax events for the checkout form can be configured. For (nearly*) each field on the checkout form you can configure which checkout panes needs to be reloaded when the value for that field is changed.
    The implementation of the ajax events themselves (along with a default configuration) lives in the (required) Ubercart Store module, so the module don't need to be enabled for ajax events on checkout to work, only to change the configuration.
    * Only these fields can be configured:
    • Fields that the user configuring the ajax events would see at the moment when he/she would enter checkout. This means that conditional appearing fields (for example a field that only appears when a certain product is in the cart or a field only visible to certain roles) won't be configurable out of the box. These fields may appear if the user configuring the ajax events would try to meet the conditions that are needed for the field to appear (such as adding the needed product to his/her own cart).
    • AND fields that are inside a checkout pane. Fields outside checkout panes aren't configurable out of the box.

Default ajax events configuration

Triggering form element Panes to update
Delivery: country Shipping quotes, Payment method
Delivery: postal code Shipping quotes, Payment method
Delivery: address selector Shipping quotes, Payment method
Billing: country Payment method

API changes

A new API for controlling ajax events on checkout will be introduced.

Subissues (reported problems after applying the patch)

  • Submodule or merge: There was discussion about if the admin section for configuring ajax checkout events should be in a separate submodule (e.g. ubercart/uc_ajax_admin) versus moving it into uc_cart. This conversation started in #68 by longwave, but the reply in #71 in support of keeping it separate was agreed upon in #75 and #94.
  • Illegal choice: Some people reported having seen illegal choice errors after applying the patch. See #75. Conclusion in #106 is that it's not caused by the patch and that it may be just a configuration issue.
  • Tax rate: There is an issue in #88 regarding tax rates not being properly set or stick. Conclusion in #98 is that it can't be fixed out of the box, because it would be too site/business specific. Documentation for this issue needs to be added, suggestion in #98 is to handle this in a follow-up, which is agreed upon in #101.

Original report by longwave

Spawned from #1263808: Ajax http error on Flat Rate Shipping Quotes and #1332194: Payment method conditions

We may want several things to happen when the user changes their country at checkout; the zone/state dropdown needs to be updated, the shipping quotes should update, and perhaps some payment method conditions need to be applied. Other modules may want to extend this further. At the moment there is no mechanism for doing so, except a crude extension in uc_quote which allows quotes to be retrieved while updating the zone/state dropdown.

As noted in the first linked issue I had an idea for adding a hook_uc_address_ajax_fields_info() or similar which could register field-level callbacks to provide multiple reactions on address field changes, executed through a common dispatcher, which it seems will be necessary if we want multiple modules to act on the checkout page from a single click.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wodenx’s picture

I actually have a patch for this almost ready

longwave’s picture

This should also perhaps tie in to the "my delivery address is the same" checkbox so the same things are guaranteed to occur if the billing address changes when the box is checked, yet something wants to react on the delivery address fields.

wodenx’s picture

Status: Active » Needs review
FileSize
9.54 KB

OK - here's a stab at this. It's a slightly different take - it actually provides the common dispatcher on the form level - not just the address level - so any field on the form can have ajax attached to it. The new hook_uc_cart_checkout_ajax() is documented in the api.php. So far, I only reworked uc_quote to take advantage of the new hook. Didn't tie it into the "my deliv address is the same" checkbox, but that should be relatively easy.

longwave’s picture

Haven't tested it, this looks good except I don't like the new hook being invoked multiple times; couldn't it just return all necessary data in one go, invoked once with module_invoke_all() and statically cached in the process callback?

longwave’s picture

This would also let us have a drupal_alter() immediately after, which might be useful if other modules want to subvert this process even further (not that I can think of a use case right now, but this is otherwise a standard Drupal pattern)

longwave’s picture

FileSize
27.21 KB

This builds on #2, simplifying the hook and adding support for country conditions to affect the list of payment methods. This is only a proof of concept, I am not sure it is possible to automatically determine which fields should be used as triggers where Rules is involved.

wodenx’s picture

FileSize
46.46 KB

The caching of the hook results is a great idea. A few things added in the attached patch:

- It occurred to me that this could be abstracted to allow ajax attachments to any form. This version does that, and then uses it to update the available shipping quotes when the delivery country is changed on the order edit form. Note - you'll need to apply this over (or under) #1376332: Shipping quote conditions are not respected on order edit form. for the quotes to actually update properly.

- I think there's a danger of namespace collision if we just use the single key of the form element to which we want to attach ajax - what if a contrib module creates a checkout pane with a 'delivery_address' key? Drupal doesn't enforce the singularity of these keys, so I think it's best to require implementations to use the complete nested path.

- Rebased the new payment options limiting code to fix collision with

commit d0440d7b7be1f2f7ac983c32b80e6f4a94a3c28e
Author: Dave Long <dave@longwaveconsulting.com>
Date:   Mon Dec 19 08:10:23 2011 +0000

    Remove unnecessary theme_uc_payment_method_select().

- I'm not sure I agree with simplifying the return of the hook to just the callback. For one thing, this provides no way to set/modify ajax options (e.g. throbber and progress) on an element without also implementing form_alter - though maybe that's ok, as those should really be done in theme functions anyway. Also, if you want to attach an existing ajax callback that uses a 'wrapper' key rather than a list of commands, this way you have to write your own callback to translate it to a command, rather than allowing the multiplexor to do it for you. On the other hand, having the functions return '#ajax' arrays is problematic for 'module_invoke_all()' - so maybe it's a wash and we should leave it as it is.

Finally, regarding finding the fields that should be used as triggers - you're right, probably not possible to find them all - but we *could* examine at least the existing data-comparison conditions, and attach ajax to form fields which are related to the compared properties. That would probably catch the majority of them.

longwave’s picture

Agreed that triggering this on more than just the checkout form would be useful, I did think of that but didn't have time to implement it.

We have actually already run into the namespace collision on "select_address", although it's not that important in that particular case.

I don't think multiplexed calls should be allowed to set effect/throbber/message properties, as we can only have one set of these per Ajax enabled element. I think the original Ajax implementation (if any) should decide this, or the multiplexer can just use a simple default if there isn't any Ajax attached to the field yet.

I think it might be possible to autodetect some fields to trigger on if we could hook into the entity property getter mechanism; do a dummy run of the Rules condition sets, record which properties were accessed, and then we know which ones should trigger an Ajax update. We would still need a way of mapping order properties back to their form fields, though.

wodenx’s picture

I don't think multiplexed calls should be allowed to set effect/throbber/message properties, as we can only have one set of these per Ajax enabled element. I think the original Ajax implementation (if any) should decide this, or the multiplexer can just use a simple default if there isn't any Ajax attached to the field yet.

Fair enough. If a module really wants to override, it can still implement form_alter or add its own #process function.

wodenx’s picture

FileSize
46.61 KB

Rebased #7.

lagreen’s picture

Hello,

Thank you for this patch it really helped me to upgrade my checkout process. I have one question regarding Ajax events - is there any way to update payment methods by changing shipping quotes?!

Thank you.

3CWebDev’s picture

I've spent good part of the evening trying to apply the patch in #10 to my ubercart modules with no success. This patch is way to large for me to apply manually and I have not had success trying to patch using Windows Patch utility, Eclipse or Tortoise GIT methods.

Any idea if/when this will be available in UC Dev download, or if it can be made available to download a patched version?

Rafal Lukawiecki’s picture

I have applied this patch to 3.x-rc4+37-dev. It seems to do something, as when I change the Country field selection browser sends a request, then it seems to be hanging, waiting for a response, until it times out. The payment info pane does not update itself, as I would have expected it to, either—different billing country triggers a different tax rule in our set-up, and so a different total. Refreshing the page by reloading it shows correct totals. Perhaps it is too early for me to expect this should have worked with just this patch?

longwave’s picture

@setfree: The patch still applies to -dev, it needs more testing before it is committed.

@Rafal: Did you clear cache after applying the patch? It works for me but you need to clear cache to pick up the new hook.

I wonder if we can augment the hook with an extra Rules event for "When updating the checkout form", with conditions for "If this field is updated" with a data selector, and actions for "Update the [pane]". The event would take the original and updated $order objects, the conditions could then check to see what changed, and the actions configure which Ajax callbacks are used. This seems like the most configurable way of allowing the checkout form to dynamically update, avoiding a bunch of checkboxes for each field/pane combination and which still won't probably be enough for many use cases.

I am not sure this will entirely be workable but I don't want to go down the route of hardcoding a bunch of cases and making people write custom code to add extra Ajax events to the checkout if at all possible.

Rafal Lukawiecki’s picture

I believe I have emptied the cache with drush cc all, but I will reboot both servers and try again. Is there anything else I could trace or check to find out why the issue described here, and in #1446176: Refresh available payment methods after applying a coupon is affecting my install?

@longwave: I have restarted, and having thoroughly emptied caches I am afraid I am still experiencing the above described problem on two different servers, even with Bartik as the theme.

TR’s picture

Status: Needs review » Needs work

Patch needs to wrap comments at 80 columns.

cglasgowit’s picture

Having major issues getting this patch deployed out - can someone host the files in question somewhere and send me a link to them?

Thanks

Rafal Lukawiecki’s picture

It looks to me like the above mentioned patch, 1373236-ajax-rework-10.patch, no longer applies to the current dev flavour of Ubercart. May I ask you to provide a refreshed version of this, and of the 1316464-update-payment-method.patch? Or should I be using these in conjunction with the currently released version of uc and uc_coupon? Many thanks.

wodenx’s picture

Status: Needs work » Needs review
FileSize
46.61 KB

Rebased #10. It should now apply to current DEV.

I think longwave's idea in #15 is excellent. But would we want to make the updating a bit more fine-grained than a whole checkout pane? Perhaps panes could somehow expose subcomponents which could be updated separately (i.e. the payment methods section and the payment preview section)?

wodenx’s picture

Is there something in particular holding this up? I'm trying to get a replacement for "uc_free_order" working, and this would make it a lot easier. I still like the idea in #15, but that could be implemented separately.

Rafal Lukawiecki’s picture

Indeed, we are hoping to use this as a U3/D7 version of uc_order_free. At present, we are using "Other" payment method with a condition that balance is zero.

As an aside, it would be extremely useful to be able to have the ability to rename Other or Check (instead of using String Overrides), but above all, to be able to have more than 1 Other for various payment method conditions.

wodenx’s picture

I was thinking the same thing - essentially a "custom" payment method factory which would allow you to create any number of custom payment methods, using rules to enable/disable them and also to determine their effects on the order status (e.g. "free order" could autmatically mark the order as "payment received." when checkout is complete).

By the way - see #1518522: Update order_total when order saved if you are still having the problem of coupons not properly updating the payment methods...

Rafal Lukawiecki’s picture

Yes, the idea of a "custom" payment method factory is excellent.

Thanks for the tip about #1518522: Update order_total when order saved, hopefully this fixes the issue I have been still experiencing.

longwave’s picture

FileSize
58.19 KB

I think this approach is sound and seems to work well for me, but it would be good to hear test results from people who have tried this patch - or is no news good news? :)

On reviewing this again I am not sure we need a hook; can't we just use $form_state to store this data in the first place? The attached patch implements this approach.

Status: Needs review » Needs work

The last submitted patch, 1373236-ajax-rework-26.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
59.19 KB

Oops.

wodenx’s picture

I haven't tested this yet, but it looks good to me. Would it be cleaner to store in $form['#uc_ajax'] rather than $form_state? Also, removing the hook means that this rather recondite feature should probably be documented somewhere else.

The only thing about this that still feels messy to me is there's no way to prevent duplication -- if 3 different modules all want to update the line items listing, then that part of the form will be passed back 3 times, which seems wrong. Not sure how to remedy this -- do we need to allow modules to specify a callback? Maybe they could specify a list of elements to replace, and uc_ajax could filter that so that no element was replaced more than once. Or it could be done even more generally - modules could simply specify a list of whole panes. I know that contradicts my comment in #21, but as I think about it, sending back the whole pane seems vastly preferable to sending back a part of the pane multiple times, and would make it much easier to check for duplicates.

If we think modules might need to implement full callbacks in certain circumstances, we could allow them to do so as the exception - i.e. the data they store into $form_state['uc_ajax'] could be either
-an array, in which case it would be interpreted as a list of panes to replace.
-a string, in which case it would be interpreted as a callback.

I can try to make that alteration if it sounds like a sensible approach.

longwave’s picture

I used $form_state as that's always accessible from the form element process callback, $form is only available from the root element so then you have to use $form_state for temporary storage or propagate $form['#uc_ajax'] to all children, and keeping one copy in $form_state seems neater.

I agree that possibly sending duplicate divs is messy but I am not sure there is a guaranteed method to solve this? Is the line items div the only likely situation this will occur? Perhaps we should always send the line items div when Ajax updating the checkout form to save explicitly having to specify it?

If we are to offer sending panes as well as callbacks I am also not sure that relying on the data type to tell what to replace is the cleanest method. I guess we could have $form_state['uc_ajax']['callbacks'] and $form_state['uc_ajax']['panes']? But is this overcomplicating things?

Finally I also don't really like the complex array nesting we're getting into, perhaps

$form_state['uc_ajax']['uc_quote']['panes']['delivery']['address']['delivery_country'] = 'uc_quote_checkout_returned_rates';

could become

$form_state['uc_ajax']['uc_quote']['panes][delivery][address][delivery_country'] = 'uc_quote_checkout_returned_rates';

so the elements are keyed the same way (e.g.) form_set_error() works?

Agreed on documentation, we just need somewhere to put it. The top of uc_ajax_attach.inc is probably the best place.

wodenx’s picture

I agree that possibly sending duplicate divs is messy but I am not sure there is a guaranteed method to solve this? Is the line items div the only likely situation this will occur? Perhaps we should always send the line items div when Ajax updating the checkout form to save explicitly having to specify it?

Not a guaranteed method, but returning panes instead of callbacks will help greatly, esp. if we refactor all the existing cart-pane modules to use this method. If we offer callbacks as well, we can make it clear in the docs that these should only be used in exceptional circumstances (i.e. when you have to use a command other than 'replace'). And the line-items div is definitely not the only place - payment methods and shipping quotes will also be updated by various different things (entering a coupon, changing address, etc.). Finally, using panes will make it much easier to implement your suggestion in #15 cleanly -- otherwise we're almost certain to have many duplicates if admins can specify ajax behaviors willy-nilly. The more I think about it, the more I think this would be the right way to do it.

I am also not sure that relying on the data type to tell what to replace is the cleanest method.

Well there are certainly plenty of Drupal precedents for this method (e.g. the variables in theme_table, or the return value from an ajax callback) where you're allowed to specify the most commonly used type of value by itself, or use a keyed array if things are more complex. So I guess the options could be:
-an array of panes
-a keyed array with a 'callback' key (or even an '#ajax' key?)

Finally I also don't really like the complex array nesting we're getting into, perhaps

$form_state['uc_ajax']['uc_quote']['panes']['delivery']['address']['delivery_country'] = 'uc_quote_checkout_returned_rates';

could become

$form_state['uc_ajax']['uc_quote']['panes][delivery][address][delivery_country'] = 'uc_quote_checkout_returned_rates';

I remember thinking about doing it that way when I originally wrote it, but I can't for the life of me remember why I chose the other method, so I'm happy to make the change.

nelslynn’s picture

I've tried this patch (1373236-ajax-rework-28.patch) with the uc_custom_payment module (http://drupal.org/project/uc_custom_payment). I set up conditions to display certain payment methods based on order:order_total. The Ajax to display a corresponding payment method does not work when I change various shipping methods and/or ship-to country that change order total. However, when I refresh the page the correct payment method(s) display. All Ubercart/Drupal modules are up to date. I'm using the latest DEV version of ubercart (4/16/12).

More info:
- when a coupon is applied on the checkout page (making the order free), the payment method does NOT change via Ajax. However, with this module it does: http://drupal.org/node/1518522
- When a taxable ship-to state is changed, the subtotal does NOT change via Ajax.
- When a shipping method is changed, the Subtotal changes great, but the Payment method does NOT.
- When a ship-to country is changed, the shipping methods change and the Subtotal changes via Ajax, but the payment method does NOT.

Mixed bag here... not sure what module needs the fix.

wodenx’s picture

Try patch at #1518522: Update order_total when order saved and see if that helps.

wodenx’s picture

Sorry - didn't see your whole comment (must have been an edit).

- when a coupon is applied on the checkout page (making the order free), the payment method does NOT change via Ajax. However, with this module it does: http://drupal.org/node/1518522

Yes that patch is necessary for any order-total based rules to work via ajax.

- When a taxable ship-to state is changed, the subtotal does NOT change via Ajax.
- When a shipping method is changed, the Subtotal changes great, but the Payment method does NOT.
- When a ship-to country is changed, the shipping methods change and the Subtotal changes via Ajax, but the payment method does NOT.

The work on this thread is designed to make it easy to add such behaviors, but none of the ones you specify are provided by default. I believe the plan is ulitimately to implement something like #15, so admins can configure behaviors specific to their site.

Rafal Lukawiecki’s picture

Confirming this works for me now. See also comment #7 on #1473062: Update shipping methods when coupon is applied.. Still hoping for the remaining AJAX update scenarios, thanks.

wodenx’s picture

FileSize
95.85 KB

Attached patch to:

- replace nested arrays with a single key
- allow specifying a wrapper along with ajax callbacks - implementing modules can say, e.g., 'payment-pane' => 'mymodule_replace_payment_pane' - which seemed the simplest way to avoid redundancy.
- create a back- and front-end for admins to associate their own behaviors a la #15 -- didn't use Rules, as that seemed overkill -- just set a variable to an array mapping triggering form elements to panes that should be replaced.
- added a test to be sure the above works.

@longwave: It occurs to me that 99% of this functionality could be implemented by a contrib module, if you're afraid to commit without testing - the only drawback is that core modules couldn't then take advantage of it -- but maybe that's fine -- maybe we just remove the existing updates (e.g. refresh payment methods when billing zip code changes), and point people who want that behavior to the contrib module?

nelslynn’s picture

Patch fails with the new DEV version of ubercart, 2012-Apr-26.
Only the changes to file uc_payment_checkout_pane.inc fails.

Rafal Lukawiecki’s picture

Is this patch still necessary for 3.1 or has this been rolled into the new version?

bluewallmedia’s picture

This is my questions too...

I am having issue with my store address settings. The Country and Zone will not stick here, /admin/store/settings/store . I can get street address 1, city, and postal code. For what it is worth, this could be related to something else as I am having a few different issues with 3.1... Most notably on checkout.

mod_fcgid: stderr: PHP Fatal error: Unsupported operand types in /home/siteuser/public_html/sites/all/modules/ubercart/payment/uc_payment/uc_payment_checkout_pane.inc on line 83, referer: https://www.sitedomain.com/

I was thinking that perhaps it cannot calculate shipping or discount coupon and that kills the checkout process.

Is anyone else experiencing this inability to set/save the store address in ubercart 3.1?

best,

Peter

bluewallmedia’s picture

As a follow Up, my checkout error above was actually because of an issue with this patch (http://drupal.org/node/1016690) that I applied to http://drupal.org/project/uc_free_order

While my check out error is resolved, I am still unable to set my store address in the store configuration admin page of ubercart 3.1.

If I figure it out, which I have to do, I'll post a fix here. If any other good souls know this fix, please let me know. It would free up my time to help someone else.

Cheers!
...

bluewallmedia’s picture

#36: 1373236-ajax-rework-36.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1373236-ajax-rework-36.patch, failed testing.

longwave’s picture

@bluewallmedia: Your problems appear to have nothing to do with this patch; please start new issues if you are having problems with setting the store address or anything else unrelated in 7.x-3.1.

bluewallmedia’s picture

Will Do. Thanks @longwave

nelslynn’s picture

New Dev version of Ubercart (4/26/2012) rejects patch in #36. Payment methods therefore are not reacting to other Ajax events in other modules. When trying to patch the latest Ubercart Dev with the patch in #36, uc_payment_checkout_pane.inc rejects the following:

***************
*** 25,32 ****
  
        $methods = _uc_payment_method_list();
        $options = array();
-       $default = NULL;
- 
        foreach ($methods as $id => $method) {
          $set = rules_config_load('uc_payment_method_' . $method['id']);
          if ($set && !$set->execute($order)) {
--- 25,30 ----
  
        $methods = _uc_payment_method_list();
        $options = array();
        foreach ($methods as $id => $method) {
          $set = rules_config_load('uc_payment_method_' . $method['id']);
          if ($set && !$set->execute($order)) {
***************
*** 43,57 ****
          drupal_goto('cart');
        }
  
-       if (count($options)) {
-         if (isset($form_state['values'])  &&
-           isset($form_state['values']['panes']['payment']['payment_method']) &&
-           in_array($form_state['values']['panes']['payment']['payment_method'], array_keys($options))) {
-           $default = $form_state['values']['panes']['payment']['payment_method'];
-         }
-         else {
-           $default = (count($options) == 1 || empty($order->payment_method)) ? key($options) : $order->payment_method;
-         }
        }
  
        if (count($options) > 1) {
--- 41,52 ----
          drupal_goto('cart');
        }
  
+       $default = $order->payment_method;
+       if (isset($form_state['values']['panes']['payment']['payment_method'])) {
+         $default = $form_state['values']['panes']['payment']['payment_method'];
+       }
+       if (!isset($options[$default])) {
+         $default = key($options);
        }
  
        if (count($options) > 1) {
rickmanelius’s picture

From #36

- When a taxable ship-to state is changed, the subtotal does NOT change via Ajax.

and the issue summary

At the moment there is no mechanism for doing so, except a crude extension in uc_quote which allows quotes to be retrieved while updating the zone/state dropdown.

This is definitely a big issue for taxes, and I tried similar hacks like the 'crude extension' mentioned above, but applying the same crude extension to changes in the billing zip (changes the sales tax) causes the form to break.

I have some client time I can put towards this. @longwave and @wodenx, is patch #36 still the best starting point?

wodenx’s picture

AFAIK, yes - though it needs to be rebased. Also, there is bug in it which I haven't fixed yet -- when replacing a whole checkout pane via ajax, the rendered form element needs to be trimmed before being passed to ajax_command_replace(). -- theme_fieldset() appends a newline to the end of the text, which causes the ajax mechanism to wrap everything in an extra <div> each time the replacement takes place.

rickmanelius’s picture

@wodenx #47
Cool. And because this is such a large patch, I'm going to do this in 2 stages. First do the rebase with the patch as is (and any modifications needed to get that working). That way if for whatever reason I can't do the last part (the extra div appearing), then someone can use that as a launch point.

I'm also in the IRC channels today if anyone wants to coordinate. I'll upload the first patch as soon as I can.

rickmanelius’s picture

FileSize
29.75 KB

Re-roll from #36 to address the rejected elements in #45.

Now onto the extra div stated in #48

Edit:
(NOTICE: I did not test anything in #36. I merely dealt with the rejections... testing will commence while I work on #48)

rickmanelius’s picture

Status: Needs work » Needs review
FileSize
33.21 KB

Ok. I just talked to @wodenx in IRC and applied trim to the second parameter in all calls to ajax_command_replace.

wodenx: rickmanelius: yeah, the div issue is simple - you just gotta find every ocurrance of ajax_command_replace and make sure the rendered element is trimmed

I just completed that and added it to the patch in #49. Here is the total patch, ready for review.

Status: Needs review » Needs work

The last submitted patch, 1373236-ajax-rework-50.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review

#50: 1373236-ajax-rework-50.patch queued for re-testing.

longwave’s picture

If the retest passes then the first test triggered #1547314: Wrong date conversion in test_gateway_charge

rickmanelius’s picture

Oh hallelujah! I turned on the uc_ajax module and then enabled a new trigger in admin/store/settings/checkout/ajax (e.g. billing:postal code -> Payment method). I would suggest we add that by default because it will effect all tax rates/rules.

Nice!

rickmanelius’s picture

Last post before I stop pinging the thread.

Because there are so many changes in this patch, and because we won't want to rebase to chase head again... the sooner we can get this reviewed, the better! My initial testing shows that it's not throwing any visible errors (php errors or user visible HTML mistakes). So if anyone else can test, that would be super helpful.

And a final thanks to @wodenx for doing so much of the heavy lifting on this one. It made it really easy to do the last few tweaks and get this knocked out!

rickmanelius’s picture

BTW, I ran a series of manual tests.

  • uc_quote works as expected
  • toggling on a saved address works
  • toggling 'billing saved as shipping' works
  • Adding a custom ajax event (such as tax updates upon billing zip) works

Therefore if another review comes back with no issues, then I think we're good unless we want to wait for 2-3 reviews because of the size of the patch in question.

rickmanelius’s picture

A friendly 'plea'... anyone on this thread wanting proper ajax calls on the checkout form... please review! :)

nelslynn’s picture

I applied the patch to the DEV version of Ubercart (however confused as what version to apply.... this link indicates the non-dev version http://qa.drupal.org/pifr/test/270828 ). Anyway a few issues which I'm not sure are related to the patch.

  • I have a tax rule set up for one state, but when I change the delivery state on the checkout page, the tax does not automatically change.
  • I have PayPal Payments Pro setup (I do not have the test gateway enabled). I enter a credit card number of 4111111111111111 and code of 111 (an invalid card), but I'm still able to get to the review page. I have the "Validate credit card numbers at checkout" set in the settings (see screenshot). But, when I go to the review page with this same cc number, then back to the checkout page, I get 'invalid card number' (see screenshot). Maybe a PayPal issue?
  • I'm using the Custom Payment module (http://drupal.org/project/uc_custom_payment). I have it as the #1 option (default). When switching back/forth between whether this payment option should show or not (via rules, check for payment balance=0), it will not default to the top method when there are two choices. (might be a module issue).

Thanks for your continued efforts!!

rickmanelius’s picture

Hi nelslynn,

I have a tax rule set up for one state, but when I change the delivery state on the checkout page, the tax does not automatically change.

This is resolved by adding a custom ajax behavior here admin/store/settings/checkout/ajax. It's not turned on by default. Add billing state as the trigger and payment information as the pane to update.

I have PayPal Payments Pro setup (I do not have the test gateway enabled). I enter a credit card number of 4111111111111111 and code of 111 (an invalid card), but I'm still able to get to the review page. I have the "Validate credit card numbers at checkout" set in the settings (see screenshot). But, when I go to the review page with this same cc number, then back to the checkout page, I get 'invalid card number' (see screenshot). Maybe a PayPal issue?

- I think that's expected behavior. It's validating if it's a valid credit card input (e.g. does it have the proper amount of digits, etc) and not actually running a check on the card itself. That occurs when the order is to be processed.

I'm using the Custom Payment module (http://drupal.org/project/uc_custom_payment). I have it as the #1 option (default). When switching back/forth between whether this payment option should show or not (via rules, check for payment balance=0), it will not default to the top method when there are two choices. (might be a module issue).

This too is not turned on by default. Goto admin/store/settings/checkout/ajax and then place 'payment method' as the trigger and 'payment method' as the pane to update.

Do you mind testing the first and third items again by adding in the trigger/update elements?

nelslynn’s picture

Hi rickmanelius,

First, thanks for your continued work on this.

This is resolved by adding a custom ajax behavior here admin/store/settings/checkout/ajax. It's not turned on by default. Add billing state as the trigger and payment information as the pane to update.

I'm not sure how I missed this, but the module "Ubercart Ajax Administration" needs to be enabled to add ajax behaviors. Not sure if this is stated clearly? Anyway, I do not see an option for "Payment information". I tried "Payment Method" but no luck.

I think that's expected behavior. It's validating if it's a valid credit card input (e.g. does it have the proper amount of digits, etc) and not actually running a check on the card itself. That occurs when the order is to be processed.

When using Ubercart 7.x-3.1 and credit card number: 4111111111111111, a valid expiration, and code: 111, I get to the review page with an error message "did not pass validation" (see screenshot).
When using Ubercart 7.x-3.x-dev and this patch. I enter credit card: 4111111111111111, a valid expiration, and code: 111, I get to the review page with NO validation error. Only after I click "Submit" do I get an error. When I click the Back button (back to /checkout) from the review page (where the credit card number field changes to: (Last) 1111), I get invalid credit card number on the /checkout page. So, the validation on the /checkout page works for "Last(4) 1111" but not for "4111111111111111". Again, maybe not related to this patch, but thought I would mention this since the Dev w/patch version differs from Ubercart 7.x-3.1

This too is not turned on by default. Goto admin/store/settings/checkout/ajax and then place 'payment method' as the trigger and 'payment method' as the pane to update.

I have a coupon which makes the order total zero. When I add/remove the coupon, the payment methods change great, but the default does not change to the top method. See screenshot. This issue however may not be significant to this patch unless selecting the top payment payment should be part of the function. If I add another rule to show the Visa payment method when 'Payment balance>0', this works great. Thank you!

longwave’s picture

I have started reviewing this patch a couple of times, but it is so big that I have run out of time. I will try again to find a few hours to look through and test it again soon.

When using Ubercart 7.x-3.1 and ... When using Ubercart 7.x-3.x-dev and this patch...

Can you try testing Ubercart 7.x-3.x-dev without this patch? This will confirm whether the problem is in 7.x-3.x-dev (in which case please start a new issue) or in this patch (in which case discussion should continue here).

rickmanelius’s picture

Hi @nelslynn,

Could you try without the uc_coupon and uc_free_order modules? The uc_coupon module currently does not have integration into this, and I KNOW there is a bug in the port of the free order module (issue #1016690: Drupal 7 Support affects me personally).

I also have a use case for ajax events triggered by the coupon module (e.g. when the price goes to zero), but we'll need this patch in place first because it's part of the core of ubercart.

Hi @longwave,
I have a deliverable in 2 days that would benefit greatly from this. So if you need help testing, ping me in IRC as I'd love for this to go RBTC! :)

nelslynn’s picture

Category: feature » bug

Hi longwave,

It looks like this is a separate issue. Using the lasted Ubercart DEV, Jun 12, unpatched, I get this behavior:

When using Ubercart 7.x-3.1 and credit card number: 4111111111111111, a valid expiration, and code: 111, I get to the review page with an error message "did not pass validation" (see screenshot).
When using Ubercart 7.x-3.x-dev and this patch. I enter credit card: 4111111111111111, a valid expiration, and code: 111, I get to the review page with NO validation error. Only after I click "Submit" do I get an error. When I click the Back button (back to /checkout) from the review page (where the credit card number field changes to: (Last) 1111), and click 'Submit' again, I get invalid credit card number on the /checkout page. So, the validation on the /checkout page works for "Last(4) 1111" but not for "4111111111111111". Again, maybe not related to this patch, but thought I would mention this since the Dev w/patch version differs from Ubercart 7.x-3.1

I might also add that I experienced something really weird when using a valid PayPal Gateway account (note the scenario above was tested using PayPal Pro Payment method, but not with PayPal Gateway account credentials). I was testing this same scenario (using credit card number 411111111111111), and when I clicked "Submit", it went through!!! PayPal actually processed the transaction using my Bank funds (not the invalid credit card)!! I couldn't believe it. I'm not sure if this is a PayPal 'rule' or something in the ubercart code. But it should be looked into?

longwave’s picture

Category: bug » feature

@nelslynn: Please, post the above as a new bug report, as it is nothing to do with this feature request.

nelslynn’s picture

Hi rickmanelius,

I'd say everything works great with this patch, with or without the coupon and custom payment modules. I get the same functionality whether the two modules are enable or not.

I was able to get the sales tax to auto change on state change by adding the behavior: Delivery State/Province: Payment method (pane to update). I do not see a "Payment Information" option as indicated previously, however the "Payment Method" option works great.

The only issue I see is with the customer payment module. When two payment options are available, the top one is not automatically selected.

Again, nice work on this functionality.

rickmanelius’s picture

Hi neslynn,

First, wodenx et al did a bulk of the work. I just did some cleanup and I'm helping with the review. But thanks :)

I agree that the situation where the default payment option is not updated might be an issue we need to resolve. I'm going to look in that as well because I also use the uc_free_order functionality.

rickmanelius’s picture

Indirectly related. #1016690: Drupal 7 Support is essentially ready when this is accepted.

longwave’s picture

Spent a bit more time reviewing this, it's looking very good apart from the following minor points:

- Unsure whether uc_ajax_admin needs to be a separate module, or whether it should just be merged into uc_cart.admin.inc
- Ajax admin UI should probably use checkboxes instead of multiselect
- Inconsistent use of trim() in Ajax callbacks (not sure this is needed in all cases?)
- Some function comments need a bit of tidying up

I can't reproduce the issue where a default payment option is not selected, though I am only testing this with three core payment methods and various rules to enable and disable them depending on country.

I'm tempted to push this into a new branch where the above can be worked on and merged when ready, rather than trying to rebase and maintain this as a single patch.

rickmanelius’s picture

Hi Longwave:

- I could argue both ways for separate versus merged. My gut tells me to leave it separate just in case some functionality can be more universally applied and is not uc_cart specific. I don't see that happening, but just in case.
- Default payment isnt' a deal breaker as it just adds one extra click required and I'm sure it's something that can be spun to a new issue if it's a problem. And if we DON'T merge this with uc_cart, it would only affect users using this additional module as opposed to everyone.

I'd rather see it merged in the main branch as is and see the issues (trim, checkboxes, etc) be spun off as separate, smaller tickets. And because it would only be used by a subset of ubercart installs, it wouldn't break sites that didn't consciously turn it on for this functionality.

But I understand that might not be ok on your standpoint, so another branch works for me. Whatever speeds up the release :)

rickmanelius’s picture

Hey @longwave. Not to pester (as I'm extremely appreciative of your time/input), but I'm trying to plan out some features against a timeline and this issue influences a few... based on #69 and #69, what course of action do you think you'll be taking?

I want to push the current patch to my production site, but not if it's going to be refactored and incompatible with the patch in its current state.
Cheers!

wodenx’s picture

Status: Needs review » Needs work

Obviously i haven't had much time to work on this lately, will try to get back to it in the next few days:

- Unsure whether uc_ajax_admin needs to be a separate module, or whether it should just be merged into uc_cart.admin.inc

I wrote it this way on the same model as views_ui, rules_admin, etc -- trying to separate the API from the UI. Some people will have simple enough setups that they won't need to customize this, so why not give them the option to leave it disabled? But either way is fine with me.

- Ajax admin UI should probably use checkboxes instead of multiselect

You mean for the "panes to update" column? I'm not sure I agree -- checkboxes would take up more space, and be more easily confused with the "remove" checkbox (though maybe that should be a button, anyway). Again, don't feel terribly strongly here, so happy to change if that's what you prefer.

- Inconsistent use of trim() in Ajax callbacks (not sure this is needed in all cases?)

trim() is necessary only when the theme function that renders a form element tacks on whitespace to the rendered element, as does theme_fieldset(), form.inc ll 2679ff):

function theme_fieldset($variables) {
  ...
  $output .= "</fieldset>\n";
  return $output;
}

I think to be safe we should always trim the rendered element before inserting, since we have no control over the theme function.

Spawning a new branch is an excellent idea -- this patch is definitely getting unwieldy.

wodenx’s picture

***EDIT Deleted duplicate post. sorry.

wodenx’s picture

Regarding the default payment method issue raised in #58 &c, I can reproduce as follows
- create a payment method with a rule which makes it unavailable when you first hit the checkout screen. make sure that this method is the lightest, and that there are some other methods which
- do something to make that payment method available
- the new payment method will appear, but the first of the previous set will remain selected.
This is a separate issue and has to do with the way uc_payment determines which default to select -- we should address this in a separate thread, but I think the desired behavior should be that if the user has consciously selected a method, it should not be reset as other methods become available, but if she has not, the lightest available method should be the default.

rickmanelius’s picture

I can also reproduce the default payment issue. But I also agree that at this point it'll need to get spun off to another ticket to keep this patch/branch manageable at this stage!

longwave’s picture

I have managed to trigger the dreaded "An illegal choice has been detected. Please contact the site administrator." when using payment method and shipping quote rules based on country, then changing the country fields and using the "My delivery information is the same as my billing information" checkbox, haven't tracked down the source yet though.

+++ b/payment/uc_payment/uc_payment_checkout_pane.inc
@@ -69,6 +69,8 @@ function uc_checkout_pane_payment($op, &$order, $form = NULL, &$form_state = NUL
         ),
+        '#prefix' => '<div id="payment-method">',
+        '#suffix' => '</div>',

Where is this used?

All good points in #71; let's ensure we use trim() everywhere, but leave this as a separate module for now. Other than this I think we are good to commit, any remaining issues can be followed up separately.

rickmanelius’s picture

Hi Longwave.
I'm assuming in #75 you know where the code lives, you're just asking when is it called and/or in what callback/function?

Happy to hear that we'll keep as a separate module and it'll move towards this being committed :)

longwave’s picture

Re: #75 that is from the patch in #50, it adds a CSS ID that doesn't seem to be used anywhere else, so I am wondering why it is needed, or if it is redundant.

rickmanelius’s picture

It seems redundant as there are already id's like form-item-panes-payment-payment-method that could easily take its place!

wodenx’s picture

Re that wrapper in #75 - that's a relic - was used to update payment methods before I changed all the core modules to replace entire panes. I think it can be safely removed.

I also was able to trigger the "invalid option" error message, but only with both shipping quotes and payment methods that were conditional. going to try to find the source right now...

rickmanelius’s picture

Considering my need for this patch, if we commit this as is I'd be happy to help on the invalid option and default method parts as followup patches. It would help me know that this core functionality is actually in the system.

Cheers!

Rafal Lukawiecki’s picture

Just wanted to check on the status of this patch, and the related patches for uc_coupon. I will be updating our site this week to the most recent dev version of ubercart, and I was wondering if these patches are still the way to go, or should I wait for them to be committed to the core of ubercart. I'm happy to report on tests, too. Is the commit likely to happen this week? Thanks.

rickmanelius’s picture

Any updates on this since #79? I know we can get the "invalid option" with a specific combination of actions, but did it make it into a dev branch yet?

kslagboom’s picture

Status: Needs work » Needs review

#50: 1373236-ajax-rework-50.patch queued for re-testing.

Rafal Lukawiecki’s picture

Please excuse my ignorance, could someone explain why this feature did not make it into 3.2, and what would it take to get it into 3.3?

rickmanelius’s picture

Version: 7.x-3.x-dev » 7.x-3.2
FileSize
33.21 KB

A re-roll of #50 which has very little modification has been done based on line numbers changing slightly. No code is changed and this simply applies more cleanly to version 7.x-3.2.

longwave’s picture

As this touches such a critical part of Ubercart, it would be nice to get some more test reports - if you're using this successfully let us know!

Also, has anyone else managed to trigger "An illegal choice has been detected. Please contact the site administrator"?

Dan Z’s picture

Please excuse my ignorance, could someone explain why this feature did not make it into 3.2, and what would it take to get it into 3.3?

Test it more and report here on the results of your tests.

nelslynn’s picture

Using Ubercart 3.2 and the patch at #85, the only issue I notice is a 'sticky' tax rate/subtotal change. I have tax set for 1 state, when I change the state dropdown and country dropdown to various values, the tax rate does not disappear, and the suptotal is not recalculated unless I click inside the Phone field (the last field). I can get all the way to entering a CC number, and the tax and subtotal will remain for the previous value, even though no tax should be charged. However, when I change the state dropdown to the state that is configured for a tax rate, the tax field and subtotal get updated immediately.

longwave’s picture

This feature will also be very useful for #420980: No payment/billing should be required when order total is $0

@nelslynn: Thank you for testing, this needs some further investigation; it sounds like tax line items are not being handled correctly if tax is removed during checkout - I don't think we considered this case before.

coolestdude1’s picture

Status: Needs review » Needs work

Just want to let others know that the patch from #85 applies cleanly to the dev branch so using 7.x-3.x works just fine for that branch. I also have added #9 from #420980: No payment/billing should be required when order total is $0.

The module installs cleanly. Used admin menu to flush for the menu link in store.
I have added the 'Coupon: Apply to order' trigger to update the 'payment methods' from the ajax admin page.

And now I am stuck, the documentation on how to use this code is few and far between.

Couple of questions-
How do you use this module? First and foremost one of my biggest problems.
How do you properly add an ajax callback?
In which function(s) should code go for properly attaching onto an ajax callback?
Where does pane replacement code go?
In the replacement of a pane how do you affect the output?

Pushed back to 'needs work' documentation needs to be improved.

granticusiv’s picture

Hiya... might be a dumb question, but I'm still fairly new to patching. Where do I place the patch file? I tried running it within the ubercart/payment/uc_payment folder without success, and then again in the ubercart folder, without success. The error message coming up is:

can't find file to patch at input line 5
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/payment/uc_payment/uc_payment.module b/payment/uc_payment/uc_payment.module
|index 2527560..6e210f6 100644
|--- a/payment/uc_payment/uc_payment.module
|+++ b/payment/uc_payment/uc_payment.module
--------------------------
File to patch:

I'm using UC 7.x-3.2

longwave’s picture

Start in the ubercart folder, and use "patch -p1". See http://drupal.org/patch/apply for more details.

3CWebDev’s picture

Any update when this patch may make it into a release? I've been keeping an eye on this thread for about eight months and have several stores that need to calculate sales tax at checkout when a delivery state is selected. I've tried multiple times to apply this patch but have not succeed so far (maybe more difficult on Windows) so I'm eagerly awaiting this patch to be released.

*[EDIT] Phew! I was finally able to apply this patch and set the new Ajax config to update on State/Zip change and it appears to be working perfectly in my test environment.

rickmanelius’s picture

Summarizing thus far...

  1. Submodule or merge. There was discussion about whether or not to leave this functionality as a separate submodule (e.g. ubercart/uc_ajax_admin) versus moving it into uc_cart. This conversation started in #68 by longwave, but the reply in #71 in support of keeping it separate was agreed upon in #75. I also agree that separate is better.
  2. Illegal choice. There still appears to be some situations noted in #75 where an illegal choice can occur given the conditions listed there. So far no one has been able to replicate despite @longwave's followup question in #86.
  3. Tax rate. There appears to be an issue in #88 regarding tax rates not being properly set or stick. This could be a configuration issue, but it needs to be tested.

And finally, based on #89, we can consider this to be a closed issue if we can resolve items 2 and 3 above... so that's where we need to focus to get this to be a done deal.

I hope people find this summary post to be helpful...

rickmanelius’s picture

Addressing issue 2 in summary post #94.

  • I have the complete patch #85 installed on my local machine and I've been using it on production now since that date.
  • I created two payment methods: credit card (authorize.net) for US and COD for non-US.
  • I created two shipment methods: USPS Parcel for US and USPS Intl. Parcel for non-US.
  • I ran a series of tests:
    1. No address details and simply toggling the billing country.
    2. Full address for both shipping and billing while toggling the billing country.
    3. "My billing information is the same" while toggling the shipping country.
  • In all situations, the payment and shipment areas updated as expected with no issues and no 'illegal choice' warnings
  • And yes, I do have the reporting turned on for my local machine where I'm testing.

Unless I did not accurately replicate @longwave's tests, my tests do not show this error being thrown. There might be a configuration difference as I'm using a test system from a live site. I'll do a quick follow post, but will leave this for now...

rickmanelius’s picture

A quick followup from #95. I tested on a completely separate development server with the patch installed with no additional changes in configuration. I had both a shipping and a payment method toggling for US/International shipping/billing combinations. All of them worked.

Unless I'm missing something, I'm going to consider that issue resolved unless someone else can recreate this (or any other) "Illegal choice" situation.

rickmanelius’s picture

Addressing issue 3 in summary post #94.

I can confirm that the tax stickiness issue in #88 is still present. Basically there are two factors that need to be addressed to correct this.

In the "Checkout form Ajax Behaviors" page (admin/store/settings/checkout/ajax), we do not have default TRIGGERING FORM ELEMENT set to fire on changes to the shipping address state/country. Additionally, we do not have a PANES TO UPDATE option that specifically targets the tax rate. This latter part is probably a little more difficult and the default option open installation is probably trivial to add into this patch.

I believe this type of configuration should be in by default, or at least if someone turns on the tax submodule, because we can most likely assume they will have a tax rate in that situation and that it will change based on city.

Once this is in place, I believe we've addressed all the remaining issues in #94 and we can set this to RBTC.

rickmanelius’s picture

Status: Needs work » Reviewed & tested by the community

Followup to #97. I actually found a combination of TRIGGERING FORM ELEMENT/PANES TO UPDATE the resolves #88. If you set it to Billing: State/Province and then "Payment Method"... you will now see the tax rate update dynamically.

BTW, whether you use shipping or billing address as the trigger depends on your state, so this might not be something we provide out of the box as a default configuration because people can use all sorts of combinations of country + province combinations (or even zip code as in the case of NY state where every county has its own sales tax and thus zip is the proper way to do it).

In short, I don't think we address #88 out of the box because it will be too site/business specific. I'm not sure where exactly a warning/message would be best to document this, but that could be another ticket.

At this point, I feel comfortable setting to RBTC (having addressed all the summary items in #94) and wait for feedback from @longwave about whether this can be committed from this point.

longwave’s picture

Thanks for the comprehensive review.

I wonder if we should add a hook_help() to some of the taxes admin pages, suggesting that if you are adding dynamic tax rules you might want to enable the Ajax admin module and alter the Ajax triggers appropriately?

rickmanelius’s picture

Hi @longwave. I think that would be an extremely good idea because I bet more than one person will get snagged by this 'gotcha' because they simply wouldn't know where to look except those pages. All it would need to do is provide a link to that ajax admin page and list a brief instruction to use the "payment method" as the "PANE TO UPDATE". We might also provide a second help message if the module isn't enabled to prompt them to do so in order to add said configuration in.

Is this hook_help() addition something you'd like added to this patch prior to commit, or is it something you could quickly add without needing an additional submission?

longwave’s picture

I think that could be handled in a followup; this issue has gone on long enough with no recent changes, and the patch is already big enough, that we probably should just get it in -dev as-is and deal with additional features separately.

I will do a (hopefully final) review of the patch and see if I can recreate the illegal choice error again when I next get a spare hour or two.

MegaChriz’s picture

I haven't reviewed the patch in detail yet, but will Ubercart checkout still work without the uc_ajax.module enabled? What will no longer work when you don't enable the module?
What are the reasons for not enabling the uc_ajax module? (If there are none, then I'd suggest to place the module contents in uc_store.ajax.inc instead. If there are good reasons, then a separate module is a good idea.)

I think the issue summary should be updated: it would be helpful to have it clear what the consequences are of the changes this patch brings.

I hope to review the patch in a few days. (Maybe I will update the issue summary myself if I get a clear enough view of where this issue stands.)

By the way: I've seen illegal choice errors many times in the past and most times this had something to do with the zone field. I will try to see if I get this error also with the patch applied.

MegaChriz’s picture

I've updated the issue summary based on my experience with the patch so far and the information provided in #94.

So far it's looking great! I was a bit skeptical about it first, expecting that it would break some of my Ubercart add ons that affect the checkout page, but it turns out to be that the ajax event system for checkout has been abstracted very well and that it works pretty good. I see room for improvements though, but I agree about handling them in followups: it's already great it is out of the box!

I did see a few minor issues in the code though: minor coding standards issues and a few spelling errors in the code comments. But nothing big at first glance.

I'm going to test it further. For example, I have not tried to extend it yet. Need to figure out how to do that, still.

And my answer to my own questions in #102: I found out the uc_ajax module is only needed to configure ajax events on checkout. I first assumed that it needed to be enabled in order to get the ajax events system, that's why I suggested in #102 moving it to the uc_store module. It turns out that the base system is actually already there, so that's a good thing.

MegaChriz’s picture

I have been able to reproduce the "illegal choice" error! Now I need to find out what the steps are to reproduce it from a clean install. So far I know it did happen in a way like longwave described in #75. From what I can tell, I have three shipping methods, three payment methods and five countries. Two of the shipping methods have conditions set for delivery country and all of the payment methods have conditions set for the billing country. With two shipping methods currently valid, I did something like this:

  1. Changed the billing country.
  2. Checked "My billing information is the same as my delivery information".
  3. Changed the shipping method.

And the "illegal choice" error got displayed in the payment pane. I know this is a bit vague description, so that's why I'm planning to try to reproduce the error from a clean install. I have no time to do thay today anymore, hopefully I have some time to do that tomorrow, else I'm afraid I will have no time until Thursday.

I've yet also to test extending the system. I have not read the whole discussion here, but from what I have read so far, I assume extending can only be done by editing the form state in a form alter. Am I right? Or is there another method/way for extending the ajax event system?

MegaChriz’s picture

Might as well post the error message saved with watchdog already. Could be helpful:
Illegal choice flatrate_5---0 in panes element.

MegaChriz’s picture

I tried to reproduce the "illegal choice" error today. It looks like they occur because the "shipping methods" or "payment methods" panes don't update when "My billing information is the same as my delivery information" get's checked. In some setups this could lead to invalid available options in these panes.

Steps to reproduce

Setup steps:

  1. Enable the following modules:
    • Required Ubercart core modules
    • Payment
    • Payment Method Pack
    • Rules UI
  2. Activate three payment methods (COD, Check, Other).
  3. Set the following condition for COD: Billing country == United States
  4. Set the following condition for Check: Billing country == Canada
  5. Add a product to the cart and go to checkout.
  6. Note that:
    • For the billing country "United States" is selected;
    • Currently only the "COD" and "Other" payment methods are available;
    • And the payment method "COD" is selected.

Trigger error (method #1):

  1. Set the billing country to "Canada". The payment pane will update: "COD" is replaced with "Check".
  2. Check "My billing information is the same as my delivery information".
  3. Uncheck "My billing information is the same as my delivery information".

The "illegal choice" appears in the billing pane. Watchdog: Illegal choice check in Payment method element.

Trigger error (method #2):

  1. Set the billing country to "Canada". The payment pane will update: "COD" is replaced with "Check".
  2. Set payment method to "Other".
  3. Check "My billing information is the same as my delivery information".
  4. Select "Check" payment method.

The "illegal choice" appears in the payment pane. Watchdog: Illegal choice check in Payment method element.

I noticed there was no error when only one payment method was valid at a time. I unchecked the "Other" payment method and I could no longer reproduce the error.

Then some good news: I also got "illegal choice" errors when following the same steps when the patch is not applied (I had set back the original Ubercart dev version to test this). Thus I think the "illegal choice" error longwave was talking about in #75 is not caused by the patch and thus a different issue or subissue (or maybe just a configuration issue?).

MegaChriz’s picture

Issue summary: View changes

Update issue summary based on my experiences with the patch so far and the information provided in #94.

MegaChriz’s picture

Extending ajax event system for checkout
I did one quick test with extending the ajax event system using the following code:

/**
 * Implements hook_form_FORM_ID_alter() for uc_cart_checkout_form().
 */
function mymodule_form_uc_cart_checkout_form_alter(&$form, &$form_state) {
  $form_state['uc_ajax']['mymodule']['panes][billing][address][billing_last_name'] = array(
    'quotes-pane' => 'mymodule_replace_test',
  );
}

/**
 * Ajax callback to test overriding in the ajax checkout event system.
 */
function mymodule_replace_test($form, $form_state, $wrapper = NULL) {
  $element['comment'] = array(
    '#type' => 'markup',
    '#markup' => t('You have triggered something to update @wrapper.', array('@wrapper' => $wrapper)),
  );
  return $element;
}

The shipping quotes pane did succesfully get replaced with the contents from my custom callback when I entered something in the field "last name" of the billing pane and then left the field, so I guess extending the system works good at this point.

Can't override the config
However, I did see a potential weak spot in the system for when you want to extend it. The config saved in the variable "uc_ajax_checkout" (or the default config) always has the last word, which means modules can't override whatever is in the config unless they set the config themselves using variable_set(), which can be problematic if they want to temporary/conditional override the config.
Maybe this problem can be solved by using weights or something similar, but on the other hand, it does look like it's a rare edge case and probably not worth it to longer holding up this issue.

longwave’s picture

Version: 7.x-3.2 » 7.x-3.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
32.77 KB

@MegaChriz: Thanks for testing and providing detailed comments. As the 'illegal choice' error only happens with certain configurations and without the Ajax patch, then I think that should be dealt with separately. The same goes for overriding uc_ajax_checkout, we can maybe add an alter hook for this in a followup if needed.

I cleaned up some of the coding standards issues and moved the help message to hook_help(), but functionality wise this patch is unchanged - apart from I changed the quotes slider at checkout to a throbber, as I think this looks far better and more consistent with the other checkout Ajax updates. If this passes the testbot, I think it's ready to commit.

rickmanelius’s picture

Status: Needs review » Reviewed & tested by the community

Hi @longwave. #108 passed and (since it essentially does not change the functionality that brought it to RBTC last time), I'll set to RBTC now as per your suggestion.

Anyone with "illegal choice" issues is encouraged to open a new ticket.

MegaChriz’s picture

The only thing I see when I (quickly) compare the patch in #108 with the previous one in #85 is that in uc_ajax_admin_help() the placeholder @target_form doesn't get replaced. My suggestion is to remove the placeholder completely and just put the phrase 'checkout' there instead. Or are there situations when using the placeholder could still be useful? (Maybe when this functionality gets extended in the follow-ups?)

longwave’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
32.8 KB

Committed! Attached is the final patch that was actually committed; this is #108 with the throbber change at checkout reverted (I realised this should be a separate issue), and I fixed the uc_ajax_admin_help() issue noted in #110 by removing @target_form - the sentence makes sense without it.

Many thanks to wodenx for writing the bulk of this patch and rickmanelius, MegaChriz and everyone else who contributed test reports to this issue.

longwave’s picture

Issue summary: View changes

Changed conclusion of subissue "illegal choice": it looks like this error is not caused by the patch and exists also without the patch applied.

MegaChriz’s picture

Great! I haven't looked at the last made changes yet, but I want to say thank you to longwave for being a friendly and hardworking maintainer of Ubercart! :)

I guess the next thing to do here is create the discussed follow-up issues. It would be nice to place links to the new issues here.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Crossed finished tasks of the "Remaining tasks".