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

Allow configuring VAT Price Generator class #3451

Conversation

kennyadsl
Copy link
Member

Description

The current VAT generation logic provided by Solidus is not flexible enough to match any business needs, see #3448.

This PR allows to easily create your own custom class to generate VAT prices and use it in Solidus. It also adds a simple explanation in Guides with a realistic example, taken from the issue mentioned above.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • [ ] I have added tests to cover this change (if needed)
  • [ ] I have attached screenshots to this PR for visual changes (if needed)

It is common that stores need to configure how VAT is calculated
and we should provide an easy extension point to make this change
as clean as possible.
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

This looks really good!
I have a suggestion though: Given that this requirement is quite common (and also given that the example we give in this PR matches Spree's VAT logic!), we might want to ship both classes within core, maybe with these class names:

  • Spree::Variant::ConstantNetVatPricegenerator (aliased to the current Spree::Variant::VatPriceGenerator)
  • Spree::Variant::ContantGrossVatPriceGenerator (the implementation from the docs in this PR)?

@kennyadsl
Copy link
Member Author

kennyadsl commented Dec 10, 2019

That's a great idea. Wondering if this should be done at a TaxRate level. I mean when we decide that a tax rate is incuded in price we could also give admin the ability to choose if they want it constant net or constant gross. Do you think it makes sense this way?

Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@kennyadsl thanks 👍

@kennyadsl
Copy link
Member Author

Opened #3461 to track the @mamhoff suggestion. Thanks!

@kennyadsl kennyadsl merged commit 0a6843a into solidusio:master Dec 17, 2019
@waiting-for-dev waiting-for-dev deleted the kennyadsl/make-vat-price-generator-configurable branch September 7, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Proposed or newly added feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants