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: use the spatial extent of a record when it cannot be computed from a layer #794

Merged
merged 8 commits into from
Feb 14, 2024

Conversation

jahow
Copy link
Collaborator

@jahow jahow commented Feb 9, 2024

Description

This PR adds:

  • parsing of spatial and temporal extents from gn4 index records
  • refactoring & simplification of the map utils services
  • when showing a dataset on the map, fallback to a record spatial extent if the layer extent could not be determined

To see it in action, open http://localhost:4200/dataset/9e1ea778-d0ce-4b49-90b7-37bc0e448300 and select the "WMS group" distribution

This PR also adds two records to the dump: one with a WMTS layer (there was none), and the generic test record from geocat.ch: https://geocat-dev.dev.bgdi.ch/geonetwork/srv/eng/catalog.search#/metadata/9e1ea778-d0ce-4b49-90b7-37bc0e448300

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

This work is sponsored by Geocat.ch.

Copy link
Contributor

github-actions bot commented Feb 9, 2024

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

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

@coveralls
Copy link

coveralls commented Feb 9, 2024

Coverage Status

coverage: 83.2% (+0.9%) from 82.294%
when pulling 25d98c0 on use-metadata-extent-fallback
into 0da0455 on main.

@jahow jahow force-pushed the use-metadata-extent-fallback branch from 8f71536 to cfc3f32 Compare February 9, 2024 20:14
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, @jahow ! Still having a hard time to not confuse all the extents reviewing, but AFAIK see all looks good:-) Is there a record to test the fallback to the record extent?

Note: When switching from one dataset to another, sometimes I saw the previous dataset showing up briefly on the map. I think we've already had this issue in the past and I doubt it's related to this PR though.

OnlineLinkResource,
} from '@geonetwork-ui/common/domain/model/record'
import { matchProtocol } from '../common/distribution.mapper'
import { LangService } from '@geonetwork-ui/util/i18n'
import { Geometry } from 'geojson'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not seem to be used.

@@ -128,8 +130,8 @@ export class MapUtilsService {
/**
* Will emit `null` if no extent could be computed
*/
getLayerExtent(layer: MapContextLayerModel): Observable<Extent | null> {
let geographicExtent: Observable<Extent>
async getLayerExtent(layer: MapContextLayerModel): Promise<Extent | null> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a particular reason to keep this a Promise here and transform it to an Observable later in the chain?

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! using promises here made everything else so much less verbose, I figured it would be worth it. I usually consider that once "in Angular" we should use observables everywhere, but it was hard not to get the benefit of async functions here.

@jahow
Copy link
Collaborator Author

jahow commented Feb 12, 2024

Thanks, @jahow ! Still having a hard time to not confuse all the extents reviewing, but AFAIK see all looks good:-) Is there a record to test the fallback to the record extent?

Thanks for the review! I linked a local record in the PR, it's the general testing record from Geocat. The "WMS Group" distribution relies on a layer in the GetCapabilities without any extent.

I agree that the logic for setting up the map is still a bit convoluted for now. I hope in the near future we'll be able to more clearly separate what is about generating a map context, and what is about interpreting it.

Note: When switching from one dataset to another, sometimes I saw the previous dataset showing up briefly on the map. I think we've already had this issue in the past and I doubt it's related to this PR though.

Yes, I think we're not clearing the map while a dataset is loading. But that should be behind a loading panel right?

@jahow jahow merged commit 3ee8e2c into main Feb 14, 2024
9 checks passed
@jahow jahow deleted the use-metadata-extent-fallback branch February 14, 2024 08:17
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