Skip to content

Move some synchronous requests to a background job (to speed up POST /barcode/:barcode/update operations) #50

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
elohanlon merged 2 commits into from
Dec 13, 2025
Merged

Move some synchronous requests to a background job (to speed up POST /barcode/:barcode/update operations) #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1aa338c
Move some synchronous requests to a background job (to speed up POST …
elohanlon Dec 11, 2025
97b74ab
Update title tag value
elohanlon Dec 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/barcode_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
32 changes: 32 additions & 0 deletions app/jobs/location_cleanup_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion app/views/layouts/application.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<!DOCTYPE html>
<html>
<head>
<title>Triclops</title>
<title>Bibdata</title>
<%= csrf_meta_tags %>
<%= csp_meta_tag %>

Expand Down
4 changes: 2 additions & 2 deletions config/application.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions config/templates/redis.template.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ development:

test:
<<: *default
port: 7379
2 changes: 2 additions & 0 deletions config/templates/resque.template.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
default: &default
run_jobs_inline: false
workers:
'*' : 1
polling_interval: 5
Expand All @@ -9,6 +10,7 @@ development:

test:
<<: *default
run_jobs_inline: true

production:
<<: *default
Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ services:
redis:
image: redis:7.0.2
ports:
- 6379:6379
- 6380:6379
volumes:
- redis-data:/data

4 changes: 2 additions & 2 deletions lib/bibdata/folio_api_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:)
Expand Down Expand Up @@ -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:)
Expand Down
73 changes: 24 additions & 49 deletions lib/bibdata/scsb.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
118 changes: 44 additions & 74 deletions spec/bibdata/scsb_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
27 changes: 27 additions & 0 deletions spec/jobs/location_cleanup_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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