Implement full-text options links using SRU - #466
Open
jazairi wants to merge 4 commits into
Open
Conversation
Coverage Report for CI Build 34530238433Coverage increased (+0.007%) to 98.311%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
❌ 5 blocking issues (5 total)
@qltysh one-click actions:
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| Rails.logger.error('Alma SRU connection error') | ||
|
|
||
| [] | ||
| { availability: [], alma_e: false } |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The implementation appears consistent end-to-end, with only a minor test-description mismatch noted for clarity.
Pull request overview
This PR extends the Alma SRU lookup flow to detect Alma electronic availability (AVE/“Alma‑E”) and surface a “Full-text options” link in Primo results when electronic availability is present, without enabling delivery data in Primo API responses.
Changes:
- Update
AlmaSru.lookup/ parsing to return{ availability: [...], alma_e: boolean }(AVA holdings + AVE presence). - Render a “Full-text options” button from the Alma SRU response when
alma_eis true, and ensure a.result-getcontainer exists for Primo results where Alma SRU is triggered. - Adjust the Stimulus content loader to move the injected
.alma-fulltext-optionselement into the fulfillment links area, and update related controller/model tests.
File summaries
| File | Description |
|---|---|
| test/models/alma_sru_test.rb | Updates assertions for the new { availability, alma_e } return shape. |
| test/controllers/alma_controller_test.rb | Adds coverage for the new “Full-text options” rendering behavior. |
| app/views/search/_result_primo.html.erb | Ensures .result-get is rendered when Alma SRU will be triggered so injected links have a target container. |
| app/views/alma/sru.html.erb | Adds “Full-text options” link rendering when @alma_e is true; keeps availability rendering. |
| app/models/alma_sru.rb | Adds AVE detection and changes lookup return type to include Alma‑E status. |
| app/javascript/controllers/content_loader_controller.js | Moves .alma-fulltext-options into .result-get after SRU HTML is injected. |
| app/controllers/alma_controller.rb | Unpacks AlmaSru.lookup hash into @availability and @alma_e for the view. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| availability: availability, | ||
| alma_e: alma_e | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current UI injection logic can produce duplicate “Full-text options” links and should be de-duped for consistent behavior and tracking.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new Alma-rendered “Full-text options” button is missing the existing .primo-link classification (affecting established hide/de-dupe behavior) and a newly added DOM-structure comment is inaccurate, both of which should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+1
to
17
| <% if AlmaSru.enabled? %> | ||
| <% if @alma_e %> | ||
| <div class="alma-fulltext-options"> | ||
| <% fulltext_url = PrimoLinkBuilder.new(record_id: params[:doc_id], context: 'L').full_record_link + '#nui.getit.service_viewit' %> | ||
| <%= link_to 'Full-text options', fulltext_url, class: 'button', data: { content_piece: 'Full-text Options' } %> | ||
| </div> | ||
| <% end %> | ||
| <% if @availability.present? %> | ||
| <div class="availability"> | ||
| <% @availability.each do |statement| %> | ||
| <p><%= link_to(sanitize(statement, tags: %w[i strong], attributes: %w[class aria-hidden]), | ||
| "#{PrimoLinkBuilder.new(record_id: params[:doc_id], context: 'L').full_record_link}#getit_link1_0", | ||
| data: {content_piece: 'Availability Link' }) %></p> | ||
| <% end %> | ||
| </div> | ||
| <% end %> | ||
| <% end %> |
Contributor
Author
There was a problem hiding this comment.
This was intentional because I didn't see a reason to hide full-text options links. If I'm missing something, I can implement this.
Why these changes are being introduced: Unless we enable delivery data in our Primo API calls, there is no way to determine the electronic availability from the Primo response. Alma SRU allows us to detect an AVE tag (electronic availability), which signals the presence of a 'full-text options' link in the Primo record. Relevant ticket(s): - [USE-663](https://mitlibraries.atlassian.net/browse/USE-663) How this addresses that need: This modifies the Alma SRU call, such that it returns a hash with an availability array (for holdings information) and an Alma-E boolean (true if an AVE tag was detected). If Alma-E is true, we construct a full-text options link. The content loader controller injects this data into the fufillment links div. Side effects of this change: The availability data is now in the `result-get` (fulfillment links) div, rather than the main `result-content` div. I haven't noticed this causing any issues in local testing.
Some records do not have the AVE tag, but do have a 959 indicating electronic access. This is likely due to pre-Alma cataloging
18 tasks
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:
Unless we enable delivery data in our Primo API
calls, there is no way to determine the
electronic availability from the Primo response.
Alma SRU allows us to detect an AVE tag
(electronic availability), which signals the
presence of a 'full-text options' link in the
Primo record.
Relevant ticket(s):
How this addresses that need:
This modifies the Alma SRU call, such that it
returns a hash with an availability array (for
holdings information) and an Alma-E boolean (true
if an AVE tag was detected).
If Alma-E is true, we construct a full-text
options link. The content loader controller
injects this data into the fufillment links div.
Side effects of this change:
The availability data is now in the
result-get(fulfillment links) div, rather than the
main
result-contentdiv. I haven't noticed thiscausing any issues in local testing.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
The query
nature 1869should return as its top result a record that has physical and electronic availability.Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing