-
Notifications
You must be signed in to change notification settings - Fork 359
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
Recommend databases based on result facets #3160
Conversation
I marked this for 10.0, given the new translation, and also the lack of time. :) |
languages/en.ini
Outdated
eds_databases = "Search Relevant Databases" | ||
eds_databases_intro = "Search directly in databases for enhanced functionality." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggested language comes from an instruction librarian colleague. But discussion welcome. I asked her to consider how to concisely explain to patrons something that's not at all obvious:
- This is not a facet to narrow down these results, like the list of sources on the Articles tab. This will actually open up the database.
- That said, the list of databases is still drawn from the EDS results to the left.
- Given that in theory all content of those databases is indexed by EDS and available here via the API, what (concisely) is the reason someone should exit to the database UI?
foreach ($records as $record) { | ||
$databaseInfo = $nameToDatabase[$record->getDbLabel()] ?? null; | ||
if ($databaseInfo) { | ||
$databases[$record->getDbLabel()] = $databaseInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is of course ridiculous to use the actual database name/label as the match point between EDS and LibGuides AZ, but without some custom configuration there doesn't seem to be any other option. In practice things are not always named the same, and LibGuides AZ may not be complete. One silver lining is that LibGuides has a second "alternate titles" field that I may use as well, to see if the EDS label is a substring of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments and ideas...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great progress on this... but now I'm wondering if we could make this even more generic and flexible. Is there really any reason this needs to be EDS-specific? Couldn't this now work with any search backend that returns a facet format consistent with EDS (e.g. Solr/Primo)? Obviously, as of right now, EDS may be the only system that provides a "Database" facet, but I imagine that might be a possibility elsewhere now or in future, and I can think of other possible use cases (e.g. use the high-level call number facets and map those to relevant subject-specific databases).
Broadening this to more use cases would require a few more relatively minor adjustments:
1.) Allow the facet field name to be specified somewhere (either directly in the recommendation module configuration line, or through a secondary setting somewhere).
2.) Allow the configuration file and section to be configured through the recommendation module configuration line.
3.) Rename a few things to get rid of "EDS".
4.) Add a new text domain for translating database names -- e.g. in the template, change $this->escapeHtml($name)
to $this->transEsc("DatabaseNames::$name")
. We wouldn't need to populate this with anything by default, but it would allow my "call number to database" idea to be implemented.
If I'm overlooking some reason why this isn't actually feasible, or if you think some parts of my idea are good and others are excessive, that's fine. I'm also open to merging this as-is. But I think if there's a reasonable opportunity to make this as reusable and flexible as possible, we might as well do that up front rather than refactoring later, if it's not too much work!
@demiankatz I decided this made a lot of sense.
Done.
I did this for the configuration file. I don't see a real need to make the section name itself a configuration option vs the complication that adds to the code.
I didn't do this, not that it's a bad idea, but I think it's a change we could make in the future if someone needed it that would be non-breaking. |
Thanks! I'll give this a closer look tomorrow, and get it merged then if it's fully ready.
Makes sense!
Agreed -- also an easy local template customization if there is a need for it. |
Yes there's one more enhancement I want to do (so far) but I think it's best to implement in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, @maccabeelevine -- I didn't end up having time to do hands-on testing today, but I did find a few more things to comment on in another eyeball review of the code. I think once these things are addressed (or you decide not to address them), it should be ready for a final hands-on review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to continuing our conversation below, I also happened to spot another small typo... :-)
GitHub seems to have hid my response to this after I changed the code enough that it can't locate where the comments apply. In short, I agree with not breaking the app and I changed this to use logging instead. Not using null coalescing though because I would need to check for the coalesced result anyway, I think exception handling is cleaner for this situation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is starting to look pretty solid to me, so I started hands-on testing... and immediately ran into a problem! :-) See details below...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @maccabeelevine -- I pushed up one small commit to add some missing styles so the icon/link combinations in your template display correctly (otherwise, there's an underlined space between the icon and the text). Other than that, this looks good to me, so I'm going to merge it now!
This introduces a recommendation module to recommend research databases based on EDS results. It loops through the results list to aggregate a list of databases (
$record->getDbLabel()
). Each listed database is a link to the individual database front-end.So, this is different than the facet that would narrow down the results to that database within VuFind. Why: Instruction librarians may teach students to use a particular research database for DB-specific functionality, discipline-specific terminology, etc. When a student doesn't know what database(s) to use, this allows VuFind to be the first stop and recommend those databases.
Unfortunately the EDS API does not provide an actual link to the database UI. It can be inferred (using a code) if the database is hosted on EBSCOhost, but that's no guarantee. So this module loads the institution's list of databases from LibGuidesAZ (via its API -- not the search box method used in the LibGuidesAZ recommendation module), caches it, and uses LibGuidesAZ data to associate the database with a URL.
TODO