-
Notifications
You must be signed in to change notification settings - Fork 0
Display availability statements when relevant #408
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
base: main
Are you sure you want to change the base?
Changes from all commits
79fb426
e8d0e71
c3d949b
359f31e
c6297af
ec897c6
b1d558f
e3a3637
b064e8f
b523a4d
6045ddd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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))} <strong>#{ERB::Util.html_escape(availability['q'].to_s)}</strong> " \ | ||
| "#{ERB::Util.html_escape(availability['c'].to_s)}".squish | ||
| phrase += " (#{ERB::Util.html_escape(availability['d'].to_s)})" if availability['d'].present? | ||
| phrase | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| 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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method has one construct - the case statement for building message text based on the item's status. I'm not sure how this can be made shorter, except for maybe dropping the error handling for the unexpected branch - but I'm not certain that's a good idea. I am open to another way of handling an unexpected status - maybe a Sentry exception would be more appropriate - but this would still result in a method of the same length. |
||
| end | ||
|
|
||
| def self._icon(icon, collection = 'fa-sharp fa-solid') | ||
| "<i class='#{collection} fa-#{icon}' aria-hidden='true'></i>" | ||
| 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 | ||
|
Comment on lines
154
to
161
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is addressed (perhaps not resolved entirely) in the next commit |
||
|
|
||
| # 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) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| <% if AlmaSru.enabled? && @availability.present? %> | ||
| <div class="availability"> | ||
| <% @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' }) %><br> | ||
| <% end %> | ||
| </div> | ||
| <% end %> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <% return unless AlmaSru.enabled? %> | ||
|
|
||
| <% data_url = almasru_path(doc_id: doc_id) %> | ||
|
|
||
| <span class="availability-container" | ||
| data-controller="content-loader" | ||
| data-content-loader-url-value="<%= data_url %>" | ||
| data-content-loader-lazy-loading-value=""> | ||
| <span class="skeleton-loader"> </span> | ||
| </span> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
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.
Line is too long. [147/120] [rubocop:Layout/LineLength]