Inclusive tax implementation is not flexible (attribute options)

OK. This fixes it in a way that's more in line with the general philosophy, modifying the price of attribute options at node_load. I still don't think it's ideal, since it is specific to uc_attribute.

The real difficulty is that uc_taxes can't know beforehand about all modules which might want to display tax inclusive prices, either on the node form or in the cart. There needs to be a way for a module to tell uc_taxes - this price should have taxes included when associated with this product.

Maybe we need to go back to a simpler implementation of price alterers. For example, a module could call uc_display_price($price, $node) in a hook_node_load() implementation, and uc_taxes could implement a hook_uc_display_price($price, $node) to add the inclusive prices.

What do you think of this approach?

Ha. I almost posted up the same patch before I found this issue.

All things considered, I think I can live with that minor issue. Maybe we can fix it later.

Tweaked the patch so that multiple taxes aren't compounded together, and reduced duplication a bit, too.

Sorry - I've been traveling. Maybe it's too late, but I wanted to post the solution I came up with on the plane, which introduces a hook_uc_product_display_price as outlined in #2. It has several advantages:
-it removes all module specific code -even product kits - from uc_taxes; neither do other modules need to check for uc_taxes.
-it displays adjusted prices for attribute options on the add-to-cart form.
-it provides a mechanism for other modules (not just uc_taxes) to have a say regarding display prices of products (which could be useful for discounts)
-it addresses the rounding issue raised by longwave in #3
I reworked this very quickly to apply to the new HEAD after #10, so it may have some rough edges, but it seems to work ok. If you like the approach I can test it more rigorously when I am back, in a week or so.

re: #15 - I see - i'd forgotten to consider the theoretical rounding error in the add-to-cart form, though it was fixed in the cart display. Attached patch fixes that.

I'm looking at #16 now and will post back.

I like this approach. Attached patch fixes one small bug in uc_attribute_uc_product_alter(). it also tries to address the issue in #18 by storing the suffixes as an array, so that the kit price lists the suffixes for all products with included taxes, but each only appears once. I guess the alternative would be to append something like "including applicable taxes" and then perhaps list the actual suffix next to each component product.

The only thing about this that feels clumsy to me is running the whole alter sequence both when the original node is loaded and again when the variant is loaded. Is this necessary for any other reason than to display the altered prices on the add-to-cart form? If so What if we moved the alter sequence from uc_product_load() to uc_product_view()?

Note: this patch should be applied over the one at #16 (so you can more easily see what I changed)

#22: 1301430-uc_product_load_variant.patch queued for re-testing.

Thanks for the git tip - didn't realize you could so easily concatenate all the diffs into a single file.

Attached patch
1. fixes that little bug in uc_product_kit_uc_product_alter
2. turns display price suffixes into an array
3. gets the rules-based discounting working with product kits.
4. moves the product alteration sequence out of the node_load - now only happens when uc_product_load_variant() is invoked explicitly.

The problem with the rule and product kits had to do with the order in which discounts were applied. We need to make sure that uc_product_kit applies its discounts before the rule, but also that uc_taxes gets the fully altered node. So i moved the rules_invoke_event to an implementation of hook_uc_product_alter(), and used module_implements_alter() to guarantee the correct execution order. I also changed "$product->display_price" field to "$product->display_price_adjustment" field, so modules wouldn't have to duplicate their basic price adjustments for both price and display_price.

Updated above patch to apply to current HEAD.

#24: 1301430-uc_product_load_variant-24.patch queued for re-testing.

That sounds like a good idea, but it seems to me it could be a separate issue. Would we not still use the uc_product_load_variant() mechanism to allow modules to make context-specific modifications to product data?

Regarding the proposed change - the big difference between cart items (the proposed cart products) and order products is that cart items are extensions of the basic product node, while order products contain a reference by nid to the product node, but not all its fields directly. Would you try to reproduce this structure for cart products? That seems like it could be a refactoring nightmare. Or would you simply add entity metadata for the extra fields, and rewrite uc_cart_get_item as an attachLoad function?

I suppose an alternative would be to make order products match cart products rather than the other way around (i.e. have an order-product contain all the fields of the underlying product node) - this would have less impact, as existing modules could still use the nid, and could gradually be refactored to access those fields directly. Also, this could be combined with #944238: Order Products: node revision ID from time of sale required so that the time-of-sale revision of the underlying node is loaded automatically.

Also - once this uc_product_load_variant() functionality is committed, I think it will provide a clean way to implement tax-inclusive prices on invoices, order review, etc.

For records... #1354460: Missing hook_uc_price_handler() needs a hook where the price can be altered based on currency like in comment # 13 or # 16.

Have been thinking a bit more about this - I think you're right, creating the "cart product" entity will streamline things a bit. Then uc_product_load_variant() can use entity load. Then I think a "product variant" is a meta-entity which can be backed by any of uc_product, uc_cart_product or uc_order_product. The common fields are:
'nid', 'title', 'model', 'qty', 'cost', 'price', 'weight', 'weight_units', 'data' -- ideally we'd enforce this in an OOP way by defining a UcProductVariant interface.

Since display_price only has to do with display, and in some cases requires more context (e.g. the complete order with line items, in order to display tax-incluve prices for past orders), I propose that we create a uc_product_variant_view($entity_type, $entity, $order) mechanism on the model of entity_view() - and modules which want to alter the display price (or any other aspect of display) can act on the renderable array directly by implementing hook_uc_product_view(). The resulting renderable array would be passed to, e.g. hook_uc_cart_display() - though ultimately this could simply become a custom view mode..

In the case of currency conversion, we need the product price to be changed such that the order value and the final billing is done in the converted currency. also this value must be stored with the order line item so that the actual billed price can be retrieved anytime. I'm not sure if just a display hook would be sufficient for this. kindly keep this in mind when finalizing the hook/api.

I think you would be able to do your alterations in the hook_uc_product_alter() - which would be invoked by uc_product_load_variant() on all products. But maybe you need the order object as context also, so you can check the user to decide which currency to use? How is that determined?

This is *not* a patch that's ready for testing - just wanted to put it here to show the direction i'm going.

@wodenx: if the changes to the product price made in the hook_uc_product_alter() be used for calcuating total order and taxes, then it's sufficient. I presume that the currency symbol can also be altered with the above hook, so that the right symbol can be shown with the price.
also will this be called for attribute based price changes?

@sukr_s - do you need the $uid for the order in order to determine which currency to use? or do you just use the global $user object?

@wodenx: global $user is used. No feature at the moment that allows a user to place an order on behalf of another user.

Finally had a little time to work on this.

Attached patch is pretty large - it also addresses #1153086: Inclusive tax not visible in cart block or checkout pane, #1300608: Tax line items are no longer stored. This is basically an elaboration of the product-variant idea proposed by longwave.

Highlights:
-creates a 'uc_cart_item' entity, and fleshes out the entity api integration of 'uc_order_product'
-does away with the display_price and display_price_suffix fields in the $node object. Instead, these become fields in the render arrays build by entity_view and node_view - and can be altered there without having to reload the entire order when, e.g., the line-items change.
-now hook_uc_product_alter() is for making substantive alterations (e.g. discounts) to a product when a variant is loaded. hook_entity_view() and hook_entity_view_alter() can now be used to make display oriented alterations.
-complete tax information is saved with every order-product, which enables us to recalculate the taxes based on the rates and products as they were at the time of purchase.
-tax-inclusive prices are now displayed properly for all products (including product kits and attributes) on the node_view screen, all cart tables, order view and invoice.

Still to-do:
-Enter prices tax-inclusively.
-Clean up the various uc_cart tables to use a common display mechanism. Maybe get rid of hook_uc_cart_display() in favor of hook_entity_view().

But these are separate issues, I think.

Because this is so big, I've attached a squashed version as well - maybe easier to see what's goin on.

Patches above probably collide with commit of #1300608: Tax line items are no longer stored. Rebased here.

Hm - well - what about doing away with the uc_cart_products table entirely, and storing cart items directly in the uc_order_products table, with an added column for cart_id? This would have the added advantage that when an order product was added, removed or updated by a checkout pane, the change would automatically propagate to the cart, and vice versa - thus fixing #1351104: Need to clear line items as well as products for cart orders in a much cleaner way.

This actually wouldn't be that hard to implement, I think. We could modify the uc_order load callback to merge in fields from the actual node for in_checkout orders, in order to retain compatibility with modules which might expect those fields to be there for cart items. And we'd have to write an update hook to copy the the cart_products over.

Just to play devil's advocate - the situation you describe could be dealt with by simply setting a flag which prevents completing checkout if the cart contents are altered - maybe just unset $_SESSION['do_review'] and $_SESSION['do_complete'].

And while I feel you about existing sites & contrib (a) I think this could be made nearly transparent, as long as people use the API (uc_cart_add_item(), etc.) and don't muck about with the database directly, and (b) if we're contemplating API changes, it seems like they should be made earlier rather than later - how many contrib modules have been ported so far? And if there is a 3.0 release, won't we then have to wait til the next MAJOR release (4.x) - and who knows when (if ever) that will happen?

Firstly, this stores tax rates against the order, but what if you delete tax rates or change the rules? (or the rules have conditions that change depending on the current user) - note I haven't had chance to test this.

