dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

Geospatial maps for item pages, search, browse

Open kshepherd opened this issue 1 year ago • 1 comments

References

  • Requires DSpace/DSpace#9918

Description

This pull request is introduced as part of a wider contribution (GND authority and external data) but is a standalone feature which has a wide application, so is opened as a separate PR.

It allows maps to be drawn on item pages, a new geo-based search results view mode, and a 'browse by geolocation' browse map.

Full usage and technical documentation of this new feature is at https://github.com/kshepherd/dspace-geospatial-maps-doc/blob/main/README.md

demo map

Instructions for Reviewers

List of changes in this PR:

  • New base and item field components (example usage commented out in untyped item and publication component templates) for drawing maps from metadata values or other input
  • New configuration properties to support this feature
  • A few new i18n keys
  • Marker assets for leaflet maps

To prepare test data, populate some item metadata with dcterms.spatial WKT point values (as per documentation).

To test item page maps, try out the item page field components that are commented out in untyped-item and publication templates. This can be tested without the backend PR.

To test the search results map, enable it in angular config and look for a new 'map' view mode beside the list and grid view mode icons. This can be tested without the backend PR, though I recommend applying it and reindexing so you can try out the "has geospatial metadata" sidebar facet.

To test the browse-by map, make sure to apply the backend PR (note- this requires the solr search core to be recreated and reindexed due to a new schema field, if you want to use the "has geospatial metadata" filter) and make sure you have the documented search configuration, filters and facets applied, then do a full-reindex and enable the browse map in angular config. It should appear in the browse menu and clicking markers should open a new search filtered by that point.

Unit tests are written for all new components.

Checklist

  • [x] My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • [ ] My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • [x] My PR passes ESLint validation using npm run lint
  • [x] My PR doesn't introduce circular dependencies (verified via npm run check-circ-deps)
  • [x] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • [x] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • [ ] My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • [x] My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • [x] My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • [x] If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • [x] If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • [ ] If my PR fixes an issue ticket, I've linked them together.

kshepherd avatar Oct 23 '24 15:10 kshepherd

Hi @kshepherd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

github-actions[bot] avatar Oct 24 '24 20:10 github-actions[bot]

I tested the PR locally and it worked fine! Thanks @kshepherd !

EikLoe avatar Mar 07 '25 10:03 EikLoe

Hi @kshepherd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

github-actions[bot] avatar Mar 11 '25 19:03 github-actions[bot]

NOTE: This PR has merge conflicts caused by the merger of #3997. You will need to rebase this PR on the latest main and update it to use "control flow" syntax by replacing any ngIf, ngFor, or ngSwitch usages in HTML templates with @if, @for or @switch. See the control flow docs or the above PR for more details.

tdonohue avatar Mar 11 '25 19:03 tdonohue

Hi @kshepherd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

github-actions[bot] avatar Mar 14 '25 14:03 github-actions[bot]

@tdonohue Could you please give this a re-review? I have addressed all your feedback, although haven't changed that commented-out field - if you think my answer is not quite satisfactory, and we should require it to be wrapped in a config.yml check (like mediaviewer etc) then let me know and i'll look to do that ASAP - i am hoping to keep it more flexible, while still showing a usage example in a template, but i could also simply move it out to documentation We have a successful test from @EikLoe , would that serve as an additional +1 for this PR?

kshepherd avatar Mar 25 '25 15:03 kshepherd

