The in-page JS used by uc_quote.module calculates the rate by parsing the value from the generated HTML. However, the value is returned as a string to set_line_item.

This leads to an invalid subtotal being calculated early in the page load. This is eventually overwritten through various other page events. However, the uc_free_order module is invoked before the other calculations are re-run.

This creates a scenario where a user has a free order, then goes to review the order and hits "Back". When they are returned to the checkout page they are presented with credit card details because the Total was not zero when the JS ran.

I have attached a simple fix.

CommentFileSizeAuthor
uc_quote_parseInt.patch680 bytesgn0rt0n
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TR’s picture

But rate is a float, isn't it?

Here's an example of the checkout markup:

<div class="form-item">
  <input name="rate[fedex_ground---GROUND_HOME_DELIVERY]" value="26.63" type="hidden">
    <label class="option">
      <input class="form-radio" name="quote-option" value="fedex_ground---GROUND_HOME_DELIVERY" type="radio">
      <img class="fedex-logo" src="/sites/all/modules/uc_fedex/uc_fedex_logo.gif" alt="FedEx Logo">
      Home Delivery (1 package): $26.63
    </label>
</div>

Your patch:

var quoteButton = $("input:radio[name=quote-option]").click(function() {
  var quoteButton = $(this);
  var label = quoteButton.parent("label").text().split(":", 2)[0];
  var rate = parseInt($("input:hidden[name=\'rate[" + quoteButton.val() + "]\']").val());

So in this example, $("input:hidden[name=\'rate[" + quoteButton.val() + "]\']").val() returns 26.63, so the parseInt() is not what we want... shouldn't this be parseFloat() ?

What's that inline jQuery supposed to do, anyway? I put a watch on the problem line and it never got executed during checkout. I don't like the whole idea of inlining the JavaScript - it would be a lot easier to maintain if it were extracted into uc_quote.js like the rest.

gn0rt0n’s picture

Status: Needs review » Needs work

You are correct, it should be parseFloat. I realized that last night before I went to bed. I was testing with the flat_rate module so my values were whole numbers and I didn't catch it.

I agree with you on the inline JavaScript. I don't like it. Took me about 2 hours of Firebug tracking to catch the problem because I was looking in the external scripts the whole time.

The only time I got this to fire was to go to the checkout screen and then review the order, or leave the credit card field blank. Then when you hit "Back" on the review order screen, or the screen force reloads due to blank credit card the code will execute very early in the refresh. Then a few cycles later the appropriate code from the JS will fire and return the correct results. Unfortunately, the uc_free_order module has already been invoked which used the initial string concatenated version.

Ultimately we should probably have uc_free_order fire as the last event on the page, but that is a separate issue to be dealt with. For the time being I wanted to expose this issue as it might catch a few people by surprise.

longwave’s picture

Status: Needs work » Fixed

Committed the parseFloat() version of this. Fortunately, this JS abomination is gone in 7.x.

Status: Fixed » Closed (fixed)

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