From e129ae4ca49f9227b0ecedfc29a23e57bc2c0f0b Mon Sep 17 00:00:00 2001 From: "Michael J. Giarlo" Date: Thu, 25 Jan 2024 16:43:17 -0800 Subject: [PATCH] Rename "Library ID" to "University ID" Connects to sul-dlss/SearchWorks#3773 This commit helps us move the access portfolio beyond using library barcode numbers, leaning instead on the university-assigned and -managed "university ID." --- app/controllers/reset_pins_controller.rb | 12 +++++---- app/controllers/sessions_controller.rb | 12 ++++----- app/services/folio_client.rb | 27 ++++++++++--------- app/views/reset_pins/change_form.html.erb | 4 +-- app/views/reset_pins/index.html.erb | 6 ++--- app/views/sessions/form.html.erb | 8 +++--- config/initializers/warden.rb | 19 ++++++++++--- config/locales/en.yml | 7 +++-- config/routes.rb | 2 +- config/settings.yml | 2 +- .../controllers/reset_pins_controller_spec.rb | 10 ++++--- spec/controllers/sessions_controller_spec.rb | 15 ++++++----- spec/features/reset_pin_spec.rb | 4 +-- 13 files changed, 75 insertions(+), 53 deletions(-) diff --git a/app/controllers/reset_pins_controller.rb b/app/controllers/reset_pins_controller.rb index b75b652a..35ef6d87 100644 --- a/app/controllers/reset_pins_controller.rb +++ b/app/controllers/reset_pins_controller.rb @@ -16,17 +16,19 @@ def index; end # a token for completing the reset # # Ignore errors indicating the patron wasn't found in the ILS; we don't want - # to leak information about presence/validity of library IDs + # to leak information about presence/validity of university IDs # # POST /reset_pin def reset suppress ActiveRecord::RecordNotFound do - patron = FolioClient.new.find_patron_by_barcode(library_id_param, patron_info: false) + patron = FolioClient.new.find_patron_by_university_id(university_id_param, patron_info: false) ResetPinsMailer.with(patron:).reset_pin.deliver_now end - flash[:success] = t 'mylibrary.reset_pin.success_html', library_id: params['library_id'] + flash[:success] = t('mylibrary.reset_pin.success_html', + university_id: params['university_id'], + university_id_label: t('mylibrary.university_id.label')) redirect_to login_path end @@ -48,8 +50,8 @@ def change private - def library_id_param - params.require(:library_id) + def university_id_param + params.require(:university_id) end def change_pin_with_token_params diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index fa2cf4e9..dd7efc41 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -13,21 +13,21 @@ def index redirect_to summaries_url if current_user? end - # Render a login form for Barcode + PIN users (Stanford single-sign-on are + # Render a login form for University ID + PIN users (Stanford single-sign-on are # authenticated using a different route) # # GET /login def form; end - # Handle login for Barcode + PIN users by authenticating them with the + # Handle login for University ID + PIN users by authenticating them with the # ILS using the Warden configuration. # - # GET /sessions/login_by_library_id - def login_by_library_id - if request.env['warden'].authenticate(:library_id) + # GET /sessions/login_by_university_id + def login_by_university_id + if request.env['warden'].authenticate(:university_id) redirect_to summaries_url else - redirect_to login_url, alert: t('mylibrary.sessions.login_by_library_id.alert') + redirect_to login_url, alert: t('mylibrary.sessions.login_by_university_id.alert') end end diff --git a/app/services/folio_client.rb b/app/services/folio_client.rb index 16f4b00b..0980a29a 100644 --- a/app/services/folio_client.rb +++ b/app/services/folio_client.rb @@ -39,8 +39,8 @@ def inspect "#<#{self.class.name}:#{object_id} @base_url=\"#{base_url}\">" end - def login(library_id, pin) - user_response = get_json('/users', params: { query: CqlQuery.new(barcode: library_id).to_query }) + def login(university_id, pin) + user_response = get_json('/users', params: { query: CqlQuery.new(externalSystemId: university_id).to_query }) user = user_response.dig('users', 0) return unless user && validate_patron_pin(user['id'], pin) @@ -57,6 +57,19 @@ def user_info(user_id) get_json("/users/#{CGI.escape(user_id)}") end + # Look up a patron by university_id and return a Patron object + # If 'patron_info' is false, don't run the full patron info GraphQL query + def find_patron_by_university_id(university_id, patron_info: true) + response = get_json('/users', params: { query: CqlQuery.new(externalSystemId: university_id).to_query }) + + if (user = response.dig('users', 0)) + return patron_info ? Folio::Patron.find(user['id']) : Folio::Patron.new({ 'user' => user }) + end + + raise ActiveRecord::RecordNotFound, + "User with #{I18n.t('mylibrary.university_id.label')} #{university_id} not found" + end + # rubocop:disable Lint/UnusedMethodArgument # FOLIO graphql call, compare to #patron_account def patron_info(patron_key, item_details: {}) @@ -176,16 +189,6 @@ def change_pin(token, new_pin) check_response(response, title: 'Assign pin', context: { user_id: patron_key }) end - # Look up a patron by barcode and return a Patron object - # If 'patron_info' is false, don't run the full patron info GraphQL query - def find_patron_by_barcode(barcode, patron_info: true) - response = get_json('/users', params: { query: CqlQuery.new(barcode:).to_query }) - user = response.dig('users', 0) - raise ActiveRecord::RecordNotFound, "User with barcode #{barcode} not found" unless user - - patron_info ? Folio::Patron.find(user['id']) : Folio::Patron.new({ 'user' => user }) - end - # Mark all of a user's fines (accounts) as having been paid # The payment will show as being made from the 'Online' service point # rubocop:disable Metrics/MethodLength diff --git a/app/views/reset_pins/change_form.html.erb b/app/views/reset_pins/change_form.html.erb index bbb51830..742dcc2e 100644 --- a/app/views/reset_pins/change_form.html.erb +++ b/app/views/reset_pins/change_form.html.erb @@ -12,7 +12,7 @@
<%= label_tag :pin, 'New PIN' %>
- <%= password_field_tag :pin, nil, class: 'form-control', 'aria-describedby': 'libraryIdHelp', autofocus: true, autocomplete: 'off' %> + <%= password_field_tag :pin, nil, class: 'form-control', 'aria-describedby': 'new-pin-help', autofocus: true, autocomplete: 'off' %>
- Your new PIN + Your new PIN
<%= submit_tag 'Change PIN', class: 'btn btn-primary' %> <%= link_to 'Cancel', root_url, class: 'btn btn-link' %> diff --git a/app/views/reset_pins/index.html.erb b/app/views/reset_pins/index.html.erb index 535c615b..341aae57 100644 --- a/app/views/reset_pins/index.html.erb +++ b/app/views/reset_pins/index.html.erb @@ -1,9 +1,9 @@

