Hi,

I have some taxes that depend upon the chosen shipping quote method. Currently serializeOrder() doesn't include that information in the order, which causes uc_quote_condition_order_shipping_method() to try to infer the quote method from the name of the line item. In my case I had methods with identical names and was getting bugs. I'm about to attach a patch that adds the method's machine name to the order. I haven't tested it with anything other than uc_taxes.

Thanks

CommentFileSizeAuthor
#9 1157494-uc_payment-shipping-method.patch1.1 KBlongwave
#1 uc_payment_serializeorder_quote_method-1157494-1.patch1.12 KBandyf

Comments

andyf’s picture

andyf commented
Status: Active » Needs review
StatusFileSize
new uc_payment_serializeorder_quote_method-1157494-1.patch1.12 KB

Status: Needs review » Needs work

The last submitted patch, uc_payment_serializeorder_quote_method-1157494-1.patch, failed testing.

andyf’s picture

andyf commented
Status: Needs work » Needs review

Think it failed due to #1096900: Releases should not be tested.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 6.x-2.4 » 6.x-2.x-dev

It is much easier for the maintainers to accept patches against -dev if possible.

andyf’s picture

andyf commented

Sorry, yep, I did check that there were no differences between the dev file and 2.4's, but must've been on autopilot when setting the version on this issue! I haven't tested on dev however.

andyf’s picture

andyf commented

#1: uc_payment_serializeorder_quote_method-1157494-1.patch queued for re-testing.

sin’s picture

sin commented

Exactly the same problem but with uc_conditional_payment + http://drupal.org/node/952532. The patch works with 2.4. Thank you!

sin’s picture

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

It saved me the second time, the same issue with #952532: Dependency on shipping method. Please commit.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1157494-uc_payment-shipping-method.patch1.1 KB

This patch is functionally the same, just reformatted to fit the coding standards a bit better. This seems to work for me, if someone else can confirm then it can be committed.

tr’s picture

tr commented

I think it's fundamentally wrong to have shipping-specific code in uc_payment.js. If it's a matter of serializing all the line items, then the code should look for line items in general, and not specifically named line items like shipping. Otherwise this same problem gets pushed to another type of line item, and won't work for custom line items.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

It is far too much work to refactor all the checkout JavaScript in 6.x to handle all possible cases, especially where other contrib may be relying on code that is already present. Committing this and #1132378: Checkout page does not contain order UID for AJAX-based conditional action calls will fix bugs in CA support for core checkout functionality, at this stage of the game I think this is the best we can aim for.

longwave’s picture

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

Committed #9.

Status: Fixed » Closed (fixed)

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