Adjust fulfillment link logic #322

JPrevost · 2025-12-18T19:22:05Z

Why are these changes being introduced:

We prefer LibKey links when available, but they are not always available.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/USE-286

How does this address that need:

Adjusts links we extract from Primo to include HTML and PDF direct links when available.
Updates Primo OpenURL logic and adjust label to "Check Availability"
If LibKey links become available, they will be used instead of the direct links from Primo by hiding them from the UI.

Document any side effects to this change:

Hiding links after they are in the UI is not ideal, but it is a workaround to ensure that we do not display duplicate links in the UI. This will be revisited when we have more time.
Introduced a new required ENV variable PRIMO_INST_ID. This was previously named differently and not included in the readme or deployed applications so our OpenURL links we broken.

Developer

Accessibility

ANDI or WAVE has been run in accordance to our guide.
This PR contains no changes to the view layer.
New issues flagged by ANDI or WAVE have been resolved.
New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
No new accessibility issues have been flagged.

New ENV

All new ENV is documented in README.
All new ENV has been added to Heroku Pipeline, Staging and Prod.
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-18T19:23:53Z

Pull Request Test Coverage Report for Build 20434217342

Details

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

Totals
Change from base Build 20434159766:	0.04%
Covered Lines:	1191
Relevant Lines:	1216

💛 - Coveralls

matt-bernhardt

I'm approving this, but have some questions that I'd like to ask while I'm looking at this. Three of them are very much non-blocking, about potential follow-up work and about the background of the data format we're parsing. The fourth is a little bit bigger, about having commented out the ctx parameter handling. I don't mind merging as is, but I'm not sure what the context of this change is, so I'm not sure if this is a temporary change or the prelude to full removal.

My final note is about an assumption I'm making - that there's going to be a styling pass by @djanelle-mit after this, because one impact of the button removals is that we're getting irregular formatting of these links now (this is the search for "exoplanet signature detection methods", about halfway down the current resuls)

matt-bernhardt · 2025-12-19T16:01:21Z

app/models/normalize_primo_record.rb


+  # Parses a link string into a hash of key-value pairs.
+  # The link string is a series of key-value pairs separated by $$, where each pair is prefixed by a single character.
+  # For example: "$$Uhttps://example.com$$TView PDF" would be parsed into { 'U' => 'https://example.com', 'T' => 'View PDF' }


Non-blocking question:
Is this construct documented anywhere? How stable is it? My assumption is that this isn't something new, but I'm unfamiliar with it as a format.

I like having a method that will decompose a single string like this into its component parts, and agree with not trying to pluck out values in this method - that happens in the context of whatever logic is fishing for something.

There is no documentation anywhere, we've never used this in Bento so it is new to our applications with this code.

The Primo API doesn't seem to ever change in terms of new features being added and the documentation is genuinely awful. Nearly everything we do with it is reverse engineered and this is sadly no different. This seems to work... but we have no idea if they ever meant it to work this way. If it stops working, we should still get our OpenURL links which do not rely on this logic. These are basically "extra" links that often (but not always) get replaced with LibKey links.

matt-bernhardt · 2025-12-19T16:06:58Z

app/models/normalize_primo_record.rb

    # Search API to redirect to the Primo UI. This is done for UX purposes,
    # as the regular Alma link resolver URLs redirect to a plaintext
    # disambiguation page.
    primo_openurl_base = [ENV.fetch('MIT_PRIMO_URL', nil),


Non-blocking comment:

This is probably not this ticket, and may be part of the refactoring that Isra is looking at, but I'm realizing that we're doing very similar things in this method and the record_link method, but each implementation is slightly different.

I don't know if it needs to be, but part of me wants to recommend a ticket to refactor how we build links into Primo, to try and adopt a common syntax / approach.

I have mixed feelings about this and have only had two sips of coffee so far so this may make more sense later...

Isn't one creating an API link and another creating UI links? While they may be similar, I don't think they'll ever be the same and abstracting the things that are similar when things are distinct feels like it could get confusing rather than simplifying over time. I'm not against looking into this further, but I want us to consider it from both a "DRY" and a "is this really repeating even if it looks like it" perspective.

matt-bernhardt · 2025-12-19T16:13:18Z

app/models/normalize_primo_record.rb

    # The ctx params appear to break Primo openurls, so we need to remove them.
-    params = Rack::Utils.parse_nested_query(primo_openurl)
-    filtered = params.delete_if { |key, _value| key.starts_with?('ctx') }
+    # params = Rack::Utils.parse_nested_query(primo_openurl)


Question:
I'm assuming that we're commenting out this filtering because it is no longer needed, and ctx parameters are okay to include. That said, if this is really the case it seems like we could just update line 328 to be URI::DEFAULT_PARSER.unescape(primo_openurl.to_param) and drop lines 324-327 entirely?

Doh!... I meant to go back and do additional testing. I couldn't find an example where the old conditions made sense but hadn't done exhaustive testing. I'll either update this after doing additional testing or open a ticket to look into this further.

@matt-bernhardt I've opened a ticket to do more testing of the OpenURLs to see if we can find any that don't work with this simpler logic. Success criteria are either to revert to stripping ctx or cleaning up the logic if we confirm we no longer need to strip that.

matt-bernhardt · 2025-12-19T16:42:48Z

app/javascript/controllers/content_loader_controller.js

+          const resultGet = this.element.closest('.result-get');
+          if (resultGet) {
+            const primoLinks = resultGet.querySelectorAll('.primo-link');
+            primoLinks.forEach(link => link.style.display = 'none');


Non-blocking comment:
This is probably a question for @djanelle-mit , but I'm wondering if we can track these removals somehow, to know how often they occur. The one test search I did for exoplanet signature detection methods returned 15 different primo links, most of which ended up being suppressed, although some remained.

My guess at a question around this would be something like: "How many times do links with the primo-link class appear on a results page? How many times to links with that class get removed from a results page?" . I don't remember what trigger options exist for Matomo tag containers, so I'm not sure what this might look like in practice.

If we're going to leave a structure like this in place for long, however, I'd like to consider having it be something that we record analytics about.

Add it to the analytics doc! I'm still operating with an assumption that we can detect if an element with a particular selector was loaded (and not seen), let's make sure we try to track this.

djanelle-mit · 2025-12-19T18:00:11Z

@matt-bernhardt I saw your comment re: styling pass. Once this merges I can tweak the CSS logic. With Jeremy out today and me out after today, would it make sense for you to merge this?

Why are these changes being introduced: * We prefer LibKey links when available, but they are not always available. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-286 How does this address that need: * Adjusts links we extract from Primo to include HTML and PDF direct links when available. * Updates Primo OpenURL logic and adjust label to "Check Availability" * If LibKey links become available, they will be used instead of the direct links from Primo by hiding them from the UI. Document any side effects to this change: * Hiding links after they are in the UI is not ideal, but it is a workaround to ensure that we do not display duplicate links in the UI. This will be revisited when we have more time. * Introduced a new required ENV variable `PRIMO_INST_ID`. This was previously named differently and not included in the readme or deployed applications so our OpenURL links we broken.

mitlib temporarily deployed to timdex-ui-pi-use-286-op-pmmrr9 December 18, 2025 19:24 Inactive

matt-bernhardt self-assigned this Dec 18, 2025

JPrevost requested a review from matt-bernhardt December 18, 2025 20:40

JPrevost force-pushed the use-286-openurl branch from 6de9fc0 to 12a9b92 Compare December 18, 2025 21:30

JPrevost temporarily deployed to timdex-ui-pi-use-286-op-pmmrr9 December 18, 2025 21:30 Inactive

JPrevost force-pushed the use-286-openurl branch from 12a9b92 to 11148ff Compare December 18, 2025 21:47

JPrevost temporarily deployed to timdex-ui-pi-use-286-op-pmmrr9 December 18, 2025 21:47 Inactive

matt-bernhardt approved these changes Dec 19, 2025

View reviewed changes

JPrevost force-pushed the use-286-openurl branch from 11148ff to adae97f Compare December 22, 2025 14:07

JPrevost temporarily deployed to timdex-ui-pi-use-286-op-pmmrr9 December 22, 2025 14:08 Inactive

JPrevost merged commit a584d35 into main Dec 22, 2025
5 checks passed

JPrevost deleted the use-286-openurl branch December 22, 2025 15:17

Adjust fulfillment link logic #322

Adjust fulfillment link logic #322

Uh oh!

Conversation

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

Uh oh!

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 20434217342

Details

💛 - Coveralls

Uh oh!

matt-bernhardt 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!

djanelle-mit commented Dec 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JPrevost commented Dec 18, 2025 •

edited by matt-bernhardt

Loading

coveralls commented Dec 18, 2025 •

edited

Loading