Hi,

Found that in file 'uc_quote.pages.inc' on line 152 value passed via Shipping method as $quote['format'] is being overridden by reformatting $quote['rate'].

      foreach ($data as &$quote) {
        if (isset($quote['rate'])) {
          $context['subject']['line_item'] = array(
            'type' => 'shipping',
            'name' => $quote['option_label'],
            'amount' => $quote['rate'],
            'weight' => 1,
          );

          $quote['format'] = uc_price($quote['rate'], $context);
        }
      }

If it is to avoid missing 'format' element, then it should first check if it is actually missing only then assign the value.

Please explain the reason if this bug is found invalid. As this interferes with formatting of Shipping quote.

Thanks & Regards,

Tajinder Singh

CommentFileSizeAuthor
#15 1393686-uc_quote-no-reformat.patch606 byteslongwave
#10 uc_quote.format.1.patch293 bytesTajinderSingh
#8 uc_quote.format.1.patch275 bytesTajinderSingh
#5 uc_quote.format.1.patch403 bytesTajinderSingh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Category: bug » feature
Priority: Major » Normal
Status: Active » Postponed (maintainer needs more info)

Why do you need to override 'format' but not 'rate'? uc_price() must be called here so price altering modules can change the shipping price if necessary.

There is no bug here, but your change could perhaps be a feature request.

TajinderSingh’s picture

TajinderSingh CreditAttribution: TajinderSingh commented

Actually was working on a Ubercart Multi-currency setup.

To show shipping rates in different currency, only alternative is the 'format' value which is not being stored in database. Otherwise if we will modify 'rate' to show shipping charges in other currency, it will get stored in database resulting in different charges paid from what were actually required.

For example:

  • In case, shipping charges in USD are 50.
  • If customer has selected GBP, the fee becoms GBP 32.30
  • Here now if we populate 'rate' with 32.30, Ubercart Shipping will store 32.30 as it is in database. So, on checkout Customer will be charged USD 32.30 instead of USD 50, which is incorrect.
  • On the other hand if we will put 32.30 with uc_price formatting received, it will only be used to show to users [only after modification mentioned]. Finally resulting in actual USD 50 being charged from customer.
  • But, in current situation format is getting overridden by uc_price formatted of 'rate' then blocking the expected behaviour.
TajinderSingh’s picture

TajinderSingh CreditAttribution: TajinderSingh commented
Status: Postponed (maintainer needs more info) » Active
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented

Have you made this change in your own copy of Ubercart? If so please post a patch.

TajinderSingh’s picture

TajinderSingh CreditAttribution: TajinderSingh commented
FileSize
uc_quote.format.1.patch403 bytes

Haven't created a patch ever, but here is a try with Meld.

TajinderSingh’s picture

TajinderSingh CreditAttribution: TajinderSingh commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, uc_quote.format.1.patch, failed testing.

TajinderSingh’s picture

TajinderSingh CreditAttribution: TajinderSingh commented
Status: Needs work » Needs review
FileSize
uc_quote.format.1.patch275 bytes

Status: Needs review » Needs work

The last submitted patch, uc_quote.format.1.patch, failed testing.

TajinderSingh’s picture

TajinderSingh CreditAttribution: TajinderSingh commented
Status: Needs work » Needs review
FileSize
uc_quote.format.1.patch293 bytes
TajinderSingh’s picture

TajinderSingh CreditAttribution: TajinderSingh commented
Status: Needs review » Reviewed & tested by the community

Please check as test cleared.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Reviewed & tested by the community » Needs review

You cannot set your own patch to RTBC.

TR’s picture

TR CreditAttribution: TR commented
Version: 6.x-2.7 » 6.x-2.x-dev

Feature requests should be made against HEAD.

TR’s picture

TR CreditAttribution: TR commented

#10: uc_quote.format.1.patch queued for re-testing.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
FileSize
1393686-uc_quote-no-reformat.patch606 bytes

I think it is safer to allow modules to return $quote['format'], but set it if they don't.

TR’s picture

TR CreditAttribution: TR commented

OK with me.

There was a distinction between 'rate' and 'format' in D5 because 'rate' contained the raw float value while 'format' contained a string with currency sign, thousands separators, etc., but when uc_price() was introduced in D6 all this was changed and we ended up storing the same thing in 'format' as we do in 'rate'. I don't see the need to have both passed around anymore, and indeed 'format' is gone in D7.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs review » Fixed

Committed #15.

Status: Fixed » Closed (fixed)

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

chriscohen’s picture

chriscohen CreditAttribution: chriscohen commented
Status: Closed (fixed) » Needs work

With this commit, it's actually broken the display of shipping rates in the uc_vat module.

The OP's feature request was NOT to recalculate the 'format' value if it's already been calculated.

It looks like, when displaying the shipping rates where uc_vat is enabled on shipping rates, it actually relies on recalculating the 'format' value.

I suppose you could say that this is a bug in uc_vat, and that it shouldn't be displaying VAT on shipping rates via this method. Or, you could argue that this is a valid way to do that, and the above patch has broken it.

It hasn't already been filed as an issue in the uc_vat project, which is odd, but I figured that since a limited number of sites use the module, and some of those sites might be older versions of ubercart, and not everyone will want to calculate and display VAT on shipping rates, that it might just have gone unnoticed.

I am not sure what the solution might be here, so I'm unable to propose anything I'm afraid.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs work » Postponed (maintainer needs more info)

uc_vat doesn't alter shipping rates directly though, as far as I can see? There is apparently no code in uc_vat.module to do this, although it may apply a "tax adjustment" depending on your settings. Can you give an example of what's now broken including some actual figures, along with details of your configuration, especially the "VAT on shipping" setting?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Postponed (maintainer needs more info) » Closed (fixed)

No further info provided.

TajinderSingh’s picture

TajinderSingh CreditAttribution: TajinderSingh commented

Thanks :)