Fixes search passthrough to Mirador#1630
Conversation
tdonohue
added
bug
integration: IIIF
tdonohue
added
the
1 APPROVAL
| const arr = r.split('&'); | ||
| const q = arr[1]; | ||
| const v = q.split('='); | ||
| const v = r.split('query='); |
There was a problem hiding this comment.
this look to me a bit fragile... are there other built-in mechanism in Angular to parse an URL to extract a parameter value?
There was a problem hiding this comment.
Use const u = new URL(r) and find what you want in u.searchParams? https://stackoverflow.com/a/24006120/2916377
Agreed: one should always think long and hard before writing one of these ad-hoc parsers for something as complex as a URL.
| mockRouteService.getPreviousUrl.and.returnValue(of(['/search?q=bird&motivation=painting','/item'])); | ||
| const localMockRouteService = { | ||
| getPreviousUrl(): Observable<string> { | ||
| return of('/search?query=test'); |
There was a problem hiding this comment.
I'm worried about queries that include url encoded character such as a space or an unicode character, other than URL with additional parameters that could go after the query param (i.e. page size, direction, etc)
|
Now using an Angular route parser. Thanks for the suggestion! You're right that the previous approach was lazy on my part. Tests are passing. I haven't been able to run in a dev environment because I've been traveling with a laptop that isn't configured properly. Will have time to fully test myself when I get back on 5/25. |
Lint fix
mspalti
force-pushed
the
mirador-search-passthrough
branch
from
4bd52e2 to
5ec460d
Compare
|
I just tested this in a development environment. The query DSpace query passthrough worked fine. I tested using a multi-word query and sorting. I believe this is ready for review. |
|
@mspalti : If you have any time in the coming days, this can be included in 7.3 if the merge conflicts are resolved before Tuesday. After Tuesday, I'll likely have to reschedule this for 7.4. Thanks! |
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
👍 Thanks @mspalti . I gave this a quick code review and it seems reasonable. I was not able to test it (as I don't have the setup you describe above), but I did install it and verify that the new code doesn't break anything (doesn't appear to based on my basic tests).
4 participants
Description
The mirador Angular component should pass DSpace discovery query terms to the Mirador viewer whenever the previous route in history was a search request. I'm not sure why this stopped working. The PR fixes the problem by simplifying the utility function that checks for a search and returns the query terms (if found). The expected behavior is that the Mirador viewer will be initialized with search results present. New tests are provided.
Instructions for Reviewers
Testing this is difficult at the moment because it requires installing and populating a separate Solr core and indexing OCR data from Item Bitstreams (in ALTO, hOCR, or MiniOcr). That's possible if you have some data available. Also, some tooling and a preconfigured core are under review for a later release in this PR #8241. Some folks will be working with the experimental IIIF search support so I would be helpful to get this minor Angular fix added to 7.3.
List of changes in this PR:
item-iiif-utilsfunction.Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
yarn run lintpackage.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.