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

Country select preferred values doesn't use country key for option value #213

Closed
steveyken opened this issue Nov 10, 2012 · 7 comments
Closed
Labels

Comments

@steveyken
Copy link
Member

When specifing preferred_countries in a country_select box you end up with Argentina where the option value is the name of the country but this should be the ISO code as that is stored in the database for normal countries that are chosen.

@balazstar
Copy link
Contributor

Hi steveyken,

A few lines in country_select.rb would fix the issue that you described.
I have not yet created a pull request because I think this issue needs further discussion.

My solution would be:

    # Add iso code for countries 
    priority_countries_iso = COUNTRIES.select{|c| priority_countries.include?(c[0]) }

    # Add iso code for selected countries if not nil
    unless selected.nil?
        selected = COUNTRIES.select{|c| selected.include?(c[1]) }.flatten
    end

    country_options += options_for_select(priority_countries_iso, selected)

But sadly there are issues with the COUNTRIES array in country_select.rb so that lookup by country code would not work.

For example:
['Aland Islands', 'FI'],
['Finland', 'FI'],
Here the country code is the same, according to ISO 3166 AALAND ISLANDS
is misspelled and should have the code AX.

I think an update of COUNTRIES array is also needed.
If you agree with the direction I will gladly make the mentioned fixes.

@steveyken
Copy link
Member Author

Hi tarbalazs, thanks for offering a solution. You're thinking exactly along the same lines as I was so please go ahead and make the changes in a pull request. Thanks for spotting the country list errors. It should be brought up to date with the latest revision of ISO_3166: http://en.wikipedia.org/wiki/ISO_3166-1 (Note the list you linked to was an older revision and didn't contain newly formed countries such as South Sudan). Could you also add Kosovo (XK) as well? This is yet to be included in the ISO list but the European Commission and other bodies are using it with this code. (See http://geonames.wordpress.com/2010/03/08/xk-country-code-for-kosovo/ )

Another thought: the current code assumes a default list of 'preferred countries' if that option is not supplied to the function. I wonder if the default behaviour should be to show no preferred countries'. It would be good to have a setting in FFCRM to set the list of preferred countries in config/settings.yml rather than a hardcoded default lists.

If you feel like doing this second part as well, we'd welcome it. But otherwise, what you describe above is great too.

Thanks for offering to assist. We do appreciate it.

Regards,
Steve

@steveyken
Copy link
Member Author

A friend just noticed that the codes for Australia and Austria are switched/incorrect:

    ['Austria', 'AU']          should be            ['Austria', 'AT']
    ['Australia', 'AS']       should be            ['Australia', 'AU']

Looks like there are more (“AS” is actually the code for “American Samoa”)

Here’s some good web references:

http://countrycode.org/
http://en.wikipedia.org/wiki/ISO_3166-1

I think it would be worth checking all codes!

@balazstar
Copy link
Contributor

Thanks Steve,

I have also noticed, I will update it from Wikipedia.

I think a migration is also needed, so that old country codes will be updated in the database.
Do you agree?

Cheers,
Balázs

@steveyken
Copy link
Member Author

Excellent and you're right, we will need to update the data. I'm in two minds about a migration though... I would suggest a rake task instead? The migration would work fine now but suppose we change the model or methods later on. It might break the migration. Generally, I think it's better to put data manipulation into a rake task.

@balazstar
Copy link
Contributor

Hi Steve,

I think this issue can be closed.

@Tin-Nguyen
Copy link

hi all, how about Kosovo in countries list? have we added it yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants