Fixes issue with navigating pnx

The results normalizer passed doc['pnx'] to the record normalizer, which does not work because there's metadata we need outside of doc['pnx']. I also ran rubocop and removed the checks for the 'pnx' field, which seemed excessive.

Author: jazairi

app/models/normalize_primo_record.rb

@@ -38,7 +38,7 @@ def title
     end
   end
 
-    def creators
+  def creators
     return [] unless @record['pnx']['display']['creator'] || @record['pnx']['display']['contributor']
 
     author_list = []
@@ -84,7 +84,7 @@ def format
   # the one that is most predictably useful to us. The record_link is constructed.
   def links
     links = []
-    
+
     # Use dedup URL as the full record link if available, otherwise use record link
     if dedup_url.present?
       links << { 'url' => dedup_url, 'kind' => 'full record' }
@@ -93,9 +93,7 @@ def links
     end
 
     # Add openurl if available
-    if openurl.present?
-      links << { 'url' => openurl, 'kind' => 'openurl' }
-    end
+    links << { 'url' => openurl, 'kind' => 'openurl' } if openurl.present?
 
     # Return links if we found any
     links.any? ? links : []
@@ -167,7 +165,7 @@ def record_link
   end
 
   def numbering
-    return unless @record['pnx'] && @record['pnx']['addata']
+    return unless @record['pnx']['addata']
     return unless @record['pnx']['addata']['volume']
 
     if @record['pnx']['addata']['issue'].present?
@@ -178,7 +176,7 @@ def numbering
   end
 
   def chapter_numbering
-    return unless @record['pnx'] && @record['pnx']['addata']
+    return unless @record['pnx']['addata']
     return unless @record['pnx']['addata']['btitle']
     return unless @record['pnx']['addata']['date'] && @record['pnx']['addata']['pages']
 
@@ -192,7 +190,7 @@ def sanitize_authors(authors)
 
   def author_link(author)
     [ENV.fetch('MIT_PRIMO_URL'),
-    '/discovery/search?query=creator,exact,',
+     '/discovery/search?query=creator,exact,',
      encode_author(author),
      '&tab=', ENV.fetch('PRIMO_TAB'),
      '&search_scope=all&vid=',
@@ -219,7 +217,7 @@ def openurl
     return unless @record['delivery'] && @record['delivery']['almaOpenurl']
 
     # Check server match
-    openurl_server = ENV['ALMA_OPENURL'][8, 4]
+    openurl_server = ENV.fetch('ALMA_OPENURL', nil)[8, 4]
     record_openurl_server = @record['delivery']['almaOpenurl'][8, 4]
     if openurl_server == record_openurl_server
       construct_primo_openurl
@@ -236,13 +234,13 @@ def construct_primo_openurl
     # 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['MIT_PRIMO_URL'],
+    primo_openurl_base = [ENV.fetch('MIT_PRIMO_URL', nil),
                           '/discovery/openurl?institution=',
-                          ENV['EXL_INST_ID'],
+                          ENV.fetch('EXL_INST_ID', nil),
                           '&vid=',
-                          ENV['PRIMO_VID'],
+                          ENV.fetch('PRIMO_VID', nil),
                           '&'].join
-    primo_openurl = @record['delivery']['almaOpenurl'].gsub(ENV['ALMA_OPENURL'], primo_openurl_base)
+    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)
@@ -256,34 +254,39 @@ def thumbnail
     # A record can have multiple ISBNs, so we are assuming here that
     # the thumbnail URL can be constructed from the first occurrence
     isbn = @record['pnx']['addata']['isbn'].first
-    [ENV['SYNDETICS_PRIMO_URL'], '&isbn=', isbn, '/sc.jpg'].join
+    [ENV.fetch('SYNDETICS_PRIMO_URL', nil), '&isbn=', isbn, '/sc.jpg'].join
   end
 
   def publisher
     return unless @record['pnx']['addata'] && @record['pnx']['addata']['pub']
+
     @record['pnx']['addata']['pub'].first
   end
 
   def best_location
     return unless @record['delivery']
     return unless @record['delivery']['bestlocation']
+
     loc = @record['delivery']['bestlocation']
     ["#{loc['mainLocation']} #{loc['subLocation']}", loc['callNumber']]
   end
 
   def subjects
     return [] unless @record['pnx']['display']['subject']
+
     @record['pnx']['display']['subject']
   end
 
   def best_availability
     return unless best_location
+
     @record['delivery']['bestlocation']['availabilityStatus']
   end
 
   def other_availability?
     return unless @record['delivery']['bestlocation']
     return unless @record['delivery']['holding']
+
     @record['delivery']['holding'].length > 1
   end
 
@@ -292,25 +295,26 @@ def other_availability?
   def frbrized?
     return unless @record['pnx']['facets']
     return unless @record['pnx']['facets']['frbrtype']
+
     @record['pnx']['facets']['frbrtype'].join == '5'
   end
 
   def dedup_url
     return unless frbrized?
     return unless @record['pnx']['facets']['frbrgroupid'] &&
                   @record['pnx']['facets']['frbrgroupid'].length == 1
-    
+
     frbr_group_id = @record['pnx']['facets']['frbrgroupid'].join
-    base = [ENV['MIT_PRIMO_URL'], '/discovery/search?'].join
-    
+    base = [ENV.fetch('MIT_PRIMO_URL', nil), '/discovery/search?'].join
+
     query = {
       query: "any,contains,#{@query}",
-      tab: ENV['PRIMO_TAB'],
-      search_scope: ENV['PRIMO_SCOPE'],
+      tab: ENV.fetch('PRIMO_TAB', nil),
+      search_scope: ENV.fetch('PRIMO_SCOPE', nil),
       sortby: 'date_d',
-      vid: ENV['PRIMO_VID'],
+      vid: ENV.fetch('PRIMO_VID', nil),
       facet: "frbrgroupid,include,#{frbr_group_id}"
     }.to_query
     [base, query].join
   end
-end
+end

app/models/normalize_primo_results.rb

@@ -9,13 +9,11 @@ def normalize
     return [] unless @results&.dig('docs')
 
     @results['docs'].filter_map do |doc|
-      next unless doc['pnx']
-      
-      NormalizePrimoRecord.new(doc['pnx'], @query).normalize
+      NormalizePrimoRecord.new(doc, @query).normalize
     end
   end
 
   def total_results
     @results&.dig('info', 'total') || 0
   end
-end
+end

test/models/normalize_primo_record_test.rb

@@ -63,7 +63,7 @@ def minimal_record
 
     # First link should be the record link
     assert_equal 'full record', normalized['links'].first['kind']
-  
+
     # Second link should be the Alma openurl
     assert_equal 'openurl', normalized['links'].second['kind']
   end
@@ -230,10 +230,10 @@ def minimal_record
   test 'includes expected link types when available' do
     normalized = NormalizePrimoRecord.new(full_record, 'test query').normalize
     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_equal 2, normalized['links'].length # Only full record and openurl
   end
 
   # Additional coverage tests for existing methods
@@ -308,9 +308,9 @@ def minimal_record
 
   test 'uses btitle when jtitle is not present for container' do
     record = full_record.deep_dup
-    record['pnx']['addata'].delete('jtitle')  # Remove jtitle
+    record['pnx']['addata'].delete('jtitle') # Remove jtitle
     record['pnx']['addata']['btitle'] = ['Book Title Only']
-    
+
     normalized = NormalizePrimoRecord.new(record, 'test').normalize
     assert_equal 'Book Title Only', normalized['container']
   end
@@ -328,4 +328,4 @@ def minimal_record
     openurl_link = normalized['links'].find { |link| link['kind'] == 'openurl' }
     assert_not_nil openurl_link
   end
-end
+end

test/models/normalize_primo_results_test.rb

@@ -4,8 +4,8 @@ class NormalizePrimoResultsTest < ActiveSupport::TestCase
   def sample_results
     {
       'docs' => [
-        { 'pnx' => JSON.parse(File.read(Rails.root.join('test/fixtures/primo/full_record.json'))) },
-        { 'pnx' => JSON.parse(File.read(Rails.root.join('test/fixtures/primo/minimal_record.json'))) }
+        JSON.parse(File.read(Rails.root.join('test/fixtures/primo/full_record.json'))),
+        JSON.parse(File.read(Rails.root.join('test/fixtures/primo/minimal_record.json')))
       ],
       'info' => {
         'total' => 10
@@ -53,26 +53,6 @@ def empty_results
     assert_empty normalized
   end
 
-  # I hope this never happens, but just in case!
-  test 'handles docs without pnx field' do
-    results_with_bad_docs = {
-      'docs' => [
-        { 'pnx' => JSON.parse(File.read(Rails.root.join('test/fixtures/primo/full_record.json'))) },
-        { 'other_field' => 'no pnx here' },
-        { 'pnx' => JSON.parse(File.read(Rails.root.join('test/fixtures/primo/minimal_record.json'))) }
-      ],
-      'info' => { 'total' => 3 }
-    }
-    
-    normalizer = NormalizePrimoResults.new(results_with_bad_docs, 'test query')
-    normalized = normalizer.normalize
-
-    # Should only normalize the 2 docs with pnx fields
-    assert_equal 2, normalized.length
-    assert_equal 'Testing the Limits of Knowledge', normalized.first['title']
-    assert_equal 'unknown title', normalized.second['title']
-  end
-
   test 'returns total results count' do
     normalizer = NormalizePrimoResults.new(sample_results, 'test query')
     assert_equal 10, normalizer.total_results
@@ -87,4 +67,4 @@ def empty_results
     normalizer = NormalizePrimoResults.new(nil, 'test query')
     assert_equal 0, normalizer.total_results
   end
-end
+end