Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Steps to reproduce:
Create a new order through the store admin
Add a product to the order
Add some shipping
Save out the order
Tax will be calculated
click the "x" next to the sales tax line item
JS alert will ask for confirmation, click ok
Page reloads w/ sales tax line item still on order but with an increments line-item ID
Very frustrating bug that is hindering me from running wholesale orders on my site: http://www.fulltuckracing.com/ftr-store
Comment | File | Size | Author |
---|---|---|---|
#12 | ubercart_disable_tax.diff | 8.34 KB | RavenHursT |
#7 | ubercart_disable_tax.diff | 5.89 KB | RavenHursT |
Comments
Comment #1
RavenHursT CreditAttribution: RavenHursT commentedLooks like the issue is happening somewhere around line 206 in uc_taxes.module... Thanks to SKH
Comment #2
skh CreditAttribution: skh commentedsubscribe
Comment #3
longwaveTax line items are automatically recalculated when you save the order, if the tax conditions match then tax will be applied.
What's your use case for removing sales tax from an order, contrary to whatever conditions you have provided?
Comment #4
TR CreditAttribution: TR commented@longwave: I haven't confirmed it, but I'll assume the OP's description is correct. One use case for removing sales tax is that the customer is a tax-exempt organization. This can happen for business-to-business sales, or for sales to government or non-profit organizations.
Comment #5
longwaveI would implement that by adding a "wholesale" role and assigning it to those customers, then excluding sales tax for orders with the wholesale role; tax would then never be applied in the first place. Not sure if that is doable without writing custom CA conditions though!
Comment #6
RavenHursT CreditAttribution: RavenHursT commentedReally? A workaround? I mean.. the "x" is there.. why not just find out how to make it work? As it stands, if the "x" is on the form.. then it should work.. you shouldn't have to go through some convoluted workaround just to make an existing button do what it's supposed to do...
I mean.. regardless of the "reasons" for wanting to exclude tax from an order, when I'm an admin, and I'm editing the line items for an order, I should be able to do just that, edit/manage/delete the line items for that order. If not, then why have the "x" button and the editable field there in the first place?
Comment #7
RavenHursT CreditAttribution: RavenHursT commentedAlright... the best I could do.. I'm a fairly newB drupal dev.. I've written a module here and there.. but nothing nearly as complicated as Ubercart... This patch (sorry about the SVN patch.. I still don't have git working on my server) will set a persistent var that will forever disable taxes on a given order if someone deletes the sales tax line item from an order. I'm sure there's a MUCH better way of doing this and I'd LOVE to see it done, but I have no idea how else to go about it..
Comment #8
longwaveThe workaround was only a suggestion, until this bug is fixed.
If possible you should set something maybe in $order->data rather than abusing the variable table like that; the patch won't be committed as-is.
Comment #9
RavenHursT CreditAttribution: RavenHursT commentedAhhh.. was looking for something like that but wasn't aware that I could just add arbitrary data onto the order like that.. perfect.. I'll make the change..
Comment #10
RavenHursT CreditAttribution: RavenHursT commentedSo I'm trying to do the following around line 495 in uc_order.order_pane.inc:
Only to find that uc_order_save() is not updating the data values on the order in the DB. I did a quick grep for 'data->' and could that in uc_order.install is saving data values directly through a db_query() call:
Should I do it the same way here? Or should I change uc_order_save() to update/save the data values each time it's called?
Comment #11
RavenHursT CreditAttribution: RavenHursT commentedOk.. so I've written the following into uc_order.order_pane.inc:
The dpm() in there verifies that the new data item has been added to the order in the database.. but what's really odd is that, by the time the edit page loads, the data collumn has been reset w/o the new item.
I've grepped through the entire module and I can't see anywhere that this is happening... Can anyone give me a hand? :-(
Comment #12
RavenHursT CreditAttribution: RavenHursT commentedAlright.. figured it out.. the admin include was overwritting the changes on the order.
I also added some order logging on line items... Let me know if this can be committed.. :-)
Comment #13
RavenHursT CreditAttribution: RavenHursT commentedBump...
Comment #14
RavenHursT CreditAttribution: RavenHursT commentedComment #16
TR CreditAttribution: TR commented@RavenHursT: Your patch is failing because it's in the wrong format. Patches need to be in -p1 format, not -p0 format like yours. See http://drupal.org/patch for more info.
If you can upload a corrected version of the patch, we can review it.
Comment #17
daroz CreditAttribution: daroz commentedAlso, for at least the 7.x brach, #1300608: Tax line items are no longer stored has some bearing on this as well. If Sales tax is to be able to be deleted the proposed patch there would not solve this problem.
Comment #18
longwaveThis patch needs work anyway. Say you mistakenly delete the tax line item, then want to re-add it without manually recalculating. How would you do that?
Comment #19
daroz CreditAttribution: daroz commentedI think this is all dancing around the core issue here.... Tax is not some generic line item, it's a legal/statutory requirement to be collected, in certain amounts, on certain products, in certain jurisdictions, and collected from non-exempt individuals/companies. I could argue that if your sales tax calculation determined (correctly) that tax should be charged, then you should not be able to delete it.
At the same time, you should be able to "recalculate" the tax, and if no longer appropriate, have it deleted accordingly. This could be in response to "fixing" a product to be exempt or taxed, or editing the shipping address, or perhaps, marking the customer tax-exempt (typically in the US reserved for non-profits and/or resellers). Only some of these actions would result in an "edit" to the order, and would require some way to recalculate the tax.
In any case, I see manually adding/editing/deleting a tax item only masks the underlying problem -- your tax calculation rule(s) are broken.
Comment #20
longwave@daroz: That was what I tried to imply in #5, but then in #6 it seems people don't expect the system to work this way.
Comment #21
longwaveSo, perhaps the real fix for this is to hide/disable the remove line item buttons and editable price fields on autocalculated line items, such as taxes?
Comment #22
daroz CreditAttribution: daroz commented@longwave re #21:
I agree, Taxes are unlike every other line item in the system, in that it's not ok to edit/add/delete them at will. And having tax line items behave the same way runs counter to what it is. I tried to cover that a bit in #1300608: Tax line items are no longer stored #34/35. But in the discussion here what I didn't catch in the other issue: that in order for it all to work, there needs to be a way to force a recalculation that's somewhat intuitive. (Edit then Save would work, but isn't intuitive.) It's to catch the outlying case of changing a product from taxable to exempt (or vice versa), or a change to the tax rules.... e.g. Admin sees that tax is "wrong" and fixes the underlying problem....
Now, to give a counter point, take shipping. It's auto calculated (in a way), but it should still be editable and removable by the admin. If that is changed/deleted the store is, perhaps, out some money. Screw up a tax calculation and it's a whole other story....
Comment #23
RavenHursT CreditAttribution: RavenHursT commentedTrue, *most* retail e-commerse shops will just handle transactions for all retail purchases and include tax. But that doesn't mean that they shouldn't be treated as what they are, a line item. Like I said in the post, I have a shop that handles wholesale orders as well. Wholesale orders do not require sales tax and therefore, tax line items should be allowed to be deleted.
As for the diff. Sorry.. I didn't realize that... if I find the time, I'll create a new diff. Is there a way to convert the diff I posted to a format more "correct"?
Comment #24
daroz CreditAttribution: daroz commentedBut your solution will require you to, on every order that shouldn't be taxed, manually intervene and edit the order to remove the Tax line item.
If a given customer is a wholesale customer, or is otherwise tax exempt, why not edit the customer, add them to a tax-exempt role (that has no extra permissions), and use your tax rules to not charge tax in the first place? This way as they check out (after their account is tagged) the order total should be correct, and you won't have them potentially overpaying every time.
If you do it this way, instead of deleting the line item all you need to do is add the role and re-save the order and it will be fixed for every new order going forward.
Comment #25
RavenHursT CreditAttribution: RavenHursT commented@daroz: That's a good solution too, for customers placing wholesale orders. But my use-case is when I'm creating an ad-hoc order through the admin interface. It still be a good idea to allow for the deletion of tax line-items through this form. In the world of accounting, a line item is a line item. I'm still not seeing why this is a debatable issue. Just because it's "too hard" to implement? I've already done so (rather messily) on my site, just so that I could handle the single use case above. My point is this: There's an "X" next to the tax line item currently. That's "X" doesn't work. It should either work as the user expects it to work, or else it should be taken out. Either way, until it's fixed, it's still a bug.
Comment #26
fp CreditAttribution: fp commentedAnother use case is when one needs to send a replacement for a defect item. This has nothing to to with a particular role.
An alternate to this would be to add a discount of the value of the tax amount but that's crufty and will mess up the tax reports.
How about 'reading' the tax value when saving an order and if the submitted amount is different than the one automatically generates, the submitted amount takes precedence? This way one could set taxes at $0 and thus resoive this situation on a per-order basis (if role doesn't apply) without loosing the tax calculation on order save.
I haven't looked at the code yet and I don't know how feasible this is but would an approach like this solve this issue?
Comment #27
DamienMcKennaThis isn't limited to just sales tax items, it's a general problem that stops you from removing any line items that are automatically added to the cart. Please note that this does not affect the "empty line" items that are added (which show between the Subtotal and Subtotal-excluding-taxes lines), only the items that the rest of the Ubercart configuration automatically adds (between the Subtotal-excluding-taxes and Total lines).
Comment #28
DamienMcKennaFYI here's a workaround:
Comment #29
Morbus Iff@28: Unfortunately, not that simple, since adding that line item will change the total order price which, in turn, changes the taxes applied. An example:
* Order total: $39.99 +Ware Sales Tax (+$1.20), +Georgia Sales Tax (+$1.60) = $42.79.
Our ultimate goal here is to make the order total $39.99.
We dutifully apply $2.80 in tax credit, and the result is:
* Order total: $37.19 +Ware Sales Tax (+$1.12), +Georgia Sales Tax (+$1.49) = $39.80.
Herein, we're 19 cents short. Well, fine, let's make the discount $2.61 then! ($2.80 - 19 cents).
But then the taxes equate to:
* Order total: $37.38 +Ware Sales Tax (+1.12), +Georgia Sales Tax (+$1.50) = $40.00.
Argh, insert more tweaking here, etc. It's certainly a workable workaround but, as the other commenters suggest, there's a conceptual problem here with having these particular line-items have the deletable-X next to them, or an editable value field, as both are ignored during order save and recalculation.
Comment #30
TR CreditAttribution: TR commentedThis needs to be addressed in D8 first then backported to D7 if there is community interest. D6 is now obsolete and we will not be fixing this issue in D6.