search

CUL-DigitalServices / libraries-gateway

Cambridge Libraries
1 stars 4 forks source link

Facets fix #233

Closed Coenego closed 10 years ago

Coenego commented 10 years ago

When drilling the search results down by selecting a facet, the correct number is now displayed.

Coenego commented 10 years ago

Changed status into "more work needed".

Since we're no longer using facet properties in the querystring (e.g. ../find-a-resource?q=Darwin&api=aquabrowser&language=English&author=Darwin&...) to drill down the results, some more work is needed for the filters displayed above the results. The new querystring now looks like: ../find-a-resource?q=Darwin&api=aquabrowser&ref=68;200;... and only contains references to the Aquabrowser facets.

Coenego commented 10 years ago

Assigning to @ucamhal for review.

ucamhal commented 10 years ago

I notice that passing an invalid base 10 int for ref:NUMBER=xxx when drilling down w/ aquabrowser causes a 500 error. Should probably ignore the bad ref, or at worst give a 400 bad request response.

ucamhal commented 10 years ago

A small thing which would be nice to fix is that the colons in the ref:123=Blah query params are getting % encoded, even though : is allowed to occur in a query string (has no special meaning). I notice that querystring.stringify is responsible for this, and has no way to control the characters it escapes. :(

Would be nice to encode spaces with + rather than %20 as well. e.g:

ucamhal commented 10 years ago

Another small thing is that the applied facet names are available in the aquabrowser response XML in the /root/feedbacks/RefineCrumbtrail element. You could avoid specifying them in the URLs and just do ref=123.

On the other hand if you want the names in the URL for readability I can understand that. You could consider ignoring the names in the URL and pulling the names from the response XML though.

ucamhal commented 10 years ago

Changes look good, faceting works as I'd expect now. Some suggestions to consider, not everything is strictly required. Assigning to @Coenego to look at.

Coenego commented 10 years ago

Reassigning to @ucamhal for review

ucamhal commented 10 years ago

Looks good, thanks.