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

[Datahub]: Handle OGC API in Downloads and Preview sections #865

Merged
merged 8 commits into from
May 6, 2024

Conversation

cmoinier
Copy link
Collaborator

@cmoinier cmoinier commented Apr 26, 2024

Thanks @Angi-Kinas for the pairing on this!

Description

This PR adds OGC API support in the downloads and preview section :

  • If any OGC API download link exists, it will be returned as a download item
  • Currently, it is not important is there are multiple links with the same type (ie: one GeoJson link for WFS & one GeoJson link for OGC)
  • If any OGC API download link of the right type exists (eg, geosjon), this link will be added to the dropdown lists of the three preview dropdowns.

Architectural changes

This depends on ogc-client version 1.1.0 (upgraded in the package).

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

Copy link
Contributor

github-actions bot commented Apr 26, 2024

Affected libs: feature-dataviz, feature-record, feature-router, util-shared, feature-catalog, feature-search, feature-map, ui-elements, feature-notifications, feature-editor, ui-catalog, ui-search, ui-widgets, ui-inputs, ui-layout,
Affected apps: datahub, metadata-editor, demo, webcomponents, search, metadata-converter, map-viewer, datafeeder,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@cmoinier cmoinier force-pushed the DH/ogc-api-support branch from 226896c to 579c1fd Compare April 26, 2024 06:32
@cmoinier cmoinier marked this pull request as ready for review April 26, 2024 06:32
Copy link
Contributor

github-actions bot commented Apr 26, 2024

📷 Screenshots are here!

@coveralls
Copy link

coveralls commented Apr 26, 2024

Coverage Status

coverage: 84.115% (+2.4%) from 81.762%
when pulling 3e2d3b0 on DH/ogc-api-support
into e07c560 on main.

Copy link
Collaborator

@tkohr tkohr left a comment

Choose a reason for hiding this comment

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

Thanks for the nice work @cmoinier ! I've left a couple of suggestions inline. Besides that I noticed some things when testing:

  • I had CORS issues when using the /dev-proxy? in our local dev compo (not sure, if this is expected)
  • The SCOT testing dataset seems to have an erroneous OGC API URL in the dump
  • What do you think of sorting the downloads by dataset instead of format? Not sure if this was specified anywhere? It would need to be discussed, but it seems more intuitive to me.

@cmoinier
Copy link
Collaborator Author

Thanks for the review @tkohr ! It was really helpful, and I hope my answers are easy to understand.
For these points :

* I had CORS issues when using the `/dev-proxy?` in our local dev compo (not sure, if this is expected)

I had CORS issues as well, unless I activated a CORS plugin. Not sure if this is what you meant.

* The SCOT testing dataset seems to have an erroneous OGC API URL in the dump

True, I knew it and forgot about it. I will update the dataset with a working URL.

* What do you think of sorting the downloads by dataset instead of format? Not sure if this was specified anywhere? It would need to be discussed, but it seems more intuitive to me.

Nothing was specified about it, but it was written in the ticket that duplicates should be left as is (ie if wfs and ogc both return the same download type) for now. So we assumed that the display would be figured out later, but Christophe said during the review that we shall meet to discuss it this week.

@cmoinier cmoinier force-pushed the DH/ogc-api-support branch 2 times, most recently from 9b3c8eb to 0044387 Compare April 30, 2024 16:02
@cmoinier cmoinier requested a review from tkohr April 30, 2024 16:11
Copy link
Collaborator

@tkohr tkohr left a comment

Choose a reason for hiding this comment

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

Thanks for the adaptions @cmoinier, especially in the dump!

Almost everything LGTM. The only thing I'm still concerned about, is that we get CORS errors using the /dev-proxy?. They occur on the API call to detect the proposed formats as well as on data fetching where the URLs are not "proxified".
It should not be a problem in the MEL use case where the API and client are on the same host, and we could also blame the host for not including a response header with a Access-Control-Allow-Origin: * (btw we should probably add the header to the MEL data API?), but I saw for WFS we proxify certain requests. @jahow what is your opinion on this?

Regarding the earlier comment about the sorting, I think we can rework this again in the future if necessary. It should not block this PR as the current sorting has already been present with multiple datasets before.

libs/feature/dataviz/src/lib/service/data.service.ts Outdated Show resolved Hide resolved
@jahow
Copy link
Collaborator

jahow commented May 2, 2024

@tkohr oh you're right, we're missing a "getProxiedUrl" for the OGC API endpoints I think, similar to WFS:

new WfsEndpoint(this.proxy.getProxiedUrl(wfsUrl)).isReady()

@jahow
Copy link
Collaborator

jahow commented May 3, 2024

@tkohr oh you're right, we're missing a "getProxiedUrl" for the OGC API endpoints I think, similar to WFS:

new WfsEndpoint(this.proxy.getProxiedUrl(wfsUrl)).isReady()

I looked into it more and using a proxy with OGC API will be more complicated than that, because ogc-client navigates through the url path by looking for "/". When the endpoint url is encoded this won't work at all.

Using a dev proxy for an OGC API endpoint will be more involved than that, I think this should be done in a separate PR.

@jahow jahow force-pushed the DH/ogc-api-support branch 3 times, most recently from 5ed600d to 2f3b391 Compare May 6, 2024 22:14
Also move all stubs to the root of the spec
@jahow jahow force-pushed the DH/ogc-api-support branch from 2f3b391 to 737aa49 Compare May 6, 2024 22:23
Copy link
Collaborator

@jahow jahow 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 looking very good, thanks for this awesome contribution! I've added stubs for the ogc api responses in the E2E tests, hopefully this will make the tests faster and more reliable.

@jahow
Copy link
Collaborator

jahow commented May 6, 2024

Oh, also I left out the changes on the dump and everything works fine 👌

@jahow jahow merged commit 287bf00 into main May 6, 2024
9 checks passed
@jahow jahow deleted the DH/ogc-api-support branch May 6, 2024 22:34
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.

4 participants