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

feat(tax): introduce tax override data models #6422

Merged
merged 8 commits into from
Feb 20, 2024
Merged

Conversation

srindom
Copy link
Collaborator

@srindom srindom commented Feb 16, 2024

What

  • Adds Tax rules to allow overrides of tax rates for certain products, product types or shipping options.

Punted to future PR

  • Currently, the creation methods only support bulk operations. A later PR will include support for singular operations, too.
  • It should be possible to add products, types, and shipping options to a tax rate after creating it. Add, remove, update will come in a later PR.

Copy link

vercel bot commented Feb 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api-reference 🔄 Building (Inspect) Visit Preview Feb 20, 2024 4:26pm
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 20, 2024 4:26pm
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
docs-ui ⬜️ Ignored (Inspect) Visit Preview Feb 20, 2024 4:26pm
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Feb 20, 2024 4:26pm

Copy link

changeset-bot bot commented Feb 16, 2024

⚠️ No Changeset found

Latest commit: 7a51965

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@srindom srindom changed the base branch from develop to feat/tax-module-juris February 16, 2024 18:01
@srindom srindom changed the base branch from feat/tax-module-juris to develop February 18, 2024 16:35
@srindom srindom changed the title Feat/tax module rules feat(tax): introduce tax override data models Feb 19, 2024
@carlos-r-l-rodrigues
Copy link
Contributor

If we make them specific for product, shipping, product_type, wouldn't it make sense to have them as link definitions?

If we are to keep them in the module, we could just create a single generic table, where you specify a reference and a reference_id and enable these specific taxes for anything.

@srindom
Copy link
Collaborator Author

srindom commented Feb 19, 2024

If we make them specific for product, shipping, product_type, wouldn't it make sense to have them as link definitions?

I wanted to include them into the tax module because this gives the tax module the feature of having tax overrides. If the rules only exist as relations we make it a task for the consumer to find the right tax rates (as opposed to exposing a taxModuleService.getItemTaxLines(arrayOfItemLikeStructures, calculationContextEgAddress)). It would probably make sense to have readonly links here to make them queriable with remote query, but wouldn't consider it strictly necessary.

If we are to keep them in the module, we could just create a single generic table, where you specify a reference and a reference_id and enable these specific taxes for anything.

Yeah this might actually be better - is it possible to have links like this?

@carlos-r-l-rodrigues
Copy link
Contributor

Yeah this might actually be better - is it possible to have links like this?

Other modules are copying the TaxRate internally at the moment. I am not sure I get which link you mean.
These would be used to calculate a tax rate given a context no? where, type is included.

@srindom
Copy link
Collaborator Author

srindom commented Feb 19, 2024

I am not sure I get which link you mean.

The one I was thinking about was one that could be used in remoteQuery to allow a query like this:

product {
  tax_rate_rules {
    tax_rate {
      rate
    }
  }
}

I.e., if it were possible for remote query to understand a link like "reference_type + reference_id".

But this was more out of curiosity, not something that is required at all.

[PrimaryKeyProp]?: ["tax_rate_id", "reference_id"]

@Property({ columnType: "text" })
reference_type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on how filters will be passed to fetch data from this table, probably individual indexes on tax_rate_id, reference_id and reference_type will be a better choice than this composite primary key.
Keeping the composite key and adding additional indexes is also an option.

@carlos-r-l-rodrigues
Copy link
Contributor

The one I was thinking about was one that could be used in remoteQuery to allow a query like this:

product {
  tax_rate_rules {
    tax_rate {
      rate
    }
  }
}

I.e., if it were possible for remote query to understand a link like "reference_type + reference_id".

But this was more out of curiosity, not something that is required at all.

This isn't implemented at the moment, but it's definitely doable

packages/tax/src/services/tax-module-service.ts Outdated Show resolved Hide resolved
packages/types/src/tax/common.ts Outdated Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit 137cc0e into develop Feb 20, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants