Respond to Copilot feedback

- Add env vars to README
- Refactor guard clause for env into separate method, expand to include
  EXL_INST_ID
- Fix typo in method comment
- Restore logger to original state using begin/ensure blocks for those
  tests
- Adds tests for updated guard clause
- Update test names to reflect empty list return, not nil return
- More robust test for env vars includes a confirmation of initial success
- Adds test helper for scrubbing cassettes of cookie values
- Scrubs cassettes

Author: matt-bernhardt

README.md

@@ -76,6 +76,8 @@ See `Optional Environment Variables` for more information.
 ### Required Environment Variables
 
 - `ALMA_OPENURL`: The base URL for Alma openurls found in CDI records.
+- `EXL_INST_ID`: The Ex Libris Institution ID. Used for constructing URLs.
+- `MIT_ALMA_URL`: The base URL for MIT Libraries' Alma instance (used to generate SRU API lookups).
 - `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.

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_base_url
+    return [] unless alma_sru_enabled?
 
     # Validate the raw identifier received. This will raise an InvalidAlmaId if validation fails.
     identifier = validate_alma_id(raw_identifier)
@@ -73,7 +73,7 @@ def self.parse_response(raw_response, reference_identifier)
 
   # validate_alma_id ensures we are only submitting valid Alma IDs to the SRU endpoint.
   #
-  # It needs to do two thigns:
+  # 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)
@@ -129,12 +129,22 @@ def self.alma_base_url
     ENV.fetch('MIT_ALMA_URL', nil)
   end
 
+  def self.alma_sru_enabled?
+    return false if alma_base_url.to_s.empty? || exl_inst_id.to_s.empty?
+
+    true
+  end
+
   def self.alma_sru_url(identifier)
-    # example identifier: 990000959610106761
-    "#{alma_base_url}/view/sru/#{ENV.fetch('EXL_INST_ID')}?version=1.2&operation=searchRetrieve&recordSchema=marcxml" \
+    # example identifier: 9935177389906761
+    "#{alma_base_url}/view/sru/#{exl_inst_id}?version=1.2&operation=searchRetrieve&recordSchema=marcxml" \
       "&query=alma.all_for_ui=#{identifier}"
   end
 
+  def self.exl_inst_id
+    ENV.fetch('EXL_INST_ID', nil)
+  end
+
   def self.setup(url, alma_client)
     alma_client || HTTP.persistent(url)
   end

test/models/alma_sru_test.rb

@@ -67,28 +67,50 @@ class AlmaSruTest < ActiveSupport::TestCase
 
   test 'lookup returns empty list for non-existent records' do
     output = StringIO.new
+    original_logger = Rails.logger
     Rails.logger = Logger.new(output)
 
-    VCR.use_cassette('alma sru nonexistent record') do
-      needle = 'alma9900000000006761'
+    begin
+      VCR.use_cassette('alma sru nonexistent record') do
+        needle = 'alma9900000000006761'
 
-      result = AlmaSru.lookup(needle)
+        result = AlmaSru.lookup(needle)
 
-      assert_equal(result, [])
+        assert_equal(result, [])
 
-      # This condition gets logged for local investigation due to control field mismatch
-      assert_match('Alma lookup failure: Control field mismatch', output.string)
+        # This condition gets logged for local investigation due to control field mismatch
+        assert_match('Alma lookup failure: Control field mismatch', output.string)
+      end
+    ensure
+      Rails.logger = original_logger
     end
   end
 
-  test 'lookup returns nil if env not set' do
-    needle = '990002935920106761'
+  test 'lookup returns empty list if alma URL not set' do
+    needle = 'alma990014651640106761'
+
+    VCR.use_cassette('alma sru single record') do
+      assert_equal(1, AlmaSru.lookup(needle).length)
+    end
+
     ClimateControl.modify(MIT_ALMA_URL: nil) do
       assert_equal([], AlmaSru.lookup(needle))
     end
   end
 
-  test 'lookup returns nil with non-complying ID' do
+  test 'lookup returns empty list if exl_inst_id not set' do
+    needle = 'alma990014651640106761'
+
+    VCR.use_cassette('alma sru single record') do
+      assert_equal(1, AlmaSru.lookup(needle).length)
+    end
+
+    ClimateControl.modify(EXL_INST_ID: nil) do
+      assert_equal([], AlmaSru.lookup(needle))
+    end
+  end
+
+  test 'lookup returns empty list with non-complying ID' do
     needle = 'foo'
 
     result = AlmaSru.lookup(needle)
@@ -99,34 +121,44 @@ class AlmaSruTest < ActiveSupport::TestCase
   test 'lookup survives failing to connect to Alma SRU' do
     alma_client = AlmaConnectionError.new
     output = StringIO.new
+    original_logger = Rails.logger
     Rails.logger = Logger.new(output)
 
     needle = 'alma990014651640106761'
 
-    assert_nothing_raised do
-      result = AlmaSru.lookup(needle, alma_client: alma_client)
+    begin
+      assert_nothing_raised do
+        result = AlmaSru.lookup(needle, alma_client: alma_client)
 
-      assert_equal([], result)
+        assert_equal([], result)
 
-      # This condition gets logged for local investigation
-      assert_match('Alma SRU connection error', output.string)
+        # This condition gets logged for local investigation
+        assert_match('Alma SRU connection error', output.string)
+      end
+    ensure
+      Rails.logger = original_logger
     end
   end
 
   test 'lookup survives Alma SRU errors' do
     alma_client = AlmaErrorResponse.new
     output = StringIO.new
+    original_logger = Rails.logger
     Rails.logger = Logger.new(output)
 
     needle = 'alma990014651640106761'
 
-    assert_nothing_raised do
-      result = AlmaSru.lookup(needle, alma_client: alma_client)
+    begin
+      assert_nothing_raised do
+        result = AlmaSru.lookup(needle, alma_client: alma_client)
 
-      assert_equal(result, [])
+        assert_equal(result, [])
 
-      # This condition gets logged for local investigation
-      assert_match('Alma lookup failure: 500', output.string)
+        # This condition gets logged for local investigation
+        assert_match('Alma lookup failure: 500', output.string)
+      end
+    ensure
+      Rails.logger = original_logger
     end
   end
 

test/test_helper.rb

@@ -32,6 +32,16 @@
   config.filter_sensitive_data('FAKE_THIRDIRON_ID') { ENV.fetch('THIRDIRON_ID').to_s }
   config.filter_sensitive_data('FAKE_THIRDIRON_KEY') { ENV.fetch('THIRDIRON_KEY').to_s }
   config.filter_sensitive_data('FAKE_OPENALEX_EMAIL') { ENV.fetch('OPENALEX_EMAIL').to_s }
+  # Filter cookie contents
+  config.before_record do |interaction|
+    cookies = interaction.response&.headers&.fetch('Set-Cookie', nil)
+    next unless cookies
+
+    interaction.response.headers['Set-Cookie'] = cookies.map do |cookie|
+      name = cookie.split('=', 2).first
+      "#{name}=<FILTERED_COOKIE>"
+    end
+  end
 end
 
 module ActiveSupport