Adjust fulfillment link logic

Why are these changes being introduced:

* We prefer LibKey links when available, but they are not always available.

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/USE-286

How does this address that need:

* Adjusts links we extract from Primo to include HTML and PDF direct
  links when available.
* Updates Primo OpenURL logic and adjust label to "Check Availability"
* If LibKey links become available, they will be used instead of the
  direct links from Primo by hiding them from the UI.

Document any side effects to this change:

* Hiding links after they are in the UI is not ideal, but it is a
  workaround to ensure that we do not display duplicate links in the UI.
  This will be revisited when we have more time.
* Introduced a new required ENV variable `PRIMO_INST_ID`. This was
  previously named differently and not included in the readme or deployed
  applications so our OpenURL links we broken.

Author: JPrevost

README.md

@@ -79,6 +79,7 @@ See `Optional Environment Variables` for more information.
 - `MIT_PRIMO_URL`: The base URL for MIT Libraries' Primo instance (used to generate record links).
 - `PRIMO_API_KEY`: The Primo Search API key.
 - `PRIMO_API_URL`: The Primo Search API base URL.
+- `PRIMO_INST_ID`: Our Ex Libris Instituion ID
 - `PRIMO_SCOPE`: The Primo Search API `scope` used in Primo UI links. Does not affect our API calls. Ask Enterprise Systems for value.
 - `PRIMO_TAB`: The Primo Search API `tab` used in Primo UI links. Does not affect our API calls. Ask Enterprise Systems for value.
 - `PRIMO_VID`: The Primo Search API `vid` (or 'view ID`) param. Used in both our API calls and Primo UI. Ask Enterprise Systems for value.

app/javascript/controllers/content_loader_controller.js

@@ -10,6 +10,16 @@ export default class extends Controller {
   load() {
     fetch(this.urlValue)
       .then(response => response.text())
-      .then(html => this.element.innerHTML = html)
+      .then(html => {
+        this.element.innerHTML = html;
+        // Hide primo links if libkey link is present
+        if (this.element.querySelector('.libkey-link')) {
+          const resultGet = this.element.closest('.result-get');
+          if (resultGet) {
+            const primoLinks = resultGet.querySelectorAll('.primo-link');
+            primoLinks.forEach(link => link.style.display = 'none');
+          }
+        }
+      })
   }
 }

app/models/libkey.rb

@@ -29,6 +29,7 @@ def self.lookup(type:, identifier:, libkey_client: nil)
       Sentry.set_tags('mitlib.libkeyurl': url)
       Sentry.set_tags('mitlib.libkeystatus': e.message)
       Sentry.capture_message('Unexpected Libkey response status')
+      Rails.logger.error("Unexpected Libkey response status: #{e.message}")
       nil
     rescue HTTP::Error
       Rails.logger.error('Libkey connection error')

app/models/normalize_primo_record.rb

@@ -107,13 +107,55 @@ def links
       links << { 'url' => record_link, 'kind' => 'full record' }
     end
 
-    # Add openurl if available
-    links << { 'url' => openurl, 'kind' => 'openurl' } if openurl.present?
+    # If $$Topenurl_article
+    if @record['pnx']['links'] && @record['pnx']['links']['openurl'] && openurl.present?
+      links << { 'url' => openurl, 'kind' => 'Check Availability' }
+    end
+
+    # Add PDF if available
+    if @record['pnx']['links'] && @record['pnx']['links']['linktopdf']
+
+      parsed_string = parse_link_string(@record['pnx']['links']['linktopdf'].first)
+
+      if parsed_string['U'].present?
+        links << { 'url' => parsed_string['U'],
+                   'kind' => 'View PDF' }
+      end
+    end
+
+    # Add HTML if available
+    if @record['pnx']['links'] && @record['pnx']['links']['linktohtml']
+
+      parsed_string = parse_link_string(@record['pnx']['links']['linktohtml'].first)
+
+      if parsed_string['U'].present?
+        links << { 'url' => parsed_string['U'],
+                   'kind' => 'View HTML' }
+      end
+    end
 
     # Return links if we found any
     links.any? ? links : []
   end
 
+  # Parses a link string into a hash of key-value pairs.
+  # The link string is a series of key-value pairs separated by $$, where each pair is prefixed by a single character.
+  # For example: "$$Uhttps://example.com$$TView PDF" would be parsed into { 'U' => 'https://example.com', 'T' => 'View PDF' }
+  def parse_link_string(link_string)
+    return unless link_string.start_with?('$$')
+
+    parts = link_string.split('$$')
+    hash = {}
+    parts.each do |part|
+      next if part.empty?
+
+      key = part[0]
+      value = part[1..-1]
+      hash[key] = value
+    end
+    hash
+  end
+
   def citation
     return unless @record['pnx']['display']
 
@@ -260,22 +302,26 @@ def openurl
 
   def construct_primo_openurl
     return unless @record['delivery']['almaOpenurl']
+    # Primo OpenURL links to some formats are not helpful
+    return if format == 'Journal'
+    return if format == 'Video'
 
     # Here we are converting the Alma link resolver URL provided by the Primo
     # Search API to redirect to the Primo UI. This is done for UX purposes,
     # as the regular Alma link resolver URLs redirect to a plaintext
     # disambiguation page.
     primo_openurl_base = [ENV.fetch('MIT_PRIMO_URL', nil),
                           '/discovery/openurl?institution=',
-                          ENV.fetch('EXL_INST_ID', nil),
+                          ENV.fetch('PRIMO_INST_ID', nil),
                           '&vid=',
                           ENV.fetch('PRIMO_VID', nil),
                           '&'].join
     primo_openurl = @record['delivery']['almaOpenurl'].gsub(ENV.fetch('ALMA_OPENURL', nil), primo_openurl_base)
 
     # The ctx params appear to break Primo openurls, so we need to remove them.
-    params = Rack::Utils.parse_nested_query(primo_openurl)
-    filtered = params.delete_if { |key, _value| key.starts_with?('ctx') }
+    # params = Rack::Utils.parse_nested_query(primo_openurl)
+    # filtered = params.delete_if { |key, _value| key.starts_with?('ctx') }
+    filtered = primo_openurl
     URI::DEFAULT_PARSER.unescape(filtered.to_param)
   end
 

app/views/libkey/lookup.html.erb

@@ -1,3 +1,3 @@
 <% if Libkey.enabled? && @libkey.present? %>
-  <%= link_to( @libkey[:text], @libkey[:link], class: 'button', style: 'border: none;' ) %>
+  <%= link_to( @libkey[:text], @libkey[:link], class: 'button libkey-link' ) %>
 <% end %>

app/views/search/_result_primo.html.erb

@@ -13,7 +13,6 @@
     </h3>
 
     <div class="result-metadata">
-
       <p class="pub-info">
         <span><%= result[:format] %></span>
         <span><%= result[:year] %></span>
@@ -63,19 +62,25 @@
       <% if result[:links].present? %>
         <% result[:links].each do |link| %>
           <% if link['kind'].downcase == 'full record' %>
+            <%# Full record link button uses the same link as the title link %>
             <% if Feature.enabled?(:record_link) %>
               <%= link_to 'View full record', link['url'], class: 'button' %>
             <% end %>
+          <%# Primo supplies PDF and HTML links in addition to the OpenURL variant that may be useful. %>
+          <%# We hide links with the `primo-link` css class if LibKey returns results. %>
           <% else %>
-            <%= link_to link['kind'].titleize, link['url'], class: 'button' %>
+            <%= link_to link['kind'], link['url'], class: 'button primo-link' %>
           <% end %>
         <% end %>
       <% end %>
+
+      <%# LibKey links are our preferred links so we call the API when possible to get these preferred links %>
       <% if Libkey.enabled? && result[:doi].present? %>
         <%= render(partial: 'trigger_libkey', locals: { kind: 'doi', identifier: result[:doi] }) %>
       <% elsif Libkey.enabled? && result[:pmid].present? %>
         <%= render(partial: 'trigger_libkey', locals: { kind: 'pmid', identifier: result[:pmid] }) %>
       <% end %>
+
       <% if result[:availability].present? %>
         <span class="availability"><%= availability(result[:availability], result[:location], result[:other_availability]) %></span>
       <% end %>

test/fixtures/primo/full_record.json

@@ -60,6 +60,26 @@
         "22110403"
       ]
     },
