TaxHelpers#rates_for_item now respects the validity period of tax rates #3768

knorrli · 2020-09-22T13:24:55Z

Description
When calculating the tax for line items whose tax is included in the price, solidus uses TaxRates in the calculation even though they are invalid according to their start_date and/or end_date. This results in incorrect tax calculations if there are multiple tax rates for the same tax category and only some of them are currently valid.

This PR ensures that TaxHelpers#rates_for_item only returns tax rates that are currently valid. Before, it would return any tax rate that matched the items' tax category.

This addresses #3766

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)

knorrli · 2020-09-22T13:29:50Z

As suggested in #3766, I have opened a PR and moved the change to the TaxHelpers#rates_for_item method.

I am currently still unsure about two things:

Maybe it would make sense to add an .active scope to TaxRate. If so, would you like me to do this in this PR directly?
I didn't create a new spec file for the TaxHelpers module and instead added a test to the DefaultTax calculator which is one of two places affected by the change. Would you like me to create a spec file for the TaxHelpers module? If so, where should it be (since TaxHelpers is in models/ for some reason...?).

I'm very open for suggestions and improvements.

aldesantis · 2020-09-23T07:28:34Z

@jugglinghobo thanks for this! Answers to your concerns below:

Maybe it would make sense to add an .active scope to TaxRate. If so, would you like me to do this in this PR directly?

Yes, I think it would be useful. 🙂 Feel free to do it in this PR!

I didn't create a new spec file for the TaxHelpers module and instead added a test to the DefaultTax calculator which is one of two places affected by the change. Would you like me to create a spec file for the TaxHelpers module? If so, where should it be (since TaxHelpers is in models/ for some reason...?).

