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/helpers/record_helper.rb b/app/helpers/record_helper.rb index 31ee0fcd..31b6f672 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. @@ -177,13 +147,7 @@ def availability(status, location, other_availability) # 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 - - # 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 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/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/models/alma_sru.rb b/app/models/alma_sru.rb index 40eadc59..7777928a 100644 --- a/app/models/alma_sru.rb +++ b/app/models/alma_sru.rb @@ -24,10 +24,13 @@ 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) + 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) @@ -68,24 +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)) - end + # Format list of entries + results = parsed_availabilities.map(&method(:format_availability)) - # 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 + # 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 @@ -132,23 +123,72 @@ def self.format_availability(availability) return '' end - phrase = "#{availability['e']&.humanize} at #{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 + 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 - def self.alma_sru_enabled? - if alma_base_url.to_s.empty? || exl_inst_id.to_s.empty? - Sentry.capture_message('Alma SRU not enabled') - return false + def self.enabled? + 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 + + @enabled + 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 - true + # 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) 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/alma/sru.html.erb b/app/views/alma/sru.html.erb new file mode 100644 index 00000000..83626ee5 --- /dev/null +++ b/app/views/alma/sru.html.erb @@ -0,0 +1,9 @@ +<% if AlmaSru.enabled? && @availability.present? %> +
+ <% @availability.each do |statement| %> + <%= 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/_result_primo.html.erb b/app/views/search/_result_primo.html.erb index 2d4bc0b4..4bc9d7ce 100644 --- a/app/views/search/_result_primo.html.erb +++ b/app/views/search/_result_primo.html.erb @@ -109,14 +109,8 @@ <%= render(partial: 'trigger_openalex_setup', locals: { doi: result[:doi], pmid: result[:pmid] }) %> <% 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 %> -
+ <% if AlmaSru.enabled? && AlmaSru.valid_alma_id?(result[:identifier]) %> + <%= render(partial: 'trigger_almasru', locals: { doc_id: result[:identifier] }) %> <% end %> <%# Trigger BrowZine lookup (render inside result-get so injected HTML diff --git a/app/views/search/_trigger_almasru.html.erb b/app/views/search/_trigger_almasru.html.erb new file mode 100644 index 00000000..d257de92 --- /dev/null +++ b/app/views/search/_trigger_almasru.html.erb @@ -0,0 +1,10 @@ +<% return unless AlmaSru.enabled? %> + +<% data_url = almasru_path(doc_id: doc_id) %> + + +   + 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..b6d9c00c --- /dev/null +++ b/test/controllers/alma_controller_test.rb @@ -0,0 +1,67 @@ +require 'test_helper' + +class AlmaControllerTest < ActionDispatch::IntegrationTest + test 'alma sru route exists with no content' do + get almasru_path + + assert_response :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_response :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_response :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_response :success + assert_select 'div.availability a', { count: 1 } + refute_includes response.body, 'and other locations' + end + end + + 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_response :success + assert_select 'div.availability a', { count: 1 } + assert_includes response.body, 'and other locations' + 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_response :success + refute response.body.blank? + + ClimateControl.modify(MIT_ALMA_URL: nil) do + get almasru_path(doc_id: needle) + + assert_response :success + assert response.body.blank? + end + end + end +end diff --git a/test/helpers/record_helper_test.rb b/test/helpers/record_helper_test.rb index 172994ad..fd698a9f 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' @@ -384,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 diff --git a/test/helpers/results_helper_test.rb b/test/helpers/results_helper_test.rb index b82d520a..adbe1aee 100644 --- a/test/helpers/results_helper_test.rb +++ b/test/helpers/results_helper_test.rb @@ -116,8 +116,8 @@ class ResultsHelperTest < ActionView::TestCase assert result_get?({ links: [{ 'kind' => '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 b73fe209..d2eec48a 100644 --- a/test/models/alma_sru_test.rb +++ b/test/models/alma_sru_test.rb @@ -41,18 +41,21 @@ 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 - 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_includes result[0], 'and other locations' end end @@ -96,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 @@ -148,59 +153,34 @@ 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 + # rubocop:disable Style/NumericLiterals + needles = [ + 990002935920106761, + '990002935920106761', + 'alma990002935920106761' + ] + # rubocop:enable Style/NumericLiterals + + 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 @@ -248,20 +228,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 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' } 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.