This bug involves a reasonably complex rules configuration for Canadian GST as follows:

There are 4 different levels of GST, so I created 4 tax rates. I used the sales tax type since the GST only gets added at checkout.

I disabled the default Rule Calculate taxes: Sales tax

I created four product pricing rules, one for each tax rate using the Apply a tax rate to a line item Action. In the Condition section of each I applied a comparison with the State/Province field since GST varies from province to province

I also have shipping enabled and entered into the Event section of each Rule so that tax is applied to both the products and to shipping.

Everything works well, except I found a bug when changing the province. When that happens and I change from a province with GST of 5% to one with GST of 12% and hit submit, I get two GST line item entries. The products line item is correct but the shipping line item one retains the GST rate from the previous province selection.

I discovered this, naturally by testing the Rules I had created. The problem is that sometimes customers will enter in the wrong province and go back and change it. When they do there is a chance that the shipping GST will be wrong.

This problem may or may not be related to the other issue I posted: http://drupal.org/node/1515598 and that by improving the frequency of processing of shipping data, both issues might be resolved.

CommentFileSizeAuthor
#3 commerce_shipping-refresh.diff5.39 KBNewZeal

Comments

googletorp’s picture

googletorp commented
Status: Active » Postponed (maintainer needs more info)

I can't see this is related to commerce shipping, and I'm not sure I understand the problem, what is a GST line item?

jazzdrive3’s picture

jazzdrive3 commented
Status: Postponed (maintainer needs more info) » Active

googletorp,

I'm running into a similar issue related to taxes. Shipping calculation rules are not being fired as often as they should be. See my posts near the end of the http://drupal.org/node/1243218 issue.

The main issue is that rules_invoke_all('commerce_shipping_calculate_rate', $line_item) is only called when a new shipping line item is created and attached to the order, and never again.

So basically, I think we need the commerce_shipping_calculate_rate event invoked at other places. What do you think the best way to do that is?

Thanks!

NewZeal’s picture

NewZeal commented
Status: Active » Needs review
StatusFileSize
new commerce_shipping-refresh.diff5.39 KB

I have implemented hook_commerce_cart_line_item_refresh() and that has solved the problem with the Tax for shipping containing old values for province, which is what the problem was.

The Problem: When I changed a province, the tax for products would calculate successfully using the new province. Meanwhile tax for shipping was being calculated using the previous province. This was occurring even when submitting the checkout form. I would get one tax value for products and another for shipping and the latter would be calculated for the wrong province.

With this hook, on the checkout submit, the shipping gets calculated twice, once using form submit with any new values that the user might have entered, and the second is a refresh to flush out old values in preparation for any Rules that might be triggered.

This hook is also called on the checkout page itself and therefore solves any problems that we have with users going back to the cart, adding items, and consequently affecting their shipping costs, so this is a solution for the other bug I posted as well: http://drupal.org/node/1515598

Please review the attached patch.

jazzdrive3’s picture

jazzdrive3 commented

Thank you for discovering that hook! I looked everywhere and couldn't find it. Seems like just what we need.

I'm not sure the entire patch is needed, however. But I just realized this issue was for shipping version 1.0. This is fixed in 2.0. If any changes are made to the cart, any shipping line item gets deleted.

However, for 2.0, the shipping recalculation rules still need to be called more often than they are. So I assume hook_commerce_cart_line_item_refresh() is the place to do it.

NewZeal’s picture

NewZeal commented

It occurred to me that this code is not a very good generic solution: this script currently only feeds the billing and delivery address into the form_values part of the calculate_quote function and that other shipping methods might have extra fields and options.

On the basis of the above I am moving my code to the custom shipping module I created and I would suggest the same for any other shipping module to ensure that all variables required for a shipping module are fed back into the function.

It appears that the shipping method I have selected is retained by the order object and there is no need to store this user selection separately.

jazzdrive3’s picture

jazzdrive3 commented
Title: Shipping is not being updated regularly enough to keep up with other events. » 'commerce_shipping_calculate_rate' event needs to be invoked more often
Status: Needs review » Active

New Zeal,

I think I'll change the title of this to reflect the more generic solution I'm proposing, at least until I get feedback from the maintainer.

Simply, that the shipping calculation rules event must be invoked in more places than when the line item is created. This will allow people to have their own calculation rules trigger when needed, and solve many problems, I think.

NewZeal’s picture

NewZeal commented

The problem with invoking the rules event is that the shipping plugin itself depends on the form values. Provided those form values are saved in the order object then they should be able to be processed any time that the event is triggered. Currently the actual shipping form selection is being saved, but if a shipping plugin has other fields that are processed then these might not be so a generic solution is not easy to achieve.

googletorp’s picture

googletorp commented
Assigned: Unassigned » rszrama
Status: Active » Needs review

Ryan can you look at this?

rszrama’s picture

rszrama commented

Yeah, on the surface this seems like a good solution, and the tax situation is a perfect use case to test it. I haven't checked the code out yet, but I can at least confirm the issue needs to be dealt with. Will try to come back around to it.

rszrama’s picture

rszrama commented

One pitfall in running the calculation more often is that you typically don't want the shipping rate to change after the user has selected it. Rate calculation rules are supposed to determine the final cost of shipping through a given solution, but in the case above it appears that we need to know the address before we can accurately calculate the taxes that may need to be included in a shipping line item.

While taxes are a good use case, they're not the only use case for when rule calculation may come into play. I think instead it would be better for #1287124: Support recalculating shipping when the address is entered to be the solution here - actually updating the shipping rates while you're on the page as the shipping address is being modified. Otherwise you end up with customer support calls like, "It said $14.50 for expedited shipping, but when I got to the review page it read $15.60. Which is it?"

What do you guys think? I know it seems above I was initially interested in this approach, but thinking on it now it seems to be insufficient.

NewZeal’s picture

NewZeal commented

In Ubercart 6.x the shipping was a javascript action that took into account all the current settings in checkout. My impression remains that you need to recalculate on page load, since shipping no longer uses javascript, and to do that, you need to remember the user applied settings. I have done that for my own solution and it works as expected and there are no surprises when going to the review page. As explained earlier in this thread, without that recalculation I was getting some surprises.

The problem, of course, is that different shipping methods will have different settings calculations and therefore there is no generic solution possible.

googletorp’s picture

googletorp commented
Status: Needs review » Closed (won't fix)

I think we should focus our effort on #1287124: Support recalculating shipping when the address is entered there's a patch for that now - that I haven't looked at yet.

I'm marking this as won't fix, because it seems that the above fix will be a better solution and should fix this problem. If anyways feels differently feel free to open the issue and we can discuss it a second time.

googletorp’s picture

googletorp commented
Issue summary: View changes

Small change to wording.