Find the default country by ISO code #810
Conversation
| # @return [Integer,nil] id of {Country} to be selected by default in dropdowns (default: nil) | ||
| preference :default_country_id, :integer | ||
|
|
||
| # @!attribute [rw] default_country_iso | ||
| # @deprecated | ||
| # @return [Integer,nil] id of {Country} to be selected by default in dropdowns (default: nil) |
There was a problem hiding this comment.
The comment says default nil, but the code 'us'
There was a problem hiding this comment.
Yep, you're right. The comment also says deprecated, which I don't intend
it to be. I'll force push once the build is through.
Am 07.02.2016 19:28 schrieb "Thomas von Deyen" [email protected]:
In core/app/models/spree/app_configuration.rb
#810 (comment):# @return [Integer,nil] id of {Country} to be selected by default in dropdowns (default: nil) preference :default_country_id, :integer
@!attribute [rw] default_country_iso
@deprecated
@return [Integer,nil] id of {Country} to be selected by default in dropdowns (default: nil)
The comment says default nil, but the code 'us'
—
Reply to this email directly or view it on GitHub
https://github.com/solidusio/solidus/pull/810/files#r52121680.
|
👍 |
|
This ties in with the taxation stuff, as using the default country I can bypass the whole idea of a default tax zone. Upcoming: A default state that behaves just like the default country. |
|
This breaks the existing |
|
Thanks for re-adding that functionality Martin. This is 👍 from me now. |
| end || first | ||
| if Spree::Config.default_country_id | ||
| ActiveSupport::Deprecation.warn("Setting your default country via its ID is deprecated. Please set your default country via the `default_country_iso` setting.", caller) | ||
| find_by(id: Spree::Config.default_country_id) || find_by!(iso: Spree::Config.default_country_iso) |
There was a problem hiding this comment.
I don't understand why you want to load the country by iso if the find by id returns nil
There was a problem hiding this comment.
So the scenario of find by ID returning nil is essentially a misconfigured shop. Previously, in that case it would return first, i.e. any country. I find it more fitting to actually return something sensible, and that would be the configured country by iso.
It's supposed to be nudging people into configuring their shops correctly. Apart from the address form and (newly) taxes, the default country is also used for stock locations, and should make sense there, too.
|
I think Don't know what do you think? |
|
Hm, isn't the country code for Germany |
Right now, the default country for address drop downs is defined using that addresses `id`, which is error-prone and makes Solidus' startup depend on the database. This commit adds a preference for the country's ISO code, which is more understandable and does not depend on the database.
Also, raise an ActiveRecord::RecordNotFound if the default country is not present. In this case, a number of core functionalities (especially Customer returns) will not work as expected.
The only configuration value actually emitted via the API is the default country id. This commit adds another value to be emitted, the default country ISO, and feeds that from the actual default country found at runtime.
Before, this would rely on the default country id. Now it uses the default country ISO.
By default, preferences are not stored anyway, and with the default country ISO code present, we do not need to first find the US and then store the ID.
Instead of the first country found, please raise an error if the default country ISO code does not have an associated country. The issue referenced in the spec, spree/spree#1142, is people complaining about no states found for Nil, which only ever happened because Spree::Country.default would silently fail before.
Also, make sure the countries created actually have correct data by providing the two-letter ISO code to the factories.
Previously, this PR broke existing shops with a default country ID with no Warning. With this commit, we get a warning, and still load the default country by ID. The fallback will do exactly what the default code is doing.
|
Rebased on current master to remove conflicts. |
|
I think this is awesome. I've always been embarrassed by the default_country_id and think this is an awesome change excellently executed. |
|
I also think |
Find the default country by ISO code
|
Thanks again! |
...so that we don't have to know database IDs on boot-up time.