diff --git a/app/controllers/requests/form_controller.rb b/app/controllers/requests/form_controller.rb index 340e27cee..13f092327 100644 --- a/app/controllers/requests/form_controller.rb +++ b/app/controllers/requests/form_controller.rb @@ -38,7 +38,7 @@ def aeon? # will post and a JSON document of selected "requestable" objects with selection parameters and # user information for further processing and distribution to various request endpoints. def submit - @submission = Requests::Submission.new(sanitize_submission(params), Patron.new(user: current_or_guest_user, session:)) + @submission = Requests::Submission.new(sanitize_submission(params), Patron.new(user: current_or_guest_user)) respond_to do |format| format.js do valid = @submission.valid? @@ -104,23 +104,11 @@ def respond_to_validation_error(submission) end def authorize_patron(user) - patron = Patron.new(user:, session:) + patron = Patron.new(user:) flash.now[:error] = patron.errors.join(", ") if patron.errors.present? patron end - def current_or_guest_user - user = super - - # store guest user information in the session for later - # TODO: Do we still hit this code? How do the user_name and email end up in the request params? - if user.guest? && params[:request].present? && params[:request][:user_name].present? - session["user_name"] = params[:request][:user_name] - session["email"] = params[:request][:email] - end - user - end - def sanitize(str) str.gsub(/[^A-Za-z0-9@\-_\.]/, '') if str.is_a? String str diff --git a/app/models/requests/access_patron.rb b/app/models/requests/access_patron.rb deleted file mode 100644 index 211d30017..000000000 --- a/app/models/requests/access_patron.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true -module Requests - # Create a Patron from session information - class AccessPatron - attr_reader :user_name, :email - def initialize(session:) - @user_name = session["user_name"] - @email = session["email"] - end - - def hash - { - last_name: user_name, - active_email: email, - barcode: 'ACCESS', - barcode_status: 0 - }.with_indifferent_access - end - end -end diff --git a/app/models/requests/patron.rb b/app/models/requests/patron.rb index 77a8bee83..42ea89757 100644 --- a/app/models/requests/patron.rb +++ b/app/models/requests/patron.rb @@ -3,15 +3,14 @@ module Requests class Patron - attr_reader :user, :session, :patron_hash, :errors + attr_reader :user, :patron_hash, :errors delegate :guest?, :provider, :cas_provider?, :alma_provider?, to: :user - def initialize(user:, session: {}, patron_hash: nil) + def initialize(user:, patron_hash: nil) @user = user - @session = session @errors = [] - # load the patron_hash from bibdata unless we are passing it in + # load the patron_hash from bibdata or alma unless we are passing it in @patron_hash = patron_hash || load_patron(user:) end @@ -96,8 +95,6 @@ def load_patron(user:) # Uncomment to fake being a non barcoded user # patron_hash[:barcode] = nil patron_hash || {} - elsif access_patron? - AccessPatron.new(session:).hash else {} end diff --git a/spec/helpers/requests/application_helper_spec.rb b/spec/helpers/requests/application_helper_spec.rb index 31afc8d49..800900fe8 100644 --- a/spec/helpers/requests/application_helper_spec.rb +++ b/spec/helpers/requests/application_helper_spec.rb @@ -11,7 +11,7 @@ "university_id" => "9999999", "patron_group" => "staff", "patron_id" => "99999", "active_email" => "foo@princeton.edu" }.with_indifferent_access end let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end describe '#isbn_string' do @@ -127,7 +127,7 @@ describe '#suppress_login' do let(:unauthenticated_patron) { FactoryBot.build(:unauthenticated_patron) } - let(:patron) { Requests::Patron.new(user: unauthenticated_patron, session: {}) } + let(:patron) { Requests::Patron.new(user: unauthenticated_patron) } let(:params) do { system_id: '9973529363506421', diff --git a/spec/mailers/requests/request_mailer_spec.rb b/spec/mailers/requests/request_mailer_spec.rb index 830855cf6..b1222682d 100644 --- a/spec/mailers/requests/request_mailer_spec.rb +++ b/spec/mailers/requests/request_mailer_spec.rb @@ -10,12 +10,12 @@ let(:user_info) do stub_request(:get, "#{Requests.config[:bibdata_base]}/patron/foo?ldap=true").to_return(status: 200, body: valid_patron_response, headers: {}) user = instance_double(User, guest?: false, uid: 'foo', alma_provider?: false) - Requests::Patron.new(user:, session: {}) + Requests::Patron.new(user:) end let(:guest_user_info) do user = instance_double(User, guest?: true, uid: 'foo') - Requests::Patron.new(user:, session: { "email" => "guest@foo.edu", 'user_name' => 'Guest Request' }) + Requests::Patron.new(user:) end before { stub_delivery_locations } diff --git a/spec/models/concerns/requests/scsb_spec.rb b/spec/models/concerns/requests/scsb_spec.rb index 3b18aaa28..8665909c8 100644 --- a/spec/models/concerns/requests/scsb_spec.rb +++ b/spec/models/concerns/requests/scsb_spec.rb @@ -9,7 +9,7 @@ "patron_id" => "99999", "active_email" => "foo@princeton.edu" }.with_indifferent_access end let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:scsb_no_format) { fixture('/SCSB-7935196.json') } let(:location_code) { 'scsbnypl' } diff --git a/spec/models/requests/access_patron_spec.rb b/spec/models/requests/access_patron_spec.rb deleted file mode 100644 index 506d2996b..000000000 --- a/spec/models/requests/access_patron_spec.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true -require 'rails_helper' - -RSpec.describe Requests::AccessPatron, requests: true do - it 'can be instantiated' do - described_class.new(session: {}) - end - - describe '#hash' do - it 'has a hash' do - expect(described_class.new(session: {}).hash).to be_an_instance_of(HashWithIndifferentAccess) - end - end -end diff --git a/spec/models/requests/clancy_item_spec.rb b/spec/models/requests/clancy_item_spec.rb index f02339103..5c960b355 100644 --- a/spec/models/requests/clancy_item_spec.rb +++ b/spec/models/requests/clancy_item_spec.rb @@ -38,7 +38,7 @@ "university_id" => "9999999", "patron_group" => "staff", "patron_id" => "99999", "active_email" => "foo@princeton.edu" }.with_indifferent_access end let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end before do @@ -153,7 +153,7 @@ "university_id" => "9999999", "patron_group" => "staff", "patron_id" => "99999", "active_email" => "foo@princeton.edu" }.with_indifferent_access end let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end before do diff --git a/spec/models/requests/form_decorator_spec.rb b/spec/models/requests/form_decorator_spec.rb index 2c4c7891b..d9e57e016 100644 --- a/spec/models/requests/form_decorator_spec.rb +++ b/spec/models/requests/form_decorator_spec.rb @@ -13,7 +13,7 @@ ldap: }.with_indifferent_access end let(:campus_unauthorized_patron) { File.open('spec/fixtures/bibdata_patron_unauth_response.json') } - let(:patron) { Requests::Patron.new(user:, session: {}, patron_hash: test_patron) } + let(:patron) { Requests::Patron.new(user:, patron_hash: test_patron) } let(:requestable) { instance_double(Requests::RequestableDecorator, stubbed_questions) } let(:hidden_field_metadata) do @@ -67,7 +67,7 @@ context 'user not yet logged in' do let(:user) { FactoryBot.build(:user, guest: true, uid: nil) } - let(:patron) { Requests::Patron.new(user:, session: {}, patron_hash: HashWithIndifferentAccess.new) } + let(:patron) { Requests::Patron.new(user:, patron_hash: HashWithIndifferentAccess.new) } it 'does not show a message' do expect(decorator.patron_message).to eq "" end diff --git a/spec/models/requests/form_no_vcr_spec.rb b/spec/models/requests/form_no_vcr_spec.rb index 78963232b..f4acc3507 100644 --- a/spec/models/requests/form_no_vcr_spec.rb +++ b/spec/models/requests/form_no_vcr_spec.rb @@ -9,7 +9,7 @@ "patron_id" => "99999", "active_email" => "foo@princeton.edu" }.with_indifferent_access end let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end context "with an object with a LOT of items" do let(:document_id) { '9933643713506421' } diff --git a/spec/models/requests/form_spec.rb b/spec/models/requests/form_spec.rb index 7ff052ec7..e7f34f110 100644 --- a/spec/models/requests/form_spec.rb +++ b/spec/models/requests/form_spec.rb @@ -9,7 +9,7 @@ "patron_id" => "99999", "active_email" => "foo@princeton.edu" }.with_indifferent_access end let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end context "with a bad system_id" do diff --git a/spec/models/requests/illiad_patron_spec.rb b/spec/models/requests/illiad_patron_spec.rb index 9e13a5ad5..f5312339a 100644 --- a/spec/models/requests/illiad_patron_spec.rb +++ b/spec/models/requests/illiad_patron_spec.rb @@ -11,7 +11,7 @@ let(:ldap_data) { { uid: 'foo', department: 'Library - Information Technology', address: 'Firestone Library$Library Information Technology', telephone: '123-456-7890', surname: 'Doe', givenname: 'Joe', email: 'joe@abc.com', pustatus: 'fac', status: 'faculty' }.with_indifferent_access } let(:user_info) do user = instance_double(User, guest?: false, uid: 'foo') - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:illiad_patron) { described_class.new(user_info) } diff --git a/spec/models/requests/illiad_transaction_client_spec.rb b/spec/models/requests/illiad_transaction_client_spec.rb index 7e98edd28..b30008348 100644 --- a/spec/models/requests/illiad_transaction_client_spec.rb +++ b/spec/models/requests/illiad_transaction_client_spec.rb @@ -6,7 +6,7 @@ let(:valid_patron) { { "netid" => "abc234", ldap: { status: "faculty", pustatus: "fac" } }.with_indifferent_access } let(:user_info) do user = instance_double(User, guest?: false, uid: 'foo') - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:requestable) do [{ "selected" => "true", "bibid" => "10921934", "mfhd" => "22241110470006421", "call_number" => "HF1131 .B485", diff --git a/spec/models/requests/patron_spec.rb b/spec/models/requests/patron_spec.rb index e010561e7..52d4b7056 100644 --- a/spec/models/requests/patron_spec.rb +++ b/spec/models/requests/patron_spec.rb @@ -4,11 +4,7 @@ # rubocop:disable RSpec/MultipleExpectations describe Requests::Patron, requests: true do subject(:patron) do - described_class.new(user:, session:, patron_hash: patron_values) - end - - let(:session) do - {} + described_class.new(user:, patron_hash: patron_values) end let(:first_name) { "Test" } let(:patron_values) do @@ -39,24 +35,12 @@ .to_return(status: 200, body: patron_with_multiple_barcodes, headers: { "Content-Type" => "application/json" }) end - it 'creates an access patron with the active barcode' do - patron = described_class.new(user:, session:) + it 'creates a patron with the active barcode' do + patron = described_class.new(user:) expect(patron.barcode).to eq('77777777') end end - context 'When an access patron visits the site' do - describe '#access_patron' do - it 'creates an access patron with required access attributes' do - patron = described_class.new(user: instance_double(User, guest?: true), - session: { email: 'foo@bar.com', user_name: 'foobar' }.with_indifferent_access) - expect(patron).to be_truthy - expect(patron.active_email).to eq('foo@bar.com') - expect(patron.last_name).to eq('foobar') - expect(patron.barcode).to eq('ACCESS') - expect(patron.eligible_for_library_services?).to be_truthy - end - end - end + context 'A user with a valid princeton net id patron record' do describe '#patron' do before do @@ -64,8 +48,7 @@ .to_return(status: 200, body: valid_patron_response, headers: {}) end it 'Handles an authorized princeton net ID holder' do - patron = described_class.new(user:, - session: { email: 'foo@bar.com', user_name: 'foobar' }.with_indifferent_access) + patron = described_class.new(user:) expect(patron).to be_truthy expect(patron.active_email).to eq('a@b.com') expect(patron.netid).to eq('jstudent') @@ -84,7 +67,7 @@ .to_return(status: 200, body: valid_barcode_patron_response, headers: {}) end it 'Handles an authorized princeton net ID holder' do - patron = described_class.new(user:, session: {}) + patron = described_class.new(user:) expect(patron).to be_truthy expect(patron.active_email).to eq('a@b.com') expect(patron.netid).to be_nil @@ -99,7 +82,7 @@ .to_return(status: 404, body: invalid_patron_response, headers: {}) end it 'Handles an authorized princeton net ID holder' do - patron = described_class.new(user:, session: {}) + patron = described_class.new(user:) expect(patron.errors).to eq(["A problem occurred looking up your library account."]) end end @@ -111,7 +94,7 @@ .to_return(status: 403, body: invalid_patron_response, headers: {}) end it 'Handles an authorized princeton net ID holder' do - patron = described_class.new(user:, session: {}) + patron = described_class.new(user:) expect(patron.errors).to eq(["A problem occurred looking up your library account."]) end end @@ -123,7 +106,7 @@ .to_return(status: 500, body: invalid_patron_response, headers: {}) end it 'cannot return a patron record' do - patron = described_class.new(user:, session: {}) + patron = described_class.new(user:) expect(patron.errors).to eq(["A problem occurred looking up your library account."]) end end @@ -154,7 +137,7 @@ it 'logs the error' do allow(Rails.logger).to receive(:error) - described_class.new(user:, session: {}) + described_class.new(user:) expect(Rails.logger).to have_received(:error).with("Unable to connect to #{bibdata_uri}/patron/foo?ldap=true") end end @@ -172,7 +155,7 @@ end it 'logs the error' do allow(Rails.logger).to receive(:error) - described_class.new(user:, session: {}) + described_class.new(user:) expect(Rails.logger).to have_received(:error).with("#{patron_url} returned an invalid patron response: Request Rejected") end end @@ -189,13 +172,13 @@ end it 'logs the error' do allow(Rails.logger).to receive(:error) - described_class.new(user:, session: {}) + described_class.new(user:) expect(Rails.logger).to have_received(:error).with("#{bibdata_uri}/patron/foo?ldap=true returned an empty patron response") end end context 'Passing in patron information instead of loading it from bibdata' do it "does not call to bibdata" do - patron = described_class.new(user: instance_double(User, guest?: false, uid: 'foo'), session: {}, patron_hash: { barcode: "1234567890" }) + patron = described_class.new(user: instance_double(User, guest?: false, uid: 'foo'), patron_hash: { barcode: "1234567890" }) expect(patron.barcode).to eq('1234567890') end end diff --git a/spec/models/requests/requestable_decorator_spec.rb b/spec/models/requests/requestable_decorator_spec.rb index 48f5fec9f..bd72318f8 100644 --- a/spec/models/requests/requestable_decorator_spec.rb +++ b/spec/models/requests/requestable_decorator_spec.rb @@ -13,7 +13,7 @@ "patron_id" => "99999", "active_email" => "foo@princeton.edu", ldap: }.with_indifferent_access end - let(:patron) { Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) } + let(:patron) { Requests::Patron.new(user:, patron_hash: valid_patron) } let(:requestable) { instance_double(Requests::Requestable, stubbed_questions) } let(:default_stubbed_questions) { { patron:, item_data?: true, circulates?: true, on_shelf?: false, recap?: false, annex?: false, holding_library_in_library_only?: false, scsb_in_library_use?: false, on_order?: false, in_process?: false, aeon?: false, ill_eligible?: false, clancy_available?: false, held_at_marquand_library?: false, item_at_clancy?: false, cul_avery?: false, eligible_for_library_services?: true } } diff --git a/spec/models/requests/requestable_spec.rb b/spec/models/requests/requestable_spec.rb index ec647d3ed..8d370581e 100644 --- a/spec/models/requests/requestable_spec.rb +++ b/spec/models/requests/requestable_spec.rb @@ -661,7 +661,7 @@ "patron_id" => "99999", "active_email" => "foo@princeton.edu" }.with_indifferent_access end let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:params) do { @@ -727,7 +727,7 @@ "patron_id" => "99999", "active_email" => "foo@princeton.edu" }.with_indifferent_access end let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:params) do { @@ -798,7 +798,7 @@ let(:valid_patron_response) { '{"netid":"foo","first_name":"Foo","last_name":"Request","barcode":"22101007797777","university_id":"9999999","patron_group":"staff","patron_id":"99999","active_email":"foo@princeton.edu"}' } let(:patron) do stub_request(:get, "#{Requests.config[:bibdata_base]}/patron/foo?ldap=true").to_return(status: 200, body: valid_patron_response, headers: {}) - Requests::Patron.new(user:, session: {}) + Requests::Patron.new(user:) end let(:params) do { diff --git a/spec/models/requests/router_spec.rb b/spec/models/requests/router_spec.rb index 26cfc9404..fc6a62f93 100644 --- a/spec/models/requests/router_spec.rb +++ b/spec/models/requests/router_spec.rb @@ -6,7 +6,7 @@ let(:user) { FactoryBot.create(:user) } let(:valid_patron) { { "netid" => "foo" }.with_indifferent_access } let(:patron) do - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:scsb_single_holding_item) { fixture('/SCSB-2635660.json') } diff --git a/spec/models/requests/submission_spec.rb b/spec/models/requests/submission_spec.rb index a2ae8cf11..25652d2af 100644 --- a/spec/models/requests/submission_spec.rb +++ b/spec/models/requests/submission_spec.rb @@ -10,7 +10,7 @@ end let(:user_info) do user = instance_double(User, guest?: false, uid: 'foo') - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end context 'A valid submission' do diff --git a/spec/models/requests/submissions/hold_item_spec.rb b/spec/models/requests/submissions/hold_item_spec.rb index 0efe66849..711f36104 100644 --- a/spec/models/requests/submissions/hold_item_spec.rb +++ b/spec/models/requests/submissions/hold_item_spec.rb @@ -6,7 +6,7 @@ let(:valid_patron) { { "netid" => "foo", university_id: "99999999" }.with_indifferent_access } let(:user_info) do user = instance_double(User, guest?: false, uid: 'foo') - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:requestable) do diff --git a/spec/models/requests/submissions/recap_spec.rb b/spec/models/requests/submissions/recap_spec.rb index 6d6ffa149..e41167e2e 100644 --- a/spec/models/requests/submissions/recap_spec.rb +++ b/spec/models/requests/submissions/recap_spec.rb @@ -7,7 +7,7 @@ let(:valid_patron) { { "netid" => "foo", "university_id" => "99999999", "active_email" => 'foo1@princeton.edu', barcode: '111222333' }.with_indifferent_access } let(:user_info) do user = instance_double(User, guest?: false, uid: 'foo') - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:scsb_url) { "#{Requests.config[:scsb_base]}/requestItem/requestItem" } let(:alma_url) { "#{Alma.configuration.region}/almaws/v1/bibs/#{bib['id']}/holdings/#{requestable[0]['mfhd']}/items/#{requestable[0]['item_id']}/requests?user_id=99999999" } @@ -198,7 +198,7 @@ let(:valid_patron) { { "netid" => "foo", "university_id" => "99999999" }.with_indifferent_access } let(:user_info) do user = instance_double(User, guest?: false, uid: 'foo') - Requests::Patron.new(user:, session: {}, patron_hash: valid_patron) + Requests::Patron.new(user:, patron_hash: valid_patron) end let(:scsb_url) { "#{Requests.config[:scsb_base]}/requestItem/requestItem" } let(:alma_url) { "#{Alma.configuration.region}/almaws/v1/bibs/#{bib['id']}/holdings/#{requestable[0]['mfhd']}/items/#{requestable[0]['item_id']}/requests?user_id=99999999" }