Yes, I think the test for this should be on the TaxHelpers module itself. You can put it in spec/models/spree/tax/tax_helpers_spec.rb. Testing modules can be a bit tricky (you'd usually create a "dummy" test class and include the module there, then test the method somehow), so let me know if you need any help.

mamhoff · 2020-10-09T09:13:08Z

@aldesantis @jugglinghobo I think this is actually a pretty important fix functionality-wise. I appreciate wanting to have very good specs that respect all the things, and maybe also a scope, but can we make issues for that and get this in? I'm seeing clients write workarounds in their exporter code for tax advisors because this doesn't work. I'd also be happy to take this over if necessary. (cc @tvdeyen @mntmn).

knorrli · 2020-10-09T12:02:29Z

@mamhoff @aldesantis I'm sorry this fell off my radar. If you can wait, I'll try to finish this tonight or tomorrow. If you don't hear back until sunday or if you need a solution quicker, feel free to take over, and if you leave out anything (e.g. scope), feel free to assign an issue to me or whatever you'd like.

Does that work for you, or would you like me to do anything else? This is my first solidus / open source contribution.

aldesantis · 2020-10-09T12:18:12Z

@jugglinghobo even if this was merged today, it still wouldn't be available until the next Solidus release, so tonight/tomorrow works perfectly. Thanks for your help on this one!

mamhoff · 2020-10-09T13:08:11Z

I just didn't want this to sit for another couple of weeks. If you're on it, great! 👍

TaxHelpers#rates_for_item considers the validity period of tax rates and only returns tax rates that are currently valid. * Added specs for TaxHelpers#rates_for_item * Removed specs from DefaultTaxCalculator spec because they don't belong there

core/spec/models/spree/tax/tax_helpers_spec.rb

* remove trailing whitespace * remove extra line at beginning of block

This scope returns all tax rates that are currently active according to their start date and expiry date, i.e. if the current time is after the start date and before the expiry date.

core/spec/models/spree/tax_rate_spec.rb

* add space between brackets

knorrli · 2020-10-10T23:11:53Z

@aldesantis

As discussed I've added a spec for TaxHelpers#rates_for_item. IMO this is all that's required to fix the bug.
I also added the TaxRate.active scope (identical to the one in Promotion), but it is unused at the moment.

Please suggest improvements and advise on how to proceed. I didn't squash yet because I thought maybe you'd like to add the scope in a separate PR.

PS: AFAICT there are no specs for Promotion.active, if the new specs are accepted then these could be reused there.

aldesantis

@jugglinghobo thanks, the tests are okay except for one minor thing. After that, GTM!

core/spec/models/spree/tax/tax_helpers_spec.rb

aldesantis

Looks great, thanks so much! 🙏

mamhoff

Nicely done!

kennyadsl

Thanks!

knorrli · 2020-10-13T11:49:23Z

Thank you guys!

TaxHelpers#rates_for_item now respects the validity period of tax rates

f8ef370

kennyadsl added this to the 2.11 milestone Oct 9, 2020

houndci-bot reviewed Oct 10, 2020

View reviewed changes

core/spec/models/spree/tax/tax_helpers_spec.rb Outdated Show resolved Hide resolved

core/spec/models/spree/tax/tax_helpers_spec.rb Outdated Show resolved Hide resolved

core/spec/models/spree/tax/tax_helpers_spec.rb Outdated Show resolved Hide resolved

knorrli added 3 commits October 10, 2020 22:20

removed :focus from spec

32c5681

addressing hound comments

22a0054

* remove trailing whitespace * remove extra line at beginning of block

added TaxRate.active scope

16272c4

This scope returns all tax rates that are currently active according to their start date and expiry date, i.e. if the current time is after the start date and before the expiry date.

houndci-bot reviewed Oct 10, 2020

View reviewed changes

resolve hound comments

a0dc6fc

* add space between brackets

aldesantis requested changes Oct 11, 2020

View reviewed changes

core/spec/models/spree/tax/tax_helpers_spec.rb Outdated Show resolved Hide resolved

resolve feedback: dummy class uses stub_const to ensure it's cleaned up

d7490ad

houndci-bot reviewed Oct 11, 2020

View reviewed changes

core/spec/models/spree/tax/tax_helpers_spec.rb Outdated Show resolved Hide resolved

resolve hound feedback: remove empty line at block start

62d7825

aldesantis approved these changes Oct 11, 2020

View reviewed changes

aldesantis requested a review from a team October 11, 2020 14:09

aldesantis added the Needs Core Team Review label Oct 11, 2020

mamhoff approved these changes Oct 13, 2020

View reviewed changes

kennyadsl approved these changes Oct 13, 2020

View reviewed changes

kennyadsl merged commit 3101760 into solidusio:master Oct 13, 2020

kennyadsl added changelog:solidus_core Changes to the solidus_core gem and removed Needs Core Team Review labels Oct 13, 2020

knorrli deleted the consider-only-active-rates-when-calculating-item-taxes branch October 13, 2020 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TaxHelpers#rates_for_item now respects the validity period of tax rates #3768

TaxHelpers#rates_for_item now respects the validity period of tax rates #3768

knorrli commented Sep 22, 2020 •

edited

Loading

knorrli commented Sep 22, 2020

aldesantis commented Sep 23, 2020

mamhoff commented Oct 9, 2020

knorrli commented Oct 9, 2020

aldesantis commented Oct 9, 2020 •

edited

Loading

mamhoff commented Oct 9, 2020

knorrli commented Oct 10, 2020

aldesantis left a comment

aldesantis left a comment

mamhoff left a comment

kennyadsl left a comment

knorrli commented Oct 13, 2020

TaxHelpers#rates_for_item now respects the validity period of tax rates #3768

TaxHelpers#rates_for_item now respects the validity period of tax rates #3768

Conversation

knorrli commented Sep 22, 2020 • edited Loading

knorrli commented Sep 22, 2020

aldesantis commented Sep 23, 2020

mamhoff commented Oct 9, 2020

knorrli commented Oct 9, 2020

aldesantis commented Oct 9, 2020 • edited Loading

mamhoff commented Oct 9, 2020

knorrli commented Oct 10, 2020

aldesantis left a comment

Choose a reason for hiding this comment

aldesantis left a comment

Choose a reason for hiding this comment

mamhoff left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

knorrli commented Oct 13, 2020

knorrli commented Sep 22, 2020 •

edited

Loading

aldesantis commented Oct 9, 2020 •

edited

Loading