From 79fb4260dccf2e4681af9eb37a2639defaa21cb3 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Thu, 18 Jun 2026 14:27:10 -0400 Subject: [PATCH 01/11] Rename method in AlmaSru model --- app/models/alma_sru.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/alma_sru.rb b/app/models/alma_sru.rb index 40eadc59..a43f919d 100644 --- a/app/models/alma_sru.rb +++ b/app/models/alma_sru.rb @@ -24,7 +24,7 @@ class InvalidAlmaId < StandardError; end # # It accepts an "alma_client" argument for use when testing, but this is not used in normal operations. def self.lookup(raw_identifier, alma_client: nil) - return [] unless alma_sru_enabled? + return [] unless enabled? # Validate the raw identifier received. This will raise an InvalidAlmaId if validation fails. identifier = validate_alma_id(raw_identifier) @@ -142,7 +142,7 @@ def self.alma_base_url ENV.fetch('MIT_ALMA_URL', nil) end - def self.alma_sru_enabled? + def self.enabled? if alma_base_url.to_s.empty? || exl_inst_id.to_s.empty? Sentry.capture_message('Alma SRU not enabled') return false From e8d0e7195425aae1f4cfcfe7545fa58b8082dcc5 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Mon, 15 Jun 2026 16:50:03 -0400 Subject: [PATCH 02/11] Adds Alma lookup route, controller, template This will be loaded via Stimulus when needed in a subsequent commit. Alma SRU controller tests --- app/controllers/alma_controller.rb | 15 ++++++ app/views/alma/sru.html.erb | 7 +++ config/routes.rb | 3 +- test/controllers/alma_controller_test.rb | 65 ++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 app/controllers/alma_controller.rb create mode 100644 app/views/alma/sru.html.erb create mode 100644 test/controllers/alma_controller_test.rb diff --git a/app/controllers/alma_controller.rb b/app/controllers/alma_controller.rb new file mode 100644 index 00000000..ba81d555 --- /dev/null +++ b/app/controllers/alma_controller.rb @@ -0,0 +1,15 @@ +class AlmaController < ApplicationController + layout false + + def sru + return unless AlmaSru.enabled? && expected_params? + + @availability = AlmaSru.lookup(params[:doc_id]) + end + + private + + def expected_params? + params[:doc_id].present? + end +end diff --git a/app/views/alma/sru.html.erb b/app/views/alma/sru.html.erb new file mode 100644 index 00000000..7410bfdc --- /dev/null +++ b/app/views/alma/sru.html.erb @@ -0,0 +1,7 @@ +<% if AlmaSru.enabled? && @availability.present? %> +
+ <% @availability.each do |statement| %> + <%= link_to(statement, '#', data: {content_piece: 'Availability Link' }) %>
+ <% end %> +
+<% end %> diff --git a/config/routes.rb b/config/routes.rb index 4ea3b654..c828f4e9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,8 +3,9 @@ get 'analyze', to: 'tacos#analyze' - get 'libkey', to: 'thirdiron#libkey' + get 'almasru', to: 'alma#sru' get 'browzine', to: 'thirdiron#browzine' + get 'libkey', to: 'thirdiron#libkey' get 'oa_work', to: 'openalex#work' get 'record/(:id)', diff --git a/test/controllers/alma_controller_test.rb b/test/controllers/alma_controller_test.rb new file mode 100644 index 00000000..5aafe45c --- /dev/null +++ b/test/controllers/alma_controller_test.rb @@ -0,0 +1,65 @@ +require 'test_helper' + +class AlmaControllerTest < ActionDispatch::IntegrationTest + test 'alma sru route exists with no content' do + get almasru_path + + assert :success + assert response.body.blank? + end + + test 'alma sru route returns nothing for gibberish content' do + needle = 'foo' + get almasru_path(doc_id: needle) + + assert :success + assert response.body.blank? + end + + test 'alma sru route returns nothing if lookup returns content with no AVA' do + VCR.use_cassette('alma sru no availability') do + needle = 'alma9935053423706761' + get almasru_path(doc_id: needle) + + assert :success + assert response.body.blank? + end + end + + test 'alma sru route returns HTML for successful lookup' do + VCR.use_cassette('alma sru single record') do + needle = 'alma990014651640106761' + get almasru_path(doc_id: needle) + + assert :success + assert_select 'div.availability a', { count: 1 } + end + end + + test 'alma sru route returns multiple statements with multiple AVA' do + VCR.use_cassette('alma sru multiple records') do + needle = 'alma990002935920106761' + get almasru_path(doc_id: needle) + + assert :success + assert_select 'div.availability a', { count: 2 } + end + end + + test 'alma sru route does nothing for valid id if required env undefined' do + VCR.use_cassette('alma sru single record') do + needle = 'alma990014651640106761' + get almasru_path(doc_id: needle) + + assert :success + refute response.body.blank? + + ClimateControl.modify(MIT_ALMA_URL: nil) do + get almasru_path(doc_id: needle) + + assert :success + assert response.body.blank? + end + end + end +end From c3d949be388929b6b73d61c7fc100562052316fa Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Mon, 15 Jun 2026 16:51:20 -0400 Subject: [PATCH 03/11] Call lookup route for each result in a set This still needs to be filtered, most likely (tho the AlmaSru model will also do its own filtering) --- app/views/search/_result_primo.html.erb | 4 ++++ app/views/search/_trigger_almasru.html.erb | 9 +++++++++ 2 files changed, 13 insertions(+) create mode 100644 app/views/search/_trigger_almasru.html.erb diff --git a/app/views/search/_result_primo.html.erb b/app/views/search/_result_primo.html.erb index 2d4bc0b4..899005fb 100644 --- a/app/views/search/_result_primo.html.erb +++ b/app/views/search/_result_primo.html.erb @@ -109,6 +109,10 @@ <%= render(partial: 'trigger_openalex_setup', locals: { doi: result[:doi], pmid: result[:pmid] }) %> <% end %> + <% if AlmaSru.enabled? %> + <%= render(partial: 'trigger_almasru', locals: { doc_id: result[:identifier] }) %> + <% end %> + <% if result[:availability].present? %>
<% if result[:links]&.find { |link| link['kind'] == 'full record' } %> diff --git a/app/views/search/_trigger_almasru.html.erb b/app/views/search/_trigger_almasru.html.erb new file mode 100644 index 00000000..7267f77c --- /dev/null +++ b/app/views/search/_trigger_almasru.html.erb @@ -0,0 +1,9 @@ +<% return unless AlmaSru.enabled? %> + +<% data_url = "/almasru?doc_id=#{doc_id}" %> + +> +   + From 359f31ed9bcf051497e80fa1b7cc8517dd4f06df Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Mon, 15 Jun 2026 19:00:24 -0400 Subject: [PATCH 04/11] Lazy-load Alma SRU content This involves adding lazy-loading support to the content loader controller, which may be a feature that other async calls would benefit from. --- .../controllers/content_loader_controller.js | 22 +++++++++++++++++-- app/views/search/_trigger_almasru.html.erb | 3 ++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/app/javascript/controllers/content_loader_controller.js b/app/javascript/controllers/content_loader_controller.js index 7c066abf..02450a68 100644 --- a/app/javascript/controllers/content_loader_controller.js +++ b/app/javascript/controllers/content_loader_controller.js @@ -1,10 +1,28 @@ import { Controller } from "@hotwired/stimulus" export default class extends Controller { - static values = { url: String } + static values = { url: String, lazyLoading: Boolean } connect() { - this.load() + if (this.lazyLoadingValue) { + // The content loader included a lazy loading directive. + this.observer = new IntersectionObserver( + (entries) => { + if (entries[0].isIntersecting) { + this.load() + this.observer.disconnect() + } + } + ) + this.observer.observe(this.element) + } else { + // Load the content immediately. + this.load() + } + } + + disconnect() { + this.observer?.disconnect() } load() { diff --git a/app/views/search/_trigger_almasru.html.erb b/app/views/search/_trigger_almasru.html.erb index 7267f77c..2f37a831 100644 --- a/app/views/search/_trigger_almasru.html.erb +++ b/app/views/search/_trigger_almasru.html.erb @@ -4,6 +4,7 @@ > + data-content-loader-url-value="<%= data_url %>" + data-content-loader-lazy-loading-value="">   From c6297af8178e9fcd5db2b6b0c7f2b48a75d49117 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Wed, 17 Jun 2026 15:49:47 -0400 Subject: [PATCH 05/11] Refactor alma ID validation and extraction - Also adds additional filter on UI invoking this path, leveraging new validation method --- app/models/alma_sru.rb | 53 ++++++++++++------ app/views/search/_result_primo.html.erb | 2 +- test/models/alma_sru_test.rb | 73 ++++++++----------------- 3 files changed, 59 insertions(+), 69 deletions(-) diff --git a/app/models/alma_sru.rb b/app/models/alma_sru.rb index a43f919d..2a2d704e 100644 --- a/app/models/alma_sru.rb +++ b/app/models/alma_sru.rb @@ -27,7 +27,10 @@ def self.lookup(raw_identifier, alma_client: nil) return [] unless enabled? # Validate the raw identifier received. This will raise an InvalidAlmaId if validation fails. - identifier = validate_alma_id(raw_identifier) + raise InvalidAlmaId unless valid_alma_id?(raw_identifier) + + # Extract numeric portion from provided raw identifier + identifier = extract_alma_id(raw_identifier) # Build URL url = alma_sru_url(identifier) @@ -71,23 +74,6 @@ def self.parse_response(raw_response, reference_identifier) parsed_availabilities.map(&method(:format_availability)) end - # validate_alma_id ensures we are only submitting valid Alma IDs to the SRU endpoint. - # - # It needs to do two things: - # 1. Remove the "alma" prefix if one is present. Otherwise, no manipulation of the submitted value should occur. - # 2. Enforce the formatting requirements for a valid alma identifier (start with "99", and end with "6761"). - def self.validate_alma_id(raw) - parsed = if raw.to_s.start_with?('alma') - raw.delete_prefix('alma') - else - raw - end - - raise InvalidAlmaId unless parsed.present? && parsed.match?(/\A99\d+6761\z/) - - parsed - end - # ava_to_hash takes an XML element that represents a single availability record # and converts it to a hash. Each code is a key, while its text is the value. def self.ava_to_hash(node) @@ -151,6 +137,37 @@ def self.enabled? true end + # extract_alma_id receives a hypothetical document ID that references an alma + # record, and strips out any `alma` prefix which _may_ exist. This is a + # compensating strategy for our discovery environment attaching this prefix to + # flag the record as coming from alma, rather than other collections. + def self.extract_alma_id(raw) + if raw.to_s.start_with?('alma') + raw.to_s.delete_prefix('alma') + else + raw.to_s + end + end + + # valid_alma_id? receives a document identifier and attempts to determine + # whether it is a reference to an alma document. This involves using a regular + # expression to confirm five attributes: + # 1. The identifier can be converted to a string + # 2. The identifier may begin with "alma" + # 3. After any "alma" prefix, the next two characters must be "99" + # 4. Following this must be a sequence of only digits + # 5. The identifier must end with "6761" + # + # The method returns "true" if all of these conditions are met, otherwise it + # returns "false". No further action is taken for failing results, as this + # method is called for a wide range of identifiers. + def self.valid_alma_id?(raw) + return false unless raw.present? + return true if raw.to_s.match?(/\A(alma)?99\d+6761\z/) + + false + end + def self.alma_sru_url(identifier) # example identifier: 9935177389906761 "#{alma_base_url}/view/sru/#{exl_inst_id}?version=1.2&operation=searchRetrieve&recordSchema=marcxml" \ diff --git a/app/views/search/_result_primo.html.erb b/app/views/search/_result_primo.html.erb index 899005fb..72ed1697 100644 --- a/app/views/search/_result_primo.html.erb +++ b/app/views/search/_result_primo.html.erb @@ -109,7 +109,7 @@ <%= render(partial: 'trigger_openalex_setup', locals: { doi: result[:doi], pmid: result[:pmid] }) %> <% end %> - <% if AlmaSru.enabled? %> + <% if AlmaSru.enabled? && AlmaSru.valid_alma_id?(result[:identifier]) %> <%= render(partial: 'trigger_almasru', locals: { doc_id: result[:identifier] }) %> <% end %> diff --git a/test/models/alma_sru_test.rb b/test/models/alma_sru_test.rb index b73fe209..a1f6adb5 100644 --- a/test/models/alma_sru_test.rb +++ b/test/models/alma_sru_test.rb @@ -148,59 +148,32 @@ class AlmaSruTest < ActiveSupport::TestCase end end - # validate_alma_id method - test 'validate_alma_id succeeds with valid numeric input' do - needle = '990002935920106761' - assert_nothing_raised do - AlmaSru.validate_alma_id(needle) - end - end - - test 'validate_alma_id succeeds despite an "alma" prefix' do - needle = 'alma990002935920106761' - assert_nothing_raised do - AlmaSru.validate_alma_id(needle) - end - end - - test 'validate_alma_id raises InvalidAlmaId with non-numeric id' do - needle = '99000293foo5920106761' - - assert_raises(AlmaSru::InvalidAlmaId) do - AlmaSru.validate_alma_id(needle) - end - end - - test 'validate_alma_id raises InvalidAlmaId with a nil input' do - needle = nil - assert_raises(AlmaSru::InvalidAlmaId) do - AlmaSru.validate_alma_id(needle) - end - end - - test 'validate_alma_id raises InvalidAlmaId without required start sequence' do - assert_raises(AlmaSru::InvalidAlmaId) do - AlmaSru.validate_alma_id('0002935920106761') - end - - assert_raises(AlmaSru::InvalidAlmaId) do - AlmaSru.validate_alma_id('alma0002935920106761') - end - end - - test 'validate_alma_id raises InvalidAlmaId without required end sequence' do - assert_raises(AlmaSru::InvalidAlmaId) do - AlmaSru.validate_alma_id('99000293592010') - end - - assert_raises(AlmaSru::InvalidAlmaId) do - AlmaSru.validate_alma_id('alma99000293592010') + # valid_alma_id? method + test 'valid_alma_id? returns true for valid inputs' do + needles = [ + 990002935920106761, + '990002935920106761', + 'alma990002935920106761' + ] + + needles.each do |needle| + assert_equal(true, AlmaSru.valid_alma_id?(needle)) end end - test 'validate_alma_id raises InvalidAlmaId with wildly invalid input' do - assert_raises(AlmaSru::InvalidAlmaId) do - AlmaSru.validate_alma_id('foo') + test 'valid_alma_id? returns false for invalid inputs' do + needles = [ + nil, + 'cdi_econis_primary_1902984668', # wildly nonconforming + '99000293foo5920106761', # non-numeric internals + '0002935920106761', # missing start sequence + 'alma0002935920106761', # missing start sequence + '99000293592010', # missing end sequence + 'alma99000293592010' # missing end sequence + ] + + needles.each do |needle| + assert_equal(false, AlmaSru.valid_alma_id?(needle)) end end From ec897c67ca03feb7ca7dfe04fa1cc07653a33a07 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Thu, 18 Jun 2026 12:11:40 -0400 Subject: [PATCH 06/11] Only return a single availability statement ** Why are these changes being introduced: Rather than render multiple availability statements, we want to just show the first, with a suffix phrase of "and other locations" if there are more locations. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-599 ** How does this address that need: This slightly refactors the formatting logic for handling the statements after they've been formatted. All blocks are formatted to start with, and then if there are multiple statements we append the suffix. Only the first statement is ever returned, as a list of one entry - so the overall contract of the .lookup() method is unchanged. ** Document any side effects to this change: It is a bit inefficient to format records that we then discard. I'm not sure that this will be a significant issue, but it should be noted. --- app/models/alma_sru.rb | 7 ++++++- test/controllers/alma_controller_test.rb | 6 ++++-- test/models/alma_sru_test.rb | 11 ++++++++--- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/app/models/alma_sru.rb b/app/models/alma_sru.rb index 2a2d704e..0fa35cf8 100644 --- a/app/models/alma_sru.rb +++ b/app/models/alma_sru.rb @@ -71,7 +71,12 @@ def self.parse_response(raw_response, reference_identifier) # Look up all AVA tags parsed_availabilities = fetch_availabilities(parsed) - parsed_availabilities.map(&method(:format_availability)) + # Format list of entries + results = parsed_availabilities.map(&method(:format_availability)) + + # Reduce list to a single item if multiples exist + results[0] += ' and other locations' if results.length > 1 + results.first(1) end # ava_to_hash takes an XML element that represents a single availability record diff --git a/test/controllers/alma_controller_test.rb b/test/controllers/alma_controller_test.rb index 5aafe45c..4ffa71d5 100644 --- a/test/controllers/alma_controller_test.rb +++ b/test/controllers/alma_controller_test.rb @@ -33,16 +33,18 @@ class AlmaControllerTest < ActionDispatch::IntegrationTest assert :success assert_select 'div.availability a', { count: 1 } + refute_includes response.body, 'and other locations' end end - test 'alma sru route returns multiple statements with multiple AVA' do + test 'alma sru route returns one statement including "and other locations" with multiple AVA' do VCR.use_cassette('alma sru multiple records') do needle = 'alma990002935920106761' get almasru_path(doc_id: needle) assert :success - assert_select 'div.availability a', { count: 2 } + assert_select 'div.availability a', { count: 1 } + assert_includes response.body, 'and other locations' end end diff --git a/test/models/alma_sru_test.rb b/test/models/alma_sru_test.rb index a1f6adb5..bd38b59c 100644 --- a/test/models/alma_sru_test.rb +++ b/test/models/alma_sru_test.rb @@ -45,14 +45,17 @@ class AlmaSruTest < ActiveSupport::TestCase end end - test 'lookup returns self-service locations first if multiples exist' do + test 'lookup returns a single entry, prioritizing self-service locations first, if multiples exist' do VCR.use_cassette('alma sru multiple records') do needle = '990002935920106761' result = AlmaSru.lookup(needle) - assert_equal('Check holdings at Barker Library Staff Retrieval - request required (FICHE No Call #)', result[0]) - assert_equal('Available at Library Storage Annex Journal Collection (LSA4) (TA.J86.H437)', result[1]) + assert_equal(1, result.length) + assert_equal( + 'Check holdings at Barker Library Staff Retrieval - request required (FICHE No Call #) and other locations', + result[0] + ) end end @@ -150,11 +153,13 @@ class AlmaSruTest < ActiveSupport::TestCase # valid_alma_id? method test 'valid_alma_id? returns true for valid inputs' do + # rubocop:disable Style/NumericLiterals needles = [ 990002935920106761, '990002935920106761', 'alma990002935920106761' ] + # rubocop:enable Style/NumericLiterals needles.each do |needle| assert_equal(true, AlmaSru.valid_alma_id?(needle)) From b1d558fd702f4c305ca2d04010b3216da961a6a0 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Thu, 18 Jun 2026 12:52:34 -0400 Subject: [PATCH 07/11] Make availability statement into a link ** Why are these changes being introduced: Up until now, the availability statements have been empty links - but we want them to go to the full records view, and specifically to the locations block of that page. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-599 ** How does this address that need: This calls the new PrimoLinkBuilder's full_record_link method, and adds the `getit_link1_0` document fragment to send the user to the list of available locations. ** Document any side effects to this change: There are two concerns to note here: 1. The document fragment we are using feels fragile, and like it may go away in the future. I also haven't checked what the fragment is for NDE. 2. This link is _very_ similar to the full record link that appears immediately before this in the UI. It will likely pass an accessibility check because it is not identical - but this feels like a technicality, and not an ideal user experience. I'm choosing to propose this anyway to see if I'm alone in thinking this - I can live with it, but it would not take much for me to support an alternative. --- app/views/alma/sru.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/alma/sru.html.erb b/app/views/alma/sru.html.erb index 7410bfdc..4ee43470 100644 --- a/app/views/alma/sru.html.erb +++ b/app/views/alma/sru.html.erb @@ -1,7 +1,7 @@ <% if AlmaSru.enabled? && @availability.present? %>
<% @availability.each do |statement| %> - <%= link_to(statement, '#', data: {content_piece: 'Availability Link' }) %>
+ <%= link_to(statement, "#{PrimoLinkBuilder.new(record_id: params[:doc_id], context: 'L').full_record_link}#getit_link1_0", data: {content_piece: 'Availability Link' }) %>
<% end %>
<% end %> From e3a3637948e6cb66b148c2e264b329460e22d9e6 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Thu, 18 Jun 2026 15:54:50 -0400 Subject: [PATCH 08/11] Remove unneeded legacy availability handling --- app/models/normalize_primo_record.rb | 16 ---------------- app/views/search/_result_primo.html.erb | 10 ---------- test/models/normalize_primo_record_test.rb | 20 -------------------- 3 files changed, 46 deletions(-) diff --git a/app/models/normalize_primo_record.rb b/app/models/normalize_primo_record.rb index 857739dd..3dade34e 100644 --- a/app/models/normalize_primo_record.rb +++ b/app/models/normalize_primo_record.rb @@ -31,8 +31,6 @@ def normalize numbering:, chapter_numbering:, thumbnail:, - availability:, - other_availability:, dedup_record: dedup_url.present? } end @@ -317,20 +315,6 @@ def subjects subs.flat_map { |sub| sub.split(' ; ') } end - def availability - return unless location - - @record['delivery']['bestlocation']['availabilityStatus'] - end - - def other_availability - return unless @record['delivery'] - return unless @record['delivery']['bestlocation'] - return unless @record['delivery']['holding'] - - @record['delivery']['holding'].length > 1 - end - # FRBR Group check based on: # https://knowledge.exlibrisgroup.com/Primo/Knowledge_Articles/Primo_Search_API_-_how_to_get_FRBR_Group_members_after_a_search def frbrized? diff --git a/app/views/search/_result_primo.html.erb b/app/views/search/_result_primo.html.erb index 72ed1697..4bc9d7ce 100644 --- a/app/views/search/_result_primo.html.erb +++ b/app/views/search/_result_primo.html.erb @@ -113,16 +113,6 @@ <%= render(partial: 'trigger_almasru', locals: { doc_id: result[:identifier] }) %> <% end %> - <% if result[:availability].present? %> -
- <% if result[:links]&.find { |link| link['kind'] == 'full record' } %> - <%= link_to(availability(result[:availability], result[:location], result[:other_availability]), result[:links].find { |link| link['kind'] == 'full record' }['url'], data: { content_piece: 'Availability Link' }) %> - <% else %> - <%= availability(result[:availability], result[:location], result[:other_availability]) %> - <% end %> -
- <% end %> - <%# Trigger BrowZine lookup (render inside result-get so injected HTML is part of the flex `.result-get` area and receives expected styles) %> <% if ThirdIron.enabled? && result[:format].downcase == 'journal' && result[:issn].present? %> diff --git a/test/models/normalize_primo_record_test.rb b/test/models/normalize_primo_record_test.rb index e9d2cf07..57ff609c 100644 --- a/test/models/normalize_primo_record_test.rb +++ b/test/models/normalize_primo_record_test.rb @@ -272,26 +272,6 @@ def cdi_record assert_empty normalized[:subjects] end - test 'returns availability status' do - normalized = NormalizePrimoRecord.new(full_record, 'test').normalize - assert_equal 'available', normalized[:availability] - end - - test 'handles missing availability' do - normalized = NormalizePrimoRecord.new(minimal_record, 'test').normalize - assert_nil normalized[:availability] - end - - test 'detects other availability when multiple holdings exist' do - normalized = NormalizePrimoRecord.new(full_record, 'test').normalize - assert normalized[:other_availability] - end - - test 'handles missing other availability' do - normalized = NormalizePrimoRecord.new(minimal_record, 'test').normalize - assert_nil normalized[:other_availability] - end - test 'uses dedup URL as full record link for frbrized records' do normalized = NormalizePrimoRecord.new(full_record, 'test').normalize full_record_link = normalized[:links].find { |link| link['kind'] == 'full record' } From b064e8f280bfdf2ce6a3cff95d58ed1ab1509779 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Thu, 18 Jun 2026 19:53:37 -0400 Subject: [PATCH 09/11] Re-implement formatting for availability blurb ** Why are these changes being introduced: Our previous implementation of the availability statements had some nice features like icons and bold styling for location names. We want to carry this forward, but want to keep the model-level implementation for this formatting. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-599 ** How does this address that need: This removes the availability() and location() record helpers, and reintroduces those features within the AlmaSru model. This includes the status handling logic of the previous implementation. ** Document any side effects to this change: Other parts of the application rely on the icon() helper in the record helper model, so we can't remove that here. As a result, there is now a duplication of that logic in two places in the application, which isn't ideal. Avoiding this would mean changing where the AlmaSru model formats its output, which I'd rather not do at this point - but I'm open to feedback about this during code review. --- app/helpers/record_helper.rb | 36 ------------------- app/models/alma_sru.rb | 21 +++++++++-- app/views/alma/sru.html.erb | 2 +- test/helpers/record_helper_test.rb | 24 ------------- test/models/alma_sru_test.rb | 56 +++++++++++++++++++++++++----- 5 files changed, 68 insertions(+), 71 deletions(-) diff --git a/app/helpers/record_helper.rb b/app/helpers/record_helper.rb index 31ee0fcd..b560a633 100644 --- a/app/helpers/record_helper.rb +++ b/app/helpers/record_helper.rb @@ -139,36 +139,6 @@ def deduplicate_subjects(subjects) subjects.map { |subject| subject['value'].uniq(&:downcase) }.uniq { |values| values.map(&:downcase) } end - # Formats availability information for display. - # Expects: - # - status: a string indicating availability status - # - location: an array with three elements: [library name, location name, call number] - # - other_availability: a boolean string indicating if there is availability at other locations - def availability(status, location, other_availability) - blurb = case status.downcase - # `available` is a common status used in Alma/Primo VE for items that are not checked out and should be - # on the shelf - when 'available' - "#{icon('check')} Available in #{location(location)}" - # `check_holdings`: unclear when (or if) this is used. Bento handled this so we did too assuming it was - # meaningful - when 'check_holdings' - "#{icon('question')} May be available in #{location(location)}" - # 'unavailable' is used for items that are checked out, missing, or otherwise not on the shelf - when 'unavailable' - "#{icon('times')} Not currently available in #{location(location)}" - # Unclear if there are other statuses we should handle here. For now we log and display a generic message. - else - Rails.logger.error("Unhandled availability status: #{status.inspect}") - "#{icon('question')} Uncertain availability in #{location(location)} #{status}" - end - - blurb += ' and other locations.' if other_availability.present? - - # We are generating HTML in this helper, so we need to mark it as safe or it will be escaped in the view. - blurb.html_safe - end - # Fontawesome helper. # # Accepts name of the icon and optionally a collection. Defaults to solid sharp collection. @@ -180,12 +150,6 @@ def icon(fa, collection = 'fa-sharp fa-solid') "" end - # Formats location information for availability display. - # Expects an array with three elements: [library name, location name, call number] - def location(loc) - "#{loc[:name]} #{loc[:collection]} (#{loc[:call_number]})" - end - private def render_kind_value(list) diff --git a/app/models/alma_sru.rb b/app/models/alma_sru.rb index 0fa35cf8..619e8b5a 100644 --- a/app/models/alma_sru.rb +++ b/app/models/alma_sru.rb @@ -123,12 +123,29 @@ def self.format_availability(availability) return '' end - phrase = "#{availability['e']&.humanize} at #{availability['q']} #{availability['c']}".squish + phrase = "#{_phrase_start(availability['e'])} #{availability['q']} #{availability['c']}".squish phrase += " (#{availability['d']})" if availability['d'].present? - phrase end + def self._phrase_start(status) + case status + when 'available' + "#{_icon('check')} Available in " + when 'check_holdings' + "#{_icon('question')} May be available in " + when 'unavailable' + "#{_icon('times')} Not currently available in " + else + Rails.logger.error("Unhandled availability status: #{status}") + "#{_icon('question')} Uncertain availability (#{status.humanize}) in " + end + end + + def self._icon(icon, collection = 'fa-sharp fa-solid') + "" + end + def self.alma_base_url ENV.fetch('MIT_ALMA_URL', nil) end diff --git a/app/views/alma/sru.html.erb b/app/views/alma/sru.html.erb index 4ee43470..ade19a2c 100644 --- a/app/views/alma/sru.html.erb +++ b/app/views/alma/sru.html.erb @@ -1,7 +1,7 @@ <% if AlmaSru.enabled? && @availability.present? %>
<% @availability.each do |statement| %> - <%= link_to(statement, "#{PrimoLinkBuilder.new(record_id: params[:doc_id], context: 'L').full_record_link}#getit_link1_0", data: {content_piece: 'Availability Link' }) %>
+ <%= link_to(statement.html_safe, "#{PrimoLinkBuilder.new(record_id: params[:doc_id], context: 'L').full_record_link}#getit_link1_0", data: {content_piece: 'Availability Link' }) %>
<% end %>
<% end %> diff --git a/test/helpers/record_helper_test.rb b/test/helpers/record_helper_test.rb index 172994ad..020525f0 100644 --- a/test/helpers/record_helper_test.rb +++ b/test/helpers/record_helper_test.rb @@ -342,30 +342,6 @@ class RecordHelperTest < ActionView::TestCase assert_nil deduplicate_subjects(subjects) end - test 'availability helper handles known statuses correctly' do - location = { - name: 'Main Library', - collection: 'First Floor', - call_number: 'QA76.73.R83 2023' - } - - available_blurb = availability('available', location, false) - assert_includes available_blurb, 'Available in' - assert_includes available_blurb, location(location) - - check_holdings_blurb = availability('check_holdings', location, false) - assert_includes check_holdings_blurb, 'May be available in' - assert_includes check_holdings_blurb, location(location) - - unavailable_blurb = availability('unavailable', location, false) - assert_includes unavailable_blurb, 'Not currently available in' - assert_includes unavailable_blurb, location(location) - - unknown_blurb = availability('unknown_status', location, false) - assert_includes unknown_blurb, 'Uncertain availability in' - assert_includes unknown_blurb, location(location) - end - test 'icon helper returns fontawesome icon with default sharp solid collection' do icon_html = icon('check') assert_includes icon_html, 'fa-sharp' diff --git a/test/models/alma_sru_test.rb b/test/models/alma_sru_test.rb index bd38b59c..2d63f511 100644 --- a/test/models/alma_sru_test.rb +++ b/test/models/alma_sru_test.rb @@ -41,7 +41,10 @@ class AlmaSruTest < ActiveSupport::TestCase result = AlmaSru.lookup(needle) - assert_equal(['Available at Rotch Library Stacks (NA680.C25 2007)'], result) + assert_equal( + [" Available in Rotch Library Stacks (NA680.C25 2007)"], + result + ) end end @@ -52,10 +55,7 @@ class AlmaSruTest < ActiveSupport::TestCase result = AlmaSru.lookup(needle) assert_equal(1, result.length) - assert_equal( - 'Check holdings at Barker Library Staff Retrieval - request required (FICHE No Call #) and other locations', - result[0] - ) + assert_includes result[0], 'and other locations' end end @@ -226,20 +226,60 @@ class AlmaSruTest < ActiveSupport::TestCase ava_hash = { 'c' => 'charlie', 'd' => 'delta', - 'e' => 'echo', + 'e' => 'available', 'q' => 'quebec' } - assert_equal('Echo at quebec charlie (delta)', AlmaSru.format_availability(ava_hash)) + assert_equal( + " Available in quebec charlie (delta)", + AlmaSru.format_availability(ava_hash) + ) end test 'format_availability returns a minimum statement if only e and q are present' do + ava_hash = { + 'e' => 'available', + 'q' => 'quebec' + } + + assert_equal(" Available in quebec", + AlmaSru.format_availability(ava_hash)) + end + + test 'format_availability supports check_holdings status' do + ava_hash = { + 'e' => 'check_holdings', + 'q' => 'quebec' + } + + assert_equal( + " May be available in quebec", + AlmaSru.format_availability(ava_hash) + ) + end + + test 'format_availability supports unavailable status' do + ava_hash = { + 'e' => 'unavailable', + 'q' => 'quebec' + } + + assert_equal( + " Not currently available in quebec", + AlmaSru.format_availability(ava_hash) + ) + end + + test 'format_availability does not fail with unexpected status' do ava_hash = { 'e' => 'echo', 'q' => 'quebec' } - assert_equal('Echo at quebec', AlmaSru.format_availability(ava_hash)) + assert_equal( + " Uncertain availability (Echo) in quebec", + AlmaSru.format_availability(ava_hash) + ) end test 'format_availability returns an empty string without both e and q present' do From b523a4ddf77810a749e98d1e94a203edbbd87d5f Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Thu, 18 Jun 2026 21:42:37 -0400 Subject: [PATCH 10/11] Side effect - fix HTML problem in icon helper --- app/helpers/record_helper.rb | 2 +- test/helpers/record_helper_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/record_helper.rb b/app/helpers/record_helper.rb index b560a633..31b6f672 100644 --- a/app/helpers/record_helper.rb +++ b/app/helpers/record_helper.rb @@ -147,7 +147,7 @@ def deduplicate_subjects(subjects) # purely for decoration. If an icon is used in a more meaningful way, we should extend this helper # to allow passing in additional aria attributes rather than always hiding. def icon(fa, collection = 'fa-sharp fa-solid') - "" + "" end private diff --git a/test/helpers/record_helper_test.rb b/test/helpers/record_helper_test.rb index 020525f0..fd698a9f 100644 --- a/test/helpers/record_helper_test.rb +++ b/test/helpers/record_helper_test.rb @@ -360,7 +360,7 @@ class RecordHelperTest < ActionView::TestCase end test 'icon helper generates properly formatted HTML' do - expected = "" + expected = "" assert_equal expected, icon('question') end end From 6045ddd581e08532438755d5995a922abc1db0e3 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Thu, 18 Jun 2026 21:42:56 -0400 Subject: [PATCH 11/11] Respond to code review feedback Update test and teardown for AlmaSru.enabled? Update result_get? check to include Alma ID format We've removed the availability, so that cause and test were meaningless. --- app/helpers/results_helper.rb | 2 +- app/models/alma_sru.rb | 15 ++++++++------- app/views/alma/sru.html.erb | 4 +++- app/views/search/_trigger_almasru.html.erb | 2 +- test/controllers/alma_controller_test.rb | 14 +++++++------- test/helpers/results_helper_test.rb | 4 ++-- test/models/alma_sru_test.rb | 2 ++ test/test_helper.rb | 4 ++++ 8 files changed, 28 insertions(+), 19 deletions(-) diff --git a/app/helpers/results_helper.rb b/app/helpers/results_helper.rb index 13aef52b..e56f4cd0 100644 --- a/app/helpers/results_helper.rb +++ b/app/helpers/results_helper.rb @@ -97,7 +97,7 @@ def full_record_url(result) # @return [Boolean] True if the result has links, availability, or ThirdIron/OpenAlex triggers def result_get?(result) renderable_links?(result) || - result[:availability].present? || + AlmaSru.valid_alma_id?(result[:identifier]) || (Feature.enabled?(:oa_always) && article?(result[:format])) || thirdiron_content?(result) end diff --git a/app/models/alma_sru.rb b/app/models/alma_sru.rb index 619e8b5a..7777928a 100644 --- a/app/models/alma_sru.rb +++ b/app/models/alma_sru.rb @@ -123,8 +123,9 @@ def self.format_availability(availability) return '' end - phrase = "#{_phrase_start(availability['e'])} #{availability['q']} #{availability['c']}".squish - phrase += " (#{availability['d']})" if availability['d'].present? + phrase = "#{_phrase_start(ERB::Util.html_escape(availability['e'].to_s))} #{ERB::Util.html_escape(availability['q'].to_s)} " \ + "#{ERB::Util.html_escape(availability['c'].to_s)}".squish + phrase += " (#{ERB::Util.html_escape(availability['d'].to_s)})" if availability['d'].present? phrase end @@ -151,12 +152,12 @@ def self.alma_base_url end def self.enabled? - if alma_base_url.to_s.empty? || exl_inst_id.to_s.empty? - Sentry.capture_message('Alma SRU not enabled') - return false - end + return @enabled if instance_variable_defined?(:@enabled) + + @enabled = alma_base_url.to_s.present? && exl_inst_id.to_s.present? + Sentry.capture_message('Alma SRU not enabled') unless @enabled - true + @enabled end # extract_alma_id receives a hypothetical document ID that references an alma diff --git a/app/views/alma/sru.html.erb b/app/views/alma/sru.html.erb index ade19a2c..83626ee5 100644 --- a/app/views/alma/sru.html.erb +++ b/app/views/alma/sru.html.erb @@ -1,7 +1,9 @@ <% if AlmaSru.enabled? && @availability.present? %>
<% @availability.each do |statement| %> - <%= link_to(statement.html_safe, "#{PrimoLinkBuilder.new(record_id: params[:doc_id], context: 'L').full_record_link}#getit_link1_0", data: {content_piece: 'Availability Link' }) %>
+ <%= 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' }) %>
<% end %>
<% end %> diff --git a/app/views/search/_trigger_almasru.html.erb b/app/views/search/_trigger_almasru.html.erb index 2f37a831..d257de92 100644 --- a/app/views/search/_trigger_almasru.html.erb +++ b/app/views/search/_trigger_almasru.html.erb @@ -1,6 +1,6 @@ <% return unless AlmaSru.enabled? %> -<% data_url = "/almasru?doc_id=#{doc_id}" %> +<% data_url = almasru_path(doc_id: doc_id) %> 'PDF', 'url' => 'https://example.com' }] }) end - test 'result_get? returns true when result has availability' do - assert result_get?({ availability: 'Available' }) + test 'result_get? returns true when result has a valid Alma ID' do + assert result_get?({ identifier: 'alma9912346761' }) end test 'result_get? returns true when ThirdIron enabled and result has doi' do diff --git a/test/models/alma_sru_test.rb b/test/models/alma_sru_test.rb index 2d63f511..d2eec48a 100644 --- a/test/models/alma_sru_test.rb +++ b/test/models/alma_sru_test.rb @@ -99,6 +99,8 @@ class AlmaSruTest < ActiveSupport::TestCase end ClimateControl.modify(EXL_INST_ID: nil) do + AlmaSru.remove_instance_variable(:@enabled) + assert_equal([], AlmaSru.lookup(needle)) end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 5c6ba377..ac5e2e3c 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -66,6 +66,10 @@ class ActiveSupport::TestCase setup do Rails.cache.clear end + + def teardown + AlmaSru.remove_instance_variable(:@enabled) if AlmaSru.instance_variable_defined?(:@enabled) + end end # Helper factory to create simple stub fetchers for `MergedSearchService` tests.