Inclusive tax not visible in cart block or checkout pane

Can you be more specific? Tax and shipping line items are not normally displayed in the cart block. Or are you talking about displaying a product price as price including tax, instead of just the product price?

Marked #1169144: Checkout only showing net of tax as duplicate

The attached patch fixes this. It is essentially bwynant's solution from #3, but it also modifies the "subtotal" line-item to add any included taxes, so as to agree with subtotal as displayed in the cart review pane.

I'd love to see this included in the official release, as it plays havoc with displaying coupons in the cart.

HI,

thank you for your patch!* But there is a little problem in the code ... You have to round the tax-amount to two decimal places. Just try to order 1000 items of a product and show in the cart - you'll see the problem.

Just add one line code to the patch:

function _uc_taxes_get_included_tax($item) {
  $node = node_load($item->nid);
  $amount = 0;
  if ($node) {
    foreach (uc_taxes_rate_load() as $tax) {
      if ($tax->display_include && in_array($node->type, $tax->taxed_product_types) && ($tax->shippable == 0 || $item->data['shippable'] == 1)) {
        $amount += $item->price * $tax->rate;


/*add this line to solve the view-problem with adding 1000 items of one product in the cart*/
+$amount = round($amount, 2);



      }
    }
  }
  return $amount;

* ubercart takes this patch in core. This is the reason, why I marked this as "critical".

Changing priority back. See http://drupal.org/node/45111
Changing status to "needs work" because of comment #8.

It's not critical but it should be a release blocker; there's no point having the inclusive tax feature if it isn't displayed everywhere.

We need to take into account the findings in #479784: Order rounding when adding rounding. Perhaps we should just commit #7 or #8 and deal with that separately, though.

Attached patch is a variation on #8, using 'uc_currentcy_prec' to control the rounding rather than assuming 2 decimal places.

Note that this problem exists in the original code as well (see uc_taxes_uc_cart_item()) - so perhaps should be a separate issue in case similar issues occur elsewhere?

The tax looks like it's getting applied twice to the "Cart contents" pane on the checkout page. Somehow, the change to the cart block is stacking with the change to theme_uc_cart_review_table(). I was testing this with PHP 5.3. Could someone check that it also happens in 5.2? I have a sinking feeling about modifying these cart item objects.

Aha. Got it.

This happens whenever hook_uc_cart_item('view', ...) is invoked more than once in a single request (e.g. when the cart block is displayed on the checkout page). This is because hook_uc_cart_item('view', ...) implementations are passed the cached cart contents by reference - so any alterations they make to an item (such as updating its price) remain in effect for the duration of the request. The attached patch fixes this by cloning cart items before passing them to hook_uc_cart_item('view', ...).

Applied #14 patch and it works. Thanx!

I was hoping to see the basic price and the taxes separated, and then the total amount, though. (at the moment it shows 'only' final price)

I subscribe and hope this not to be categorized as a 'minor bug'. I can't believe my online clients won't matter paying 1160€ instead of 1000€ as shown on the checkout screen !!!!! :-(

I don't dare to change priority but I would say
Priority - major
Status - active (to solve showing both amounts separately) and patch-to-be-ported (to apply this patch to a new Ubercart version).

I have to agree that the Subtotal line shouldn't be altered that way. The taxes will be listed right under it with a total, and I think that should be enough. The list of products at the top of the page still shows the sell price with the included tax (even if you don't qualify for the tax).

Same as #14, but removed uc_taxes_uc_line_item_alter(), so that the Subtotal line only contains the sum of the product prices.

I've been looking on and off at this for a while, and my only real conclusion is that the existing code is horribly ugly, but difficult to fix. Having to calculate display tax on products, cart items and order items separately isn't ideal, and I think we can and should make it more consistent - but at the expense of breaking APIs.

I propose something like uc_product_adjust($node) which will take a cloned node object (which could be just a plain display node, or a node with cart item data properties) and adjust sell_price, cost, weight, model, shippable as necessary via hooks (so uc_attribute and similar modules can do whatever they need to adjust the original node), then invoke another hook to get the "display price", which uc_taxes can implement to deal with VAT. It would also be useful to have both the untaxed and taxed prices available in node displays, Views, etc. for stores that sell B2B and B2C.

There is then still the issue of what to do with historical order data. For VAT users ideally we want to be able to display net price, VAT amount, and/or gross amount for each product line item to be able to generate invoices that comply to VAT regulations. However, to do this, we should be storing the VAT amount as calculated at checkout instead of recalculating every time - in fact we can't recalculate every time at present if the tax rate has been deleted (unless we store them in the order data), or the original node has been deleted (as $node->type is needed for the calculation).

So, I think we need to store the amount of tax for each row in uc_order_products. But then this is made more complicated by the existence of multiple tax rates (and compounding), which I haven't figure out how to solve yet.

And finally, there's the upgrade path to deal with - what to do with old orders that don't have this data available?

VAT invoice reference: http://customs.hmrc.gov.uk/channelsPortalWebApp/channelsPortalWebApp.por...

Note that the proposed uc_product_adjust() also opens us up to things like dynamic discounting directly in node displays (we could invoke Rules from the hook and let it modify the node price fields), and possibly Ajax price/node display updates when attributes are adjusted (effectively bringing uc_aac into core, which would be nice).

This would also help solve #613498: uc_product_add_to_cart_data does not respect non-shippable attributes if we could let modules manipulate $node->shippable from a single hook.

This sounds like a great, but extensive, addition. Can the patch from #17 be committed as an interim measure?

#941454: Store amount of tax charged per product is related to the point I made in #18 about storing more tax information.

OK, so solving this in the long run is hard, but we can still do a better job in the meantime while looking ahead to fix it in a better way later.

Attached patch is a variant of #17. A new $item->display_price concept is introduced for cart items, which means we don't have to clone the products; instead the calculation work is done once in hook_uc_cart_alter().

Not sure whether this approach is entirely better or worse than #17, but I think we do want to try and make a distinction between sell price and display price, and this lets us introduce the concept while (hopefully) being able to expand on it later.

If we're introducing this concept (which I think is a good idea), it makes sense to back it up to the product level (rather than the cart item level), eseniailly as you describe in #18. What do you think of this version, which tries to implement the first part of your idea from that post?

A couple of notes:
- The omission of the suffix in the cart block is deliberate, since that is often in a sidebar and there's not much room.
- hook_uc_product_alter() is not, strictly speaking, necessary - could just use the Drupal node api - cf this old thread.
- not sure how the suffix should be handled for product kits, so I left that alone.

If we go this way, maybe it makes sense to eliminate hook_uc_cart_item('view', ... ) entirely?

Note that this will require any existing module which implements hook_uc_cart_display() to be rewritten to set the '#total' field to $item->display_price. Probably would be helpful to add a rule to the coder-review plug-in.

I think I would prefer to use the node API to change the product data. Most of the arguments in that thread don't apply because uc_product adds data to the node before any hook_nodeapi() is called. I also think this makes it easier to develop, because most people would expect to be able to use the node API anyway.

Product kits are complicated, and the proper way to handle included taxes on them is very complicated. Each component of the kit should be taxed individually, but only for the amount they actually contribute to the price of the whole kit.

On the whole, though, I like the idea. Getting a consistent way to display the total price is what we're looking for here, and I think this is a good way to achieve that.

We do need to be careful that we don't screw up the display_price in $node->content. That's the larger price you see near the image, on default themes. It and $node->content['sell_price'] should both probably be set to $node->display_price instead. And that makes me think uc_product_view() needs to be changed a little bit.

I'm working up a patch to address these issues. Should have it shortly.

OK, have a look at this version. It uses Node API to alter the display price and sets $node->content['display_price'] and $node->content['sell_price'] to $node->display_price, and adds a $node->content['base_price'] field which contains $node->sell_price (based on longwave's note in #18 " It would also be useful to have both the untaxed and taxed prices available in node displays, Views, etc. for stores that sell B2B and B2C."). It also tries to deal with product kits correctly.

Some questions/issues:

-I don't like having to implement hook_uc_cart_item() in uc_taxes just to deal with product kits - I'd prefer to keep all the display_price altering in hook_node_load(). This would be possible if hook_uc_cart_display() were invoked for the cart pane on the checkout page. What is the rationale behind not doing this?

-Like longwave's patch from #22, this does away with hook_uc_cart_item('view', ...) entirely. So now there's no way for a module to alter the cart display in all contexts (block, cart, checkout). You have to use two separate form_alter invocations for the cart and checkout panes, and a theme override for the block.

-Items are still not listed with tax-inclusive prices on the checkout review page. Is that the desired behavior? I don't know what the expectation would be in a VAT country.

Some of the issues could be resolved by standardizing the way the cart contents are displayed in different contexts. I'd propose a single uc_cart_table() function, which takes a contextual argument to determine which columns to display (i.e. the "remove" button and "qty" input should only be present on the cart page). Then, modules could implement hook_tapir_table_alter() to modify display. But that's another issue.

hook_node_load() doesn't take a reference, and the $display_item['#suffix'] gets displayed after the cart table once it's put into the $form.

I'd like to get rc1 out soon, so I'm really tempted to just commit this patch as-is, despite the problems it brings up. There's nothing saying we can't fix it later, even if it means bending some APIs to do it.

Therefore, I can't think of a good reason that uc_checkout_pane_cart() can't use hook_cart_display() for the view or the review step. That shouldn't be hard to sort out.

Taking out hook_uc_cart_item('view') makes it inconvenient, but not impossible to work with cart items. I can live with that for a while, but we'll probably have to put something back to replace it eventually.

Why do we now need base price, sell price and display price? Can't we have display price as the tax inclusive one, and sell price as the original one?

OK. Have invoked hook_cart_display() for the cart contents checkout pane. Making this work on the review pane will be trickier, because uc_order doesn't use node_load() to retrieve product info. We'd either have to change this, or save the display_price into the order. Not sure which is better.

As to replacing cart_item('view', ...) functionality - I think consolidating all the cart-view code into a single, alterable tapir table would go a long way

Note: this patch should be applied ON TOP of #27.

Trying to change the way the order products are displayed is going to get us into the territory of storing the historical tax data pretty quick, I think. Once that's done, showing the price with that tax data included won't be a problem.

Yeah, I guess the sell price could be shown without the tax and the display price could be shown with it when necessary. This also helps keep symmetry between stuff at the $node level and the $node->content level, which is what I was worried about before.

$qty wasn't used in theme_uc_cart_review_table(), so I put it where I thought it was meant to go.

Applies to HEAD.

I agree about sell_price and display_price, and yep that's where $qty was meant to go.
This looks good to me...

OK, then. Committed.

question: where are things at right now, in terms of a fix that will actually store the amount of each tax on each individual line item? the above discussion seems to indicate that this is necessary, and is the next step to take. (see #30: "Trying to change the way the order products are displayed is going to get us into the territory of storing the historical tax data pretty quick, I think. Once that's done, showing the price with that tax data included won't be a problem.")

i URGENTLY need to be able to show both pre and post-tax amounts on each line item, for completion of a long-overdue project, so i am wondering what the possible timeframe for this fix might be?

thanks
Peter Fisera
Earth Angel Consulting

oops! i just realized this thread is about the 7.x version. has this same fix been considered for 6.x, even just as a patch?

@goatvirus: uc_vat goes some way to doing that...

Warning: upgrade to 6.x-2.6 seems to have broken uc_vat compatibility:
#1170992: double VAT
#1081130: VAT in invoice calculated on Total not subtotal (ubercart > 2.4)

@mandreato - thanks for heads-up on that! i'll keep that in mind. i still haven't figured out if the VAT module is total overkill for this situation, and may add more confusion (in which case i am gonna bite the bullet and write some more code, something like in this patch: http://drupal.org/node/941454 but store the actual amounts, not just flags as to which items were taxable)... or if VAT is the best way to go for this.

@goatvirus: well, I think that #941454: Store amount of tax charged per product would be a big improvement for 6.x version, but my hope is that uc_vat will return compatible with 6.x-2.6 because it adds many useful options.

This broke all the attribute price adjustments in 7.x-3.0-rc1! See #1301004: Price Options Not Working. I intend to revert this patch once I get confirmation in that other issue that the revert fixes things. We'll probably have to roll a 7.x-3.0-rc2 release immediately, since this did so much damage, so if you want this change in rc2 please fix up the patch as soon as possible.

#1301004: Price Options Not Working is fixed, although tax-inclusive attribute option prices won't be calculated correctly. I've split that out into #1301430: Inclusive tax implementation is not flexible (attribute options) as it's not really related to the original issue here.

Hi,

the checkout with tax show very good, but on the next page /cart/checkout/review the form show price without tax :-(.
The function theme_uc_checkout_pane_cart_review in uc_cart_checkout_pane.inc build the output, but the variable items has no price with tax infos. Where is the variable filled?

@christian death: This was noted in #26 through #29 in this issue, and is yet to be resolved. We should consistently display prices including tax for EU users on the review screen, in order history and in invoices, but currently there is no mechanism for this.

I think we will perhaps have to extend the uc_order_products schema to include a display price field, or some way of consistently calculating the display price given the sell price (as this has to also be able to handle orders created or edited directly in the admin pages)

Fixing #42/#43 would also solve #941454: Store amount of tax charged per product I guess.

Here is a patch that makes a display_price for order_products the way that we currently have for nodes. I feel like being consistent makes it easier to work with rather than having another column to keep updated as things change (such as whether the tax conditions are fulfilled). I had to tweak the createOrder() function in uc_cart.test since it wasn't getting the order object from uc_order_load(). Preliminary tests seem to indicate that this doesn't affect any existing tests.

Now, that won't fix #941454, but I think adding that bit of data to the line item when it's created isn't too hard.

How will uc_taxes_uc_order() calculate the same display price consistently even if tax rates or nodes have been deleted or changed?

Oh gosh. I just realized that the patch isn't respecting the tax conditions, either. Back to the drawing board.

New patch that waits until the line items have been calculated, then uses the stored rate to increase the products' display price. It stores which products get which taxes in the line item data, which is a port of the patch in #941454. Do we need another batch update to provide that for historical orders?

Several issues:
-Stored Line items for the existing cart order need to be cleared -- otherwise the order_product_id is not correctly updated. This is really a separate issue -- see #1351104: Need to clear line items as well as products for cart orders.
-Modifying the $order->products in hook_line_item_alter() doesn't seem right. For one thing - if uc_order_load_line_items_display() is called more than once, the adjustment will be applied more than once (try putting the cart contents pane below the payment method pane - the tax is doubled on the cart review page).
-Inclusive tax suffixes are not displayed properly.
-This patch is incompatible with the work pending at #1301430: Inclusive tax implementation is not flexible (attribute options). The main difference is that the display price is there stored as an adjustment, which makes it easier for several modules to modify the display price simultaneously. Also, suffixes are stored as an array, which prevents the same suffix from appearing more than once. But in broader terms, it seems to me that the uc_product_alter() hook could be extended for order_products - either by creating a uc_order_product_alter() hook, or by invoking uc_product_alter with enough info in $product->data to allow modules to do the right thing - e.g. in the case of uc_taxes, use stored tax rates or live tax rates depending on whether it's a saved order or the cart contents.

Hm. After looking at this a little bit more I can see why you put it into line_item_alter - we need the line items loaded before we can adjust the product prices, but for various reasons we also need all the products loaded before we can load the line items - sort of a catch 22. And it does make sense that the display price alteration should be a result of loading the line items. But it still seems too magical to me to silently alter the products when the line items are loaded - and I don't see any easy way to prevent duplication of the adjustments since uc_order_load_line_items() can be called many times in a single request.

Do we might need a new hook to do this cleanly? eg. hook_uc_order('display', ...) or hook_uc_order_product_display_alter(&$order_product, $order)?

I know in #45 you said you didn't want another column (presumably for display_price?), but that seems the easiest way to solve this as there would be no need to do any calculations on load, and will make it much easier to allow Views to display this data, for example.

Or we could let uc_taxes manage an extra "tax" column for each order product, then display either sell_price or sell_price+tax depending on configuration? I am not sure there is any other use for the "display price" mechanism at present, so perhaps this will work at least until we come up with another use case for it?

Tax seems like such a critical thing to get right that we don't want to be recalculating all the time, and having a separate column would make the tax reporting much easier as well.

Well - another use case might be discounts applied directly to product prices, where you still might want the discount listed as a line item (e.g. "You saved x"). But I suppose there could be other ways to accomplish this.

One issue with saving as a column is that you'd have to be sure to update that column if an order was edited ex post - e.g. to correct an erroneously applied tax or tariff. That's what's nice about saving the data in the line-item and then calculating at load - if the line item is deleted, the display price adjustment goes away.

But if we calculate at load, we have to be sure we will always have all the data available to do that calculation - we can't rely on the existence of product nodes or tax rates, as they may have been deleted.

Good point - so that means we have to store not only the taxed product/line-item id, but the actual amount of tax (or the rate) charged on each.

There are further problems here - the actual tax rate charged *is* saved (and used in the patch at #49), but not whether or not the tax should be displayed inclusively. In fact, the *only* feature of a tax which is preserved in completed orders is the rate. If you edit anything else (e.g. the products to which it applies) and then save a completed order, the new tax settings are applied, so the original tax data are lost. This is

It seems to me there are some things we want updated if the tax is edited (perhaps tax-inclusiveness, probably the suffixes), and others (the rate, whether the tax applies at all) which should be fixed once an order moves past a certain stage. I think we should also provide a mechanism to alter the tax line item in detail - e.g. an 'edit' $op for the line item callback which would return a form element.

Then there's the question of what stage in the order process should freeze the line-items - seems to me it should be anything 'post-checkout' (rather than just 'payment received' or 'processing' as is used now to freeze the rate) - once an order has been submitted to a payment processor, for example, the tax info shouldn't change automatically if a tax rate is edited.

This is really a separate issue, I guess - more an extension of #1300608: Tax line items are no longer stored - but related, since the proper handling of tax line items will affect how/where we use them to alter the display price of associated products.

Posted a patch for the issues raised in #57 at #1300608: Tax line items are no longer stored.

The more I think abou this, the more I think we need to move the display_price altering code to a new hook - one that is invoked once the whole order has been populated (probably just before the product is to be displayed). There's just not enough context available earlier. This will help not only here, but with another issue I've just discovered:
1. Create a tax that applies to a product and displays price inclusively, but has a condition set so it will only apply if, e.g., order is shippable.
2. Add non-shippable product to cart and proceed to checkout
3. The cart-contents block shows the tax-inclusive price, but the payment preview shows no tax.

This indicates that we need the order context (if such exists) to determine what inclusive tax to display.

Thinking of this in terms of the uc_product_load_variant thread, the fact is there are some alterations that have to occur before the line items are calculated, and some that have to occur afterwards, so it makes sense to split this into two hook - maybe 'uc_product_variant_alter' and 'uc_product_view' ?

This will be fixed in #1301430: Inclusive tax implementation is not flexible (attribute options)