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.

Sign up for GitHub

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

Jump to bottom

Display side recommendations with combined search #3135

Merged
merged 18 commits into from Oct 5, 2023
Merged

Display side recommendations with combined search #3135

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
846c61f
Show side recommendations with combined search
maccabeelevine Oct 2, 2023
8810025
Fix php-cs-fixer
maccabeelevine Oct 2, 2023
030473e
Merge branch 'dev' into combined-side-recommendations
maccabeelevine Oct 3, 2023
c87ca5e
Fix else block style
maccabeelevine Oct 3, 2023
b10ae89
Fix typo
maccabeelevine Oct 3, 2023
ebe1e73
Fix noRecommend logic
maccabeelevine Oct 3, 2023
f5b18ab
Change join to implode
maccabeelevine Oct 3, 2023
e502ff2
Remove code for approach 3, discarded for now.
maccabeelevine Oct 3, 2023
3a3d110
Fix php-cs-fixer
maccabeelevine Oct 3, 2023
0e8750f
Force order of recommendation modules
maccabeelevine Oct 3, 2023
0fdbc46
Use actual recommendation module class name
maccabeelevine Oct 4, 2023
c38257d
Warn not to duplicate recommendation modules
maccabeelevine Oct 5, 2023
1dc9b02
Remove reference to include_recommendations_side = true
maccabeelevine Oct 5, 2023
db74e0b
Merge branch 'dev' into combined-side-recommendations
demiankatz Oct 5, 2023
d1a8321
Remove unused variable
maccabeelevine Oct 5, 2023
881b1c5
Protect against recommendation errors
maccabeelevine Oct 5, 2023
270cbda
Merge branch 'combined-side-recommendations' of https://github.com/ma…
maccabeelevine Oct 5, 2023
0a71c70
Catch non-array
maccabeelevine Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions config/vufind/combined.ini
View file
Open in desktop
Expand Up @@ -28,6 +28,11 @@
; specified recommendations will be loaded into the top
; area of the column. If set to false, recommendations
; will be suppressed (default).
; include_recommendations_side = If set to true, standard 'side' recommendations
; will be displayed in the sidebar column. If set to an array of
; recommendation settings (as per searches.ini), the specified recommendations
; will be loaded into the side column. If set to false, side recommendations
; will be suppressed (default).
; filter = One or more filters to apply to search results displayed in the column.
; Use multiple "filter[] = ..." lines if multiple filters are needed.
; hiddenFilter = One or more hidden filters to apply to search results displayed in
Expand Down
34 changes: 24 additions & 10 deletions module/VuFind/src/VuFind/Controller/CombinedController.php
View file
Open in desktop
Expand Up @@ -225,6 +225,14 @@ public function resultsAction()
$settings = $this->serviceLocator->get(\VuFind\Config\PluginManager::class)
->get('config');

// Identify if any modules use include_recommendations_side.
$columnSideRecommendations = [];
foreach ($config as $column => $subconfig) {
foreach ($subconfig['include_recommendations_side'] ?? [] as $recommendation) {
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
$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.

}
}

// Build view model:
return $this->createViewModel(
[
Expand All @@ -237,6 +245,7 @@ public function resultsAction()
'supportsCart' => $supportsCart,
'supportsCartOptions' => $supportsCartOptions,
'showBulkOptions' => $settings->Site->showBulkOptions ?? false,
'columnSideRecommendations' => $columnSideRecommendations,
]
);
}
Expand Down Expand Up @@ -325,18 +334,23 @@ protected function adjustQueryForSettings($settings, $searchType = null)
// Override the search type:
$query->type = $searchType;

// Always leave noresults active (useful for 0-hit searches) and
// side inactive (no room to display) but display or hide top based
// on include_recommendations setting.
if ($settings['include_recommendations'] ?? false) {
$query->noRecommend = 'side';
if (is_array($settings['include_recommendations'])) {
$query->recommendOverride
= ['top' => $settings['include_recommendations']];
}
// Always leave noresults active (useful for 0-hit searches).
// Display or hide top based on include_recommendations setting.
// Display or hide side based on include_recommendations_side setting.
$recommendOverride = [];
$noRecommend = [];
if (is_array($settings['include_recommendations'] ?? false)) {
$recommendOverride['top'] = $settings['include_recommendations'];
} else {
$noRecommend[] = 'top';
}
if (is_array($settings['include_recommendations_side'] ?? false)) {
$recommendOverride['side'] = $settings['include_recommendations_side'];
} else {
$query->noRecommend = 'top,side';
$noRecommend[] = 'side';
}
$query->recommendOverride = $recommendOverride;
$query->noRecommend = count($noRecommend) ? implode(',', $noRecommend) : false;
}

/**
Expand Down
22 changes: 22 additions & 0 deletions themes/bootstrap3/templates/combined/results-list.phtml
View file
Open in desktop
Expand Up @@ -68,6 +68,28 @@
<?php foreach (($top = $results->getRecommendations('top')) as $current): ?>
<?=$this->recommend($current)?>
<?php endforeach; ?>
<?php foreach (($side = $results->getRecommendations('side')) as $current):
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
$recommendationClass = array_reverse(explode('\\', $current::class))[0];
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
$recommendationJs = <<<JS
function addRecommendation_$recommendationClass() {
const recommendationContainer = document.querySelector('#search-sidebar .recommendation_container__$recommendationClass');
const recommendation = document.getElementById('recommendation__$recommendationClass');
recommendationContainer.append(recommendation);
demiankatz marked this conversation as resolved.
Show resolved Hide resolved
recommendation.style.display = 'inherit';
}

if (document.readyState != 'loading') {
addRecommendation_$recommendationClass();
} else {
document.addEventListener('DOMContentLoaded', addRecommendation_$recommendationClass);
}
JS;
?>
<?=$this->inlineScript(\Laminas\View\Helper\HeadScript::SCRIPT, $recommendationJs, 'SET')?>
<div id="recommendation__<?=$recommendationClass?>" style="display: none;">
<?=$this->recommend($current)?>
</div>
<?php endforeach; ?>
<?=
$this->context()->renderInContext(
'search/controls/showing.phtml',
Expand Down
18 changes: 17 additions & 1 deletion themes/bootstrap3/templates/combined/results.phtml
View file
Open in desktop
Expand Up @@ -73,7 +73,23 @@
'showBulkOptions' => $this->showBulkOptions,
];
?>
<?=$this->context($this)->renderInContext('combined/stack-' . $placement . '.phtml', $viewParams); ?>

<?php if (!empty($columnSideRecommendations)): ?>
<div class="<?=$this->layoutClass('mainbody')?>">
<?=$this->context($this)->renderInContext('combined/stack-' . $placement . '.phtml', $viewParams); ?>
</div>

<div class="<?=$this->layoutClass('sidebar')?>" id="search-sidebar">
<?php foreach ($columnSideRecommendations as $columnSideRecommendation): ?>
<div class="recommendation_container__<?=$columnSideRecommendation?>">
<?php // Content is added via JS in results-list.phtml. ?>
</div>
<?php endforeach; ?>
</div>
<?php else: ?>
<?=$this->context($this)->renderInContext('combined/stack-' . $placement . '.phtml', $viewParams); ?>
<?php endif; ?>

<?php $recs = $combinedResults->getRecommendations('bottom'); ?>
<?php if (!empty($recs)): ?>
<div>
Expand Down