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

Add validity period for Spree::TaxRate #1953

Merged
merged 1 commit into from
Jun 12, 2017

Conversation

mtylty
Copy link
Contributor

@mtylty mtylty commented May 23, 2017

This code adds a valid_from and valid_until datetime field to Spree::TaxRate so that tax rates can be scoped in time.

Null values will be treated as valid so that a tax rate is valid by default and you can just ignore this feature if you don't need it.

The Spree::Calculator::DefaultTax will always return 0 in case the tax rate is not in its validity period.

I was unsure how to implement it so I started with a scope but ended up thinking it's not really needed. @mamhoff if you could point me in the right direction I'd refactor it.

Ref. #1857

@mtylty
Copy link
Contributor Author

mtylty commented May 23, 2017

This is also missing validations for dates but I can imaging we want this to have validations so that there can only be one valid tax rate at a time.

@mtylty mtylty changed the title [WIP] Add validity period for Spree::TaxRate Add validity period for Spree::TaxRate May 25, 2017
@mtylty
Copy link
Contributor Author

mtylty commented May 25, 2017

I ended up duplicating code that's in Spree::Promotion to duplicate the same starts_at/expires_at logic. It made perfect sense to me to use that logic in Spree::TaxRate as well.

@mtylty mtylty force-pushed the mtylty/time-aware-tax-rates branch from a5bd420 to 162d5a2 Compare May 25, 2017 15:44
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.

I think this is a great step forward for the internal taxation system. However would you mind squashing the commits into one? The in-PR refactoring isn't something future readers need to see. :)

@@ -22,6 +22,7 @@
<col style="width: 10%">
<col style="width: 10%">
<col style="width: 10%">
<col style="width: 10%">
Copy link
Contributor

Choose a reason for hiding this comment

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

We're now way over 100% with these width values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see what I can do... I'm not that good with style and html 😅

This code adds a starts_at and expires_at datetime field to
Spree::TaxRate so that tax rates can be scoped in time.

Null values will be treated as valid so that a tax rate is valid by
default and you can just ignore this feature if you don't need it.

The Spree::Calculator::DefaultTax will always return 0 in case the tax
rate is not valid (either not started or expired).

Ref. solidusio#1857
@mtylty mtylty force-pushed the mtylty/time-aware-tax-rates branch from 162d5a2 to 10d5260 Compare May 31, 2017 16:09
@kennyadsl kennyadsl merged commit aa21865 into solidusio:master Jun 12, 2017
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.

5 participants