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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RavenHursT’s picture

Looks like the issue is happening somewhere around line 206 in uc_taxes.module... Thanks to SKH

skh’s picture

subscribe

longwave’s picture

Priority: Critical » Normal

Tax 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?

TR’s picture

@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.

longwave’s picture

I 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!

RavenHursT’s picture

Really? 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?

RavenHursT’s picture

FileSize
5.89 KB

Alright... 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..

longwave’s picture

Status: Active » Needs work

The 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.

RavenHursT’s picture

Ahhh.. 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..

RavenHursT’s picture

So I'm trying to do the following around line 495 in uc_order.order_pane.inc:

      if (intval($arg1['li_delete_id']) > 0) { 
        $line_item = uc_order_get_line_item($arg1['li_delete_id']);
        uc_order_delete_line_item($arg1['li_delete_id']);
        if($line_item->type == 'tax'){
          $order = uc_order_load($arg1['order_id']);
          $order->data['taxes_disabled'] = TRUE;
          dpm('output: '.uc_order_save($order));
        }
        drupal_set_message(t('Line item removed.'));
      }

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:

  db_query("UPDATE {uc_orders} SET data = '%s' WHERE order_id = %d", serialize($order->data), $order->order_id);

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?

RavenHursT’s picture

Ok.. so I've written the following into uc_order.order_pane.inc:

    case 'edit-process':
      if (is_array($arg1['line_items'])) {
        foreach ($arg1['line_items'] as $line) {
          if (is_numeric($line['li_id']) && intval($line['li_id']) > 0) {
            uc_order_update_line_item($line['li_id'], $line['title'], $line['amount']);
          }
        }
      }
      if (intval($arg1['li_delete_id']) > 0) { 
        $line_item = uc_order_get_line_item($arg1['li_delete_id']);
        uc_order_delete_line_item($arg1['li_delete_id']);
        if($line_item->type == 'tax'){
          $order = uc_order_load($arg1['order_id']);
          $order->data['taxes_disabled'] = TRUE;
          db_query("UPDATE {uc_orders} SET data = '%s' WHERE order_id = %d", serialize($order->data), $order->order_id);
          $order = uc_order_load($arg1['order_id']);
          dpm(db_result(db_query("SELECT data FROM {uc_orders} WHERE order_id = %d", $order->order_id)));
        }
        drupal_set_message(t('Line item removed.'));
      }
      return;

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? :-(

RavenHursT’s picture

FileSize
8.34 KB

Alright.. 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.. :-)

RavenHursT’s picture

Bump...

RavenHursT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, ubercart_disable_tax.diff, failed testing.

TR’s picture

Version: 6.x-2.6 » 6.x-2.x-dev

@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.

daroz’s picture

Also, 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.

longwave’s picture

This 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?

daroz’s picture

I 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.

longwave’s picture

@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.

longwave’s picture

So, 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?

daroz’s picture

@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....

RavenHursT’s picture

True, *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"?

daroz’s picture

But 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.

RavenHursT’s picture

@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.

fp’s picture

Another 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?

DamienMcKenna’s picture

Title: Sales Tax Line Item will not delete from manual order through admin » Line items can not be removed from manual order through admin

This 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).

DamienMcKenna’s picture

FYI here's a workaround:

  • Add an "Empty line" item.
  • Set the Line Item Title to e.g. "Credit for fees".
  • Set the amount to the total of the fees that are to be canceled out with a minus sign in front, e.g. if there are $5 worth of fees to remove set the value to -5.
Morbus Iff’s picture

@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.

TR’s picture

Version: 6.x-2.x-dev » 8.x-4.x-dev
Issue summary: View changes

This 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.