Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display side recommendations with combined search #3135

Merged

Conversation

maccabeelevine
Copy link
Member

@maccabeelevine maccabeelevine commented Oct 2, 2023

Per @demiankatz there was no specific decision to omit side recommendations from the combined search screen; it was just too complicated to deal with at the time. Screen sizes have also increased in the decade+ since combined search was originally implemented, so it is worth a second look.

image

@maccabeelevine maccabeelevine changed the title Combined side recommendations Display side recommendations with combined search Oct 2, 2023
@maccabeelevine
Copy link
Member Author

maccabeelevine commented Oct 2, 2023

There are several potential approaches.

  1. Generate side recommendations based on the content of each module, and display them directly adjacent to that module's results. This would mirror the way top recommendations work in combined search, via include_recommendations config for each module. However I don't think the layout would be practical.
  2. Generate side recommendations based on the content of each module (as above), but display those recommendations in the usual sidebar. That's what I've implemented here, via a new include_recommendations_side config. The recommendations are initially rendered (invisible) within the results area, and moved to the sidebar via JS.
  3. Generate side recommendations based on the combined search results. This is usable for modules like EPF that do not actually depend on any results from the individual search modules. I've implemented it as well but commented it out for discussion purposes. It works via adding side[] elements within the [RecommendationModules] section of the config.

@maccabeelevine maccabeelevine marked this pull request as ready for review October 2, 2023 20:04
@maccabeelevine
Copy link
Member Author

  1. Generate side recommendations based on the content of each module (as above), but display those recommendations in the usual sidebar. That's what I've implemented here, via a new include_recommendations_side config. The recommendations are initially rendered (invisible) within the results area, and moved to the sidebar via JS.

I should call out a few assumptions / limitations of this approach:

  • At most one search module should enable any particular recommendation module. I can't think of a use case where you would want to duplicate recommendation modules. I do suppose there would be reasons to have a single recommendation module consider the results from more than one search, but I'd argue that should be done with some more advanced version of approach 3 above.
  • It's not clear in the UI which search module is producing which recommendation module. I don't think that matters, given the prior limitation.
  • The order of the recommendation modules is potentially dependent on the timing of AJAX responses from each search module, so it could vary between searches. I don't know that this is worth fixing but could be investigated.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @maccabeelevine, this seems like a reasonable start -- just see a few comments below.

Some other general ideas:

1.) I agree that having side recommendations by every individual column is unwieldy, though maybe there's some kind of approach where we have toggleable recommendations that expand or open in a lightbox in response to a button on each column. I'm fine with the current approach and am not suggesting that we need to do this -- just brainstorming it as another possible option that we could try.

2.) Regarding unpredictable loading order of side facets: what if, instead of creating one big placeholder where we move the recommendations, we instead create a separate container for each column? Then we could guarantee the order of the containers, which would make the order of the recommendations more predictable. I assume we have enough information in the template to match things up, though maybe I'm wrong about that -- I didn't review closely.

module/VuFind/src/VuFind/Controller/CombinedController.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Search/Combined/Options.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Controller/CombinedController.php Outdated Show resolved Hide resolved
themes/bootstrap3/templates/combined/results.phtml Outdated Show resolved Hide resolved
themes/bootstrap3/templates/combined/results-list.phtml Outdated Show resolved Hide resolved
@maccabeelevine
Copy link
Member Author

1.) I agree that having side recommendations by every individual column is unwieldy, though maybe there's some kind of approach where we have toggleable recommendations that expand or open in a lightbox in response to a button on each column. I'm fine with the current approach and am not suggesting that we need to do this -- just brainstorming it as another possible option that we could try.

@demiankatz Yeah that's an interesting thought. I can see that in the future if someone posits a particular recommendation module that really benefits from being associated directly with the individual search results. For my own needs (i.e. LibGuidesProfile), I'd rather the results be immediately visible and not associated with the individual search module, even though that's where the logic comes from. So as you say I'd rather not try this one, at least until I have a real use case.

2.) Regarding unpredictable loading order of side facets: what if, instead of creating one big placeholder where we move the recommendations, we instead create a separate container for each column? Then we could guarantee the order of the containers, which would make the order of the recommendations more predictable. I assume we have enough information in the template to match things up, though maybe I'm wrong about that -- I didn't review closely.

This is a good thought, I am worried about the complexity it might add but I want to look into it.

@demiankatz
Copy link
Member

Thanks, @maccabeelevine! I agree that there's no need to implement column-specific side recommendations without having a compelling use case first, so we can put that idea on the back burner for now. Good luck with the ordering of the recommendations. Let me know when you're ready for me to test/review further.

