Almost a year ago I turned tax rates into configuration entities. However, I never finished modifying Ubercart to actually USE those configuration entities - they exist, you can create them, save them, delete them, clone them, edit them, but they are not used to apply taxes. They exist in parallel with the old way of storing tax rates in the database. All the tests use the database taxes, not the entity taxes, and all the uc_tax functions in uc_tax.module still use the old database taxes.
I've had time to revisit this and attack the problem again now that Drupal has a stable version of D8. Configuration entities are not sufficient for what we need to do, so I've upgraded them to use tax rate plugins. A default plugin which calculates a tax rate as a fixed percentage (replicating our old tax system) is included. Using plugins allows us to integrate with web services as well as make complicated tax calculations that can't be represented by a fixed percentage.
A patch with these changes is attached. They still aren't used to actually calculate taxes, but it's a lot of code and I need to do this in stages.
The next step needs to be actually changing uc_tax.module this time, so that it uses these configuration entities.
The last step will be to remove all the old database code.
Comment | File | Size | Author |
---|---|---|---|
#39 | ubercart-tax-rewrite-2672628-39.patch | 32.36 KB | v8comp |
#16 | jurisdiction.patch | 8.76 KB | TR |
| |||
#14 | jurisdiction.patch | 7.83 KB | TR |
#12 | jurisdiction.patch | 7.3 KB | TR |
#11 | test-correction.patch | 1.29 KB | TR |
|
Comments
Comment #2
TR CreditAttribution: TR commented(Don't read too much into the passing tests - all that means is I didn't disturb the existing code, I just added new code which isn't being used yet by the tests.)
Comment #4
TR CreditAttribution: TR commentedRefactor tax tests, add helper function to create a tax rate.
Comment #6
TR CreditAttribution: TR commentedTests for the TaxRate UI.
Comment #9
TR CreditAttribution: TR commentedComment #11
TR CreditAttribution: TR commented#6 failed because I forgot to include my changes to the test base.
Comment #12
TR CreditAttribution: TR commentedMove jurisdiction out of config and into plugin.
Comment #14
TR CreditAttribution: TR commentedAgain.
Comment #16
TR CreditAttribution: TR commentedAgain.
Comment #17
TR CreditAttribution: TR commentedSee also #2708801: Tax not being added at all. Apparently at some point the old tax UI got broken, which makes this important to finish before a beta release.
Comment #18
david.czinege CreditAttribution: david.czinege at Drupers commentedI have started rewriting this uc_tax module, but it is not finished yet. The current version uses the TaxRate configuration entity with the TaxRate plugin. The old sollution is removed:
- database tables are removed
- Unnecessary forms are deleted
- Unnecessary routes are deleted
- I created 2 new tax calculation functions:
- public function calculateProductTax(EntityInterface $item, OrderInterface $order=NULL)
- public function calculateOrderTax(OrderInterface $order, TaxRateInterface $tax)
- I put a line into the CartForm.php to allow other modules to alter the cart display data.
So the uc_tax is not ready.
The following tasks remain:
- The tests need to be rewrite, so i think this patch will be failed after testing.
- I have not dealed with the order view and edit pages yet.
- When the shipping line item is selected as a taxed line item, the original price is shown in the Calculate shipping cost checkout pane.
I need your feedback. Is this a good solution so far what i did?
Comment #20
longwaveI think this looks like a good approach. The tax implementation is always going to be complicated but as far as I can see this is going in the right direction.
Can you post separate patches for removing the old tables/routes/forms and for this new implementation? We may as well remove the broken old code first, that will make the patch easier to review I think.
Comment #21
TR CreditAttribution: TR commentedRemoving the old code is trivial, but that's what most of the patch is. The important part is rewriting the tests and making sure that AS A MINIMUM, the tests pass. But I think leaving the old code in for now is important because it demonstrates the functionality that needs to be re-implemented. That's why I list it as the last step in the issue summary. Throwing the old code away before we have a replacement almost guarantees some functionality (perhaps something that never had tests - which is a lot of things) will be forgotten. I suggest leaving the removal of the old code out of your patch entirely.
Your patch seems to ignore a lot of the tax functionality. While it's a start, it's a long way from finished.
Attached is a patch to change one of the test classes to use the new system. You can convert the other test class yourself using this as an example. If these tests don't pass, your code isn't ready for review.
Comment #22
TR CreditAttribution: TR commentedActually the patch in #21 has a line of debugging code in there. Use this one instead.
Comment #23
CJdriver CreditAttribution: CJdriver commentedIs there some way I can help with this? I see there hasn't been an update in 3 months. Is the #18 patch usable?
Comment #24
mrphilipwarner CreditAttribution: mrphilipwarner commentedI've applied the patch. I found that the configured tax amount was added to the subtotal, but was not included in the total value of the cart, nor was the tax line broken out and annotated at checkout. If the tax is not added to the total and carried forward to payment then this fix is unusable, as it stands.
To mitigate I'm having to hard code my tax amount into my product price then break it out on the checkout page and invoice pages with additional code. Luckily for me I have a very simple product offering with a single tax, but that's not likely to work for most people, who should consider Ubercart very carefully in light of this before installing. It's a shame that I was able to get this far (almost at the point of no return) without knowing about the tax issue, which is a major shortcoming.
Comment #25
PascalMortier CreditAttribution: PascalMortier commented@mrphilipwarner Can you provide the code you have used? I have also a problem with taxes and I don't know how to add an extra line item to the order.
Comment #26
mrphilipwarner CreditAttribution: mrphilipwarner commented@PascalMortier - sorry not to have seen this earlier. Here is what I had to do...
1. In modules/ubercart/uc_cart/uc_cart.theme.inc I had to add a '$tax' variable to compute my tax each time, and then modify the row that renders it:
2. In modules/ubercart/uc_cart/src/Plugin/Block/CartBlock.php I also added a $tax variable to calculate the tax from the total amount:
3. In modules/ubercart/uc_order/templates/uc-order-invoice.html.twig I had to modify the subtotal row and add another row for the tax:
I think that's it, but if not it should give you enough to work with to fill in any gaps.
Comment #27
PascalMortier CreditAttribution: PascalMortier commentedHey mrphilipwarner,
Thank you for explanation ! It's a great help !
Comment #28
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedPrevious submitted patch doesn't apply anymore on latest dev branch.
I have re-rolled it and made some changes.
This new attached patch now works correctly: https://www.drupal.org/files/issues/2018-09-15/2672628-ubercart-tax-rewrite-28.patch" title="2672628-ubercart-tax-rewrite-28.patch
It fixes the following:
{{ content.display_price }}
field in your Twig.I have not removed previous code for the following reasons:
ux_cart.routing.yml
)Comment #29
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedSome errors in my previous patch in PluginBase.
Here's a fixed version of the patch: https://www.drupal.org/files/issues/2018-09-15/2672628-ubercart-tax-rewrite-29.patch.
I haven't managed to inject the entity_type.manager service within
TaxRatePluginBase
.Here is a small to do for anyone who'd like to contribute quickly :)
We would need to get ride of these lines:
Comment #31
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedUploading new patch without coding standard issues.
Comment #32
TR CreditAttribution: TR commentedYou're going to have to add my patch to the StoredTaxTest from #22 into yours, and you're also going to have to modify the InclusiveTaxTest and include it with your patch like I explained in #21. The minimum standard is that the existing tests still work, then we need to work on more comprehensive tests.
Also, your patch in #31 actually has one MORE coding standards issue than your patch in #29? See https://www.drupal.org/pift-ci-job/1069552
You don't have to fix existing coding standards problems because that will only clutter the patch with unrelated changes - those are usually handled in separate coding standards issues. But I don't want to add any additional coding standards problems.
Comment #33
UTCbrij CreditAttribution: UTCbrij as a volunteer and commentedFYI, "jurisdiction" is misspelled on lines 47, 340, and 341 of 2672628-ubercart-tax-rewrite-30.patch.
Comment #34
v8comp CreditAttribution: v8comp commentedHere is an attempt at combining the above as suggested, with updated tests and some tweaks for coding standards.
There are also some minor fixes to (hopefully) make the functionality work as per Drupal 7.
Still a couple of known issues:
* Inclusive suffix does not appear on the cart items on the checkout page
* Inclusive price and suffix do not appear on the invoice
But I think everything else is working.
Comment #35
v8comp CreditAttribution: v8comp commentedSorry some rubbish in that last file
Comment #36
v8comp CreditAttribution: v8comp commentedSorry, try again.
Comment #37
TR CreditAttribution: TR commentedThanks for working on this.
First, the PHP 5.5 test failure is nothing to worry about - Drupal core 8.8.x recently stopped supporting PHP 5. But Drupal core 8.7.x still does support it, that's why we still test against PHP 5.5. I've now changed the standard set of tests so that it no longer tries to test 8.8.x with PHP 5.5.
Second, the PHP 7.3 tests show 30 branch failures which are unrelated to this patch (the occur because Ubercart needs to use the -dev version of Rules, but the testbot doesn't load the -dev version).
However, the PHP 7.3 tests do show a number of failures related to this patch, which will need to be fixed.
Now, as to the details of the patch, I have a number of questions/comments:
1) In uc_tax.services.yml you're injecting the entity.manager service, when it should be the entity_type.manager service. Likewise, the variables/function names you added are entity_manager, setEntityManager, etc. - these should all include the word "type" so that it's clear this is the entity type manager. But see 2) ...
2) Why are you injecting the entity type manager into the TaxRatePluginManager? As far as I can see, you don't use it anywhere.
3) Should not add the MessengerTrait to the TaxRateListBuilder - the DraggableListBuilder parent class already has that.
4) A number of changes should be done in separate issues. For example,
is something that appears in a number of different plugins in Ubercart, and is a legacy of when Ubercart was first ported to pre-8.0 core Drupal. This should be fixed for all the plugins in a separate issue, not just with the taxes. I don't think it's a good idea to allow list types here, but we can discuss that in the other issue.
5)
+ 'tax_jurisdiction' => isset($tax_settings['jurisdiction']) ? $tax_settings['jurisdiction'] : '',
I hate seeing code like this, because it tells me that something hasn't been initialized properly. The jurisdiction should always have a string value - it should never be null. If it's not set sometimes, then the fix is to make sure it is initialized rather that test it with isset() every time we go to use it.6) Why did you change the signature of uc_tax_apply_item_tax()? Why does calculateProductTax() need the Order as one of its parameters?
7) It seems the only reason you're injecting entity type manager into the TaxRatePluginBase is to use it here:
but why do you need to re-load the node at this point - it's being passed in as a parameter, so it should already be loaded and have all its attributes/options, and all the hooks/alterers should have been called already?
Comment #38
v8comp CreditAttribution: v8comp commented(edit) Sorry I didn't see the reply above, will work on those items and get back to you.
Comment #39
v8comp CreditAttribution: v8comp commented1) Sloppy on my part - have changed for now, maybe we can remove it entirely if not needed.
3) Its included in the Controllers and Forms but doesn't seem to be in the DraggableListBuilder from what I can see? I was getting this error:
Maybe I'm missing something, still getting used to Drupal 8 and seems to be harder to find things in the documentation.
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Config!Entity!Dra...
I tried not to make too many changes to what matthieuscarset contributed where it was working so I'm not 100% sure on the motivation for some of the remaining changes but will try to comment.
2), 7) Yes that's the only reason the entity type manager is there, see also comment #29 above. The item passed is not always a node object (e.g. could be a cart item, also see todo in uc_tax_apply_item_tax() function). So the Tax Rate Override Field would be missing in some cases. I've tweaked this a little to make it clearer but maybe a better approach is needed. Is it still possible to load a single field without the whole entity? Other suggestions? I don't use or require this functionality so just trying not to upset it for others who do.
4) Removed, I can remove any other changes you don't want in this patch, just working from matthieuscarset starting point (and making it work for my site). Yes not sure why a list would be required.
5) Looking at uc_tax_rate_load() it appears that the initial idea was to have both the old tax rates from the uc_tax table and the new plugins working at once, possibly in line with your comments about leaving old code in #21? That's not going to work though so I think the isset() thing is just a hangover from that and have removed.
6) You're right the order parameter isn't needed, I guess it was a train of thought that never left the station?
I hesitate to add more problems but does it also seem a little odd that calculateOrderTax() has a TaxRate object passed to it? The rate comes from the plugin but the line item and product types come from the passed object. I also wonder if calculateProductTax should check that the product passed is one of the applicable types.
Comment #40
TR CreditAttribution: TR commentedOh, I see - I'm not sure why I didn't comment on that back when he posted it, other than the tests didn't pass and I may have been busy at the time.
3) Actually MessengerTrait is 'use'd in EntityListBuilder. DraggableListBuilder subclasses ConfigEntityListBuilder which itself subclasses EntityListBuilder, so this is definitely inherited. It's easier to see if you look at the class docs instead of the source code - the class docs tell you where inherited properties/methods come from:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21...
The "Call to undefined method" is maybe because you need to rebuild your cache after changing the class definition? I'm not sure, but the method is definitely there.
5)
Looking at uc_tax_rate_load() it appears that the initial idea was to have both the old tax rates from the uc_tax table and the new plugins working at once, possibly in line with your comments about leaving old code in #21?
That's probably the case. But since having both stopped working some time back, there's no need to keep trying - let's just forget about the old way and make sure the new way works.does it also seem a little odd that calculateOrderTax() has a TaxRate object passed to it?
Yes, I was going to mention that, but I didn't have time to track it down and see if that was really needed.The other thing I want to look into is how the calculateTax() method was split into two: calculateProductTax() and calculateOrderTax(). I'm not sure that's the best way to do things, but regardless if this is done there's duplicate code that should be shared in order to reduce the possibility of error. In Ubercart, taxes have always been on the order, not on individual line items. Without an accompanying order, "product tax" might be meaningless (e.g. if taxes are destination-based, and some customers are tax exempt, then we have no way to calculate a "product tax".)
Comment #41
v8comp CreditAttribution: v8comp commentedThanks for the pointers - somehow I've been using the wrong Drupal installation for testing, its only 8.5 and looks like the MessengerTrait was added to EntityListBuilder in 8.6.x.
The per-product calc is for displaying inclusive prices when configured, and now that we have plugins it needs to go through that mechanism rather than just multiplying by the rate. I'm working on improving this, I think it can be simplified in the right way and also avoid that entity type manager injection.
Also found some more issues that need fixing. The TaxRate entity has a rate property and getter/setter methods that are never used because the plugin defines its own settings (as it should). The data saved against the order so the inclusive prices can always be displayed correctly after checkout isn't right. There are also some extra controllers and forms that are unused (looks like they must have been started for the initial D8 port before deciding to convert to entities), I guess I shouldn't remove them yet but I'd like to at least add comments to the ones that aren't used and should be removed in future as its a bit confusing.
Hoping to find some more time this week, will work on an updated patch.