Create integration with Libkey #303

matt-bernhardt · 2025-12-10T16:54:13Z

The need

The application needs to look up DOI or PMID values using the Libkey service in order to find the best possible fulfillment links for records that come back from our various indices.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/use-232

The response

This defines a route, controller, model, and view template that will receive a DOI or PMID, look them up in Libkey, and return the suggested link.

Libkey's response object includes a "best integrator link", which we are choosing here to accept uncritically.

There are two checks performed at various levels of this integration:

The controller makes sure that Libkey is enabled, by making sure that the proper env vars have been defined.
The model makes sure that the proper parameters have been passed - we require both a type (either "doi" or "pmid") and an identifier. The format of the identifier is not specified. This value is used to create the URL that is sent to Libkey, as part of the URL path.

The env vars created here are optional, because GeoData doesn't use this integration.

Document any side effects to this change:

Rubocop is flagging the lookup method as being too complex and too long. I'm not sure the best way to resolve this, other than to split it into "part A, setup" and "part B, communication" - but this feels like it might be a distinction without a difference. Maybe a common "external connection" model class could inform both the Tacos and Libkey models, since they both use similar error handling patterns?

There is a third value in the Libkey response, "link type", which is currently unused. I've flagged it as potentially being unneeded, but wanted to make sure Dave / UX knows it exists.

Developer

Accessibility

The accessibility of this change will be checked in the next PR, which places the returned link in context of the search results.

New ENV

All new ENV is documented in README.
ENV has been populated in the review app, but not in the pipeline or other tiers.
ENV has not changed.

Approval beyond code review

UXWS/stakeholder approval has been confirmed.
UXWS/stakeholder review will be completed retroactively.
UXWS/stakeholder review is not needed.

Additional context needed to review

E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.

Code Reviewer

Code

I have confirmed that the code works as intended.
Any CodeClimate issues have been fixed or confirmed as
added technical debt.

Documentation

The commit message is clear and follows our guidelines
(not just this pull request message).
The documentation has been updated or is unnecessary.
New dependencies are appropriate or there were no changes.

Testing

There are appropriate tests covering any new functionality.
No additional test coverage is required.

coveralls · 2025-12-10T16:55:56Z

Pull Request Test Coverage Report for Build 20148462907

Details

41 of 41 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 98.207%

Totals
Change from base Build 20147708147:	0.07%
Covered Lines:	1150
Relevant Lines:	1171

💛 - Coveralls

JPrevost

I'm approving but think we may need more details on whether a single link or a series of links is what we are expecting here. We can assume your interpretation of the work is what is desired or map additional links coming back into a data structure where we can select which ones to display in the UI now... your call.

JPrevost · 2025-12-10T20:15:12Z

app/models/libkey.rb

+    return unless %w[doi pmid].include?(type)
+
+    url = libkey_url(type, identifier)
+    Rails.logger.debug(url)


