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

Adding capability for CSV + GeoJSON download of composite data; COUNTRY=jordan #1339

Conversation

gislawill
Copy link
Collaborator

Description

This addresses #1336 by adding support for CSV + GeoJSON files of CDI data.

How to test the feature:

  • Open Jordan surge deployment
  • Enable a CDI layer
  • Select the download icon and download in both CSV and GeoJSON

Checklist - did you ...

Test your changes with

  • REACT_APP_COUNTRY=rbd yarn start
  • REACT_APP_COUNTRY=cambodia yarn start
  • REACT_APP_COUNTRY=mozambique yarn start
  • Add / update necessary tests?
  • Add / update outdated documentation?

Screenshot/video of feature:

https://www.loom.com/share/a3c8cc4955a44112a0b3ed4983bbdf7b

Copy link

github-actions bot commented Sep 11, 2024

Build succeeded and deployed at https://prism-1339.surge.sh
(hash b23d219 deployed at 2024-09-13T21:58:08)

@gislawill
Copy link
Collaborator Author

"build and test api" is failing due to an unrelated outage. It should pass when we rerun in a couple hours

@gislawill
Copy link
Collaborator Author

Heads up, @wadhwamatic, geojson downloads are fixed in d1295b3

Copy link
Member

@wadhwamatic wadhwamatic left a comment

Choose a reason for hiding this comment

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

this looks good from my side now, thanks @gislawill

@ericboucher - can you take a quick look?

Copy link
Collaborator

@ericboucher ericboucher 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!

Two small recommendations:

  • Enhance typing around layer.type to make the code and button activation a bit cleaner
  • Update getFilename to be more informative and contain country name + selected date for more layer types (composite in particular)

@gislawill
Copy link
Collaborator Author

Thanks for the review @ericboucher! I've updated the file name and done a small simplification of the layer types (all within LayerDownloadOptions.tsx). Could you say more on what you're thinking for the typing around layer.type? My mind jumps to using enums but not sure that'll be significantly more manageable.

Two small recommendations:

  • Enhance typing around layer.type to make the code and button activation a bit cleaner
  • Update getFilename to be more informative and contain country name + selected date for more layer types (composite in particular)

@gislawill gislawill force-pushed the 1336-feature-request-allow-users-to-download-cdi-q-multi-results branch from 655cced to b23d219 Compare September 13, 2024 21:51
@gislawill
Copy link
Collaborator Author

gislawill commented Sep 13, 2024

Hey @valpesendorfer, we've got some python tests that are hitting this service: https://api.earthobservation.vam.wfp.org/ows/?service=WCS&request=GetCoverage&....

We're seeing 500 responses which is causing our tests to fail. It appears to have been happening for a couple weeks. Any idea what's going on?

A side note, we're in the process of moving away from hitting these endpoints in our tests (using VCR.py to intercept requests and use stored responses) — we'll need the correct 200 response to store first of course.

=================================== FAILURES ===================================
_____________________________ test_stats_endpoint1 _____________________________

    def test_stats_endpoint1():
        """
        Call /stats with known-good parameters.
        This endpoint can be slow (>1 min) so this test is deactivated by default.
        """
        response = client.post(
            "/stats",
            headers={"Accept": "application/json"},
            json={
                "geotiff_url": "https://api.earthobservation.vam.wfp.org/ows/?service=WCS&request=GetCoverage&version=2.0.0&coverageId=wp_pop_cicunadj&subset=Long([95](https://github.com/WFP-VAM/prism-app/actions/runs/10856592733/job/30131528361?pr=1339#step:5:96).71,[96](https://github.com/WFP-VAM/prism-app/actions/runs/10856592733/job/30131528361?pr=1339#step:5:97).68)&subset=Lat(19.42,20.33)",
                "zones_url": "https://prism-admin-boundaries.s3.us-east-2.amazonaws.com/mmr_admin_boundaries.json",
                "group_by": "TS_PCODE",
                # TODO - re-add once geonode is back online.
                # "wfs_params": {
                #     "url": "https://geonode.wfp.org/geoserver/ows",
                #     "layer_name": "mmr_gdacs_buffers",
                #     "time": "2022-10-24",
                #     "key": "label",
                # },
                "geojson_out": True,
            },
        )
>       assert response.status_code == 200
E       assert 500 == 200
E        +  where 500 = <Response [500 Internal Server Error]>.status_code

tests/test_api.py:122: AssertionError
------------------------------ Captured log call -------------------------------
ERROR    app.caching:caching.py:88 500 Server Error: Internal Server Error for url: https://api.earthobservation.vam.wfp.org/ows/?service=WCS&request=GetCoverage&version=2.0.0&coverageId=wp_pop_cicunadj&subset=Long(95.71,96.68)&subset=Lat(19.42,20.33)

@wadhwamatic
Copy link
Member

gtg, thanks @gislawill!

@gislawill
Copy link
Collaborator Author

I'll merge this despite the failed test as we know it's unrelated. @ericboucher, I'm happy to make any updates to the layer typing in a follow up pr

@gislawill gislawill merged commit 326be39 into master Sep 13, 2024
5 of 6 checks passed
@gislawill gislawill deleted the 1336-feature-request-allow-users-to-download-cdi-q-multi-results branch September 13, 2024 22:24
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.

[Feature Request]: Allow users to download CDI / q-multi results
3 participants