From aa3a3f13ec67a05b06f6018fbe2875a5c3f935f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20A=2E=20Matienzo?= Date: Tue, 1 Sep 2026 14:15:04 -0700 Subject: [PATCH 1/4] AP-870: Make location requests OCLC limit configurable Read FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS through altmedia configuration and expose it on Rails configuration so LocationRequest::MAX_OCLC_NUMBERS can be adjusted without a code change. Add a sample env value and targeted coverage for config parsing, display formatting, and the over-limit spreadsheet check. Co-authored-by: Codex GPT-5 --- .env.sample | 1 + app/models/location_request.rb | 2 +- config/altmedia.yml | 1 + config/application.rb | 1 + spec/config/altmedia_spec.rb | 37 ++++++++++++++++++++++++++++ spec/models/location_request_spec.rb | 37 ++++++++++++++++++++++++++++ 6 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 spec/config/altmedia_spec.rb diff --git a/.env.sample b/.env.sample index 81cc6343..1e7d898f 100644 --- a/.env.sample +++ b/.env.sample @@ -7,3 +7,4 @@ FRAMEWORK_ALMA_SANDBOX_KEY=KEY_GOES_HERE LIT_TIND_API_KEY=KEY_GOES_HERE # set a warning message when we know we're getting rate limited by OCLC FRAMEWORK_LOCATION_REQUESTS_ALERT="Location Requests that involve WorldCat lookups are currently being rate limited by OCLC. We have contacted OCLC support and are awaiting resolution." +FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS=50000 diff --git a/app/models/location_request.rb b/app/models/location_request.rb index ab01487a..5b145eba 100644 --- a/app/models/location_request.rb +++ b/app/models/location_request.rb @@ -8,7 +8,7 @@ class LocationRequest < ActiveRecord::Base # Batch size for inserting LocationRecords BATCH_SIZE = 10_000 - MAX_OCLC_NUMBERS = 50_000 + MAX_OCLC_NUMBERS = Rails.application.config.location_requests_max_oclc_numbers RESULT_ARGS = %i[oclc_number wc_symbols wc_error ht_record_url ht_error].freeze MSG_NO_OCLC_NUMBERS = 'No OCLC numbers found in input spreadsheet'.freeze diff --git a/config/altmedia.yml b/config/altmedia.yml index 63a9f876..297ba6f6 100644 --- a/config/altmedia.yml +++ b/config/altmedia.yml @@ -16,6 +16,7 @@ default: &default alma_api_key: <%= ENV["FRAMEWORK_ALMA_API_KEY"].presence || 'fake-api-key' %> alma_sandbox_key: <%= ENV["FRAMEWORK_ALMA_SANDBOX_KEY"].presence || 'fake-api-key' %> location_requests_alert: <%= ENV["FRAMEWORK_LOCATION_REQUESTS_ALERT"] %> + location_requests_max_oclc_numbers: <%= Integer(ENV["FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS"].presence || 50_000) %> paypal_payflow_url: <%= ENV["PAYPAL_PAYFLOW_URL"] || 'https://payflowlink.paypal.com' %> paypal_payflow_login: <%= ENV["PAYPAL_PAYFLOW_LOGIN"] || 'ucblibrary' %> tind_base_uri: <%= ENV["LIT_TIND_BASE_URL"] || 'https://digicoll.lib.berkeley.edu/' %> diff --git a/config/application.rb b/config/application.rb index e80b167b..1e3d3451 100644 --- a/config/application.rb +++ b/config/application.rb @@ -87,6 +87,7 @@ def log_active_storage_root!(active_storage_root) # alert message on location requests when we know we're being rate limited config.location_requests_alert = config.altmedia['location_requests_alert'] + config.location_requests_max_oclc_numbers = config.altmedia['location_requests_max_oclc_numbers'] # Tind set values for marc inserts config.tind_resource_types = config.tind_marc['resource_types'] diff --git a/spec/config/altmedia_spec.rb b/spec/config/altmedia_spec.rb new file mode 100644 index 00000000..2dfaba08 --- /dev/null +++ b/spec/config/altmedia_spec.rb @@ -0,0 +1,37 @@ +require 'rails_helper' + +RSpec.describe 'altmedia configuration' do + + def with_location_requests_max_oclc_numbers(max_oclc_numbers) + env_var = 'FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS' + original_max_oclc_numbers = ENV.fetch(env_var, nil) + set_env(env_var, max_oclc_numbers) + yield + ensure + set_env(env_var, original_max_oclc_numbers) + end + + def set_env(env_var, value) + value ? ENV[env_var] = value : ENV.delete(env_var) + end + + def altmedia_config + Rails.application.config_for(:altmedia) + end + + it 'defaults the location requests max OCLC numbers to 50,000' do + with_location_requests_max_oclc_numbers(nil) do + expect(altmedia_config[:location_requests_max_oclc_numbers]).to eq(50_000) + end + end + + it 'reads the location requests max OCLC numbers from FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS' do + with_location_requests_max_oclc_numbers('123') do + expect(altmedia_config[:location_requests_max_oclc_numbers]).to eq(123) + end + end + + it 'exposes the location requests max OCLC numbers through Rails configuration' do + expect(Rails.application.config.location_requests_max_oclc_numbers).to eq(Rails.application.config.altmedia[:location_requests_max_oclc_numbers]) + end +end diff --git a/spec/models/location_request_spec.rb b/spec/models/location_request_spec.rb index 92c21ea1..1991f500 100644 --- a/spec/models/location_request_spec.rb +++ b/spec/models/location_request_spec.rb @@ -38,6 +38,23 @@ def assert_same_contents(expected_path, actual_attachment) expect(actual_blob).to eq(expected_blob) end + def uploaded_file_with_oclc_numbers(oclc_numbers) + Dir.mktmpdir(File.basename(__FILE__)) do |tmpdir| + original_path = 'spec/data/location/input-file-empty.xlsx' + new_path = File.join(tmpdir, "#{oclc_numbers.size}.xlsx") + + ss = BerkeleyLibrary::Util::XLSX::Spreadsheet.new(original_path) + c_index = ss.find_column_index_by_header!(BerkeleyLibrary::Location::Constants::OCLC_COL_HEADER) + oclc_numbers.each_with_index do |oclc_num, i| + r_index = 1 + i # skip header row + ss.set_value_at(r_index, c_index, oclc_num) + end + ss.save_as(new_path) + + uploaded_file_from(new_path, mime_type: mime_type_xlsx) + end + end + # ------------------------------- # setup / teardown @@ -83,6 +100,13 @@ def assert_same_contents(expected_path, actual_attachment) end end + describe '.max_oclc_numbers' do + it 'returns the configured max OCLC numbers formatted for display' do + expect(LocationRequest::MAX_OCLC_NUMBERS).to eq(Rails.application.config.location_requests_max_oclc_numbers) + expect(LocationRequest.max_oclc_numbers).to eq(Rails.application.config.location_requests_max_oclc_numbers.to_fs(:delimited)) + end + end + describe :create do it 'accepts an input file' do req = LocationRequest.create!(**valid_attributes) @@ -321,6 +345,19 @@ def assert_same_contents(expected_path, actual_attachment) expect(req.location_records.count).to eq(expected_count) end + it 'raises ArgumentError when the input contains more OCLC numbers than configured' do + stub_const('LocationRequest::MAX_OCLC_NUMBERS', 2) + + attributes = valid_attributes.except(:input_file) + attributes[:input_file] = uploaded_file_with_oclc_numbers(%w[1 2 3]) + req = LocationRequest.create!(**attributes) + + expect { req.ensure_location_records! } + .to raise_error(ArgumentError, I18n.t('location_request.errors.max_oclc_numbers', max: '2')) + + expect(req.location_records).not_to exist + end + it 'handles large numbers of records' do # NOTE: tested with up to 1 million, but it's slow (~4 minutes) expected_count = 15_000 From fe348fda735c856737b7bc580ab8a5450d47b621 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20A=2E=20Matienzo?= Date: Tue, 1 Sep 2026 14:29:17 -0700 Subject: [PATCH 2/4] address PR feedback from @davezuckerman --- .env.sample | 2 +- config/altmedia.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.env.sample b/.env.sample index 1e7d898f..9f5f230a 100644 --- a/.env.sample +++ b/.env.sample @@ -7,4 +7,4 @@ FRAMEWORK_ALMA_SANDBOX_KEY=KEY_GOES_HERE LIT_TIND_API_KEY=KEY_GOES_HERE # set a warning message when we know we're getting rate limited by OCLC FRAMEWORK_LOCATION_REQUESTS_ALERT="Location Requests that involve WorldCat lookups are currently being rate limited by OCLC. We have contacted OCLC support and are awaiting resolution." -FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS=50000 +FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS=10000 diff --git a/config/altmedia.yml b/config/altmedia.yml index 297ba6f6..c945d123 100644 --- a/config/altmedia.yml +++ b/config/altmedia.yml @@ -16,7 +16,7 @@ default: &default alma_api_key: <%= ENV["FRAMEWORK_ALMA_API_KEY"].presence || 'fake-api-key' %> alma_sandbox_key: <%= ENV["FRAMEWORK_ALMA_SANDBOX_KEY"].presence || 'fake-api-key' %> location_requests_alert: <%= ENV["FRAMEWORK_LOCATION_REQUESTS_ALERT"] %> - location_requests_max_oclc_numbers: <%= Integer(ENV["FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS"].presence || 50_000) %> + location_requests_max_oclc_numbers: <%= Integer(ENV["FRAMEWORK_LOCATION_REQUESTS_MAX_OCLC_NUMBERS"].presence || 10_000) %> paypal_payflow_url: <%= ENV["PAYPAL_PAYFLOW_URL"] || 'https://payflowlink.paypal.com' %> paypal_payflow_login: <%= ENV["PAYPAL_PAYFLOW_LOGIN"] || 'ucblibrary' %> tind_base_uri: <%= ENV["LIT_TIND_BASE_URL"] || 'https://digicoll.lib.berkeley.edu/' %> From af009621bffdf687f9e4e276175924716e94ed43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?mar=C3=ADa=20a=2E=20matienzo?= <73732+anarchivist@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:15:36 -0700 Subject: [PATCH 3/4] Apply suggestion from @awilfox Co-authored-by: Anna Wilcox --- spec/config/altmedia_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/config/altmedia_spec.rb b/spec/config/altmedia_spec.rb index 2dfaba08..e3f41980 100644 --- a/spec/config/altmedia_spec.rb +++ b/spec/config/altmedia_spec.rb @@ -19,9 +19,9 @@ def altmedia_config Rails.application.config_for(:altmedia) end - it 'defaults the location requests max OCLC numbers to 50,000' do + it 'defaults the location requests max OCLC numbers to 10,000' do with_location_requests_max_oclc_numbers(nil) do - expect(altmedia_config[:location_requests_max_oclc_numbers]).to eq(50_000) + expect(altmedia_config[:location_requests_max_oclc_numbers]).to eq(10_000) end end From 2509338b38b4e4c21b5c88bb46d9f9d6c2812fcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20A=2E=20Matienzo?= Date: Tue, 1 Sep 2026 18:28:32 -0700 Subject: [PATCH 4/4] AP-870: Fix location request specs for 10K OCLC limit Update the configuration spec for the new 10,000 default and keep the large-record LocationRequest model spec above the batch boundary without depending on the default cap. Co-authored-by: Codex GPT-5 --- spec/models/location_request_spec.rb | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/spec/models/location_request_spec.rb b/spec/models/location_request_spec.rb index 1991f500..42fefa5f 100644 --- a/spec/models/location_request_spec.rb +++ b/spec/models/location_request_spec.rb @@ -360,32 +360,18 @@ def uploaded_file_with_oclc_numbers(oclc_numbers) it 'handles large numbers of records' do # NOTE: tested with up to 1 million, but it's slow (~4 minutes) - expected_count = 15_000 + expected_count = LocationRequest::BATCH_SIZE + 1 + stub_const('LocationRequest::MAX_OCLC_NUMBERS', expected_count) oclc_numbers = Array.new(expected_count) { |i| (expected_count + i).to_s } oclc_numbers.shuffle! - Dir.mktmpdir(File.basename(__FILE__)) do |tmpdir| - original_path = 'spec/data/location/input-file-empty.xlsx' - new_path = File.join(tmpdir, "#{expected_count}.xlsx") - - ss = BerkeleyLibrary::Util::XLSX::Spreadsheet.new(original_path) - c_index = ss.find_column_index_by_header!(BerkeleyLibrary::Location::Constants::OCLC_COL_HEADER) - oclc_numbers.each_with_index do |oclc_num, i| - r_index = 1 + i # skip header row - ss.set_value_at(r_index, c_index, oclc_num) - end - ss.save_as(new_path) - - input_file = uploaded_file_from(new_path, mime_type: mime_type_xlsx) - - attributes = valid_attributes.except(:input_file) - attributes[:input_file] = input_file + attributes = valid_attributes.except(:input_file) + attributes[:input_file] = uploaded_file_with_oclc_numbers(oclc_numbers) - req = LocationRequest.create!(**attributes) - req.ensure_location_records! + req = LocationRequest.create!(**attributes) + req.ensure_location_records! - expect(req.location_records.count).to eq(expected_count) - end + expect(req.location_records.count).to eq(expected_count) end end