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.

Comments

TR created an issue. See original summary.

TR’s picture

Issue summary: View changes

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

  • TR committed 8d3925b on 8.x-4.x
    Issue #2672628 by TR: Convert TaxRate entities to use plugins, Part I.
    
TR’s picture

Refactor tax tests, add helper function to create a tax rate.

  • TR committed 8686f50 on 8.x-4.x
    Issue #2672628 by TR: Refactor tax tests, add helper function to create...
TR’s picture

Tests for the TaxRate UI.

Status: Needs review » Needs work

The last submitted patch, 6: more-tests.patch, failed testing.

The last submitted patch, 6: more-tests.patch, failed testing.

TR’s picture

Status: Needs work » Needs review
FileSize
5.78 KB

  • TR committed d5850f9 on 8.x-4.x
    Issue #2672628 by TR: More TaxRate tests.
    
TR’s picture

#6 failed because I forgot to include my changes to the test base.

TR’s picture

Move jurisdiction out of config and into plugin.

Status: Needs review » Needs work

The last submitted patch, 12: jurisdiction.patch, failed testing.

TR’s picture

Status: Needs work » Needs review
FileSize
7.83 KB

Again.

Status: Needs review » Needs work

The last submitted patch, 14: jurisdiction.patch, failed testing.

TR’s picture

Status: Needs work » Needs review
FileSize
8.76 KB

Again.

TR’s picture

Issue tags: +beta blocker

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

david.czinege’s picture

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

Status: Needs review » Needs work

The last submitted patch, 18: 2672628-ubercart-tax-rewrite-18.patch, failed testing.

longwave’s picture

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

TR’s picture

FileSize
5.33 KB

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

TR’s picture

FileSize
5.32 KB

Actually the patch in #21 has a line of debugging code in there. Use this one instead.

CJdriver’s picture

Is there some way I can help with this? I see there hasn't been an update in 3 months. Is the #18 patch usable?