From ea0aa1c21f68823c974ec35d6bddc0a6dbd8f74d Mon Sep 17 00:00:00 2001 From: Justin Coyne Date: Fri, 11 Aug 2023 11:59:43 -0500 Subject: [PATCH] Extract logic about querying service points to a new class --- app/jobs/submit_folio_request_job.rb | 4 +- app/models/folio/pickup_destination.rb | 6 +- app/models/folio/request_abilities.rb | 17 ++--- app/models/folio/service_point.rb | 4 ++ app/services/folio/service_point_store.rb | 32 +++++++++ app/services/folio/types.rb | 40 +++-------- .../pickup_libraries_dropdown_spec.rb | 2 +- spec/helpers/pickup_libraries_helper_spec.rb | 27 ++----- spec/models/folio/pickup_destination_spec.rb | 70 ++----------------- 9 files changed, 75 insertions(+), 127 deletions(-) create mode 100644 app/services/folio/service_point_store.rb diff --git a/app/jobs/submit_folio_request_job.rb b/app/jobs/submit_folio_request_job.rb index 82edd619e..6eb416d45 100644 --- a/app/jobs/submit_folio_request_job.rb +++ b/app/jobs/submit_folio_request_job.rb @@ -94,13 +94,13 @@ def place_title_hold def pickup_location_id @pickup_location_id ||= begin code = service_point_code(request.destination) - Folio::Types.instance.service_point_id(code) + Folio::Types.service_points.find_by(code:)&.id end end def service_point_code(destination) # Check if comparable service point code exists, otherwise return default - return destination if Folio::Types.instance.valid_service_point_code?(destination) + return destination if Folio::Types.service_points.find_by(code: destination) # During cutover and migration, we may still need to depend on the service point defined in settings Settings.libraries[destination]&.folio_pickup_service_point_code || Settings.folio.default_service_point diff --git a/app/models/folio/pickup_destination.rb b/app/models/folio/pickup_destination.rb index 819db9c53..221353576 100644 --- a/app/models/folio/pickup_destination.rb +++ b/app/models/folio/pickup_destination.rb @@ -13,14 +13,16 @@ def initialize(destination_code) @code = destination_code end + attr_reader :code + def display_label - Folio::Types.instance.service_point_name(@code) + Folio::Types.service_points.find_by(code:).name end # For paging scheduling, we must map from service point to library if folio # Otherwise, the library code is what we will be receiving def paging_code - Folio::Types.instance.map_to_library_code(@code) + Folio::Types.map_to_library_code(code) end end end diff --git a/app/models/folio/request_abilities.rb b/app/models/folio/request_abilities.rb index cf6b99174..795ede6e6 100644 --- a/app/models/folio/request_abilities.rb +++ b/app/models/folio/request_abilities.rb @@ -67,22 +67,23 @@ def pickup_destinations location_restricted_service_point_codes end - # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity + # Find service point which is default for this particular campus def default_pickup_destination - # Find service point which is default for this particular campus - pickup = Folio::Types.instance.service_points.select do |_k, v| - v.is_default_for_campus.present? && v.is_default_for_campus == request.holdings.first&.effective_location&.campus&.code - end.values.map(&:code) - pickup.present? ? pickup[0] : Settings.folio.default_service_point + campus_code = request.holdings.first&.effective_location&.campus&.code + service_points = if campus_code + Folio::Types.service_points.where(is_default_for_campus: campus_code).map(&:code) + else + [] + end + service_points.first || Settings.folio.default_service_point end private # Returns default service point codes def default_pickup_service_points - Folio::Types.instance.service_points.select { |_k, v| v.is_default_pickup }.values.map(&:code) + Folio::Types.service_points.where(is_default_pickup: true).map(&:code) end - # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity def additional_pickup_service_points # Map library to a service point diff --git a/app/models/folio/service_point.rb b/app/models/folio/service_point.rb index 4a607fb00..e3f7cca6c 100644 --- a/app/models/folio/service_point.rb +++ b/app/models/folio/service_point.rb @@ -11,5 +11,9 @@ def self.from_dynamic(json) is_default_pickup: json.dig('details', 'isDefaultPickup'), is_default_for_campus: json.dig('details', 'isDefaultForCampus')) end + + def pickup_location? + pickup_location == true + end end end diff --git a/app/services/folio/service_point_store.rb b/app/services/folio/service_point_store.rb new file mode 100644 index 000000000..12f0f3cf6 --- /dev/null +++ b/app/services/folio/service_point_store.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Folio + # A cache of service point data + class ServicePointStore + def initialize(data_from_cache) + @data = data_from_cache.map { |p| Folio::ServicePoint.from_dynamic(p) } + end + + attr_reader :data + + def find_by(args) + if args.key?(:code) + data.find { |candidate| candidate.code == args[:code] } + elsif args.key?(:id) + data.find { |candidate| candidate.id == args[:id] } + else + raise "unknown argument #{args.inspect}" + end + end + + def where(args) + if args.key?(:is_default_for_campus) + data.select { |candidate| candidate.is_default_for_campus == args[:is_default_for_campus] } + elsif args.key?(:is_default_pickup) + data.select { |candidate| candidate.is_default_pickup == args[:is_default_pickup] } + else + raise "unknown argument #{args.inspect}" + end + end + end +end diff --git a/app/services/folio/types.rb b/app/services/folio/types.rb index a76304d8d..877ee24c8 100644 --- a/app/services/folio/types.rb +++ b/app/services/folio/types.rb @@ -6,9 +6,8 @@ module Folio # accessing the types. class Types class << self - delegate :policies, :circulation_rules, :criteria, :get_type, :locations, :libraries, - :map_to_library_code, :map_to_service_point_code, :service_point_name, :service_point_code, - :service_point_id, :valid_service_point_code?, to: :instance + delegate :policies, :circulation_rules, :criteria, :get_type, :locations, :libraries, :service_points, + :map_to_library_code, :map_to_service_point_code, to: :instance end def self.instance @@ -46,7 +45,7 @@ def circulation_rules end def service_points - get_type('service_points').map { |p| Folio::ServicePoint.from_dynamic(p) }.index_by(&:id) + @service_points ||= ServicePointStore.new(get_type('service_points')) end def policies @@ -94,40 +93,31 @@ def get_type(type) def map_to_library_code(service_point_code) return service_point_code if service_point_code == 'SCAN' - return nil unless valid_service_point_code?(service_point_code) + service_point = service_points.find_by(code: service_point_code) + return nil unless service_point - service_point_id = service_point_id(service_point_code) - library_id = library_for_service_point_id(service_point_id) + library_id = library_for_service_point_id(service_point.id) # Find the library code associated with this library id libraries.key?(library_id) ? libraries[library_id]['code'] : nil end - # Find the service point ID based on this service point code - def service_point_id(service_point_code) - service_points.values.find { |v| v.code == service_point_code }&.id - end - # Find the library id for the location with which this service point is associated def library_for_service_point_id(service_point_id) loc = locations.values.find { |location| location['primaryServicePoint'] == service_point_id } loc && loc['libraryId'] end - # Check if valid service point - def valid_service_point_code?(service_point_code) - service_points.values.any? { |v| v.code == service_point_code } - end - # Given a library code, retrieve the primary service point, ensuring pickup location is true def map_to_service_point_code(library_code) # Find library id for the library with this code library_id = library_id(library_code) # Get the associated location and related service point service_point_id = service_point_for_library(library_id) - # Find the service point ID based on this service point code - service_point = service_point_by_id(service_point_id) - service_point.present? && service_point.pickup_location == true ? service_point.code : nil + + service_point = service_points.find_by(id: service_point_id) + + service_point.code if service_point&.pickup_location? end def library_id(library_code) @@ -140,16 +130,6 @@ def service_point_for_library(library_id) loc && loc['primaryServicePoint'] end - def service_point_by_id(service_point_id) - service_points.key?(service_point_id) && service_points[service_point_id] - end - - # Get the name for the service point given the code - def service_point_name(code) - # Find the service point with the same code, and return the name - service_points.values.find { |v| v.code == code }&.name - end - private # rubocop:disable Metrics/MethodLength diff --git a/spec/features/pickup_libraries_dropdown_spec.rb b/spec/features/pickup_libraries_dropdown_spec.rb index d266f4771..87aa18d37 100644 --- a/spec/features/pickup_libraries_dropdown_spec.rb +++ b/spec/features/pickup_libraries_dropdown_spec.rb @@ -4,7 +4,7 @@ RSpec.describe 'Pickup Libraries Dropdown' do let(:is_folio) { (Settings.ils.bib_model == 'Folio::Instance') } - let(:folio_pickup_lib_total) { Folio::Types.instance.service_points.select { |_k, v| v.is_default_pickup }.values.length } + let(:folio_pickup_lib_total) { Folio::Types.service_points.where(is_default_pickup: true).length } let(:standard_pickup_lib_total) { is_folio ? folio_pickup_lib_total : Settings.default_pickup_libraries.count } let(:media_library) { is_folio ? 'MEDIA-CENTER' : 'MEDIA-MTXT' } let(:media_label) { is_folio ? 'Media Center' : 'Media Microtext' } diff --git a/spec/helpers/pickup_libraries_helper_spec.rb b/spec/helpers/pickup_libraries_helper_spec.rb index 7b70b5fe0..81f06093b 100644 --- a/spec/helpers/pickup_libraries_helper_spec.rb +++ b/spec/helpers/pickup_libraries_helper_spec.rb @@ -8,23 +8,6 @@ %w[ABC XYZ] end - let(:service_points_json) do - <<~JSON - [ - { - "id": "ABC", - "code": "ABC", - "discoveryDisplayName": "Library 2" - }, - { - "id": "XYZ", - "code": "XYZ", - "discoveryDisplayName": "Library 1" - } - ] - JSON - end - before do allow(Settings).to receive(:libraries).and_return( { @@ -33,9 +16,13 @@ } ) - allow(Folio::Types.instance).to receive(:service_points).and_return( - JSON.parse(service_points_json).map { |p| Folio::ServicePoint.from_dynamic(p) }.index_by(&:id) - ) + allow(Folio::Types.service_points).to receive(:find_by).with(code: 'ABC').and_return(instance_double(Folio::ServicePoint, + name: "Library 2" + )) + + allow(Folio::Types.service_points).to receive(:find_by).with(code: 'XYZ').and_return(instance_double(Folio::ServicePoint, + name: "Library 1" + )) end it 'sorts the libraries by the name of the library (and not the code)' do diff --git a/spec/models/folio/pickup_destination_spec.rb b/spec/models/folio/pickup_destination_spec.rb index ace4554bc..85a975ad7 100644 --- a/spec/models/folio/pickup_destination_spec.rb +++ b/spec/models/folio/pickup_destination_spec.rb @@ -3,75 +3,17 @@ require 'rails_helper' RSpec.describe Folio::PickupDestination do - subject(:pickup_destination) do - described_class.new( - 'GREEN-LOAN' - ) - end - - let(:service_points_json) do - <<~JSON - [ - { - "code": "GREEN-LOAN", - "id": "a5dbb3dc-84f8-4eb3-8bfe-c61f74a9e92d", - "discoveryDisplayName": "Green Library" - } - ] - JSON - end - - before do - allow(Folio::Types.instance).to receive(:service_points).and_return( - JSON.parse(service_points_json).map { |p| Folio::ServicePoint.from_dynamic(p) }.index_by(&:id) - ) - end + let(:pickup_destination) { described_class.new('GREEN-LOAN') } describe '#display_label' do - it 'returns service point name' do - expect(subject.display_label).to eq 'Green Library' - end + subject { pickup_destination.display_label } + + it { is_expected.to eq 'Green Library' } end describe '#paging_code' do - let(:locations_json) do - <<~JSON - [ - { - "id": "4573e824-9273-4f13-972f-cff7bf504217", - "code": "GRE-STACKS", - "discoveryDisplayName": "Stacks", - "libraryId": "f6b5519e-88d9-413e-924d-9ed96255f72e", - "primaryServicePoint": "a5dbb3dc-84f8-4eb3-8bfe-c61f74a9e92d" - } - ] - JSON - end - - let(:libraries_json) do - <<~JSON - [ - { - "id": "f6b5519e-88d9-413e-924d-9ed96255f72e", - "name": "Green Library", - "code": "GREEN", - "campusId": "c365047a-51f2-45ce-8601-e421ca3615c5" - } - ] - JSON - end - - before do - allow(Folio::Types.instance).to receive(:libraries).and_return( - JSON.parse(libraries_json).index_by { |p| p['id'] } - ) - allow(Folio::Types.instance).to receive(:locations).and_return( - JSON.parse(locations_json).index_by { |p| p['id'] } - ) - end + subject { pickup_destination.paging_code } - it 'returns library code for service point' do - expect(subject.paging_code).to eq 'GREEN' - end + it { is_expected.to eq 'GREEN' } end end