+    "links": {
+      "openurl": [
+        "$$Topenurl_article"
+      ],
+      "openurlfulltext": [
+        "$$Topenurlfull_article"
+      ],
+      "backlink": [
+        "$$Uhttp://pascal-francis.inist.fr/vibad/index.php?action=getRecordDetail&idt=21178602$$DView record in Pascal Francis$$Hfree_for_read"
+      ],
+      "thumbnail": [
+        "$$Tsyndetics_thumb_exl"
+      ],
+      "linktopdf": [
+        "$$Uhttps://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/pdf/20464433$$EPDF$$P50$$Gjstor$$H"
+      ],
+      "linktohtml": [
+        "$$Uhttps://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/20464433$$EHTML$$P50$$Gjstor$$H"
+      ]
+    },
     "facets": {
       "frbrtype": [
         "5"

test/models/normalize_primo_record_test.rb

@@ -73,14 +73,29 @@ def cdi_record
     assert_equal 'full record', normalized[:links].first['kind']
 
     # Second link should be the Alma openurl
-    assert_equal 'openurl', normalized[:links].second['kind']
+    assert_equal 'Check Availability', normalized[:links].second['kind']
   end
 
   test 'handles missing links' do
     normalized = NormalizePrimoRecord.new(minimal_record, 'test').normalize
     assert_empty normalized[:links]
   end
 
+  test 'parse_link_string creates expected data structure' do
+    # Strings that don't start with $$ should not be processed
+    link_string = 'https://example.com?param1=value1&param2=value2'
+    assert_nil NormalizePrimoRecord.new(full_record, 'test').send(:parse_link_string, link_string)
+
+    # Extract properly formatted links
+    link_string = '$$Uhttps://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/pdf/20464433$$EPDF$$P50$$Gjstor$$H'
+    expected = { 'U' => 'https://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/pdf/20464433', 'E' => 'PDF', 'P' => '50', 'G' => 'jstor', 'H' => '' }
+    assert_equal expected, NormalizePrimoRecord.new(full_record, 'test').send(:parse_link_string, link_string)
+
+    link_string = '$$Uhttps://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/20464433$$EHTML$$P50$$Gjstor$$H'
+    expected = { 'U' => 'https://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/20464433', 'E' => 'HTML', 'P' => '50', 'G' => 'jstor', 'H' => '' }
+    assert_equal expected, NormalizePrimoRecord.new(full_record, 'test').send(:parse_link_string, link_string)
+  end
+
   test 'normalizes citation' do
     normalized = NormalizePrimoRecord.new(full_record, 'test').normalize
     assert_equal 'Journal of Testing, Vol. 2, Issue 3', normalized[:citation]
@@ -278,8 +293,10 @@ def cdi_record
     link_kinds = normalized[:links].map { |link| link['kind'] }
 
     assert_includes link_kinds, 'full record'
-    assert_includes link_kinds, 'openurl'
-    assert_equal 2, normalized[:links].length # Only full record and openurl
+    assert_includes link_kinds, 'Check Availability'
+    assert_includes link_kinds, 'View PDF'
+    assert_includes link_kinds, 'View HTML'
+    assert_equal 4, normalized[:links].length
   end
 
   # Additional coverage tests for existing methods
@@ -351,14 +368,14 @@ def cdi_record
   test 'handles openurl server validation' do
     # Test with matching server
     normalized = NormalizePrimoRecord.new(full_record, 'test').normalize
-    openurl_link = normalized[:links].find { |link| link['kind'] == 'openurl' }
+    openurl_link = normalized[:links].find { |link| link['kind'] == 'Check Availability' }
     assert_not_nil openurl_link
 
     # Test with mismatched server. Should log warning but still return URL
     record = full_record.deep_dup
     record['delivery']['almaOpenurl'] = 'https://different.server.com/openurl?param=value'
     normalized = NormalizePrimoRecord.new(record, 'test').normalize
-    openurl_link = normalized[:links].find { |link| link['kind'] == 'openurl' }
+    openurl_link = normalized[:links].find { |link| link['kind'] == 'Check Availability' }
     assert_not_nil openurl_link
   end