Conversation
Coverage Report for CI Build 35034177386Coverage increased (+0.02%) to 98.324%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
❌ 12 blocking issues (12 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 } |
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 | ||
| } |
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
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
| <% 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 %> |
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.
There was a problem hiding this comment.
I wonder if this is why we get one set of links unstyled still though?
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
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
Nice, this is working really well.
One oddity and one question before I dig into the actual code.
Oddity:
- there are certain "Full-text options" links that are not styled the same as others. Example: see fifth-ish result in "sea otters" search. It seems odd these are not styled the same. I suspect they are generated in a different process, but it is worth looking into and deciding whether to update.
Question:
- This seems to only be handling Alma records and not CDI. I believe some code being removed in this PR was handling CDI records. Compare searches for "Journal of Heat Transfer Policy on Reporting Uuncertainties in Experimental Measurements and Results" in both Primo and SML to see that Primo is showing Full Text Option links (which seem to go to ILL requests in some cases but that is likely good and intentional
tldr; we might need both the Primo Normalization (for CDI) and SRU (for Alma) to fully cover these fulfillment links?
|
@JPrevost Oh yeah, that makes sense re: CDI, since there wouldn't be SRU lookups. I can add the normalizer code back in. The styling for records with 'Full-text options' and no other links is something I thought was resolved, but I clearly missed an edge case. Thanks for catching that. |
2289add to
09ac9fb
Compare
Why these changes are being introduced: The initial implementation of the 'Full-text options' only addresses Alma records, by making AlmaSRU calls and checking for the presence of certain metadata. (Either the AVE tag, or a 959 indicating electronic access.) Relevant ticket(s): - [USE-663](https://mitlibraries.atlassian.net/browse/USE-663) How this addresses that need: Since CDI records don't have the 'Alma-E' flag in their PNX, we have to check for a couple other indicators: 1. `delivery.almaOpenurl` 2. `links.openurl` This checks for the presence of OpenURL links, which in turn signal a 'Full-text options' section. In some cases, the full-text option is actually an ILL request, but this feels like a feature given that ILL is generally a quick and reliable way to retrieve CDI content. Side effects of this change: Handles an unrelated bug with the initial ALma record implemention, where the `result-get` div does not render if 'Full-text options' is the only available link.
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
There is still one condition without the same styling as the others.
A few methods could use some docs. Otherwise, this works how I think it should based on my understanding of wtf we're trying to do here :)
| @availability = AlmaSru.lookup(params[:doc_id]) | ||
| result = AlmaSru.lookup(params[:doc_id]) | ||
| @availability = result[:availability] | ||
| @alma_e = result[:alma_e] |
There was a problem hiding this comment.
This method may benefit from documentation. Because of the context of this PR, I understand it, but I suspect in a few months we may not remember what the difference between availability and alma_e are (and maybe even noting that alma_e is only a portion of the full text options link... it's complicated so docs will help)
| this.element.outerHTML = html | ||
|
|
||
| // Keep Alma availability in `.result-content` but place “Full-text options” | ||
| // with fulfillment links in the descendant `.result-get` container. |
There was a problem hiding this comment.
I wonder if this logic is why some links aren't styled the same? Probably not, but it still isn't consistent for one class of links.
| # lookup is the primary method of interacting with this model. | ||
| # | ||
| # It will receive an Alma ID, validate it, look it up in the Alma SRU, and return a formatted result. | ||
| # It will receive an Alma ID, validate it, look it up in the Alma SRU, and return availability info and Alma-E status. |
There was a problem hiding this comment.
Can we add context as to what these returned values mean? I feel like availability is really physical holdings-ish maybe and Alma-E is a specific condition we check for certain full text options links but it'll be good to document them as clearly as we can
| <% 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 %> |
There was a problem hiding this comment.
I wonder if this is why we get one set of links unstyled still though?
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