I *think* that with the commit of #1300608: Tax line items are no longer stored, complete tax information is saved with the tax line-item. The product type is also saved. For orders past the in_checkout stage, this historical information is used, and the rules are ignored (since they were already applied when the order was originally created). That's all taken care of in uc_taxes_filter_rates(). The upshot is that tax on old orders should be calculated the same even if products, tax rates or rules change. A new test to be sure this is the case would definitely be a welcome addition.

Why change theme_uc_product_add_to_cart()? This seems to be reverting the fix for #1104640: Error messages are displayed one page too late when submitting the 'Add to cart' form

Yeah - this is an issue. The problem is that the uc_attribute form_alter implementation needs to call node_view() to calculate the display price for different attributes (to avoid the theoretical rounding issue you describe in #15). This means the form needs to be built outside of the view hook. I borrowed this solution from theme_uc_product_kit_add_to_cart() (apparently the fix for the referenced issue didn't make it there) - but it's not really the right way to do it. I'll think more about it.

I also think we should keep the _uc_attribute_alter_form() code flow rework from my original patch and the new theme_uc_qty() out of this patchset if possible, as they aren't really related here.

Agree about the theme_uc_qty(). The _uc_attribute_alter_form() rework exists to display attribute price adjustments tax inclusively, so it is relevant - however, it probably needs to be rethought in light of the above.

Regarding rebasing - I took a stab at it (thanks for the "rebase -i" tip) and was able to squash some of the bad commits - but things are pretty interrelated.

I absolutely agree about adding more tests - just haven't had time to do it yet. And separating out the classes should be easy enough.

This version fixes the issue with invoking the form builder from a theme function, pulls the classes into separate files and adds quite a few tests. Didn't revert the theme_uc_qty() because it provides a way to make the quantity available to entity_view_alter() handlers as a number (rather than, e.g. "2 x").

It looks like #1376724: Refactor _uc_attribute_alter_form() conflicts with the first change in the latest patch. I believe I have rebased this patch onto 7.x-3.x correctly.

Well, I don't see the attribute elements on the add to cart form any more, so that looks like a fail.

Thanks for looking at this - I was waiting for review of my new patch at #1376724: Refactor _uc_attribute_alter_form() before undertaking this...

OK - I think this is now rebased to HEAD.

@IslandUsurper - I think I know why you lost the elements on the add-to-cart form - it's probably bc of (what I consider to be) a bug in Drupal core (see #1364382: User data array should be unserialized before attaching to node.) - I had it patched on my box so I didn't notice it. Very last patch in this sequence fixes it.

What's with the testbot anyway?

We need more tests. Though the tests pass, adding something to the cart doesn't put the selected options on the cart item any more.

I'll get a test written up, if wodenx or somebody can figure out what's wrong with the patch.

OK, I see the problem - now just have to figger out how to fix it.

Not completely happy with this solution - but alternative approaches also have drawbacks. The problem is that _uc_attribute_alter_form() needs to call node_view() to get the display-altered price of each option, so there has to be some way to prevent infinite recursion. Other ideas
- Get the display price of each option when the attribute is loaded - but this requires refactoring uc_attribute_load() so it doesn't call node_load, or we'd have the same problem.
- Add a new view mode for products that doesn't show the add to cart form?
- ??

hang on - found another problem.

OK - some things seem to have got lost in a rebase - I think I've corrected them, and rebased this fix again into the main patch that splits out the hooks.

#61: 1301430-product-variant-61.patch queued for re-testing.

#65: 1301430-product-variant-65.patch queued for re-testing.

68 patch works fine. Running on my website without errors. Patched the latest dev version. Finally after hundreds of times trying to patch it I made it work with the custom php execute block for entity_info_cache_clear();

UPDATE: after patching and enabling some extra ubercart modules i get this error when I click Checkout link:

Notice: Undefined offset: 0 in uc_checkout_pane_quotes() (line 691 of sites/all/modules/ubercart/shipping/uc_quote/uc_quote.module).
    Notice: Undefined offset: 1 in uc_checkout_pane_quotes() (line 695 of sites/all/modules/ubercart/shipping/uc_quote/uc_quote.module).
    Notice: Undefined index: in uc_checkout_pane_quotes() (line 697 of sites/all/modules/ubercart/shipping/uc_quote/uc_quote.module).
    Notice: Undefined offset: 1 in uc_checkout_pane_quotes() (line 699 of sites/all/modules/ubercart/shipping/uc_quote/uc_quote.module).
    PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect decimal value: '' for column 'amount' at row 1: INSERT INTO {uc_order_line_items} (order_id, type, title, amount, weight, data) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 12 [:db_insert_placeholder_1] => shipping [:db_insert_placeholder_2] => [:db_insert_placeholder_3] => [:db_insert_placeholder_4] => 1 [:db_insert_placeholder_5] => N; ) in drupal_write_record() (line 6888 of includes/common.inc).

Also after applying the 68 patch I applied the patch from http://drupal.org/node/1388984

Update: problems gets fixed when Shipping quotes module is disabled.

Regarding #69, cf #996236: drupal_flush_all_caches() does not clear entity info cache. Until the fix there lands, do we need to put a call to "entity_info_cache_clear();" in, say, hook_init() to guarantee this patch applies smoothly?

One problem with 68: uc_product.api.php isn't updated to include documentation on the new hook.

Applying the last patch on the 31 december dev version leads to "PHP Fatal error: Class 'UcCartItemController' not found in includes/common.inc on line 7522"

Any idea?

tahiticlic, make sure your cache gets cleared after you apply the patch. The registry needs to be updated so that the right file gets loaded.

I don't have the same problems as #69, but since I had to grab an old test site since my laptop died, I can't be sure how standard the site is.

If just clearing the caches doesn't work, try using drush or the devel module "execute php code" block to execute "entity_info_cache_clear()" - see #996236: drupal_flush_all_caches() does not clear entity info cache.

Re #69 - I'm fairly certain this is not an issue with this patch, as I get the same set of messages with the latest DEV if I enable uc_quote but have no shipping methods defined. I'll open a new issue.

Allright, clearing cache with drush solves the problem. Thanks!

Has anyone attempted and/or is there a way to compile a list of contrib modules that have been ported to D7 - at least at drupal.org? If that were possible, we could in theory alert the maintainers to any API changes that come it at this late stage. Of course, that doesn't solve the problem of custom modules that might be broken.

#78: 1301430-product-variant-78.patch queued for re-testing.

@longwave: I've been leaving this issue up to you because you use inclusive taxes and I don't, but one of the contribs that I maintain is uc_custom_price, which will be affected by this I think. What I can do is apply the patch and see if I can re-write uc_custom_price to work properly with the new hook. I won't be able to get to that until the weekend.

An RC4 is fine with me as long as it is followed very shortly by 3.0 - this issue seems to be the only major thing that the release is waiting for.

Marking "needs work" because #71 still needs to be addressed. Likewise, this is definitely something that should be documented in the release notes.

@wodenx: I have a local copy of all the D7 contributions - what would you like me to search for in there?

I thought I'd posted this already, but apparently not:
@TR - I've been holding off on updating the documentation until this settles, assuming the API might change again. Regarding the list of contrib, I was just suggesting that we could open issues in their queues informing them of the API change - but a search for $display_price would probably help, as would one for implementations of hook_uc_cart_item, as that is more or less obsolete (with the possible exception of 'can_ship'). I can also try to write some coder_review tests to cover these changes.

Life's been getting in my way lately. My work laptop crashed a couple of weeks ago, and I've been trying to salvage things while rebuilding my usual workflow on my spare computer.

Here's documentation for the hook. As far as I can tell, everything in Ubercart itself works as it should (within this issue, anyhow). Unless someone comes back with a problem from another module, I think this is ready to commit.

Now, I think I need to go completely rebuild my test sites.

Hang on - I think there are a few loose ends to tie up. Primarily - I think we need to get rid of the last vestiges of hook_uc_cart_item()... it's not even invoked anymore for load and view - and needn't be for remove (if we use entity_delete) - which leave's can_ship - which should really be moved into uc_order anyway, since the same logic is used in both places.

OK - this gets rid of hook_uc_cart_item(). There is one small loss of functionality - the 'checkout' op is no longer invoked - but this was unused in core, and modules can accomplish the same thing by responding to hook_uc_checkout_complete(). But the changes here are in 3 additional patches -
1) remove old 'load' op
2) remove 'remove' op
3) remove 'can_ship' (replaced with new hook_uc_order_product_can_ship()

Not really - the best we could do is set a flag in each cart item just before invoking entity_delete - since they're cached, the flag should persist to the 'delete' hooks. But why can't modules just implement hook_uc_checkout_complete() if they want to do special processing when an order is checked out?

Something like this.

Latest dev tested, taxes not included in option prices when using tax inclusive prices. New issue or?

Re: #88

These are the contributed modules that use display_price:
http://drupal.org/project/course
http://drupal.org/project/magazeen
http://drupal.org/project/uc_auction
http://drupal.org/project/uc_custom_price
http://drupal.org/project/uc_price_per_role
http://drupal.org/project/uc_price_visibility
http://drupal.org/project/uc_varprice

These are the contributed modules that implement hook_uc_cart_item():
http://drupal.org/project/uc_auction
http://drupal.org/project/uc_custom_price
http://drupal.org/project/uc_favorder
http://drupal.org/project/uc_hotel
http://drupal.org/project/uc_price_per_role
http://drupal.org/project/uc_varprice