Add filtering capability to facet list pop-up

[mtrojan-ub]

This PR is related to the short demo on the previous community call (July 11, 2023) which has been recorded by @demiankatz and can be found within the minutes: https://vufind.org/wiki/community_call:minutes20230711

As discussed, we can dive deeper into technical details at the next community call in August.

At the moment this is just a first proof-of-concept. Since i'm not very familiar yet with VuFind's JS code nor with deeper search capabilities (& also compatibilities with search backends other than Solr), i'd be very happy if especially @crhallberg and @EreMaijala could also have a look and provide feedback on this one!

TODO

• Fix or discuss case sensitivity issues
• Add clear/reset button to filter control
• Fix problem with applied facets getting lost after applying filters and choosing an option (with OR/NOT facets)
• Make sure new control is optimized for accessibility
• Hide new control when Javascript is disabled, since it won't work
• Remove snooze from Mink test if possible
• Fix vertical alignment of filter label across themes
• Fix appearance of reset button
• Fix "breaking out of lightbox" bug revealed by test added in Expand Mink test to exercise more button after sorting facets. #3252.
• Make sure full test suite passes before merging
• Note translation limitation when adding changelog note after merging

[demiankatz]

Thanks for sharing the prototype, @mtrojan-ub! I'm giving this a 9.1 milestone right now to be sure we keep it under discussion on future community calls, though depending on how it evolves, we may end up moving it to 10.0.

Since the whole functionality seems to revolve around the existing setFacetContains mechanism, it seems that we could easily make this backend-agnostic by simply checking to see if the current Params object supports setFacetContains and only displaying the search/limit box when it does. That will automatically display it for Solr and hide it for any other backend that doesn't have the necessary mechanism... and then adding support is just a matter of implementing the appropriate facet filtering in the search classes (if supported by the backend).

[demiankatz]

Also, how would you feel about renaming this PR to "Add filtering capability to facet list pop-up" to more specifically describe what we're adding? Or is there more to it than just that?

[mtrojan-ub]

Thanks for the feedback, @demiankatz!

I added a short capability check for setFacetContains in the template and also renamed this PR, although we should be aware that the view can also be opened outside of a pop-up e.g. by opening the link in a new browser tab.

I had quite a struggle to make this work in both cases (with + without lightbox), thats why the keyup event is registered multiple times... do you have any good advice on improving this? Maybe the code has to be combined with the existing VuFind.lightbox_facets logic in facets.js? It also seems like the current logic causes a conflict with the existing "more" button inside the popup.

[demiankatz]

Thanks, @mtrojan-ub -- I'm requesting a review from @crhallberg to see if he can help with the issues you raise.

[mtrojan-ub]

Another question: Since the Logic of the new AjaxHandler is very similar to the existing logic in SearchController.facetListAction, i wonder what would be the best spot to create a shared function which would then be used by both. Any suggestions? (Maybe a controller plugin or feature trait?)

[demiankatz]

Another question: Since the Logic of the new AjaxHandler is very similar to the existing logic in SearchController.facetListAction, i wonder what would be the best spot to create a shared function which would then be used by both. Any suggestions? (Maybe a controller plugin or feature trait?)

I don't think a controller plugin is the best option, since I think it could get awkward using controller plugins outside the context of a controller. A trait might be a good option, but if you think users might want to extend/override this, then it might be better to just create a new top-level helper, following the precedent of the HierarchicalFacetHelper, perhaps.

[mtrojan-ub]

So now i understand why the existing AJAX logic regarding the more button did not use a separate AJAX endpoint! I tried to write a FacetListHelper, but then i was unable to delegate things from the AJAX endpoint since some of the environment objects to call e.g. initByRequest cannot be acquired properly.

Maybe it's best to add the AJAX-related logic inside the existing endpoint and leave it there as before, instead of adding a separate non-controller AJAX endpoint.

[demiankatz]

Thanks for the progress, @mtrojan-ub! I've resolved @EreMaijala's comments that seem to have been fully addressed, and I left comments on a couple of other areas that may need more work and/or confirmation from his end.

The full test suite is passing on my end, and the UI passes my visual inspection... but I still think we need to be careful about accessibility issues, as noted in my comments above.

[demiankatz]

Also, regarding the concerns about urlBase/searchAction, could we at least add some validation to prevent these from being abused? For example, I assume these should always be relative rather than absolute values. Could we just throw an exception if they contain "://" to prevent people from trying to do weird XSS things?

[mtrojan-ub]

Regarding XSS: Not sure about this one => the variables will simply be passed through, and if there's a real vulnerability i guess that the calls would be executed on the JS side. Shouldn't this already be covered by using the correct CSP settings?

[demiankatz]

Regarding XSS: Not sure about this one => the variables will simply be passed through, and if there's a real vulnerability i guess that the calls would be executed on the JS side. Shouldn't this already be covered by using the correct CSP settings?

I don't think there's an opportunity to execute malicious scripts inside the context of VuFind, but it is possible to craft links that will lead to a third-party site from within the VuFind interface. For example, I made this link on my test server:

http://localhost/vufind_test/Search/FacetList?type=AllFields&facet=building&facetop=AND&facetexclude=0&facet=building&facetexclude=0&facetop=AND&facetpage=1&facetsort=index&urlBase=https://gamebooks.org%2Fvufind_test%2FSearch%2FFacetList%3Ftype%3DAllFields%26facet%3Dbuilding%26facetop%3DAND%26facetexclude%3D0&searchAction=https://gamebooks.org%2Fvufind_test%2FSearch%2FResults&contains=test&facetsort=index&ajax=1

And this displays the facet list in VuFind, but each facet link leads to gamebooks.org instead of staying inside VuFind. While I think it's pretty improbable that somebody would find a way to take advantage of this in a real-world situation, it seems that with the right "social engineering," some harm could be done.

[demiankatz]

@mtrojan-ub, as proposed earlier, I think closing this loophole may be as simple as this:

diff --git a/module/VuFind/src/VuFind/Controller/AbstractSearch.php b/module/VuFind/src/VuFind/Controller/AbstractSearch.php
index 6df6cd623d..71d04d5067 100644
--- a/module/VuFind/src/VuFind/Controller/AbstractSearch.php
+++ b/module/VuFind/src/VuFind/Controller/AbstractSearch.php
@@ -875,6 +875,12 @@ class AbstractSearch extends AbstractBase
         $ajax = (int)$this->params()->fromQuery('ajax', 0);
         $urlBase = $this->params()->fromQuery('urlBase', '');
         $searchAction = $this->params()->fromQuery('searchAction', '');
+        // $urlBase and $searchAction should be relative URLs; if there is an
+        // absolute URL passed in, this may be a sign of malicious activity and
+        // we should fail.
+        if (str_contains($urlBase . $searchAction, '://')) {
+            throw new \Exception('Unexpected absolute URL found.');
+        }
         $options = $results->getOptions();
         $facetSortOptions = $options->getFacetSortOptions($facet);
         $sort = $this->params()->fromQuery('facetsort', null);

What do you think? Also interested in @EreMaijala's opinion. It would be preferable to find a way to avoid passing the URLs around at all, but this might be an adequate safety measure without requiring major refactoring.

[mtrojan-ub]

Well if it's really that simple then of course i don't mind!

[demiankatz]

Thanks, @mtrojan-ub -- I've pushed up my proposed improvement, and will await @EreMaijala's re-review when time permits.

[EreMaijala]

Nice, everything seems to work great now. Just one suggestion on cleaning up the paging links.

[demiankatz]

Thanks for the approval, @EreMaijala. The full test suite is passing for me. I've asked @sturkel89 to give this another look (probably sometime next week, as I know she's busy with other things today). If she can't find any new problems, I think we can merge, but I'll wait for her report before putting in my final approval.

[sturkel89]

I've tested this branch in all three themes and facet filtering seems to be working properly. I tested Exclude and Or facets in the lightbox, and they worked correctly. I tried using keyboard commands (tab, shift-tab) to move around, and all worked as expected. Switching between sorting by result count vs. alphabetical works well, and the X to clear your search appears appropriately when you start entering text and filtering your list.

My only comment at this point has to do with WHERE the filter box in facet lists shows up. I had previously tested it with the Author facet, as that seemed like the place where it was most likely to be useful. However, this time I paid close attention and checked every facet, and was surprised to see that the filter box only appears in a subset of facets. Is this by design?

In my test, the filter box appears in lightbox view in the following facets:

• Library
• Author
• Genre

Example:

In my test, the filter box does NOT appear in lightbox view in the following facets:

• Format
• Language

Example:

In the default database I'm using with the standard results set of 326 entries, there are several text-based facets that have too few options for there to be a "see all..." menu that would prompt a lightbox to appear. I'm sure catalogs with many more entries would generate lightboxes; would these lightboxes get a search box, or not?

• Institution
• Call number
• Era
• Region

If it's intentional that the filter box is available in only selected facets, then we're ready to merge. If it's expected that every "see all..." facet lightbox will allow filtering of the list, then we are not yet done.

[demiankatz]

Thanks, @sturkel89. It is expected that some facets do not support filtering; this is the result of limitations caused by the "facet translation" feature. However, you raise a good point that this may be confusing to users who do not understand the reason for the feature to appear inconsistently.

Maybe it is worth adding a comment above the translated_facets[] lines in facets.ini saying something like:

; Please note that the "filter" textbox control in the expanded facet list
; will be disabled for translated facets due to technical limitations of its
; implementation. Filtering can only be applied to raw values in the index.

Of course, that is of limited value since I doubt many people will notice that it is there.

Some other things we might want to consider doing:

1.) Maybe it is worth adding a setting to disable the filter, so institutions that don't feel they need it and want to avoid potential confusion can easily turn it off. (It may already be easy enough to hide it with CSS, though, by just applying display: none to .facet-lightbox-filter).

2.) Maybe it is worth building an alternate facet translation mechanism where all the different translated values are indexed into separate Solr fields, and VuFind switches the requested/displayed fields based on the active language. I've done this as a customization in some instances I have managed, and maybe it's worth promoting it to a standard feature. This would give us an alternate solution that would be compatible with filtering, and then we could recommend it in the documentation.

Obviously, I don't want to hold up this PR any further -- I believe that it is mature enough to merge, and we can address these other ideas as follow-ups. However, I'll wait to see if @mtrojan-ub and @EreMaijala have any further thoughts before I push the button (especially in case we want to at least add my proposed comment first).

I'll also be sure that the changelog note addresses this limitation.

[EreMaijala]

I'm ok with this being merged as is. The comment in ini file is a good idea, but it's not easy to avoid user confusion.

[mtrojan-ub]

I also think it's ok to be merged like this.

Some more thoughts on the latest discussion:
Related to the facet translation mechanism, i also agree that it would be a good thing to provide a solr-based solution in the long term. We already have something similar based on custom Solr plugins but there might be a better approach in the meantime.
Regarding the capability check, i think it is ok as-is. If there is any confusion i think it will be more on the side of the end-user, so another approach might be to display a help text but on the other hand the facets should stay lightweight, and if you start showing online help texts for certain behaviour the question always is what's important / what to show & what to hide, so i suggest we just wait for feedback and can still add some text/info at a later point if necessary.

[sturkel89]

This all sounds good to me! Looking forward to the addition of this feature. Thanks for everyone's work on this!

[demiankatz]

Thanks, everyone! I've added my proposed comment text as suggested, and I have made some minor updates to the Mink test to bring it up to date with @EreMaijala's latest test stability improvements. I'll merge shortly!