Reset/Request PIN

<%= form_tag reset_pin_url, method: :post do %>
- <%= label_tag :library_id, 'Library ID' %> - <%= text_field_tag :library_id, nil, class: 'form-control', 'aria-describedby': 'libraryIdHelp', autofocus: true %> - Last 10 digits above the barcode on your library card + <%= label_tag :university_id, t('mylibrary.university_id.label') %> + <%= text_field_tag :university_id, nil, class: 'form-control', 'aria-describedby': 'university-id-help', autofocus: true %> + <%= t('mylibrary.university_id.help_text') %>
<%= submit_tag 'Reset/Request PIN', class: 'btn btn-primary' %> diff --git a/app/views/sessions/form.html.erb b/app/views/sessions/form.html.erb index 0c8b6142..fe5232d9 100644 --- a/app/views/sessions/form.html.erb +++ b/app/views/sessions/form.html.erb @@ -1,10 +1,10 @@

<%= proxy_login_header %>

-<%= form_tag login_by_library_id_url, method: :post do %> +<%= form_tag login_by_university_id_url, method: :post do %>
- <%= label_tag :library_id, 'Library ID' %> - <%= text_field_tag :library_id, nil, class: 'form-control col-8 col-sm-4', 'aria-describedby': 'libraryIdHelp', autofocus: true %> - Last 10 digits above the barcode on your library card + <%= label_tag :university_id, t('mylibrary.university_id.label') %> + <%= text_field_tag :university_id, nil, class: 'form-control col-8 col-sm-4', 'aria-describedby': 'university-id-help', autofocus: true %> + <%= t('mylibrary.university_id.help_text') %>
diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index 37c7f42e..dd23aee9 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -46,16 +46,27 @@ def uid end end -Warden::Strategies.add(:library_id) do +Warden::Strategies.add(:university_id) do + # Reviewing numbers input in the FOLIO "External System ID", it appears that + # we have user records with 8, 9 and 10 digits. + # + # * Core community users (who presumably would be accessing our applications + # using auth and NOT inputting an ID) have 8 digits in the FOLIO External + # System ID field + # * Courtesy card holders (the primary user group that we're trying to help + # gain access in sul-requests & My Account) have either 9 or 10 digits in + # FOLIO External System ID field. def valid? - params['library_id'].present? && params['pin'].present? + params['university_id'].present? && + params['university_id'].match?(/\d{8,10}/) && + params['pin'].present? end def authenticate! - response = ApplicationController.ils_client_class.new.login(params['library_id'], params['pin']) + response = ApplicationController.ils_client_class.new.login(params['university_id'], params['pin']) if response&.key?('patronKey') || response&.key?('id') - u = { username: params['library_id'], patron_key: response['patronKey'] || response['id'] } + u = { username: params['university_id'], patron_key: response['patronKey'] || response['id'] } success!(u) else fail!('Could not log in') diff --git a/config/locales/en.yml b/config/locales/en.yml index 149aba2a..0d35f9b3 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -38,6 +38,9 @@ en: time_tomorrow: 'Tomorrow at %-l:%M%P' hello: "Hello world" mylibrary: + university_id: + label: 'University ID' + help_text: 'This is an 8- to 10-digit number that serves as a replacement for the Library ID that was previously utilized.' renew_item: success_html: Success! "%{title}" was renewed. error_html: Sorry! Something went wrong and "%{title}" was not renewed. @@ -64,7 +67,7 @@ en: success_html: Success! Your new PIN is ready to use. invalid_token_html: Sorry! That reset PIN link is invalid or expired. Enter your library ID to request a new link. reset_pin: - success_html: Check your email! A PIN reset link has been sent to the address associated with library ID %{library_id} + success_html: Check your email! A PIN reset link has been sent to the address associated with %{university_id_label} %{university_id} request_failed_html: Sorry! Something went wrong, possibly due to a connection failure. Try again or contact us for help. reset_pins_mailer: reset_pin: @@ -83,7 +86,7 @@ en: request_failed_html: Sorry! Something went wrong, possibly due to a connection failure. Please contact us for help resolving your fines. payment_failed_html: Sorry! Payment failed. Please contact us for help resolving your fines. sessions: - login_by_library_id: + login_by_university_id: alert: Unable to authenticate. login_by_sunetid: error_html:

Unable to log in. Your SUNet ID is not linked to a library account.

diff --git a/config/routes.rb b/config/routes.rb index a1e56675..301d69ba 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -20,7 +20,7 @@ get 'feedback' => 'feedback_forms#new' get '/sessions/login_by_sunetid', to: 'sessions#login_by_sunetid', as: :login_by_sunetid - post '/sessions/login_by_library_id', to: 'sessions#login_by_library_id', as: :login_by_library_id + post '/sessions/login_by_university_id', to: 'sessions#login_by_university_id', as: :login_by_university_id get '/login', to: 'sessions#form', as: :login get '/logout', to: 'sessions#destroy', as: :logout diff --git a/config/settings.yml b/config/settings.yml index 2859a9cd..055666c8 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -36,7 +36,7 @@ GLOBAL_ALERT: <%= true %> borrow_direct_reshare: enabled: false -# Adding Illiad +# Adding Illiad sul_illiad: https://sul-illiad-test.stanford.edu/ illiad_api_key: 'AbC123' diff --git a/spec/controllers/reset_pins_controller_spec.rb b/spec/controllers/reset_pins_controller_spec.rb index 07adc907..ff7f9de6 100644 --- a/spec/controllers/reset_pins_controller_spec.rb +++ b/spec/controllers/reset_pins_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe ResetPinsController do - let(:mock_client) { instance_double(FolioClient, find_patron_by_barcode: patron, ping: true) } + let(:mock_client) { instance_double(FolioClient, find_patron_by_university_id: patron, ping: true) } let(:patron) do instance_double(Folio::Patron, display_name: 'Patron', barcode: 'PATRON', email: 'patron@example.com', pin_reset_token: 'abcdef') @@ -35,13 +35,15 @@ context 'with unauthenticated requests' do describe '#reset' do it 'sends the reset pin email' do - expect { post :reset, params: { library_id: '123456' } }.to change { ActionMailer::Base.deliveries.count }.by(1) + expect { post :reset, params: { university_id: '123456789' } } + .to change { ActionMailer::Base.deliveries.count } + .by(1) end it 'sets flash messages' do - post :reset, params: { library_id: '123456' } + post :reset, params: { university_id: '123456789' } - expect(flash[:success]).to match(/associated with library ID 123456/) + expect(flash[:success]).to match(/associated with University ID 123456789/) end end end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index edf35de9..fbd14fed 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -83,30 +83,31 @@ end end - describe 'POST login_by_library_id' do + describe 'POST login_by_university_id' do context 'with a valid login' do before do - allow(mock_client).to receive(:login).with('abc', '123').and_return('patronKey' => 1) + allow(mock_client).to receive(:login).with('01234567', '123').and_return('patronKey' => 1) end it 'logs in the user' do - post(:login_by_library_id, params: { library_id: 'abc', pin: '123' }) + post(:login_by_university_id, params: { university_id: '01234567', pin: '123' }) - expect(warden.user).to include username: 'abc', patron_key: 1 + expect(warden.user).to include username: '01234567', patron_key: 1 end it 'redirects the user to the summary page' do - expect(post(:login_by_library_id, params: { library_id: 'abc', pin: '123' })).to redirect_to summaries_url + expect(post(:login_by_university_id, params: { university_id: '01234567', pin: '123' })) + .to redirect_to summaries_url end end context 'with an invalid login' do it 'redirects failed requests back to the login page' do - expect(post(:login_by_library_id)).to redirect_to login_url + expect(post(:login_by_university_id)).to redirect_to login_url end it 'sets an alert' do - get(:login_by_library_id) + get(:login_by_university_id) expect(flash[:alert]).to include('Unable to authenticate.') end end diff --git a/spec/features/reset_pin_spec.rb b/spec/features/reset_pin_spec.rb index 475d6cc2..00fa7384 100644 --- a/spec/features/reset_pin_spec.rb +++ b/spec/features/reset_pin_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe 'Reset Pin' do - let(:mock_client) { instance_double(FolioClient, find_patron_by_barcode: patron, ping: true, change_pin: nil) } + let(:mock_client) { instance_double(FolioClient, find_patron_by_university_id: patron, ping: true, change_pin: nil) } let(:patron) do instance_double(Folio::Patron, email: 'jdoe@stanford.edu', display_name: 'J Doe', barcode: '123', pin_reset_token: 'foo') @@ -26,7 +26,7 @@ it 'allows user to reset pin' do visit reset_pin_path - fill_in('library_id', with: '123456') + fill_in('university_id', with: '123456') click_on 'Reset/Request PIN' expect(page).to have_css '.flash_messages', text: 'Check your email!' end