Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve tax API, split out Custom Rates plugin #4785

Merged
merged 28 commits into from
Nov 14, 2018

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Oct 31, 2018

Resolves #4710
Resolves #4711
Resolves #4712
Resolves #4687
Impact: breaking
Type: feature

Related docs updates: reactioncommerce/reaction-docs#707

Changes

  • We've created a new taxes-rates plugin in the included folder, and all features related to custom rates have been moved there. This includes the "Custom Rates" panel in tax settings; the Taxes collection and its related schemas; the "taxes/addRate", "taxes/editRate", and "taxes/deleteRate" Meteor methods, and the "Taxes" Meteor publication.
  • In the core taxes plugin, the general settings have been converted to React, in a GeneralTaxSettings component. This new component now allows you to set a single active tax service (or none). This replaces the previous way of enabling or disabling each service. All services are now enabled, but only one is active per shop.
  • The core taxes plugin has a new API for registering tax services (such as the included "Custom Rates" service, or a custom Avalara service for example). They are registered by passing in a taxServices array to registerPackage:
import Reaction from "/imports/plugins/core/core/server/Reaction";
import calculateOrderGroupTaxes from "./server/no-meteor/util/calculateOrderGroupTaxes";
import getTaxCodes from "./server/no-meteor/util/getTaxCodes";

Reaction.registerPackage({
  label: "Custom Rates",
  name: "reaction-taxes-rates",
  icon: "fa fa-university",
  autoEnable: true,
  taxServices: [
    {
      displayName: "Custom Rates",
      name: "custom-rates",
      functions: {
        calculateOrderGroupTaxes,
        getTaxCodes
      }
    }
  ]
  // ...
});

Every service must provide one function: calculateOrderGroupTaxes. It may also provide a getTaxCodes function to return a list of tax codes so that operators can select from a list of tax codes rather than having to enter as free text. Refer to updated public docs for more information about these functions.

  • All tax-related fields on ProductVariant, CatalogProductVariant, CartItem, OrderItem, Cart, and OrderFulfillmentGroup are now removed from core code and instead extended onto the schemas within the core taxes plugin. This is true for both SimpleSchemas and GraphQL schemas.
  • The core taxes plugin has two new GraphQL queries: taxCodes and taxServices
  • The core taxes plugin also has two new React HOCs that can be used for getting the list of tax codes or registered tax services: withTaxCodes and withTaxServices
  • The core taxes plugin adds a getFulfillmentGroupTaxes mutation (internal mutation, not GraphQL). This is called by the order and cart plugins to (re)calculate taxes for orders and carts. Most of the actual calculation work is done by the calculateOrderGroupTaxes function that was registered by the active tax service.
  • Some tax-related fields on Cart, CartItem, Order, OrderFulfillmentGroup, and OrderItem have been moved, renamed, added, or removed. We've attempted to remove all unused fields, and group or rename other fields for clarity. One example is the taxes array, which now has a different schema and appears for individual items as well as the full cart or order fulfillment group.
  • On Products documents, taxable is now isTaxable. This change had previously been made in the Catalog schema and now is made in Products to match.

Breaking changes

These changes are breaking in a number of ways. There are breaking changes since 2.0.0-rc.5 release, but this also completes a broader set of breaking changes between 1.x and 2.x. The major version breaking changes are listed above.

In particular, for the Custom Rates plugin, be aware that the taxCode value is now used for filtering which products should be taxed at that rate. This requires a review of all your products to ensure that they have a tax code specified, in addition to being marked as taxable. If you'd rather not do this review, you can revert to the old behavior of ignoring tax codes by editing each of your Custom Rates entries, clearing the the "Tax Code" field, and saving.

If you are upgrading from 1.x and use only Custom Rates for taxes, data migrations should provide a seamless transition. Most changes are breaking only for third-party non-included tax plugins. However, please verify after upgrading that the correct tax service is active.

Testing

