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

Adds Download GeoJSON button #2138

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rfontanarosa
Copy link
Collaborator

@rfontanarosa rfontanarosa commented Feb 4, 2025

closes #2013

Registrazione.schermo.2025-02-04.alle.17.14.04.mov
Screenshot 2025-02-04 alle 17 15 04

@rfontanarosa rfontanarosa self-assigned this Feb 4, 2025
@rfontanarosa rfontanarosa marked this pull request as ready for review February 4, 2025 16:15
@auto-assign auto-assign bot requested a review from nwkotto February 4, 2025 16:16
@rfontanarosa rfontanarosa requested review from gino-m and removed request for nwkotto February 4, 2025 16:16
functions/src/export-geojson.spec.ts Outdated Show resolved Hide resolved
functions/src/export-geojson.ts Outdated Show resolved Hide resolved
functions/src/export-geojson.ts Outdated Show resolved Hide resolved
res.status(HttpStatus.FORBIDDEN).send('Permission denied');
return;
}
const ownerId = canImport(user, surveyDoc) ? undefined : userId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this logic be reversed? If canImport is true, the ownerId is the userId? Or is this the correct logic? Maybe a comment would help clarify, if this is right.

functions/src/export-geojson.ts Show resolved Hide resolved
functions/src/export-geojson.ts Outdated Show resolved Hide resolved
/**
* Iterates over all LOIsin a job returning a valid GeoJSON file.
*/
export async function exportGeojsonHandler(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite a long function. This is a nit, but any way this could be split up into useful chunks of logic? e.g. error checking, writing rows, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a fair few things from the function that are not tested here, especially error cases. I understand not testing everything, but anything that's especially important that you think we should test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After investigating the export-csv test logic, I found that the jsonstream library uses a queue for writing to the stream and I didn't find a way to capture this.
Maybe we can change the mocking strategy, so instead of mocking the entire response, I'll likely get better results by focusing my tests on the transformation of he LOI data into GeoJSON features.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wdyt @nwkotto ?

@rfontanarosa rfontanarosa requested a review from nwkotto February 6, 2025 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Export job data] Allow data to be exported as GeoJSON
2 participants