| Project: | Ubercart |
| Version: | 5.x-1.7 |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Island Usurper |
| Status: | closed (fixed) |
| Issue tags: | VAT |
Issue Summary
In order to report sales tax to your state taxing authority (at least in Washington state, not sure about in other states/countries, but probably something similar applies), you need to know the following, for each order you ship:
- Taxable amount
- Tax rate
- Amount of tax charged
In Washington state, you also need to know
- Tax location code (a 4-digit code recording city/county, as you have to charge tax based on delivery location)
The Ubercart tax module is only storing the amount of tax charged (i.e. the line item) in the order. The other information, although part of the calculation, is not stored. It also cannot necessarily be easily inferred from information in the order. So it would be good if the tax module would store all of that information, so that it could be displayed in a report. It would also be good to have a report that displayed such tax information, based on a range of dates -- right now there is no report in Ubercart that displays tax information at all.
So I think this would be a good feature to add to the Ubercart tax module.
This applies to both the 6.x-2.0-beta2 version and the 5.x version of Ubercart. I filed an issue on the ubercart.org site about this, but Lyle said he prefers to have issues on the tax module filed here on drupal.org, so here it is. I'm going to close the issue on ubercart.org to avoid duplication.
I plan to work up a patch for the uc_taxes module to implement this.
Comments
#1
#2
I don't know if you plan to implement this, or if you already have some ideas, or if you are waiting to see if I will submit a patch, but I looked into some options today. This is probably obvious to someone more familiar with the Ubercart code base, but just in case, here are my ideas:
a) Save expanded tax information as separate line items in the order (e.g. a line item for Taxable Amount, and another for Tax Rate). However, the line items API currently doesn't allow for invisible line items, so that would need to be modified in order to go that route (I don't think you'd want to display these "line items" to the user, just store them for reporting). I'm not sure if it is a good idea anyway, because it seems like line items are meant to be displayed to the user, and there might be a lot of recoding to do to retool this.
b) Save expanded tax information in the $order->data array (which is serialized and saved in the uc_orders table). The problem with this is that hook_order( 'save' ) [when the tax calculation is done] is called after the base order information has already been saved to the database, so adding $order->data at that point won't get it into the database. Oh well.
c) Save expanded tax information in its own table, something like what the uc_order_line_items table does. I would call this table uc_order_tax_info or something like that, and have columns tax_info_id, order_id, name, rate, tax_amount, taxable_amount, juristiction. To implement this, I would make the following changes:
* hook_calculate_taxes functions would add fields for taxable amount, rate, and (optionally) juristiction (which would default to the name field) to their returned tax arrays/objects
* uc_line_item_tax('load') would add these new fields to the line items it returns (even though they aren't part of official line item arrays)
* uc_taxes_order( 'save' ) would save the information to the database (removing previously-saved information for the same order_id).
I guess option (c) is the only one that is viable. Thoughts?
#3
Would it be a bad idea to just add those fields in the uc_order db schema?
#4
They would need to be added to the uc_order_line_items table, since it is theoretically possible to have more than one tax-related line item on an order, and this information is one-to-one on line items.
I don't think it makes sense to add specifically tax-related fields to the line order table, but I think it would make sense to add a generic "data" field (similar to what is in the uc_order table) so that individual line item modules (such as tax-related modules) could add data to each line item. The line item module would need to serialize/unserialize the data when it stored/retrieved line items.
I think this storage scheme would make sense from a database perspective, since this information is one-to-one on the tax line items, and it's possible other line item modules would need "data" stored as well.
I also think it would work well from a practical perspective.
#5
Here's a patch for this.
It modifies the uc_order module to add a new "data" field to uc_line_items. This works the same as the "data" fields in uc_orders and uc_products -- serialize/unserialize on save/load.
It modifies the uc_taxes module to (a) put the tax rate, taxable amount, and jurisdiction in the new line item data field, and (b) save any data information coming from other modules that calculate taxes in the line item too.
I have verified that this information is getting saved in the database, both from uc_taxes and from my own Washington sales tax module that hooks into uc_taxes.
I haven't yet created a report to show the tax information, but at least getting it saved in the database is a good first step. I think that the report should go into a separate module, because uc_taxes currently doesn't depend on uc_report, and vice versa.
#6
#7
Just a note that the patch is against Bazaar repository for 6.x, revision 1569. I'm a bit new to Bazaar, so please let me know if I should use a different command to make the patch so it is easier to see/apply. Thanks!
#8
Here is a Sales Tax Report module, compatible with the previous patch. It lets you choose a date range and order status, and gives you total taxable amounts and tax collected, grouped by jurisdiction (i.e. the name of the tax, if the tax is coming from the base uc_taxes module) and rate. (This means that if the rate changes sometime during the reporting period, you'll get two different line items in the report.)
It seems to work fine... enjoy. :)
#9
I am not a techy and do nothing with coding. I am a storefront owner that uses Ubercart, and configs it throught Drupal. My site is www.VolidayGreetings.com.
I require this functionality in Tennessee as well. But I have no idea how to install this code. Can someone give me step-by-step instruction on how to install this patch?
Thank you,
Andrew
#10
I'm not sure I'd recommend installing this patch yet. I'm not part of the "official" Ubercart team, and I am not sure they will choose to add it to Ubercart. If they do, they might change how it works.
The thing is, if you install this patch, Ubercart will begin storing the information needed for reporting (juristiction, taxable amount, tax rate -- it already stores the amount of tax collected) in the database in a particular format. If the Ubercart module maintainers later decide to do this a different way, and you upgrade to that version of Ubercart, your stored data will be somewhat useless (of course, I guess you could run the tax report for the period you were using the "patched" Ubercart, and at least you would have the information).
So... well, maybe it's not so bad. But wait a couple of days and see what the response is from "Island Usurper", who is actually one of the Ubercart maintainers.
Then, if you find that you actually did want to install this patch... well, I'm not sure I would recommend it. You would need to edit several files (as indicated in the taxinfo.diff file), and then run update.php, which would probably irreversibly change your Drupal/Ubercart database. The .info and .module files are a new module for reporting; installing them is easy enough.
Anyway, the short answer to your question is, if you are a non-programmer, I would recommend waiting until this functionality is added to Ubercart. See comment below that I am about to make as well...
#11
I woke up this morning wondering if this was the best way to add this functionality after all.
The reason is that the tax report could get slow if there were a lot of transactions. The information for reporting, in this patch, is stored serialized in the line items table. Which means that you can't use the MySQL engine to aggregate it for the report. So the report is aggregated via a PHP loop, with one entry per transaction.
On the other hand, the advantage of doing it this way is that other tax-related modules (and perhaps other line-item modules) can store/retrieve whatever data they want in the "data" field of the line item. It's probably a good idea to have that field there. But if we made a uc_taxes table with the specific fields needed for reporting (jurisdiction, rate, taxable amount, with the line item ID as the primary key), the MySQL query could do the grouping.
I guess I'm not sure whether a PHP loop or a MySQL grouping query tends to be faster, but my gut feeling would be the MySQL grouping query.
Then again, maybe it doesn't matter. I'm not sure how many taxable transactions someone who would be using this report would be likely to report on. In WA, smaller businesses (less than $60K per year) have to report sales taxes quarterly, while larger ones do it monthly. Only things sold within the state are taxed... so probably we're not talking about really huge numbers of transactions in the typical tax report? Maybe the report defaults in the report module should be set to 1 month or 3 months back rather than a year, though?
Thoughts?
I'd be happy to work up a patch for the alternative method (a separate uc_taxes table) if you think that would be better. Just let me know.
#12
Also, for Andrew/Kevin above, here's a Drupal page on how to apply patches: http://drupal.org/node/60108
#13
I dunno, unless you're talking about tens (probably more like hundreds) of thousands of tax items, you probably won't get much of a drag... and even so, that's only on the report, nothing that would mess up the customer.
I haven't applied/tested the patch yet, but I plan to do a little benchmarking... you probably could too if ya want, just generate a buttload of 'em and see how it handles. =)
#14
My initial reaction is to believe that it would be better if the tax report data should be stored in a separate table. This separates it from the rest of the line items, and you don't generally need to know about the line item data and the tax data at the same time. However, there aren't any hooks that would allow the tax data to be updated whenever the line item changed (or was deleted).
That probably means that these hooks should be written, and that ought to lead to a more robust line items API in the long run.
#15
I'm not sure how essential it would be to have hooks to update the tax data if the line item was deleted or updated. Wouldn't most/all of those deletes/updates come from the tax module itself? The only way the tax module would know how to update the taxable amount, tax rate, etc. would be if the order was modified, in which case the tax module would be brought into the loop to update its line items.
And as the reporting is going to have to go off a join between uc_orders -> uc_order_line_items -> uc_taxes_tax_info (or whatever that table is called) anyway, any extraneous items would not participate in the join.
Thoughts?
Anyway, I can work up a patch having the tax info in a separate table. Should have time today or tomorrow, if you would rather do it that way.
It might still make sense to have the "data" field in the line items table. It's parallel to what uc_orders and uc_products have, and flexible. So you could still apply the parts of that patch that don't involve uc_taxes.
#16
Actually, in looking over how to patch with tax info being stored in a different table...
I think the only way to keep a handle on the tax information while the order is being calculated/updated is for it to be part of the line item, at least for most of the calculation. So I think we still need the 'data' component of the line item to exist, and for it to be handled as I did in my patch above (e.g. there are several places in uc_order.module where line items are calculated, then copied into an array - that process needs to copy in the 'data' component). Otherwise, we won't have the jusrisdiction, taxable amount, and rate information stored anywhere during order calculation.
Given that, the only thing that could be changed is the database storage. That is, instead of storing in uc_order_line_items, in new serialized field 'data' as I did in my patch above, we'd store it in a new table. I guess that could work -- we'd use uc_taxes_order( $op = 'save') to remove the info from $line_item['data'] and store it in the aux table, removing any previously-stored information correlated to that particular line item.
Retrieval would be a bit complex though, since it looks like the 'load' operation of hook_order is called before the line items are loaded from the DB, and there is no obvious hook that would allow us to attach the information from our special table after the line items are loaded, as far as I can see.
Anyway, even if there was such a hook, I think this is a bit complex, and I think the best thing is just to store the info in uc_order_line_items, field 'data', as in my patch above.
Thoughts? Is there something I'm missing (not being one of the main authors of UC)?
#17
Don't want to hijack this thread, just wanted to mention that a patch like in #5 has also top priority for the VAT issue and it is just good if it solves two issues at once. Initially I had working an additional field "tax_rate" both in uc_order_products as well as uc_order_line_items which would be enough for the EU, thinking more global a field "tax_amount" (or your way through data) would be more save.
My first review of the patch makes me believe that my VAT contrib will be able to live with it.
Thanks,
Al
#18
There is now a hook_line_item_alter() (uc_order.module, line 1175) that can be used to add the tax information to the line items as they are loaded from the database. It came about through an unrelated issue, but there's no reason that you can't use it.
I've updated the patch so that the install function avoids having different queries for the DB types.
I'm willing to commit this, unless you want to start using hook_line_item_alter(). Just let me know.
#19
Let's go with this patch. I think it's easiest all around, and may be useful to others (such as comment #17 above). Thanks!
Will you also add the new Tax Report module from #8 also, or would you rather I just post it on ubercart.org as a contrib module?
#20
Just to clarify, what I mean by "easier all around" is that the tax info has to get carried around on the PHP array of line items, until it is time to save the order in the DB. Without something like a ['data'] or ->data component on each line item, there is no way to attach the tax info to the PHP line item. So at least part of the patch is needed, in order to carry the info until such time as the order (and line items) are saved.
So if we are going to allow modules to add a 'data' component to a line item, I think it should be saved to the DB.
So might as well use that for taxes, hence this patch. That's my 2 cents anyway.
#21
I think the tax module needs to populate the data column for its line items in an update function. I'll get that done and include the tax report, since it really is necessary for everyone who collects taxes.
#22
And here's the patch. I tried to make uc_taxes_update_6003() dependent on uc_order_update_6007(). So, if you're applying this patch for the first time, you might possibly need to run update.php twice to fill in the data column for the tax line items.
#23
And by "here", I meant in this comment. :P
#24
I don't think that update function is actually a good idea.
It is going to go back and update all those line items, to make them think they have valid data on the tax rate, jurisdiction name, and taxable amount. When in reality, it was fudged. If the tax rates changed (which they do frequently), the update function is dividing previously-calculated tax amounts by the current tax rate, which isn't correct. For taxes that are calculated in a custom way (e.g. by a different module that ties into the tax module), the calculation tihs update function is doing is probably completely wrong.
I personally think it is better to leave the information out than to put dubious information into the database. Let people know that there is no tax reporting information before a certain date, but don't give them the impression the report is correct when it is likely not correct.
Maybe I should create a patch to the reporting module that alerts you there was no tax data before x date somehow? But don't do this database update, please.
#25
Here's another idea: Have two segments to the tax report:
1) The one I submitted above, that would run off the correctly-stored full tax data, whatever is stored after the patched module is installed.
2) A second part that would just list the amount of sales tax collected and the tax name, excluding line items for which we have the full information reported in report segment (1). That way someone who knows what the rates were in the past can get a report with the information that is actually stored for those dates, and do the back calculation correctly.
This way, there is no possibly-bogus "data" stored in the database, and people can get out the full range of correct information that is actually stored. I think this is the best route forward. You can't have wrong information in reports -- these are used to prepare taxes, subject to audits, etc.
Adding the second report segment should be an easy modification to the tax report module I wrote. I'll work something up tomorrow if I can, or else on the plane on the way to DrupalCon (I'm flying out in two days for some pre-con visits with friends. My test box is also my laptop. :) ).
So again, I think the patch in #18 is the right one, and I'd recommend against the patch in #23 that tries to back-populate the table with likely incorrect information.
#26
Here's a new version of the tax report module. It reports both old and new items, with an explanation.
I also added a separate column for the tax name, since sub-modules that implement taxes may not use the same text for both.
#27
OK, I think you have a good point.
The only thing left to do is to clean up the code style so it conforms to the coding standards. Mostly I see Windows line endings and spaces around parentheses that shouldn't be there.
#28
Since we want to make a new release this week, I decided to go ahead and commit the patch from #18 and the tax report module after cleaning up the spots I mentioned. Thanks for putting this together, jhodgdon.
Here's the combined patch and module as they were committed.
#29
This is actually probably important enough to backport to the 1.x branch.
#30
I made some more stylistic changes as I was porting, so those will be committed to the 2.x branch as well. Here is the UC 1.x patch.
#31
You're welcome. Sorry about the windows LFs and style differences between my () and coder. I've been doing it that way for more years than Drupal has existed, sigh. :)
See you at DrupalConDC in a few days?
--Jennifer
#32
Jennifer, I look forward to seeing you there! It took me a while to get used to all the Drupal coding standards at first, and I don't think I even knew about the differences in line endings when I started contributing. ; )
You've been a great help, and I really appreciate the effort you've put into this patch and its follow-through! If you have an account on Ubercart.org, let me know so I can hook you up with a user badge.
#33
I have looked through this, and I am not entirely sure if it is going to help me.
Is there a uc_tax_report.module for the 6.x version of ubercart? If so, what combination of patches and such should I be putting together to make it work (comment numbers is good).
Thanks,
Will
#34
I'm jhodgdon on ubercart.org as well as here.
Cheers,
Jennifer
#35
Wait until the next beta release, and there will be a tax report in 6.x.
#36
swill, grab #28 for 6.x. I really want to get that beta release out today, though. DrupalCon is nigh.
#37
Finally got around to committing it to the 5.x-1.x branch.
#38
Automatically closed -- issue fixed for 2 weeks with no activity.