Skip to content
This repository has been archived by the owner on Jul 18, 2023. It is now read-only.

[TAAS-18] Add countries support #14

Closed
wants to merge 3 commits into from
Closed

[TAAS-18] Add countries support #14

wants to merge 3 commits into from

Conversation

pjf
Copy link
Contributor

@pjf pjf commented Mar 24, 2017

This supports a countries endpoint. Sample output:

{
    "id": "2", 
    "iso2": "AX", 
    "iso3": "ALA", 
    "label": {
        "default": "\u00c5land Islands", 
        "hrinfo": "\u00c5land Islands", 
        "m49": "\u00c5land Islands", 
        "reliefweb-name": "\u00c5land Islands", 
        "reliefweb-web": "Aland Islands (Finland)"
    }, 
    "m49": "248"
}

This includes code from PR #13.

Things to note:

  • b3cdfe9 is the config change. Everything else is from PR Add link and multimap support #13 which we need to run. Please just comment on b3cdfe9 here.
  • I've taken exception to the ISO-2 code being called pcode. It's now iso2.
  • The label field is now a map. label.default is the preferred term, but one can now select which labelling scheme to use.
  • This is allegedly a countries sheet, but contains a number of territories and regions, like American Samoa and the US Virgin Islands. I'd like confirmation that the data we're using is indeed what we want to publish.
  • We're no longer publishing lat/long, as this is controversial.
  • I'm not publishing the old HRinfo ID, but can if desired.
  • I'm not publishing the Admin Level, since it should always be zero for countries. In addition, territories and other regions in this sheet are mmissing an admin level, and it feels like this should not be an optional field.

Feedback from review:

Required:

  • Change label.hrinfo to label.humanitarianresponse.
  • Add in lat/long.

Suggested:

  • Publish old HRinfo ID
  • Publish admin level

ToDo

@pjf pjf requested review from guillaumev and emmajane March 24, 2017 03:44
@emmajane
Copy link
Contributor

emmajane commented Mar 24, 2017

To change:

Opinions:

  • I think we should publish the old HR.info ID so that HR.info can consume this data back in if needed. Open to additional feedback from @attiks.
  • pcode is the term that's used by OCHA internally. I would be hesitant to change this. cc @andrejverity
  • I would publish Admin Level 0 and then leave it blank (or an explicit null?) where it's not zero so that it can be filtered on. @andrejverity and @guillaumev will have a better sense of how this Admin Level gets used though.

@andrejverity
Copy link
Member

@emmajane I looked at the GSS, and I can't see any Pcode mention. Just ISO2 so I am not sure where the pcode came into the equation? Or where am I missing it.

Regardless, since we are talking about countries, I am happy to default to global standards (i.e. iso2) and label it that way. Null values are okay if the country does not exist in ISO2.

Not sure if Helen would have any info on why p-code would have been included in an Admin 0 / Country taxonomy.

@emmajane
Copy link
Contributor

@andrejverity thanks for double checking. I think we're good on this front and I am simply mistaken about what the headings were previously.

@pjf
Copy link
Contributor Author

pjf commented Mar 25, 2017

Thanks everyone! I've updated the top-post with the results of review, and have a few changes to make before this is to be merged.

pjf added a commit that referenced this pull request Mar 27, 2017
For our `countries.json` output:

- Added `hrinfo_id`
- Added `admin_level`
- Added `geolocation` (lat/lon)
- Tweaked `label.hrinfo` → `label.humanitarianresponse`

These were based upon the feedback in PR #14
pjf added a commit that referenced this pull request Mar 27, 2017
For our `countries.json` output:

- Added `hrinfo_id`
- Added `admin_level`
- Added `geolocation` (lat/lon)
- Tweaked `label.hrinfo` → `label.humanitarianresponse`

These were based upon the feedback in PR #14
@pjf
Copy link
Contributor Author

pjf commented Mar 27, 2017

Have updated with 47708ad which incorporates al the feedback from this review. Records now look like this:

        "admin_level": "0", 
        "geolocation": {
            "lat": "33.83147477", 
            "lon": "66.02621828"
        }, 
        "hrinfo_id": "181", 
        "id": "1", 
        "iso2": "AF", 
        "iso3": "AFG", 
        "label": {
            "default": "Afghanistan", 
            "humanitarianresponse": "Afghanistan", 
            "m49": "Afghanistan", 
            "reliefweb-name": "Afghanistan", 
            "reliefweb-web": "Afghanistan"
        }, 
        "m49": "4"
    } 

And here's one with lots of nulls:

    {
        "admin_level": null, 
        "geolocation": {
            "lat": null, 
            "lon": null
        }, 
        "hrinfo_id": null, 
        "id": "256", 
        "iso2": null, 
        "iso3": null, 
        "label": {
            "default": "Sark", 
            "humanitarianresponse": "Sark", 
            "m49": "Sark", 
            "reliefweb-name": "Sark", 
            "reliefweb-web": "Sark"
        }, 
        "m49": "680"
    }

Things to note:

  • We use lat/long nested under geolocation because the existing locations API does that, and I presume we want backwards compatibility where possible.
  • I'm not sure of hrinfo_id as a label, but humanitarianresponse_id feels unwieldy. Can totally change it if desired.

I'm happy for this to be merged, although it would be lovely to have PR #13 (the code it's based on) reviewed first.

@andrejverity
Copy link
Member

andrejverity commented Mar 27, 2017 via email

@pjf
Copy link
Contributor Author

pjf commented Mar 27, 2017

@andrejverity :

Question: what is the difference between reliefweb-name and reliefweb-web?

That's a fantastic question! I believe Helen knows the answer, but I do not. The spreadsheet has separate "Name" and "Website" terms for ReliefWeb (columns M and N), and I'm not sure of the difference between them either.

Question 2: why do we have HR.info ID, but not for the others?

We have the HR.info ID because @emmajane requested it. I can't export the ID for other services as the spreadsheet doesn't contain them.

@pjf pjf mentioned this pull request Mar 29, 2017
pjf added 3 commits March 31, 2017 00:09
This establishes a countries endpoint that provides alternate terms and
ISO 2 and 3 letter codes.
For our `countries.json` output:

- Added `hrinfo_id`
- Added `admin_level`
- Added `geolocation` (lat/lon)
- Tweaked `label.hrinfo` → `label.humanitarianresponse`

These were based upon the feedback in PR #14
@pjf
Copy link
Contributor Author

pjf commented Mar 30, 2017

With #13 merged this is now a config-only change.

@pjf
Copy link
Contributor Author

pjf commented Mar 30, 2017

This has further discussions on JIRA I need to review before we can merge.

@andrejverity
Copy link
Member

Added clarifying questions on JIRA for Helen

@pjf
Copy link
Contributor Author

pjf commented Apr 1, 2017

After the discussions on JIRA I'm closing this PR in favour of a more minimal version. We'll add additional functionality in as it gets passed discussion and we have consensus on what should be exported and how it's presented.

@pjf pjf closed this Apr 1, 2017
@pjf pjf deleted the taas-18 branch April 1, 2017 11:34
@pjf pjf restored the taas-18 branch April 1, 2017 11:35
@pjf pjf deleted the taas-18 branch April 6, 2017 11:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants