-
Notifications
You must be signed in to change notification settings - Fork 0
Add models for Primo Search API integration #227
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
Conversation
Pull Request Test Coverage Report for Build 18170449977Details
💛 - Coveralls |
JPrevost
left a comment
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.
Initial feedback. Will formalize the review when you make the changes to normalizer you noted in Slack yesterday.
|
@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! |
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.
4add3a2 to
bef712d
Compare
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