Make the default pricer country-aware #1125
Conversation
mamhoff
changed the title
| @@ -25,6 +25,10 @@ def ==(other) | |||
| state_id == other.state_id && country_id == other.country_id | |||
| end | |||
|
|
|||
| def country | |||
| Spree::Country.find_by(id: country_id) | |||
There was a problem hiding this comment.
Should we consider memoizing this?
There was a problem hiding this comment.
I wouldn't do it. The Spree::Tax::TaxLocation is usually a pretty throwaway object. The big reason why we need this method is that countries are referenced by ID rather that ISO code (in which case I could just use that and not use a DB lookup).
Also: This query can return nil, in which case memoizing won't help.
mamhoff
changed the title
| private | ||
|
|
||
| def sum_of_included_vats | ||
| return 0 unless variant.tax_category | ||
| variant.tax_category.tax_rates.for_address( |
There was a problem hiding this comment.
has_many :tax_rates
has_many :vat_rates, -> { where(included_in_price: true) }
The relation can be nil and is validated to actually lead to a country if it's there.
In order to find the country_iso of an order or line item when the order is still in cart state and doesn't have a real address, we need the tax location object to be able to give back a real activerecord country. This commit accomplishes that.
We need to add a country_iso attribute to the default pricing options. However, once we do that, we also need to teach the pricing options class to do a fallback lookup if it can't find the country for the price in question.
The default rate is there in order to calculate net prices for things. We won't need that going forward as we will have prices for all the things.
This method will return an unrounded net amount for a price.
This adds a price generator class that generates all necessary prices for a variant. It infers the prices from the `default_price` and the VAT rates' zones. This also introduces a price migrator class that will do the right thing for all variants.
This just runs the PriceGenerator.
..so that we don't repeat the same ugly private method twice.
Before, the price of a variant would be set in the validation stage. This is not ideal as we want variants to be essentially valid once they get there. This commit separates the price setting and the validation.
This adds a country field to the form for new prices, allows searching by country, and adds a country column to the prices index table. The country field is disabled for persisted records, as with the currency.
This price generator is only necessary for shops with VAT. This name change reflects this coupling. Also added an early return to show that the thing does nothing when there are no VATs. Rename Pricer to PriceSelector "To price" can be understood as "setting a price" or "selecting a price" "Selecting a price" is much closer to what we actually want here.
This does a number of small things: 1. Do not display the `is_default` boolean. We don't know what its for. 2. Left-align some of the longer text fields in the prices table. 3. Sort by variant and country_iso by default.
Rather than using a hard-to-understand callback that would always create prices whenever you update the backend price, this commit introduces a checkbox in both the variant and product form that will update VAT prices for that product if you tick the checkbox. The only exception to this is new products/variants, which will always run the VatPriceGenerator before validation.
When setting up a new product, we need the tax category to generate all the necessary prices. Without a form field, that's impossible.
|
It's done - pending positive code reviews. This PR finally generates correct prices including VAT that can be changed by a backend user, and gets rid of the default zone. Much of the changed lines are specs for migrating an existing VAT store and class renames. Most notable is the class The interface for prices is still a bit rough around the edges, as #1167 testifies. For now, I believe it's absolutely usable, though. |
By adding a scope to variants that is included in the searcher class and having the `PriceSelector` select the price in Ruby, we avoid an n+1 on every un-cached product query.
|
Quick warning from your friendly neighborhood core team! We're okay with this now in the "We can't see anything wrong or can suggest any better ways to do any of it", so unless we get any kind of feedback in the next 24 hours we're going to merge it! |
|
🎉 💥 |
|
Cheers on this milestone PR! |
| { currency: Spree::Config.currency } | ||
| { | ||
| currency: Spree::Config.currency, | ||
| country_iso: Spree::Config.admin_vat_country_iso |
There was a problem hiding this comment.
why the admin_vat_country_iso is used here, rather than default_country_iso?
There was a problem hiding this comment.
Because when you create a new price for an item, you're most likely doing that in the backend as an admin, and you'll expect the new price to be with the VAT the other prices in the admin area. The default country iso is the one used in the frontend.
| # nil is added to the array so we always have an export price. | ||
| def country_isos_requiring_price | ||
| return [nil] unless variant.tax_category | ||
| [nil] + variant_vat_rates.map(&:zone).flat_map(&:countries).flat_map(&:iso) |
There was a problem hiding this comment.
why do we need this [nil] (for all country)? What's an export price?
There was a problem hiding this comment.
An export price is a price without VAT, as in countries with VAT, if you export to a country where no VAT applies, you have to change the price to exclude VAT. We add nil because a price with nil is used for shipping to any country we have no information about. Otherwise we'd need to create a price for each country, which is redundant and tedious to work with.
This is an addition to the standard pricer class to query for a specified country price first, and then query for a fallback price.