Non-blocking: we don't often leave debug statements in the code. I'm not against starting to leave some in and expecting devs to set their log_level appropriately if they want to see them but that may be best to discuss more broadly before leaving these in. (I'm sure there are debug lines in our code, but I think in general we pull them out before merge)

Eh... there may be a better way to handle this, having now dealt with the Sentry integration in the next part of the ticket.

My thinking was in preparation for the branch just below this where we receive a non-200 response status from Libkey (really, statuses beyond 200 or 404 - which are both expected in normal operations). It doesn't have to be a message in the logs, though. I'm open to defining how we want to find out about events like this, but do think we should hear about it.

I'm treating this comment as blocking for now, until I have an understanding of how we want the application to behave when Libkey responds with something other than a 200 or 404 response status.

I've refactored this approach, and would be interested in your review on the new code. The debugging lines are both removed, and the branch that handles non-200 responses from Libkey now sends a message and some metadata to Sentry for analysis.

JPrevost · 2025-12-10T20:16:10Z

app/models/libkey.rb

+    {
+      link: external_data['data']['bestIntegratorLink']['bestLink'],
+      text: external_data['data']['bestIntegratorLink']['recommendedLinkText'],
+      type: external_data['data']['bestIntegratorLink']['linkType'] # Not sure whether this belongs here.


I'm not sure what this comment is referring to now and I suspect it will be even more confusing later...more context or removing the comment is likely to lead to less confusion later.

The response from Libkey includes three fields, but currently we're only using two in the payload back to the UI (link and text).

I suppose the simplest posture would be to just drop type entirely, but talk with UX about what they want out of the larger API response object (we're only plucking the best integration link for now, but there's a lot more in there).

This thread is related in my mind to the others below it, which all deal with "what does UX want to appear from Libkey"

JPrevost · 2025-12-10T20:18:42Z

app/views/libkey/lookup.html.erb

@@ -0,0 +1,3 @@
+<% if Libkey.enabled? && @libkey.present? %>
+  <%= link_to( @libkey[:text], @libkey[:link], class: 'button button-primary' ) %>


I assume we'll style this differently in upcoming PRs (either the initial integration or Dave's follow up). We'll also have multiple links available right? HTML, PDF like Primo UI has?

The ticket for this says that styling will happen afterwards, so I'm choosing to ignore that issue for now.

The thing I'm not sure about, but which will be part of the handoff with Dave in the following PR, is whether the Libkey integration should result in only one Libkey link or not. There will already be others that come back from Primo / TIMDEX, but my understanding of the Libkey integration was that we wanted to keep things simple and just show one libkey link (the "best integration link" in their response).

If that needs to be adjusted, my preference is to have that conversation as part of the UI integration in the next PR.

JPrevost · 2025-12-10T20:24:35Z

app/models/libkey.rb

+    return unless external_data['data']['bestIntegratorLink']
+
+    {
+      link: external_data['data']['bestIntegratorLink']['bestLink'],


Are we only getting bestLink back or are we also seeing multiple links and only using bestLink in this implementation?

It's unclear to me what the intended USE UI behavior is (i.e. just Best Link or also HTML, Browse links, etc)

The API response includes many more fields, but my thought for now was to start with the best integration link only. The next PR will have a more involved review that includes UX weighing in about their presentation and UX requirements.

matt-bernhardt · 2025-12-10T21:15:33Z

Thanks for your comments, Jeremy. I see the approval, but wanted to try and come to an agreement about how /where to notify in the case that we get a response status from Libkey that's not 200 or 404 (following up on the debug statement I left in the model, which looking now isn't the right way to handle this).

The other three comments I'd like to postpone for now, and discuss those in the context of the eventual UI integration in the next PR.

JPrevost · 2025-12-11T21:41:10Z

@matt-bernhardt sentry integration looks like what we've done before and I'm fine with tweaks when you do the UI bits.

** Why are these changes being introduced: The application needs to look up DOI or PMID values using the Libkey service in order to find the best possible fulfillment links for records that come back from our various indices. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-232 ** How does this address that need: This defines a route, controller, model, and view template that will receive a DOI or PMID, look them up in Libkey, and return the suggested link. Libkey's response object includes a "best integrator link", which we are choosing here to accept uncritically. There are two checks performed at various levels of this integration: - The controller makes sure that Libkey is enabled, by making sure that the proper env vars have been defined. - The model makes sure that the proper parameters have been passed - we require both a type (either "doi" or "pmid") and an identifier. The format of the identifier is not specified. This value is used to create the URL that is sent to Libkey, as part of the URL path. The env vars created here are optional, because GeoData doesn't use this integration. ** Document any side effects to this change: Rubocop is flagging the lookup method as being too complex and too long. I'm not sure the best way to resolve this, other than to split it into "part A, setup" and "part B, communication" - but this feels like it might be a distinction without a difference. Maybe a common "external connection" model class could inform both the Tacos and Libkey models, since they both use similar error handling patterns? There is a third value in the Libkey response, "link type", which is currently unused. I've flagged it as potentially being unneeded, but wanted to make sure Dave / UX knows it exists.

mitlib temporarily deployed to timdex-ui-pi-use-232-pa-uhkllz December 10, 2025 16:56 Inactive

matt-bernhardt requested review from JPrevost and jazairi December 10, 2025 17:06

JPrevost self-assigned this Dec 10, 2025

JPrevost approved these changes Dec 10, 2025

View reviewed changes

mitlib temporarily deployed to timdex-ui-pi-use-232-pa-uhkllz December 11, 2025 19:24 Inactive

matt-bernhardt added 2 commits December 11, 2025 16:46

Update logging and libkey status handling

b139640

matt-bernhardt force-pushed the use-232-part1 branch from f17e92d to b139640 Compare December 11, 2025 21:47

mitlib temporarily deployed to timdex-ui-pi-use-232-pa-uhkllz December 11, 2025 21:47 Inactive

matt-bernhardt merged commit 357dd9f into main Dec 11, 2025
5 checks passed

matt-bernhardt deleted the use-232-part1 branch December 11, 2025 21:48

		@@ -0,0 +1,3 @@
		<% if Libkey.enabled? && @libkey.present? %>
		<%= link_to( @libkey[:text], @libkey[:link], class: 'button button-primary' ) %>

Create integration with Libkey #303

Create integration with Libkey #303

Uh oh!

Conversation

matt-bernhardt commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The need

The response

Document any side effects to this change:

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

coveralls commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 20148462907

Details

💛 - Coveralls

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matt-bernhardt commented Dec 10, 2025

Uh oh!

JPrevost commented Dec 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matt-bernhardt commented Dec 10, 2025 •

edited

Loading

coveralls commented Dec 10, 2025 •

edited

Loading