Add support for Primo NDE links by jazairi · Pull Request #367 · MITLibraries/timdex-ui

jazairi · 2026-03-06T18:56:52Z

Why these changes are being introduced:

There is a need to selectively enable the Primo NDE feature.

Relevant ticket(s):

USE-367

How this addresses that need:

This adds a Primo NDE feature that allows us to enable NDE links for searches and full records. Primo links are now abstracted to a Primo Link Builder to reduce duplication.

Side effects of this change:

The Primo Link Builder is the latest example of
a service being stored in the models/ directory. We may want to consider moving our services to a
separate directory for clarity's sake.

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. (N/A: NDE feature will be added selectively, as needed)
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

See the ticket for example NDE links. The links in the PR build, which has the feature enabled, should match these.

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.

Why these changes are being introduced: There is a need to selectively enable the Primo NDE feature. Relevant ticket(s): - [USE-367](https://mitlibraries.atlassian.net/browse/USE-367) How this addresses that need: This adds a Primo NDE feature that allows us to enable NDE links for searches and full records. Primo links are now abstracted to a Primo Link Builder to reduce duplication. Side effects of this change: The Primo Link Builder is the latest example of a service being stored in the `models/` directory. We may want to consider moving our services to a separate directory for clarity's sake.

coveralls · 2026-03-06T18:58:46Z

Pull Request Test Coverage Report for Build 22870066925

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

29 of 29 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 98.166%

Totals
Change from base Build 22584961347:	0.03%
Covered Lines:	1338
Relevant Lines:	1363

💛 - Coveralls

qltysh · 2026-03-06T18:58:56Z

❌ 2 blocking issues (2 total)

Tool	Category	Rule	Count
rubocop	Lint	Class has too many lines. [255/100]	1	❌
rubocop	Lint	Method has too many lines. [11/10]	1	❌

app/models/primo_link_builder.rb

JPrevost

Added a few questions/comments to consider before we wrap this up

JPrevost · 2026-03-09T12:44:00Z

app/helpers/results_helper.rb

-                          search_scope: 'all',
-                          vid: ENV.fetch('PRIMO_VID')
-                        })
+    PrimoLinkBuilder.new(query_term: params[:q]).search_link


Nice use of the builder pattern. It does a great job consolidating this logic and making it more testable.

JPrevost · 2026-03-09T12:51:06Z

test/helpers/search_helper_test.rb

  test 'primo_search_url generates correct Primo URL' do
    query = 'machine learning'
-    expected_url = 'https://mit.primo.exlibrisgroup.com/discovery/search?query=any%2Ccontains%2Cmachine+learning&vid=01MIT_INST%3AMIT'
+    expected_url = 'https://mit.primo.exlibrisgroup.com/discovery/search?query=any%2Ccontains%2Cmachine+learning&tab=all&search_scope=all&vid=01MIT_INST%3AMIT'


Were the links returned by this helper previously incorrect?

Not exactly. The expected url string excluded a couple of params, which I don't think are necessary, but it made sense to me to include them since Primo is such a black box.

JPrevost · 2026-03-09T12:56:42Z

app/models/primo_link_builder.rb

+#   builder.full_record_link
+#   # => "https://mit.primo.exlibrisgroup.com/discovery/fulldisplay?docid=alma123&..."
+class PrimoLinkBuilder
+  def initialize(query_term: nil, record_id: nil, context: nil)


It would be helpful to explain these input variables. While query_term and record_id are easily understandable, I'm genuinely not sure what context is.

Context is used for the display endpoint only and indicates whether it's Alma/local ("L") or CDI (uhh...something else). Will document.

JPrevost · 2026-03-09T13:03:37Z

app/models/primo_link_builder.rb

+  # @param tab [String] The Primo tab parameter (default: 'all')
+  # @param search_scope [String] The search scope (default: 'all')
+  # @return [String] The complete search URL
+  def search_link(tab: 'all', search_scope: 'all')


Is there a reason the search_link and full_record_link methods use different defaults? Is it possible they both might want to default to whatever the tab is set at unless it is not set then use all like ENV.fetch('PRIMO_TAB', 'all)?

There is good documentation as to what tab, scope, and vid do in our primo_search model. I'm not sure if the best play is to note that for details as to what they are to see that model or to copy it here. Neither are ideal.

Good point. In fact, both tab and scope can probably come from env.

JPrevost · 2026-03-09T13:06:21Z

test/models/primo_link_builder_test.rb

+    end
+  end
+
+  test 'full_record_link generates production URL by default' do


discovery and production being used interchangeably in test names might confuse us once NDE becomes production. Suggestion: update this test to use discovery URL naming

JPrevost · 2026-03-09T13:20:30Z

test/models/primo_link_builder_test.rb

+# Test NDE link generation
+require 'test_helper'
+
+class PrimoLinkBuilderTest < ActiveSupport::TestCase


I suspect the intent of testing the way we are here is to avoid fragile tests, but it makes me nervous not to have any full link testing. I think most of these are fine as is, but maybe one more test for each type of link "search_link|full_record_link generates a full link as expected" or something?

jazairi · 2026-03-09T19:01:11Z

@JPrevost Let me know if the updated docs are better or worse. (I'm not sure one way or the other.)

qltysh · 2026-03-09T19:02:49Z

app/models/primo_link_builder.rb

+      lang: lang,
+      tab: tab
+    }
+    base_url + URI.encode_www_form(params)


Method has too many lines. [11/10] [rubocop:Metrics/MethodLength]

mitlib temporarily deployed to timdex-ui-pi-use-367-rdwdhoc9m March 6, 2026 18:58 Inactive

qltysh bot reviewed Mar 6, 2026

View reviewed changes

app/models/primo_link_builder.rb Show resolved Hide resolved

jazairi assigned JPrevost and matt-bernhardt Mar 6, 2026

jazairi requested review from JPrevost and matt-bernhardt March 6, 2026 22:10

jazairi unassigned JPrevost and matt-bernhardt Mar 6, 2026

JPrevost self-assigned this Mar 9, 2026

JPrevost reviewed Mar 9, 2026

View reviewed changes

Address code review feedback

d0a5784

jazairi requested a review from JPrevost March 9, 2026 19:00

jazairi temporarily deployed to timdex-ui-pi-use-367-rdwdhoc9m March 9, 2026 19:01 Inactive

qltysh bot reviewed Mar 9, 2026

View reviewed changes

Update tests to expect scope from env

21e1518

jazairi deployed to timdex-ui-pi-use-367-rdwdhoc9m March 9, 2026 19:04 View deployment

Conversation

jazairi commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why these changes are being introduced:

Relevant ticket(s):

How this addresses that need:

Side effects of this change:

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

coveralls commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 22870066925

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

qltysh bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

jazairi commented Mar 9, 2026

Uh oh!

qltysh bot Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jazairi commented Mar 6, 2026 •

edited

Loading

coveralls commented Mar 6, 2026 •

edited

Loading

qltysh bot commented Mar 6, 2026 •

edited

Loading