@maccabeelevine
Copy link
Member Author

@demiankatz I fixed the order of the sidebar recommendations. Was not too much work but I did have to do some cludgy string parsing with the module name, improvements welcome. Ready for any other review.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @maccabeelevine -- I think we may have ended up with slightly different visions here (or maybe I didn't express my original idea correctly). In any case, some thoughts below...

$columnSideRecommendations = [];
foreach ($config as $column => $subconfig) {
foreach ($subconfig['include_recommendations_side'] ?? [] as $recommendation) {
$columnSideRecommendations[] = strtok($recommendation, ':');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking of using the column name rather than the recommendation name as the identifier. I think it's entirely predictable that the column name will match the search class ID, whereas recommendation modules might be using aliases (or case-insensitive versions) that do not exactly match actual class names.

The disadvantage to using column name is that you'll run into conflicts with filtered columns, so the filter part would have to be stripped off and the results deduplicated... but I can't think of a sensible scenario where you would want to load different recommendation modules for different filtered versions of the same backend into a single column -- that would be a recipe for confusion -- so I'm not sure if it's a big problem.

And deduplication is still a problem to worry about if we stick with the recommendation-based approach, because there are several scenarios where the same recommendation module might be loaded multiple times, either in the same column or across multiple columns (e.g. there are some use cases where we load SideFacets multiple times with different configurations if we want to break up the facet list with different content in between the chunks).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem using column name (i.e. search backend name), at least by itself, is that it doesn't solve the sorting issue if you have more than one recommendation module for the same backend. As an example, the LibGuidesProfile recommendation module (in call number mode) doesn't work unless the SideFacets recommendations are loaded from the same Solr backend. I think there may also be a scenario (not handled yet) where we want the recommendations displayed in opposite order than the search backends they are generated from. I'm thinking specifically of displaying Books (Solr) before Articles, but in recommendations displaying databases based on the articles (a reocmmendation module just suggested to me this morning -- stay tuned!) ahead of the LibGuidesProfile generated by Solr call numbers. So for both reasons I think if we are going to stable sort, it should be by recommendation module.

I think I see what you are saying about the problem with aliases or capitalization differences. I didn't run into that with the five recommendation modules I tested, though, and I just compared the three lists (top, side, noresults) in searches.ini to the actual filenames of the recommendation modules, and I don't see any differences -- even crazy names like PubDateVisAjax match. If it comes up, would it be possible to create additional aliases for those?

That said, another approach could be to prefix the include_recommendation_side with an explicit order, i.e. include_recommendations_side[] = {sortIndex}:SideFacets:Results:CheckboxFacets. But I'm thinking that's overkill.

I agree that deduplication is a problem either way. I think the best approach here is just to add a note to the combined.ini explanation of include_recommendations_side saying not to do it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using the recommendation module class name is the best option, maybe the easiest solution is to actually look up the class names in CombinedController instead of using the short name. You could use the configuration to fetch the actual Recommendation object from VuFind\Recommend\PluginManager, and get its class name. This would introduce a small amount of overhead in terms of running constructors, but I don't think it would be very expensive (especially since the non-AJAX recommendations are going to get instantiated anyway, so no duplicate work will be done for those). You could then just md5 the class name to create the HTML element ID, and then you wouldn't have to do as much string manipulation in the template either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense and certainly avoids any bugs from future drift of the alias name and class names. Implemented. The only thing is I did a string replace of the backslashes to underscores instead of MD5, because I think it's better for cod readability, styling, etc. to have them legible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I'm out of office today, but I'll try to test this out later in the week.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best approach here is just to add a note to the combined.ini explanation of include_recommendations_side saying not to do it :)

Just did this as well.

themes/bootstrap3/templates/combined/results-list.phtml Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maccabeelevine, I was about to do some hands-on testing here, but in one final review of the code I noticed what might be a significant issue that we should probably resolve one way or the other before I proceed further.

module/VuFind/src/VuFind/Controller/CombinedController.php Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @maccabeelevine -- I tried this out and it looks great. Just a few minor, nitpicky thoughts from a final review. If you feel any of these are unnecessary or bad ideas, feel free to push back. Either way, I can get this approved as soon as they're resolved.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good to me now and is working in my test environment. Thanks, @maccabeelevine!

@demiankatz demiankatz merged commit 964902b into vufind-org:dev Oct 5, 2023
7 checks passed
@maccabeelevine maccabeelevine deleted the combined-side-recommendations branch October 5, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants