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

dev/core#2027 Add/update to UK county list #18470

Merged
merged 1 commit into from
Sep 16, 2020

Conversation

JKingsnorth
Copy link
Contributor

@civibot
Copy link

civibot bot commented Sep 14, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor

This doesn't need a test IHMO - it's mostly checking the changes seem right.

@seamuslee001
Copy link
Contributor

@MikeyMJCO does this look right to you?

@seamuslee001
Copy link
Contributor

Test fails relate and also @JKingsnorth should there be an update statement for the ones you are correcting rather than just inserting?

@eileenmcnaughton
Copy link
Contributor

I think the previous line is missing a semi-colon, causing the test fails

@JKingsnorth
Copy link
Contributor Author

@seamuslee001 the reason for not doing an update/delete on the changed ones is explained here: https://lab.civicrm.org/dev/core/-/issues/2027 (they're not geographically equivalent).

But really the 'renamed' ones should be added as part of the update still - so I'll make that change.

@JKingsnorth JKingsnorth changed the title dev/core#2027 Add/update to UK county list WIP dev/core#2027 Add/update to UK county list Sep 15, 2020
@JKingsnorth JKingsnorth changed the title WIP dev/core#2027 Add/update to UK county list dev/core#2027 Add/update to UK county list Sep 15, 2020
@JKingsnorth
Copy link
Contributor Author

Added the renamed counties as part of the upgrade script. Please see https://lab.civicrm.org/dev/core/-/issues/2027 for comments and discussion.

@eileenmcnaughton
Copy link
Contributor

@MikeyMJCO if you give this the thumbs up I will merge....

@homotechsual
Copy link
Contributor

I do :-)

@seamuslee001
Copy link
Contributor

@JKingsnorth can you rebase this please

@homotechsual
Copy link
Contributor

I have one very minor gripe, Rhondda, Cynon, Taff the official usage is Rhondda Cynon Taf though I believe the comma separated non-Welsh taff spelling is the ISO one. I would prefer the correct/official usage. https://www.rctcbc.gov.uk/

@eileenmcnaughton
Copy link
Contributor

argh @JKingsnorth it's stale again - sorry!

@JKingsnorth
Copy link
Contributor Author

JKingsnorth commented Sep 16, 2020

@eileenmcnaughton - Rebased! (pesky civicrm_generated.mysql...)

@MikeyMJCO - going back to what we were talking about 'standardising on the ISO list', yes it has the commas in the ISO list (https://www.iso.org/obp/ui/#iso:code:3166:GB) and it even has the commas in the 'Welsh language' version it supplies on that list as well, which is probably wrong!

I think, as with other areas, we should leave it with the ISO values to keep consistency? Even though this is counter-intuitive, when people actually know better than the standard.

(And maybe write to ISO and get them to change their standard :P)

p.s. I've learnt my lesson not to touch state_province in core ever again, there are bigger fish to fry :P

@homotechsual
Copy link
Contributor

@eileenmcnaughton - Rebased! (pesky civicrm_generated.mysql...)

@MikeyMJCO - going back to what we were talking about 'standardising on the ISO list', yes it has the commas in the ISO list (https://www.iso.org/obp/ui/#iso:code:3166:GB) and it even has the commas in the 'Welsh language' version it supplies on that list as well, which is probably wrong!

I think, as with other areas, we should leave it with the ISO values to keep consistency? (And maybe write to ISO and get them to change their standard :P)

Sure, I'll poke the Data Management team at Welsh Government and get it on their radar.

@eileenmcnaughton eileenmcnaughton merged commit 907b9f2 into civicrm:master Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants