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.

CommentFileSizeAuthor
#39 ubercart-tax-rewrite-2672628-39.patch32.36 KBv8comp
#36 ubercart-tax-rewrite-2672628-36.patch33.42 KBv8comp
#35 2672628-ubercart-tax-rewrite-35.patch33.37 KBv8comp
#34 2672628-ubercart-tax-rewrite-34.patch42.32 KBv8comp
#31 2672628-ubercart-tax-rewrite-30.patch14.29 KBmatthieuscarset
#29 2672628-ubercart-tax-rewrite-29.patch13.69 KBmatthieuscarset
#28 2672628-ubercart-tax-rewrite-28.patch14.05 KBmatthieuscarset
#22 taxtest.patch5.32 KBTR
#21 taxtest.patch5.33 KBTR
#18 2672628-ubercart-tax-rewrite-18.patch33.41 KBdavid.czinege
#16 jurisdiction.patch8.76 KBTR
#14 jurisdiction.patch7.83 KBTR
#12 jurisdiction.patch7.3 KBTR
#11 test-correction.patch1.29 KBTR
#9 more-tests.patch5.78 KBTR
#6 more-tests.patch5.74 KBTR
#4 tests.patch5.31 KBTR
tax-rate-plugins.patch33.37 KBTR
Support from Acquia helps fund testing for Drupal Acquia logo

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?

mrphilipwarner’s picture

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

PascalMortier’s picture

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

mrphilipwarner’s picture

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

  // line 42
  foreach ($items as $item) {
    $subtotal += $item->price->value * $item->qty->value;
    $tax += (($item->price->value * $item->qty->value) - (($item->price->value * $item->qty->value)/120)*100);
    $rows[] = array(
      'data' => array(
        array(
          'data' => array(
            '#theme' => 'uc_qty',
            '#qty' => $item->qty->value,
          ),
          'class' => array('qty'),
        ),
        array(
          'data' => array('#markup' => $item->title->value . uc_product_get_description($item)),
          'class' => array('products'),
        ),
        array(
          'data' => array(
            '#theme' => 'uc_price',
            // '#price' => (($item->price->value/120)*100) * $item->qty->value,
            '#price' => $subtotal - $tax,
          ),
          'class' => array('price'),
        ),
      ),
      'no_striping' => TRUE,
    );
  }

  // Add the tax ldap_first_reference
  $rows[] = array(
    'data' => array(
      array(
        'data' => array(
          '#theme' => 'uc_qty',
          '#qty' => 1,
        ),
        'class' => array('qty'),
      ),
      array(
        'data' => array('#markup' => 'VAT'),
        'class' => array('products'),
      ),
      array(
        'data' => array(
          '#theme' => 'uc_price',
          '#price' => round($subtotal,2) - (round($subtotal - $tax,2))
        ),
        'class' => array('price'),
      ),
    ),
    'no_striping' => TRUE,
  );

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:

public function build() {
    $cart = \Drupal::service('uc_cart.manager')->get();
    $product_count = count($cart->getContents());

    // Display nothing if the block is set to hide on empty and there are no
    // items in the cart.
    if (!$this->configuration['hide_empty'] || $product_count) {
      $items = array();
      $item_count = 0;
      $total = 0;
      $subtotal = 0;
      $tax = 0;
      if ($product_count) {
        /** @var \Drupal\uc_cart\CartItemInterface $item */
        foreach ($cart->getContents() as $item) {
          $display_item = \Drupal::moduleHandler()->invoke($item->data->module, 'uc_cart_display', array($item));

          if (count(Element::children($display_item))) {
            $items[] = array(
              'nid' => $display_item['nid']['#value'],
              'qty' => $display_item['qty']['#default_value'],
              // $display_item['title'] can be either #markup or #type => 'link', so render it.
              'title' => drupal_render($display_item['title']),
              'price' => ($display_item['#total']/120)*100,
              'desc' => isset($display_item['description']['#markup']) ? $display_item['description']['#markup'] : FALSE,
            );
            $subtotal += ($display_item['#total']/120)*100;
            $total += $display_item['#total'];
            $item_count += $display_item['qty']['#default_value'];
            $tax += $display_item['#total'] - (($display_item['#total']/120)*100);
          }

          // add the tax line
          $items[] = array(
            'nid' => FALSE,
            'qty' => FALSE,
            // $display_item['title'] can be either #markup or #type => 'link', so render it.
            'title' => 'VATE',
            'price' => $total - $subtotal,
            'desc' => FALSE,
          );
        }

      }

      // Build the cart links.
      $summary_links['view-cart'] = array(
        'title' => $this->t('View cart'),
        'url' => Url::fromRoute('uc_cart.cart'),
        'attributes' => array('rel' => ['nofollow']),
      );

      // Only add the checkout link if checkout is enabled.
      if (\Drupal::config('uc_cart.settings')->get('checkout_enabled')) {
        $summary_links['checkout'] = array(
          'title' => $this->t('Checkout'),
          'url' => Url::fromRoute('uc_cart.checkout'),
          'attributes' => array('rel' => ['nofollow']),
        );
      }

      $build['block'] = array(
        '#theme' => 'uc_cart_block',
        '#items' => $items,
        '#item_count' => $item_count,
        '#total' => $total,
        '#summary_links' => $summary_links,
        '#collapsed' => $this->configuration['collapsed'],
      );

      // Add the cart block CSS.
      $build['#attached']['library'][] = 'uc_cart/uc_cart.block.styles';

      // If the block is collapsible, add the appropriate JS.
      if ($this->configuration['collapsible']) {
        $build['#attached']['library'][] = 'uc_cart/uc_cart.block.scripts';
      }

      return $build;
    }
  }

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:

                    <tr>
                      <td nowrap="nowrap" width="20%">
                        {{ 'Subtotal:'|t }}&nbsp;
                      </td>
                      <td width="80%">
                        £{{((order_subtotal[1:] / 120) * 100)|round(2)}}
                      </td>
                    </tr>

                    <tr>
                      <td nowrap="nowrap" width="20%">
                        {{ 'VAT:'|t }}&nbsp;
                      </td>
                      <td width="80%">
                        £{{(order_subtotal[1:] - ((order_subtotal[1:] / 120) * 100))|round(2)}}
                      </td>
                    </tr>

I think that's it, but if not it should give you enough to work with to fill in any gaps.

PascalMortier’s picture

Hey mrphilipwarner,
Thank you for explanation ! It's a great help !

matthieuscarset’s picture

FileSize
14.05 KB

Previous 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:

  1. Price on Product Display now shows included taxe (as long as you use the {{ content.display_price }} field in your Twig.
  2. Cart and Checkout forms also now show prices with taxes included

I have not removed previous code for the following reasons:

  1. No time to go through eveything...
  2. Module works seamlessly with existing code
  3. I've found out some pieces of code are being used by existing shop websites (e.g ux_cart.routing.yml)
matthieuscarset’s picture

Status: Needs work » Needs review
FileSize
13.69 KB

Some 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:

    // @todo to be injected.
    $this->entityTypeManager = \Drupal::entityTypeManager();

Status: Needs review » Needs work

The last submitted patch, 29: 2672628-ubercart-tax-rewrite-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

matthieuscarset’s picture

Uploading new patch without coding standard issues.

TR’s picture

Assigned: TR » Unassigned

You'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.

UTCbrij’s picture

FYI, "jurisdiction" is misspelled on lines 47, 340, and 341 of 2672628-ubercart-tax-rewrite-30.patch.

v8comp’s picture

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

v8comp’s picture

Sorry some rubbish in that last file

TR’s picture

Thanks 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,

-      ->condition('field_type', 'number')
+      ->condition('field_type',
+        ['integer', 'float', 'decimal', 'list_integer', 'list_decimal']
+      )

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:

+  public function calculateProductTax(EntityInterface $item, OrderInterface $order = NULL) {
+    $node = $this->entityTypeManager->getStorage('node')->load($item->id());

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?

v8comp’s picture

(edit) Sorry I didn't see the reply above, will work on those items and get back to you.

v8comp’s picture

1) 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:

Call to undefined method Drupal\\uc_tax\\TaxRateListBuilder::messenger() in ubercart/uc_tax/src/TaxRateListBuilder.php on line 169

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.

TR’s picture

just working from matthieuscarset starting point

Oh, 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".)

v8comp’s picture

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