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

Feature/acp regions #342

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Feature/acp regions #342

wants to merge 4 commits into from

Conversation

Levia
Copy link
Contributor

@Levia Levia commented Oct 23, 2019

Description

This is to amend the representation of biopama countries and replace the is_biopama bool with an acp_region string to also represent the ACP region countries belong to.
This means that countries with acp_region equal to nil are not ACP countries.

This PR specifically is about updating the CSV source and the importer.
Changes made on the db can be found on the related pp-db PR.

Notes

  • Related PP-API PR
  • Run importer: Wdpa::BiopamaCountriesImporter.import
  • Regions for ACP countries have been provided by JRC as those names do not match with already existing regions. This is also the reason why these regions have been currently added as a new field in the countries table. Another potential approach would have been doing a Single Table Inheritance with the region model so that, in the future, we could add different regions for different mappings. However, that requires important changes within the code which would need to be properly scheduled in.

Comment on lines 7 to 8
csv = CSV.read(BIOPAMA_COUNTRIES_CSV)
csv.shift # remove headers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these two lines can be removed now you are using the foreach at line 10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot about those. Fixed now. Thanks, @lucacug !

@lucacug
Copy link
Contributor

lucacug commented Oct 31, 2019

Happy now!!

@stacytalbot
Copy link
Contributor

@Levia @lucacug should this have been merged in?

@Levia
Copy link
Contributor Author

Levia commented Dec 10, 2019

@stacytalbot We were waiting for JRC to approve this before merging and going on production, but I think they never came back to us (or at least to me). Might be worth double checking internally with the rest of the team.

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.

3 participants