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: OGC API fixes #872

Merged
merged 5 commits into from
May 17, 2024
Merged

Datahub: OGC API fixes #872

merged 5 commits into from
May 17, 2024

Conversation

tkohr
Copy link
Collaborator

@tkohr tkohr commented May 7, 2024

Description

This PR applies a couple of fixes for the OGC API integration into the datahub:

  • Prevent the download list from breaking when WFS and OGC API links are present and the latter do not succeed due to CORS => visible in the SCOT test dataset
  • Fix a bug on app load in the OGC API form
  • Recover the message displayed for download links generated by WFS

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 => I think the bug fixes are for features introduced after the latest release (besides the recovered message)
  • The documentation website 📚 has received the love it deserves

This work is sponsored by MEL.

tkohr added 3 commits May 7, 2024 16:07
this can happen on app load as there is no ngIf which would bother the animation when opening the form
…sts do not succeed

this prevent download list with WFS links from breaking
Copy link
Contributor

github-actions bot commented May 7, 2024

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

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

Copy link
Contributor

github-actions bot commented May 7, 2024

📷 Screenshots are here!

@coveralls
Copy link

coveralls commented May 7, 2024

Coverage Status

coverage: 83.23% (+1.3%) from 81.89%
when pulling 53b7078 on ogc-api-fixes
into 287bf00 on main.

}
)
} catch (error) {
return Promise.resolve([])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the error can be caught much later, this is already the case for WFS for instance. The errors are caught here:

What's happening is maybe that
1/ even if an error is caught it is not shown (could be shown using an overlay for instance?)
2/ when an error is caught, it is assumed that the error comes from wfs; ut now, it can come from somewhere else, so the logic has to be adjusted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the problem that the WFS links don't show up anymore when there is an error with the OGC API links, is that both are in downloadLinks at the error handling where you point to above. Maybe we'll have to split up the two and not do a combineLatest here!? I'll have a look...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added 62c6e68 to catch the error later, but I still needed some additional error catching and resolving the Promise to allow displaying the WFS links. Also, converting the Promise to an Observable with from did not prevent the need of resolving the Promise. Do you think it makes more sense this way?

Indeed, the error messages still need to be adapted as well and eventually displayed to the user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this looks better now, we could probably have a common handleError callback which assigns the error and returns of([]) and use that both for WFS and OGC API. I'm not sure using a Promise is needed here anyway? But yes to have the equivalent of of you probably need to do from(Promise.resolve(...)), that makes sense to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hum, I was not able to refactor this further with a common handleError function. Any attempt I did to try to catch the error once the Promise has been transformed into en Obervable broke the download list, so I needed to keep catching it within the Promise.
I've added a separate translation key for the OGC API error. If I saw right, ogc-client does not provide more info on the error as for WFS, so there is just one generic message so far. It is displayed in the preview components like the map, but I decided to not also add it to the download list to not "pollute" the UI when there might be many links already displayed from working sources.

tkohr added 2 commits May 13, 2024 16:19
also add fixture to test that erroneous OGC API does not break download list
and seperate generic OGC API error message from WFS messages
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.

thank you! not tested but it looks really good 👍

@tkohr tkohr merged commit 8918855 into main May 17, 2024
9 checks passed
@tkohr tkohr deleted the ogc-api-fixes branch May 17, 2024 11:35
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.

3 participants