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

Make cart taxes configurable via Spree::Store object #933

Merged
merged 8 commits into from Mar 16, 2016
Merged

Make cart taxes configurable via Spree::Store object #933

merged 8 commits into from Mar 16, 2016

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Feb 29, 2016

In VAT countries, carts (orders without addresses) have to be shown with
adjustments for the country whose taxes the cart's prices supposedly include.
This might differ from Spree::Store to Spree::Store. We're introducting
the cart_tax_country_iso setting on Spree::Store for this purpose.

Previously the setting for what country any prices include
Spree::Zone.default_tax. That, however, would also implicitly tag all
prices in Spree as including the taxes from that zone. Introducing the cart
tax setting on Spree::Store relieves that boolean of some of its
responsibilities.

This PR also introduces the Spree::TaxRate.for_address scope. It's not fully being used here yet, but that's coming up later.

end

context 'with a state object' do
let(:args) { {state: state} }

Choose a reason for hiding this comment

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

Space inside { missing.
Space inside } missing.

@mamhoff mamhoff changed the title Tax by address instead of by zone #2 WIP Tax by address instead of by zone #2 Mar 4, 2016
@mamhoff mamhoff changed the title WIP Tax by address instead of by zone #2 Make cart taxes configurable via Spree::Store object Mar 10, 2016
@mamhoff
Copy link
Contributor Author

mamhoff commented Mar 10, 2016

Added changelog entry, rebased on current master. Ready for review!

@@ -23,6 +23,12 @@ def self.default
where(default: true).first || new
end

def default_cart_tax_location
@default_cart_tax_location ||= begin
Copy link
Member

Choose a reason for hiding this comment

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

Why begin?

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 thought because of the multiline expression. But - having written code that doesn't need it, I guess it can go here as well.

mamhoff and others added 8 commits March 14, 2016 16:03
This class is intended to behave like an address when passed
into Spree::Zone.for_address, but not have any of the complex behaviour
of Spree::Address.
This scope finds all relevant tax rates for an address in one SQL query.
Instead of setting instance-wide whether orders in cart state should
be taxes using some default country, this lays the groundwork for making
that setting only store-wide.
In order to configure how each store within the same Solidus instance
handles a cart without a tax address, this method uses the
`cart_tax_country_iso` setting to create a Spree::Tax::TaxLocation with
that country.
Yet another step in removing the default tax zone: When the order does not
have a tax_address, refer to the store's default cart tax location.

This required quite a few spec setup tweaks as it's the first time
I'm actually using the mandatory store on an order, , but nothing grave.
This configuration setting, which is valid for the entire Solidus
store instance, will tell Solidus which VATs are included in prices
entered in the backend.

I am splitting up the meaning of Spree::Zone.default_tax here, because
that setting conflates two questions:
1. Are we entering backend prices including VAT?
2. Should carts without a tax address be taxed?

The second setting should be `Spree::Store`-specific.
The current default behaviour in Solidus is one setting for two
behaviour changes: What taxes are included in the prices in the backend,
and how carts without a shipping address are taxed.

While doing the work in separating the two settings, it does make sense for
most shops to have both be equal. This just sets the `cart_tax_country_iso`
for a new store to be equal to the Spree::Config for the backend prices.
@jhawthorn
Copy link
Contributor

Looks reasonable to me 👍

@gmacdougall
Copy link
Member

👍

gmacdougall added a commit that referenced this pull request Mar 16, 2016
Make cart taxes configurable via `Spree::Store` object
@gmacdougall gmacdougall merged commit 6483070 into solidusio:master Mar 16, 2016
@mamhoff mamhoff mentioned this pull request Mar 16, 2016
23 tasks
@mamhoff mamhoff deleted the new_default_tax_address 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