@kshepherd : I can give this another look today hopefully. Just a note though that the backend has a minor merge conflict that needs cleanup (it's just a config conflict it seems).

tdonohue avatar Mar 25 '25 15:03 tdonohue

@tdonohue As an example, you can set dcterms.spatial to Point ( +008.660000 +047.336111 ). So basically use the geocordinates.

pnbecker avatar Mar 25 '25 20:03 pnbecker

@tdonohue you can see the browse map in production here: https://zop.zb.uzh.ch/browse/map

pnbecker avatar Mar 26 '25 01:03 pnbecker

@kshepherd : Please note this is failing its own tests. I'll still see if I can find time to review it later today (I have a lot on my plate though, so it may be tomorrow)

tdonohue avatar Mar 26 '25 17:03 tdonohue

@tdonohue this should now be ready, all tests are passing. I also updated my documentation at https://github.com/kshepherd/dspace-geospatial-maps-doc/blob/main/README.md to include example formats for bounding box and point data, and the new config properties

kshepherd avatar Mar 27 '25 13:03 kshepherd

Hi @kshepherd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

github-actions[bot] avatar Mar 28 '25 15:03 github-actions[bot]

Hi @kshepherd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

github-actions[bot] avatar Mar 28 '25 16:03 github-actions[bot]

I have tested all the functionalities: displaying the map in search (after fixing the previously mentioned error), the new facet, displaying the map in an item, and the browse-by-map functionality. Everything has worked well for me. I'm sharing an example of a map with multiple points and a bounding box.

image

The only issue I found is that the browse-by-map functionality only displays 100 points. From what I’ve seen, the number of results returned by a facet is limited to 100. More results can be added by configuring the rest.search.max.results option (but at most 1000, as I believe the API pagination is limited to 1000 items). This configuration key is used here: DiscoverQueryBuilder.java.

Since the browse-by-map functionality can be disabled, I think documenting this limitation should be sufficient.

toniprieto avatar Mar 28 '25 16:03 toniprieto

Thanks @toniprieto ! I have addressed the view mode button control flow as you noted, and removed the console output. I agree with you that we should document the implication of rest.search.max.results on the browse map, but leave it up to the user to change (since the feature is disabled by default anyway)

kshepherd avatar Mar 28 '25 17:03 kshepherd

thanks @tdonohue for the extra testing. I'll look into the issues here, but regarding the browse map, I think the discovery rebuild might not be quite enough, as we add a new field to the Solr schema. So you might need to make sure you copy the Solr search core config over and perhaps even delete the core and allow it to be recreated with the new field, to avoid that "point is not a valid search filter" error - that is something we'll have to document as well

kshepherd avatar Mar 28 '25 22:03 kshepherd

If a map is displayed showing London for an item with no dcterms.spatial metadata then that seems like a bug to me - yes, it's a default point, but we should not display a map at all if no relevant data is present

kshepherd avatar Mar 28 '25 22:03 kshepherd

@tdonohue About the error "point is not a valid search filter," I also encountered it during the initial tests, but it was resolved by enabling the facet <ref bean="searchFilterGeospatialPoint"/> in the discovery.xml of the backend. It fails because the facet used by the search is not enabled.

toniprieto avatar Mar 28 '25 22:03 toniprieto

@kshepherd if I understand @tdonohue correctly, London is shown if you switch on the map view on a list of search results without any geospatial item. It does not show a wrong marker, its just the default position of the map without markers. @tdonohue did I understand you correctly?

pnbecker avatar Mar 28 '25 23:03 pnbecker

@pnbecker is correct. There’s no point displayed. It’s just that the default view of the map is a view of London. It’s not necessarily a bug, it just seemed odd to me. But if there’s no way to fix that, we can leave it as is.


From: Pascal-Nicolas Becker @.> Sent: Friday, March 28, 2025 6:01:22 PM To: DSpace/dspace-angular @.> Cc: Tim Donohue @.>; Mention @.> Subject: Re: [DSpace/dspace-angular] Geospatial maps for item pages, search, browse (PR #3540)

@kshepherdhttps://github.com/kshepherd if I understand @tdonohuehttps://github.com/tdonohue correctly, London is shown if you switch on the map view on a list of search results without any geospatial item. It does not show a wrong marker, its just the default position of the map without markers. @tdonohuehttps://github.com/tdonohue did I understand you correctly?

— Reply to this email directly, view it on GitHubhttps://github.com/DSpace/dspace-angular/pull/3540#issuecomment-2762788671, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AADWFHKBN6GEA26DPFBVZH32WXBDFAVCNFSM6AAAAABQPDDO2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRSG44DQNRXGE. You are receiving this because you were mentioned.Message ID: @.***>

[pnbecker]pnbecker left a comment (DSpace/dspace-angular#3540)https://github.com/DSpace/dspace-angular/pull/3540#issuecomment-2762788671

@kshepherdhttps://github.com/kshepherd if I understand @tdonohuehttps://github.com/tdonohue correctly, London is shown if you switch on the map view on a list of search results without any geospatial item. It does not show a wrong marker, its just the default position of the map without markers. @tdonohuehttps://github.com/tdonohue did I understand you correctly?

— Reply to this email directly, view it on GitHubhttps://github.com/DSpace/dspace-angular/pull/3540#issuecomment-2762788671, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AADWFHKBN6GEA26DPFBVZH32WXBDFAVCNFSM6AAAAABQPDDO2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRSG44DQNRXGE. You are receiving this because you were mentioned.Message ID: @.***>

tdonohue avatar Mar 28 '25 23:03 tdonohue

I see. Here are some changes:

  1. New "DEFAULT_CENTRE_POINT" configuration, documented in dfault-app-config.ts and config.example.yml - it uses Istanbul as a default value since it shows more of the (old) world nicely and isn't so "anglo centric default" ;)
  2. New behaviour in browse and search map layer logic, where instead of using the default zoom level, it instead zooms out to level 1 and shows an i18n-controlled "No results" message
  3. As per @toniprieto 's comment, i18n added for search.filters.applied.f.point (displayed if someone clicks a browse marker icon)
  4. ^^^ I will also push some backend changes so that the geospatial point and "has point" filters are enabled in discovery.xml by default, which will further help enable the feature - since they are not included as facets by default, it won't affect the facets displayed "out of the box" but it will ensure that clicking browse markers use a valid filter without any other user action

I believe the combination of zoom level and tooltip should give better UX now, when there are no results. If you click next page on a search page and the new page has results, the map will behave normally, zoom in to the bounds of the furthest markers, etc.

screenshot_2025-03-30_16:31:17_selection screenshot_2025-03-30_16:35:39_selection

kshepherd avatar Mar 30 '25 14:03 kshepherd

Merging with +2 approval. This obviously needs the documentation for this feature moved to our DSpace 9.0 docs: https://wiki.lyrasis.org/display/DSDOC9x So, flagging this as "needs documentation" until that is completed. Thanks again @kshepherd !

tdonohue avatar Mar 31 '25 19:03 tdonohue