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

Only check once whether we're in a non-default non-order zone #913

Merged
merged 1 commit into from Mar 3, 2016
Merged

Only check once whether we're in a non-default non-order zone #913

merged 1 commit into from Mar 3, 2016

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Feb 26, 2016

When calling Spree::TaxRate#adjust, Spree::TaxRate#compute_amount will
come up with negative amount only if we're processing a refund. This commit
checks for the amount of the reimbursement to be negative instead of running
a rather expensive call to Spree::Zone.contains.

When calling Spree::TaxRate#adjust, Spree::TaxRate#compute_amount will
come up with negative amount only if we're processing a refund. This commit
checks for the amount of the reimbursement to be negative instead of running
a rather expensive call to Spree::Zone.contains.
@cbrunsdon
Copy link
Contributor

👍, its slightly unclear why that check is happening to anyone not deeply familiar with this chunk of code, but as we are (lols, "we") working on removing it entirely I'm cool with it

@jhawthorn
Copy link
Contributor

Took me a minute to fully understand. The compute_amount method is doing the same check, and setting the amount to negative only if included_in_price && !default_zone_or_zone_match. So if included_in_price && amount > 0, we know that default_zone_or_zone_match is truthy.

👍

gmacdougall added a commit that referenced this pull request Mar 3, 2016
…usting

Only check once whether we're in a non-default non-order zone
@gmacdougall gmacdougall merged commit 817d60c into solidusio:master Mar 3, 2016
@mamhoff mamhoff mentioned this pull request Mar 16, 2016
23 tasks
@mamhoff mamhoff deleted the only-lookup-zone-once-when-adjusting branch May 24, 2016 19:46
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