@aldeed aldeed self-assigned this Oct 31, 2018
@aldeed aldeed added this to the Quandary milestone Oct 31, 2018
@aldeed aldeed changed the title [WIP] Improve tax API, split out Custom Rates plugin Improve tax API, split out Custom Rates plugin Oct 31, 2018
@brent-hoover
Copy link
Collaborator

Added Akarshit as a reviewer as he is testing out the API by actually writing a tax plugin

@brent-hoover
Copy link
Collaborator

brent-hoover commented Nov 7, 2018

When saving a default tax code the fields gets emptied out after save (ignore the translation thing, I just needed to reset)

2018-11-07_08-12-37

@brent-hoover
Copy link
Collaborator

Testing this in the "classic UI" I don't get tax showing at all.

@brent-hoover
Copy link
Collaborator

Testing in Storefront on the develop branch with Custom Rates enabled also not seeing taxes

2018-11-07_09-54-04

@brent-hoover
Copy link
Collaborator

Also get this error when checking out

=> App running at: http://localhost:3000/
02:08:15.737Z ERROR Reaction: createOrder: error creating payments Effective tax rate must be of type Number
02:08:15.745Z ERROR Reaction:  (errorId=cjo6ixoqp000037wbj7pkz7uw, userId=rsB7XKbyb3XBnnxoZ)

@brent-hoover
Copy link
Collaborator

So it looks like taxes are not being calculated because isTaxable is set to false. However I am not able to toggle "Taxable" in the PDP. I am able to toggle this on the release-2.0.0-rc.6 branch

@brent-hoover
Copy link
Collaborator

So even when I set isTaxable manually on the variant I am not able to get taxes to come up with "Custom Rates.

It also appears that something about the migration is not working as the default product is not getting isTaxable set correctly.

@brent-hoover
Copy link
Collaborator

Even when creating a new product I can't set "Taxable" in the PDP

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to get taxes to calculate in either Classic or Storefront. Also not able to set "Taxable" on products in the PDP

@aldeed
Copy link
Contributor Author

aldeed commented Nov 9, 2018

@Akarshit @zenweasel Here is a starterkit branch with the UI fix: reactioncommerce/example-storefront#424

It should work with that branch of storefront and this branch of Reaction API

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aldeed This is still not working for me. If I have a product (like the default sample product) that has options and I set "isTaxable on the variant, this is not properly being carried over to the options so isTaxable is being set to false on all options which is where the tax calculator looks to see if something is taxable so taxes never show up.

@brent-hoover
Copy link
Collaborator

@aldeed Also, do we need a migration to set the Tax Code for all existing products to RC_TAX if Custom Rates are in use? It seems like you were saying that this Tax Code needs to be set for it to calculate tax as well.

@aldeed
Copy link
Contributor Author

aldeed commented Nov 9, 2018

@zenweasel It is now working when purchasing options, and I fixed the Publish button indicator for the tax fields.

I'm nervous about trying to automatically figure out "if custom rates is enabled" since in the past you could enable multiple tax plugins at once. I wouldn't want to set all variants to RC_TAX if in reality they were already set properly for some other plugin.

Some possible ideas:

  • Migration to set taxCode to RC_TAX only if taxCode is currently 0000, which was previously the default.
  • Just document the change in the release notes, with an example MongoDB command they can run manually to set all to RC_TAX
  • Otherwise, since we don't yet have any UI to add additional tax codes, we could NOT require the tax code be set. We could add a plugin shop setting filterByTaxCode and have that be false by default. Then it would tax any product that is taxable without also needing RC_TAX tax code on it (like the behavior prior to this PR).
  • Lastly, I could just plain remove the tax code check and go back to how it worked before this. I think it's sort of confusing that it shows RC_TAX in the custom rates table if it doesn't actually check that, but maybe not as confusing as fixing it.

@spencern Any thoughts on this?

@aldeed aldeed dismissed brent-hoover’s stale review November 9, 2018 16:21

Fixed reported issues

@brent-hoover
Copy link
Collaborator

brent-hoover commented Nov 12, 2018

It seems to be calculating tax now but when setting "Charge tax on Shipping" to be true it doesn't seem to be calculating tax on shipping.

2018-11-12_08-53-45

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still does not appear to be calculating tax correctly when "Charge tax on shipping" is set

@brent-hoover
Copy link
Collaborator

Multi-jurisdiction pricing does seem to be working

}

/**
* @summary Modifies a fulfillment group, adding `taxRate` and `tax` properties to each item
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this function actually modified the group object.

@aldeed
Copy link
Contributor Author

aldeed commented Nov 12, 2018

Charge tax on shipping has never been implemented in custom rates plugin. I'm not sure if other tax plugins use it, but if not, we could potentially remove it. It seems like kind of a weird setting to have anyway since I would think that would usually be determined by the jurisdiction rules that the tax plugin uses.

@spencern
Copy link
Contributor

@aldeed @zenweasel I lean towards option 2:

Just document the change in the release notes, with an example MongoDB command they can run manually to set all to RC_TAX

@spencern
Copy link
Contributor

We should have a way to charge tax on shipping for the custom rates - on a per-locale basis. If that's not been implemented to this point, it could be a separate bit of work.

@brent-hoover
Copy link
Collaborator

brent-hoover commented Nov 12, 2018

@aldeed Question: tax plugins can add additional settings still, correct? I know Avalara has about another 10-15 settings that they require the user to set.

I think what's probably missing with "charge tax on shipping' would be a "shipping tax code". I know with Avalara that was how it determined how to charge or exclude shipping from the calculation.

@brent-hoover brent-hoover dismissed their stale review November 13, 2018 03:21

Problems fixed

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have the issue of charging tax on shipping but that probably needs to be separate from this PR. So approving so we can get this merged and move forward.

@Akarshit
Copy link
Contributor

@aldeed I am trying to place a order with tax and am getting the following pop-up:
screenshot 2018-11-13 at 4 51 00 pm

aldeed and others added 5 commits November 13, 2018 09:38
Individual tax service plugins should implement
this setting if necessary
Do not require the item tax code to match in order to charge taxes
if the tax rate record does not include a taxCode. If the fix to
filtering Custom Rates taxes by tax code is an undesired
breaking change, you can revert to the old behavior by clearing out
the "Tax Code" field in the Custom Rates definition now.
Co-Authored-By: aldeed <aldeed@gmail.com>
@aldeed
Copy link
Contributor Author

aldeed commented Nov 13, 2018

OK, so I've addressed the remaining things for now:

  • I added taxCode to the custom rates add/edit form so now you can remove RC_TAX if you don't want the rate filtered based on the product tax code needing to match. I added a note about this in the breaking changes section of this PR description.
  • Removed that "Tax shipping" check box since it doesn't appear that Avalara uses that either. Adding some way to tax shipping with the Custom Rates plugin is a separate project. I'll make an issue.
  • Fixed it so that the "default tax code" actually does get set on new variants when you create them.

@Akarshit That checkout error appears to be an issue with storefront, that it isn't including the tax when trying to create the order. I'll fix that in the starterkit repo.

Pretty sure this branch should be good to merge now, but I'll let it hang out for another day or two while you're still testing the custom plugin against it.

@aldeed
Copy link
Contributor Author

aldeed commented Nov 13, 2018

@Akarshit Actually I was wrong. I believe that error would mean that your plugin's calculation function is returning 1.78 for taxSummary.tax when called for a cart, but is returning 0 for that when called for the order group. It's hard to say whether there is any core bug involved without seeing what your function is returning in each case.

@aldeed
Copy link
Contributor Author

aldeed commented Nov 14, 2018

I'm going to merge this since it's big and approved. Any more issues that are found can be submitted and fixed in a follow-up PR.

@aldeed aldeed merged commit ec2b737 into release-2.0.0-rc.7 Nov 14, 2018
@aldeed aldeed deleted the refactor-aldeed-split-tax-rates-plugin branch November 14, 2018 20:52
@spencern spencern mentioned this pull request Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants