Support filters and custom heading on recommendation modules #3192

maccabeelevine · 2023-11-03T21:09:24Z

This adds a hiddenFilter capability on recommendation modules that derive from AbstractSearchObject. It also adds an optional custom heading.

I have a few specific use cases in mind:

The primary search results are from a tab with a hidden filter -- say a media-specific Solr search. An EDSResults sidebar could show media-specific results from EDS (or pick your other backend).
The primary search results are blended, but filters narrow down the results from one or more backend. A recommendation module could search the same backend but apply a different filter.
The primary search results are combined, and include several Solr bentos, each filtered on "high use" content like books or media. As above, a CatalogResults recommendation module called something like "Additional Sources" could be filtered on the combined NOT of all of the bentos, i.e. all the other resources no one will actually use, that we don't want in the more prominent bento real estate.

As a visual for demonstration only, here's an EDS primary results that also has a CatalogResults recommendation module that I've randomly filtered on videos.

TODO

Add :[hidden filters key]:[custom heading] to the rest of the affected modules in searches.ini
Add deprecation notes to changelog and JIRA about InjectSearchRunnerFactory when merging

config/vufind/searches.ini

maccabeelevine · 2023-11-03T21:29:28Z

module/VuFind/src/VuFind/Recommend/InjectSearchRunnerFactory.php

@@ -70,6 +70,10 @@ public function __invoke(
        if (!empty($options)) {
            throw new \Exception('Unexpected options passed to factory.');
        }
-        return new $requestedName($container->get(SearchRunner::class));
+        $module = new $requestedName($container->get(SearchRunner::class));
+        if (method_exists($module, 'setConfigManager')) {


First off, there may be a better way to make the config manager available within the recommendation module. Even if not, method_exists seems ugly but I'm not sure what is best. Maybe an interface that I check with instanceof, and define the setter in a trait? But that seems overkill and I couldn't come up with a middle ground.

Maybe the solution is just to create a different factory. The InjectSearchRunnerFactory is designed to do exactly what it says: inject the SearchRunner into the constructor. But now you want to inject the SearchRunner AND set the config manager. Maybe the easiest solution is to create an AbstractSearchObjectFactory -- then you can do all the injections you want safely since the factory is tailored for AbstractSearchObject and can assume that's how it will be used.

(I think the only reason we have InjectSearchRunnerFactory is because the various Results modules didn't inherit from a common base until you refactored them, and we probably didn't think to change the factory at that time).

In any case, if you agree with this approach, we can change all the Results modules and see if anything else is still using InjectSearchRunnerFactory. If not, we can probably deprecate it and remove it in 11.0.

This makes sense; implemented. And yes InjectSearchRunnerFactory is only used for those specific modules.

themes/bootstrap3/templates/Recommend/CatalogResults.phtml

maccabeelevine · 2023-11-03T21:33:30Z

config/vufind/searches.ini

@@ -254,10 +254,14 @@ WorkKeys = year
 ;
 ; Available modules recommended for use in the side area:
 ;
-; CatalogResults:[GET parameter]:[result limit]
+; CatalogResults:[GET parameter]:[result limit]:[hidden filters key]:[custom heading]


[hidden filters key] would work in any AbstractSearchObject recommendation module. [custom heading] would also if I update the templates. I don't know how far to go documenting that for each of these recommendation modules. Every AbstractSearchObject recommendation module already has a lot of duplicate explanation (which makes sense since they're near-identical implementations).

I think it's worth applying similar language to all the other modules in this family, though I don't mind waiting until everything is finalized first since the details might change in the meantime. If you prefer to wait, we should put a TODO checkbox at the top of the PR to keep track of this item.

I added a TODO

I might also refactor the various AbstractSearchObject modules somehow so this explanation can be in a single place under side modules.

demiankatz

Thanks, @maccabeelevine -- I haven't done a thorough review or test yet, but I started by replying to your comments and sharing a few general ideas. :-)

config/vufind/searches.ini

demiankatz · 2023-11-06T15:13:36Z

config/vufind/searches.ini

@@ -254,10 +254,14 @@ WorkKeys = year
 ;
 ; Available modules recommended for use in the side area:
 ;
-; CatalogResults:[GET parameter]:[result limit]
+; CatalogResults:[GET parameter]:[result limit]:[hidden filters key]:[custom heading]


I think it's worth applying similar language to all the other modules in this family, though I don't mind waiting until everything is finalized first since the details might change in the meantime. If you prefer to wait, we should put a TODO checkbox at the top of the PR to keep track of this item.

config/vufind/searches.ini

themes/bootstrap3/templates/Recommend/CatalogResults.phtml

demiankatz · 2023-11-06T15:22:26Z

module/VuFind/src/VuFind/Recommend/InjectSearchRunnerFactory.php

@@ -70,6 +70,10 @@ public function __invoke(
        if (!empty($options)) {
            throw new \Exception('Unexpected options passed to factory.');
        }
-        return new $requestedName($container->get(SearchRunner::class));
+        $module = new $requestedName($container->get(SearchRunner::class));
+        if (method_exists($module, 'setConfigManager')) {


Maybe the solution is just to create a different factory. The InjectSearchRunnerFactory is designed to do exactly what it says: inject the SearchRunner into the constructor. But now you want to inject the SearchRunner AND set the config manager. Maybe the easiest solution is to create an AbstractSearchObjectFactory -- then you can do all the injections you want safely since the factory is tailored for AbstractSearchObject and can assume that's how it will be used.

(I think the only reason we have InjectSearchRunnerFactory is because the various Results modules didn't inherit from a common base until you refactored them, and we probably didn't think to change the factory at that time).

In any case, if you agree with this approach, we can change all the Results modules and see if anything else is still using InjectSearchRunnerFactory. If not, we can probably deprecate it and remove it in 11.0.

demiankatz

Thanks, @maccabeelevine, this is shaping up nicely. A couple more minor suggestions below. Once you've responded to those, I think it should be safe to finish up the remaining documentation formatting.

I've added a TODO note to myself to document the deprecation of the old factory.

I'll do some hands-on testing of this once you've responded to this latest round of feedback.

config/vufind/searches.ini

module/VuFind/src/VuFind/Recommend/AbstractSearchObject.php

config/vufind/searches.ini

maccabeelevine · 2023-11-06T20:41:17Z

config/vufind/searches.ini

+; LibGuidesResults:[GET parameter]:[result limit]:[ini section]:[custom heading]
 ;       Display LibGuides research guides search results matching the terms found in
 ;       the specified GET parameter (default = "lookfor"), limited to a specified
 ;       number of matches (default = 5).
+;       Hidden filters may be defined in the specified section of the LibGuides.ini
+;       file; see [CatalogResultsVideoFilter] below for an example of this. The
+;       optional custom heading overrides the default for this recommendation module.
 ; LibGuidesResultsDeferred:[GET parameter]:[result limit]
 ;       Same as LibGuidesResults, but loaded via AJAX.
-; LibGuidesAZResults:[GET parameter]:[result limit]
+; LibGuidesAZResults:[GET parameter]:[result limit]:[ini section]:[custom heading]
 ;       Display LibGuides A-Z Databases search results matching the terms found in
 ;       the specified GET parameter (default = "lookfor"), limited to a specified
 ;       number of matches (default = 5).
+;       Hidden filters may be defined in the specified section of the LibGuidesAZ.ini
+;       file; see [CatalogResultsVideoFilter] below for an example of this. The
+;       optional custom heading overrides the default for this recommendation module.


I've added the documentation to these other modules for discussion purposes. Two obvious problems though.

[custom heading] doesn't work until I implement it in the templates. I can either add it to the templates or remove it from the doc for now.

Filtering doesn't work for many of these backends -- definitely not for LibGuides / LibGuidesAZ, not yet implemented for EPF, and I don't know about Web. I assume it's implemented for Summon but can't test that. So I'll presumably remove the [ini section] from those but wanted to get your take first.

1.) I think you might as well go ahead and implement the custom headings for all of the templates. I wonder if it would actually make sense to just create a $heading property and getHeading() method in AbstractSearchObject. Each AbstractSearchObject subclass could set a different default property, and then the template could just display the result of calling getHeading(). Then it's a single configurable setting instead of an override option.

2.) Maybe you should reverse the order of the heading and ini section settings so it makes more sense to drop the ini section when unsupported. Otherwise you'll have to put in an extra colon separator to override the heading, and that might cause confusion. I also wonder if we should drop the word "hidden" since we're not using hidden filters here, just regular filters. We're filtering the results fetched from the backends, but we don't need to use the hidden filter mechanism since this isn't a normal user-initiated search with faceting/filtering controls. And regarding where filtering is supported, you're correct that it should work for Summon. It should also work for Web, since that's a standard Solr-based search.

This all made sense to me. I've implemented the default heading property as an abstract method so it's easier to check compliance. Everything else I did as suggested.

I forgot that we removed the client-side filtering settings from EDS.ini recently since they don't work. So I removed the [ini section] for EDS and EPF, although it's something that's worth investigating along with the larger issue.

I will need help from someone to test the Summon and Web functionality.

…on't support it

demiankatz

One minor suggestion. Once this is adjusted, I'll help test WebResults. I think we'll have to take Summon on faith (but given the nature of the changes, I'm okay with that).

demiankatz · 2023-11-13T20:28:21Z

module/VuFind/src/VuFind/Recommend/AbstractSearchObject.php

+     *
+     * @var string
+     */
+    protected $iniSection;


Maybe call this $filterIniSection in case we have another .ini section for some other thing in future?

demiankatz

The recent merge of #3209 has created a bunch of conflicts here. Do you mind resolving them when time permits?

maccabeelevine · 2023-11-14T18:24:58Z

The recent merge of #3209 has created a bunch of conflicts here. Do you mind resolving them when time permits?

All set.

demiankatz

This looks good to me, and I tested that WebResults works as expected. Thanks again!

Support hiddenFilters on recommendation modules

fe486fe

maccabeelevine commented Nov 3, 2023

View reviewed changes

config/vufind/searches.ini Outdated Show resolved Hide resolved

maccabeelevine commented Nov 3, 2023

View reviewed changes

config/vufind/searches.ini Outdated Show resolved Hide resolved

maccabeelevine commented Nov 3, 2023

View reviewed changes

themes/bootstrap3/templates/Recommend/CatalogResults.phtml Outdated Show resolved Hide resolved

maccabeelevine commented Nov 3, 2023

View reviewed changes

maccabeelevine marked this pull request as ready for review November 3, 2023 21:34

demiankatz added this to the 10.0 milestone Nov 6, 2023

demiankatz added the improvement label Nov 6, 2023

demiankatz requested changes Nov 6, 2023

View reviewed changes

maccabeelevine added 2 commits November 6, 2023 17:45

Simplify ini section for hidden filters

e8fcda8

Replace InjectSearchRunnerFactory with AbstractSearchObjectFactory

971f09b

maccabeelevine changed the title ~~Support hidden filters on recommendation modules~~ Support hidden filters and custom heading on recommendation modules Nov 6, 2023

maccabeelevine added 2 commits November 6, 2023 19:26

Fix tests

d9ba2b5

Add missing doc param

ac8edfe

demiankatz requested changes Nov 6, 2023

View reviewed changes

config/vufind/searches.ini Outdated Show resolved Hide resolved

module/VuFind/src/VuFind/Recommend/AbstractSearchObject.php Outdated Show resolved Hide resolved

maccabeelevine added 2 commits November 6, 2023 20:18

Improve doc.

cf8b4ed

Remove $iniSection variable

c94ba8e

demiankatz reviewed Nov 6, 2023

View reviewed changes

config/vufind/searches.ini Outdated Show resolved Hide resolved

Document other modules (even if wrong)

19b6e2c

maccabeelevine commented Nov 6, 2023

View reviewed changes

Fix phpcbf

2d12789

demiankatz added the small Minor changes to relatively few files label Nov 7, 2023

maccabeelevine added 3 commits November 7, 2023 20:06

Flip config of $iniSection and $customHeading

8c5ae0c

Set default headings within code, and drop filter from modules that d…

700fc46

…on't support it

Remove word 'hidden'

c7f7d2f

maccabeelevine changed the title ~~Support hidden filters and custom heading on recommendation modules~~ Support filters and custom heading on recommendation modules Nov 7, 2023

maccabeelevine added 2 commits November 7, 2023 21:30

Document new param on Deferred modules

a778f1e

Merge branch 'dev' into recommendation-filter

b5f2e74

demiankatz requested changes Nov 13, 2023

View reviewed changes

Rename $iniSection for clarity

88e676e

demiankatz requested changes Nov 14, 2023

View reviewed changes

Merge branch 'dev' into recommendation-filter

a20dcc4

Rewrap line for consistency.

4b80e52

demiankatz approved these changes Nov 14, 2023

View reviewed changes

demiankatz merged commit ec7a698 into vufind-org:dev Nov 14, 2023
7 checks passed

maccabeelevine deleted the recommendation-filter branch November 14, 2023 22:03

demiankatz mentioned this pull request Nov 15, 2023

Fix deprecation warnings in tests; make PHPUnit more strict. #3224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support filters and custom heading on recommendation modules #3192

Support filters and custom heading on recommendation modules #3192

maccabeelevine commented Nov 3, 2023 •

edited by demiankatz

Loading

maccabeelevine Nov 3, 2023 •

edited

Loading

demiankatz Nov 6, 2023

maccabeelevine Nov 6, 2023

maccabeelevine Nov 3, 2023

demiankatz Nov 6, 2023

maccabeelevine Nov 6, 2023

maccabeelevine Nov 6, 2023

demiankatz left a comment

demiankatz Nov 6, 2023

demiankatz Nov 6, 2023

demiankatz left a comment

maccabeelevine Nov 6, 2023

demiankatz Nov 7, 2023

maccabeelevine Nov 7, 2023

demiankatz left a comment

demiankatz Nov 13, 2023

maccabeelevine Nov 13, 2023

demiankatz left a comment

maccabeelevine commented Nov 14, 2023

demiankatz left a comment

Support filters and custom heading on recommendation modules #3192

Support filters and custom heading on recommendation modules #3192

Conversation

maccabeelevine commented Nov 3, 2023 • edited by demiankatz Loading

maccabeelevine Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

maccabeelevine commented Nov 14, 2023

demiankatz left a comment

Choose a reason for hiding this comment

maccabeelevine commented Nov 3, 2023 •

edited by demiankatz

Loading

maccabeelevine Nov 3, 2023 •

edited

Loading