Once an order is no longer a shopping cart, you can manually adjust the unit price of its line items and trust the values not to be automatically recalculated based on the current product price. The problem is that even though the unit price of the line item updates, it does not appear to reset the data array / reapply taxes. This is visible by updating the unit price of a completed order and then going to its view tab... you'll still see the old order total based on the original product price because the line item has stale data (specifically the components array in $line_item->data).

CommentFileSizeAuthor
#5 Screen shot 2011-06-08 at 5.07.43 PM.png150.97 KBessbee

Comments

wildleaf’s picture

wildleaf commented

I'm having the exact same problem. This makes manually creating an order with special price impossible. What's the status on this bug? Is it a difficult fix? Thank you.

rszrama’s picture

rszrama commented
Issue tags: +rcblocker

It's not a super difficult task, but it is tricky. The question is, how should we handle the recreation of the price component array. I'm going to float an idea and sit on it, and I'll make sure to come back to it before rolling an RC. Basically, I think what we can do is rebuild it in these steps:

  • Add a base_price component for the base price on the product.
  • Add an additional component (non-existent atm, but maybe named manual_adjustment) that indicates the difference between the base price and administrator supplied amount.
  • Allow the tax module to hook in (again, via a non-existent hook, no ideas on name atm) here and specify that we should calculate taxes. Knowing which Rules to invoke will be the tricky part, as we don't want to run every product pricing rule, just the ones that apply to the calculation of taxes. So we can't depend on the event...

This approach has the drawback of not allowing administrators to supply custom prices including VAT, but I'm ok with that for 1.0. It's a shame to not support that here, but it's kind of fallout from the poor nature of the architecture behind the line item edit form. :-/

googletorp’s picture

googletorp commented

I have created a patch for the price field that handles the data components when they are missing or needs to be updated. I figured it was a bit silly that everywhere a price needs to be set (including in contrib modules) they need to know about the price component. This should be handled by the price module IMO.

I haven't done any work on letting other modules know about price changes. Right now we only have taxes on shopping cart orders which can't have their prices alters through the UI, so until work for this is done, we don't really have anything to ingrate with. I imagine it would make more sense to add tax handling for these types of line items and the needed hook/rule that needs to be implemented here in one go.

Related issue: #1159166: Add tax handling for non product line items.

Anyways I have switched over to doing commerce development on a Drupal sandox: http://git.drupal.org/sandbox/googletorp/1176472.git

The commit for this issue

And the diff

googletorp’s picture

googletorp commented
Status: Active » Needs review

Need to get better at setting status the first time: Needs review.

essbee’s picture

essbee commented
StatusFileSize
new Screen shot 2011-06-08 at 5.07.43 PM.png150.97 KB

Playing around with this patch and editing line item prices on orders via the admin interface sees some interesting behaviour.

Prices that are entered tax inclusive get the same original amount of tax added for every edit - so if you edit and save the order 3 times the tax is 3 times what it was on the original price - the modified price being ignored.
This tax however is only the amount displayed in the component breakdown, the total appears to ignore.

A screenshot might help explain better - see the attached, which is annotated.

essbee’s picture

essbee commented

Looking further into this, to maintain a logical UX I think there would need to be a check made on the tax inclusive status of the line item so that if the base price is over written, it is assumed to be over written with a price in the same inclusive state.

This would obviously then necessitate re-calculating the tax component included, to update the price data for the line item.

essbee’s picture

essbee commented

Have marked as duplicated http://drupal.org/node/1094266 and http://drupal.org/node/1152444 as they both have the same fundamental root cause - namely taxes/totals not being updated.

googletorp’s picture

googletorp commented
Component: Line item » Price
Priority: Normal » Critical

I've bugfixed the patch, to handle all situations and found another situation where it applies.

I found that custom price fields don't by default get their components added. This means that if you use rules to change the product price value to be the same of as a custom field, (fx price field used for discount prices), then the order total don't include that to the order total resulting in those products being "free".

Regarding the tax issue that essbee describes, we might want to tackle that as a separate issue, as it probably isn't related to this patch, but instead revealed by it. Having this weird inconsistent price handling which is further complicated by the way commerce_price_component_add works is something we should fix ASAP IMO.

The link to the updated commit in my sandbox

hunziker’s picture

hunziker commented

Im not sure, but potentially you could reuse some of this module:
http://drupal.org/files/issues/commerce_order_total.zip

Here are more Information about it (http://drupal.org/node/1098028#comment-4327954). This approach tries to add an additional layer to enable a custom order total calculation. I know that this approach was rechecked, but for some inspiration it could potentially help.

rszrama’s picture

rszrama commented
Status: Needs review » Fixed

@googletorp - the code has changed a bit since your patch, but maybe the changes will make it clearer why we need to keep component handling the way it is (at least for now). If you look at the actions in commerce_line_item.rules.inc, you'll see that they're now adding a price component of any type the administrator wants. This means a Rule might specify it's adding a discount to the order instead of just calculating against the base price. Additionally, I don't see your change to the presave hook accommodating prices that include VAT; in such cases, the base_price component will not equal the visible price amount of the field, as VAT will have been added in for the sake of display. One change we might consider keeping though is defining the components array in presave if it's missing altogether and setting a base_price component.

At this point, I am going to stick with the plan I outlined in #2 and take essbee's thoughts in #6 into consideration. In other words, we'll recalculate and preserve whatever taxes are already in the array. Eventually we need a better way to recalculate taxes on an order, and I assume that will come at the same time as the solution to the issue googletorp links above regarding taxing other line items.

The logic I'm following here is that regardless of whatever steps were taken to make a price what it is on an order, altering it on an order says "This is the price now." This will not change the taxability but it will wipe out the history of any discounts or conversions that have taken place to make a price what it is. I'm not going to use a separate price component to show the difference as I mentioned above though, because I don't know how to best represent that as a component (basically, there's no way to hide that information from users).

For now, see this commit: http://drupalcode.org/project/commerce.git/commitdiff/d0a6e8f

Feedback welcome; I've tested this with no tax, sales tax, a single VAT, and two compound VATs.

essbee’s picture

essbee commented

You've made my day Ryan, if this patch does what it says on the box I'll be one happy chappy.
Testing now. Will provide feedback asap.

essbee’s picture

essbee commented

BRILLIANT.
Can not comment for all use cases, but this certainly works flawlessly for me.
My scenario is two taxes, one VAT type, one Sales type. Prices are all entered inclusive of the VAT.
Modifying prices in the admin interface updates the order totals and all taxes perfectly.

Ryan, your a legend.

rszrama’s picture

rszrama commented

w00t! Glad to hear it's working. : )

Out of curiosity... will any of your products end up with both a VAT and a Sales tax type? Or are those for two separate markets?

essbee’s picture

essbee commented

The only thing I am missing now is the ability to trigger tax calculation (of non-inclusive taxes) without using a cart status to trigger (as this will also recalculate sell price). I can override the price post tax calculation andall updates fine, but would be great to avoid those statuses all together (and give the ability to run a cartless system). That would also give the ability to create orders as an administrator (eg phone orders etc).

Give the taxes are now being recalculated outside of the cart statuses when we modify a price this must be possible, I just can't see how to trigger a rule.

rszrama’s picture

rszrama commented

Yeah, I think we do have a separate issue for that - I envision it being a button on the form that you click to recalculate taxes on the line items of the order. I was more curious from a big picture perspective, though, if you actually serve a tax jurisdiction that combines a VAT with a sales tax. I didn't think the code I wrote would work for the combination of both types of tax on the same line item.

essbee’s picture

essbee commented

Well it seems to work beautifully here.
I'm working with wine, which in Australia gets a hideous 29% tax (WET) when the wholesaler sells to the retailer, the GST of 10% atop that.
I have put in all the prices inclusive of the WET and leave the GST to be calculated as a sales tax (as this is how most in the wholesale side think of things).

Out of interest is there any reason not to just calculate non inclusive taxes on any order save event?

essbee’s picture

essbee commented

This would be the issue you were talking about regarding manual calculation http://drupal.org/node/1094266
I closed it when I went through trying to round up all the various issues that were basically just different forms of the totals/taxes calculation issue, to focus it all in on a single issue (this issue).

I had thought that the fix for this issue would automagically fix that issue also, guess I was mistaken. I'll reopen it.

rszrama’s picture

rszrama commented

Ahh, cool. Thanks for finding that. : )

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