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

[FF] Support .geojson exports on Case Export page #34000

Merged
merged 15 commits into from
Feb 14, 2024

Conversation

Charl1996
Copy link
Contributor

@Charl1996 Charl1996 commented Jan 22, 2024

Product Description

Demo video

A new export file type has been added to the case export page (sitting behind a FF), exporting case data to a .geojson file. The export file structure follows the standard geojson file structure. This exported file is intended to be imported into any GIS tool, for example ArcGIS.

The geojson file consists of features where each feature has a "geometry" section and a "properties" section. HQ will thus need to know which exported properties will define this "geometry" section (the rest will be assumed to be the "properties" of the geometry feature). The geo-property is determined by looking through the Data dictionary for the specific case type that is being exported and searching for the property with data type set to GPS (this feature depends on the CASE_IMPORT_DATA_DICTIONARY_VALIDATION feature flag).

Example of geo json export configuration:
image

If multiple geo-properties are detected, the user can use the Select geo property select input to specify which geo-property's coordinates will be used in the .geojson file to define the GPS point.

Example

Cases on HQ with "person" case type:
image

Imported "person" cases as visualized using ArcGIS Online.
image

Technical Summary

Ticket

A new export writer has been created, GeoJSONWriter, which writes to a .geojson file. The writer looks at the configured selected_geo_property value (a new field on the ExportWriter class) to determine which property should define the geometry section of each feature in the geojson file (see example of geojson file). All other export columns is considered to be properties of the feature(s).

Note: the exported .geojson file contains one feature collection which corresponds to one exported table, although this loop iterates over potentially multiple tables; I'm not 100% sure in what context multiple tables would be configured, so any input there might be helpful.

Feature Flag

New feature flag (SUPPORT_GEO_JSON_EXPORT)

Safety Assurance

Safety story

Tested locally and used the HQ generated .geojson file to import it into ArcGIS Online successfully.

Automated test coverage

Added some unit tests for the new export writer.

QA Plan

QA planned. Ticket to be linked.

Migrations

There's no migrations, but a new field is added to the ExportWriter class. No data migration necessary as this feature is completely new ground.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@Charl1996 Charl1996 added awaiting QA QA in progress. Do not merge product/feature-flag Change will only affect users who have a specific feature flag enabled labels Jan 22, 2024
@Charl1996 Charl1996 changed the title Cs/sc 3242 export geojson [FF] Support .geojson exports on Case Export page Jan 22, 2024
Copy link
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

Overall looks good, I've left a few comments.

I'm curious to hear if you did any testing around case exports containing repeat groups?

corehq/apps/export/views/new.py Show resolved Hide resolved
corehq/ex-submodules/couchexport/writers.py Show resolved Hide resolved
corehq/apps/export/views/new.py Outdated Show resolved Hide resolved
corehq/apps/export/views/new.py Show resolved Hide resolved
corehq/apps/export/views/new.py Outdated Show resolved Hide resolved

def _close(self):
feature_collections = []
for table, data in self.tables.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

We loop through each table in the export, however a few lines below we only grab the first table's features. Do we require this loop if we're only interested in the first table?

Copy link
Contributor

@SmittieC SmittieC left a comment

Choose a reason for hiding this comment

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

Looks good to me once Zandre's comments have been addressed

Copy link
Contributor

@kaapstorm kaapstorm left a comment

Choose a reason for hiding this comment

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

This is great. Just the one super nitpicky spelling thing.

corehq/toggles/__init__.py Outdated Show resolved Hide resolved
corehq/toggles/__init__.py Outdated Show resolved Hide resolved
@Charl1996 Charl1996 requested a review from mkangia January 30, 2024 09:50
@mkangia
Copy link
Contributor

mkangia commented Jan 30, 2024

@Charl1996
thanks for tagging me on this. I see you already have a solid review from Zandre and Norman.
Considering this is a big one, I will skip going through it, let me know if you suggest I should take a pass.

@Charl1996 Charl1996 added QA Passed and removed awaiting QA QA in progress. Do not merge labels Feb 12, 2024
Copy link
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

Looks good. No blocking feedback from my side.

Copy link
Contributor

@SmittieC SmittieC left a comment

Choose a reason for hiding this comment

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

A test is failing. Otherwise looks good

@@ -582,7 +582,7 @@ def get_features(self, table, data):
result = row[geo_data_index].split(" ")
lat = result[0]
lng = result[1]
except ValueError:
except IndexError:
Copy link
Contributor Author

@Charl1996 Charl1996 Feb 13, 2024

Choose a reason for hiding this comment

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

The IndexError will occur if the row[geo_data_index] value is not correct (not in a form like "lat lng ..."), hence safe to ignore. Also, ValueErrors are not expected anymore.

@Charl1996 Charl1996 merged commit d541a90 into master Feb 14, 2024
13 checks passed
@Charl1996 Charl1996 deleted the cs/SC-3242-export-geojson branch March 11, 2024 10:19
@Charl1996 Charl1996 mentioned this pull request Mar 14, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled QA Passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants