From 1aa338c29f4d3b4c0b7620781ec8aed4481ca46d Mon Sep 17 00:00:00 2001 From: Eric O Date: Thu, 11 Dec 2025 01:45:29 -0500 Subject: [PATCH 1/2] Move some synchronous requests to a background job (to speed up POST /barcode/:barcode/update operations) --- app/controllers/barcode_controller.rb | 1 + app/jobs/location_cleanup_job.rb | 32 +++++++ config/application.rb | 4 +- config/templates/redis.template.yml | 1 + config/templates/resque.template.yml | 2 + docker-compose.yml | 2 +- lib/bibdata/folio_api_client.rb | 4 +- lib/bibdata/scsb.rb | 73 +++++---------- spec/bibdata/scsb_spec.rb | 118 +++++++++---------------- spec/jobs/location_cleanup_job_spec.rb | 27 ++++++ 10 files changed, 136 insertions(+), 128 deletions(-) create mode 100644 app/jobs/location_cleanup_job.rb create mode 100644 spec/jobs/location_cleanup_job_spec.rb diff --git a/app/controllers/barcode_controller.rb b/app/controllers/barcode_controller.rb index 6f7ef7f..e8f54a9 100644 --- a/app/controllers/barcode_controller.rb +++ b/app/controllers/barcode_controller.rb @@ -2,6 +2,7 @@ class BarcodeController < ApplicationController before_action :authenticate, only: [:update] + skip_before_action :verify_authenticity_token, only: [:update] # No need for CSRF token for API token auth endpoint rescue_from Faraday::Error, with: :handle_faraday_error rescue_from Bibdata::Exceptions::UnresolvableHoldingsPermanentLocationError, diff --git a/app/jobs/location_cleanup_job.rb b/app/jobs/location_cleanup_job.rb new file mode 100644 index 0000000..c15bf9f --- /dev/null +++ b/app/jobs/location_cleanup_job.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +# This job is generally queued by a POST request to the /barcodes/#{barcode}/update endpoint, which is made by the +# ReCAP SCSB system during an item accession. During that request, we synchronously flip the parent holdings location, +# and then we queue this job to asynchronously perform post-accession actions (like clearing the holdings temporary +# location, item permanent location, and item temporary location). +class LocationCleanupJob < ApplicationJob + queue_as :location_cleanup + + def perform(barcode) # rubocop:disable Metrics/MethodLength + # Clear item record permanent location + Bibdata::FolioApiClient.instance.update_item_record_location( + item_barcode: barcode, + location_type: :permanent, + new_location_code: nil + ) + + # Clear item record temporary location + Bibdata::FolioApiClient.instance.update_item_record_location( + item_barcode: barcode, + location_type: :temporary, + new_location_code: nil + ) + + # Clear parent holdings record temporary location + Bibdata::FolioApiClient.instance.update_item_parent_holdings_record_location( + item_barcode: barcode, + location_type: :temporary, + new_location_code: nil + ) + end +end diff --git a/config/application.rb b/config/application.rb index bc9a4cf..12f1cda 100644 --- a/config/application.rb +++ b/config/application.rb @@ -43,11 +43,11 @@ class Application < Rails::Application # config.eager_load_paths << Rails.root.join("extras") # Load custom configs - config.bibdata = Rails.application.config_for(:bibdata) + config.bibdata = config_for(:bibdata) config.folio = config_for(:folio) # Use Resque for ActiveJob - config.active_job.queue_adapter = :resque + config.active_job.queue_adapter = Rails.application.config_for(:resque)[:run_jobs_inline] ? :inline : :resque config.active_job.queue_name_prefix = "bibdata.#{Rails.env}" config.active_job.queue_name_delimiter = '.' end diff --git a/config/templates/redis.template.yml b/config/templates/redis.template.yml index 002ecb6..551bb1f 100644 --- a/config/templates/redis.template.yml +++ b/config/templates/redis.template.yml @@ -9,3 +9,4 @@ development: test: <<: *default + port: 7379 diff --git a/config/templates/resque.template.yml b/config/templates/resque.template.yml index d640450..489beff 100644 --- a/config/templates/resque.template.yml +++ b/config/templates/resque.template.yml @@ -1,4 +1,5 @@ default: &default + run_jobs_inline: false workers: '*' : 1 polling_interval: 5 @@ -9,6 +10,7 @@ development: test: <<: *default + run_jobs_inline: true production: <<: *default diff --git a/docker-compose.yml b/docker-compose.yml index cb087c8..2d7f642 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -7,7 +7,7 @@ services: redis: image: redis:7.0.2 ports: - - 6379:6379 + - 6380:6379 volumes: - redis-data:/data diff --git a/lib/bibdata/folio_api_client.rb b/lib/bibdata/folio_api_client.rb index 388eefe..9bcfbf7 100644 --- a/lib/bibdata/folio_api_client.rb +++ b/lib/bibdata/folio_api_client.rb @@ -59,7 +59,7 @@ def update_item_record_location(item_barcode:, location_type:, new_location_code rescue Faraday::Error => e raise Bibdata::Exceptions::LocationNotFoundError, 'Could not update item record permanent location to '\ "\"#{new_location_code}\". "\ - "FOLIO error message: #{e.response[:body]}" + "FOLIO error message: #{e.response&.fetch(:body) || e.message}" end def update_item_parent_holdings_record_location(item_barcode:, location_type:, new_location_code:) @@ -100,7 +100,7 @@ def update_item_parent_holdings_record_location(item_barcode:, location_type:, n rescue Faraday::Error => e raise Bibdata::Exceptions::LocationNotFoundError, 'Could not update holdings record permanent location to '\ "\"#{new_location_code}\". "\ - "FOLIO error message: #{e.response[:body]}" + "FOLIO error message: #{e.response&.fetch(:body) || e.message}" end def clear_item_record_temporary_location(item_barcode:, location_type:, new_location_code:) diff --git a/lib/bibdata/scsb.rb b/lib/bibdata/scsb.rb index 944a728..90f27fa 100644 --- a/lib/bibdata/scsb.rb +++ b/lib/bibdata/scsb.rb @@ -53,8 +53,7 @@ def self.merged_marc_record_for_barcode(barcode, flip_location:) if flip_location perform_location_flip!( - barcode, item_record, holdings_record, - current_holdings_permanent_location_code, material_type_record&.fetch('name') + barcode, current_holdings_permanent_location_code, material_type_record&.fetch('name') ) # After performing a flip, invoke this method again with `flip_location: false` to retrieve the latest updated @@ -79,23 +78,15 @@ def self.enhance_base_marc_record!( end def self.perform_location_flip!( - barcode, item_record, holdings_record, current_holdings_permanent_location_code, material_type_name + barcode, current_holdings_permanent_location_code, material_type_name ) - # If this item record has a permanent location, we want to clear it. - # Do this before updating the holdings location, since a holdings update - # will result in an item version update. - clear_item_permanent_location_if_present!(barcode, item_record) - # If the current holdings permanent location does not equal the desired flipped location, # update the holdings record permanent location to the flipped location. update_holdings_permanent_location_if_required!( barcode, current_holdings_permanent_location_code, material_type_name ) - # If this record has a holdings temporary location or item temporary location, - # send a notification email because assignment of temporary locations is - # undesirable for us and that needs to be manually corrected. - send_notification_email_if_temporary_locations_found(barcode, item_record, holdings_record) + LocationCleanupJob.perform_later(barcode) end # rubocop:disable Metrics/AbcSize @@ -152,43 +143,27 @@ def self.update_holdings_permanent_location_if_required!( end # rubocop:enable Metrics/AbcSize - def self.clear_item_permanent_location_if_present!(barcode, item_record) - return if item_record['permanentLocationId'].blank? - - self.location_change_logger.unknown( - "#{barcode}: Trying to clear item permanent location "\ - "(was originally FOLIO location #{item_record['permanentLocationId']})" - ) - Bibdata::FolioApiClient.instance.update_item_record_location( - item_barcode: barcode, location_type: :permanent, new_location_code: nil - ) - self.location_change_logger.unknown( - "#{barcode}: Cleared item permanent location "\ - "(was originally FOLIO location #{item_record['permanentLocationId']})" - ) - end - - def self.send_notification_email_if_temporary_locations_found( - barcode, item_record, holdings_record - ) - temporary_location_notification_messages = [] - if item_record['temporaryLocationId'].present? - error_message = 'Found unwanted item temporary location.' - self.location_change_logger.unknown("#{barcode}: #{error_message}") - temporary_location_notification_messages << error_message - end - if holdings_record['temporaryLocationId'].present? - error_message = 'Found unwanted parent holdings temporary location.' - self.location_change_logger.unknown("#{barcode}: #{error_message}") - temporary_location_notification_messages << error_message - end - - return if temporary_location_notification_messages.empty? - - BarcodeUpdateErrorMailer.with( - barcode: barcode, errors: temporary_location_notification_messages - ).generate_email.deliver - end + # def self.send_notification_email_if_temporary_locations_found( + # barcode, item_record, holdings_record + # ) + # temporary_location_notification_messages = [] + # if item_record['temporaryLocationId'].present? + # error_message = 'Found unwanted item temporary location.' + # self.location_change_logger.unknown("#{barcode}: #{error_message}") + # temporary_location_notification_messages << error_message + # end + # if holdings_record['temporaryLocationId'].present? + # error_message = 'Found unwanted parent holdings temporary location.' + # self.location_change_logger.unknown("#{barcode}: #{error_message}") + # temporary_location_notification_messages << error_message + # end + + # return if temporary_location_notification_messages.empty? + + # BarcodeUpdateErrorMailer.with( + # barcode: barcode, errors: temporary_location_notification_messages + # ).generate_email.deliver + # end # Returns the value if present, otherwise returns nil. def self.get_original_876_x_value(marc_record) diff --git a/spec/bibdata/scsb_spec.rb b/spec/bibdata/scsb_spec.rb index 16716f7..bdc4449 100644 --- a/spec/bibdata/scsb_spec.rb +++ b/spec/bibdata/scsb_spec.rb @@ -156,15 +156,10 @@ expect(Bibdata::Scsb).to receive(:update_holdings_permanent_location_if_required!).with( barcode, current_holdings_permanent_location_code, material_type_name ) - expect(Bibdata::Scsb).to receive(:clear_item_permanent_location_if_present!).with( - barcode, item_record - ) - expect(Bibdata::Scsb).to receive(:send_notification_email_if_temporary_locations_found).with( - barcode, item_record, holdings_record - ) + expect(LocationCleanupJob).to receive(:perform_later).with(barcode) Bibdata::Scsb.perform_location_flip!( - barcode, item_record, holdings_record, current_holdings_permanent_location_code, material_type_name + barcode, current_holdings_permanent_location_code, material_type_name ) end end @@ -241,73 +236,48 @@ end end - describe '.clear_item_permanent_location_if_present!' do - it 'clears the item permanent location if the item currently has a permanent location' do - - expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Trying to clear item permanent location/) - expect(Bibdata::FolioApiClient.instance).to receive( - :update_item_record_location - ).with( - item_barcode: barcode, location_type: :permanent, new_location_code: nil - ) - expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Cleared item permanent location/) - - Bibdata::Scsb.clear_item_permanent_location_if_present!( - barcode, item_record - ) - end - - it 'does not try to clear the item permanent location if the item does not currently have a permanent location' do - item_record['permanentLocationId'] = nil - expect(Bibdata::FolioApiClient.instance).not_to receive(:update_item_record_location) - Bibdata::Scsb.clear_item_permanent_location_if_present!( - barcode, item_record - ) - end - end - - describe '.send_notification_email_if_temporary_locations_found' do - it 'sends an email with one error if the item has a temporary location set' do - item_record['temporaryLocationId'] = 'some-location-id' - holdings_record.delete('temporaryLocationId') - expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Found unwanted item temporary location/) - expect(BarcodeUpdateErrorMailer).to receive(:with).with(barcode: barcode, errors: [an_instance_of(String)]) - Bibdata::Scsb.send_notification_email_if_temporary_locations_found( - barcode, item_record, holdings_record - ) - end - - it "sends an email with one error if the item's parent holdings record has a temporary location set" do - item_record.delete('temporaryLocationId') - holdings_record['temporaryLocationId'] = 'some-location-id' - expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Found unwanted parent holdings temporary location/) - expect(BarcodeUpdateErrorMailer).to receive(:with).with(barcode: barcode, errors: [an_instance_of(String)]) - Bibdata::Scsb.send_notification_email_if_temporary_locations_found( - barcode, item_record, holdings_record - ) - end - - it "sends an email with two errors if the item and its parent holdings record both have temporary locations set" do - item_record['temporaryLocationId'] = 'some-location-id' - holdings_record['temporaryLocationId'] = 'some-location-id' - - expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Found unwanted item temporary location/) - expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Found unwanted parent holdings temporary location/) - expect(BarcodeUpdateErrorMailer).to receive(:with).with( - barcode: barcode, errors: [an_instance_of(String), an_instance_of(String)] - ) - Bibdata::Scsb.send_notification_email_if_temporary_locations_found( - barcode, item_record, holdings_record - ) - end - - it 'does not try to send an email if the item and its parent holdings record do not have temporary locations set' do - expect(BarcodeUpdateErrorMailer).not_to receive(:with) - Bibdata::Scsb.send_notification_email_if_temporary_locations_found( - barcode, item_record, holdings_record - ) - end - end + # describe '.send_notification_email_if_temporary_locations_found' do + # it 'sends an email with one error if the item has a temporary location set' do + # item_record['temporaryLocationId'] = 'some-location-id' + # holdings_record.delete('temporaryLocationId') + # expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Found unwanted item temporary location/) + # expect(BarcodeUpdateErrorMailer).to receive(:with).with(barcode: barcode, errors: [an_instance_of(String)]) + # Bibdata::Scsb.send_notification_email_if_temporary_locations_found( + # barcode, item_record, holdings_record + # ) + # end + + # it "sends an email with one error if the item's parent holdings record has a temporary location set" do + # item_record.delete('temporaryLocationId') + # holdings_record['temporaryLocationId'] = 'some-location-id' + # expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Found unwanted parent holdings temporary location/) + # expect(BarcodeUpdateErrorMailer).to receive(:with).with(barcode: barcode, errors: [an_instance_of(String)]) + # Bibdata::Scsb.send_notification_email_if_temporary_locations_found( + # barcode, item_record, holdings_record + # ) + # end + + # it "sends an email with two errors if the item and its parent holdings record both have temporary locations set" do + # item_record['temporaryLocationId'] = 'some-location-id' + # holdings_record['temporaryLocationId'] = 'some-location-id' + + # expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Found unwanted item temporary location/) + # expect(Bibdata::Scsb.location_change_logger).to receive(:unknown).with(/Found unwanted parent holdings temporary location/) + # expect(BarcodeUpdateErrorMailer).to receive(:with).with( + # barcode: barcode, errors: [an_instance_of(String), an_instance_of(String)] + # ) + # Bibdata::Scsb.send_notification_email_if_temporary_locations_found( + # barcode, item_record, holdings_record + # ) + # end + + # it 'does not try to send an email if the item and its parent holdings record do not have temporary locations set' do + # expect(BarcodeUpdateErrorMailer).not_to receive(:with) + # Bibdata::Scsb.send_notification_email_if_temporary_locations_found( + # barcode, item_record, holdings_record + # ) + # end + # end end describe '.location_change_logger' do diff --git a/spec/jobs/location_cleanup_job_spec.rb b/spec/jobs/location_cleanup_job_spec.rb new file mode 100644 index 0000000..5c0506e --- /dev/null +++ b/spec/jobs/location_cleanup_job_spec.rb @@ -0,0 +1,27 @@ +require 'rails_helper' + +RSpec.describe LocationCleanupJob do + let(:instance) { described_class.new } + let(:barcode) { 'CU23392169' } + + describe '#perform' do + it 'performs the expected operations' do + expect(Bibdata::FolioApiClient.instance).to receive(:update_item_record_location).with( + item_barcode: barcode, + location_type: :permanent, + new_location_code: nil + ) + expect(Bibdata::FolioApiClient.instance).to receive(:update_item_record_location).with( + item_barcode: barcode, + location_type: :temporary, + new_location_code: nil + ) + expect(Bibdata::FolioApiClient.instance).to receive(:update_item_parent_holdings_record_location).with( + item_barcode: barcode, + location_type: :temporary, + new_location_code: nil + ) + instance.perform(barcode) + end + end +end From 97b74abc3db199db6b541473c1121b595e59c9cd Mon Sep 17 00:00:00 2001 From: Eric O Date: Sat, 13 Dec 2025 12:49:42 -0500 Subject: [PATCH 2/2] Update title tag value --- app/views/layouts/application.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index b96da5c..c0edfea 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -1,7 +1,7 @@ - Triclops + Bibdata <%= csrf_meta_tags %> <%= csp_meta_tag %>