Add models for Primo Search API integration - #227
Merged
Conversation
Pull Request Test Coverage Report for Build 18170449977Details
💛 - Coveralls |
JPrevost
reviewed
Sep 30, 2025
JPrevost
reviewed
Sep 30, 2025
JPrevost
left a comment
Member
There was a problem hiding this comment.
Initial feedback. Will formalize the review when you make the changes to normalizer you noted in Slack yesterday.
Contributor
Author
|
@JPrevost I ended up pushing the changes in a few separate commits. The first fixes the bulk of the initial feedback. The second involved renaming the NormalizePrimo model, so it felt useful to make it a separate commit for ease of review. The last two commits are just things I forgot to address in the first commit. 🙂 Let me know if I missed anything, or if anything else comes up in the remainder of the review. Thanks! |
JPrevost
approved these changes
Oct 1, 2025
Why these changes are being introduced: The first iteration of USE UI will include the ability to toggle between Ex Libris CDI and TIMDEX results. Relevant ticket(s): * [USE-30](https://mitlibraries.atlassian.net/browse/USE-30) How this addresses that need: This adds a Primo Search model that calls the Primo Search API, a Normalize Primo Record model parses the each result and returns a normalized record, and a Normalize Primo Results model that batches record normalization. Side effects of this change: * For better or worse, much of the normalization code has been repurposed from Bento. This felt acceptable as we do not anticipate this to be a long-term solution. However, when we also normalize TIMDEX records, we'll want to revisit the normalized fields. * While working on this, I noticed that most of the TIMDEX normalization logic happens in the views. It would be better to move this to a normalization model, as we've done here with Primo. I opened [USE-73](https://mitlibraries.atlassian.net/browse/USE-73) to address this. * It's not technically a side effect of this changeset, but it's worth noting that we still periodically see some GeoData test failures on random test runs. I've documented this in [USE-72](https://mitlibraries.atlassian.net/browse/USE-72). * We'll want to keep on eye on which methods error in case more guard clases are needed. We may also want to refactor to use more safe navigation.
jazairi
force-pushed
the
use-30-primo-models
branch
from
October 1, 2025 17:31
4add3a2 to
bef712d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why these changes are being introduced:
The first iteration of USE UI will include the
ability to toggle between Ex Libris CDI and
TIMDEX results.
Relevant ticket(s):
How this addresses that need:
This adds a Primo Search model that calls the
Primo Search API, and a Normalize Primo model that parses the each result and returns a normalized
record.
Side effects of this change:
acceptable as we do not anticipate this to be a
long-term solution.
views. It would be better to move this to
a normalization model, as we've done here with
Primo. I opened USE-73 to address this.
periodically see some GeoData test failures on
random test runs. I've documented this in USE-72.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
Unfortunately, since this doesn't touch views or controllers, the best way to confirm functionality is through manually testing in the Rails console. You can try running a new search (
search = PrimoSearch.new.search('popcorn', 10)), then grabbing a record from it (search['docs'].first), and looking at the normalized record (NormalizePrimo.new(record).normalize).Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing