Merge changes for v2.2.1 to master (#96)

* Merge changes for v2.1.6 (#92) * DISCOVERYACCESS-7627 - for rbenv users * DISCOVERYACCESS-7627 - rails 6 required for cornell-blacklight * DISCOVERYACCESS-7627 - bump version number * DISCOVERYACCESS-7627 - rails 6 for cornell-blacklight * DISCOVERYACCESS-7627 - try rails 7 * DISCOVERYACCESS-7627 - back to rails 6.1 * DISCOVERYACCESS-7627 - bump version number * DISCOVERYACCESS-7627 - forgot save all * Update release notes Co-authored-by: James Reidy <[email protected]> * DISCOVERYACCESS-7838: update my account login page text * Merge changes for v2.2.0 (ReShare) to dev (#94) * WIP * Eliminate ilsapi cgi script reference (talk directly to illiad6.cgi) * Display pickup location for BD * Differentiate pending and available ReShare requests * Bump to v2.2.0 * Ensure ReShare results are only for the user's netid * Remove unused functions * Linting * Skip ILL items with status 'Request Sent to BD' * Avoid duplicate request entries Co-authored-by: James Reidy <[email protected]> Co-authored-by: Melissa Wallace <[email protected]>
cul-it · Jan 17, 2023 · e5746ea · e5746ea
1 parent 30bedf6
commit e5746ea
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 89 deletions.
diff --git a/app/assets/javascripts/my_account/account.js.coffee b/app/assets/javascripts/my_account/account.js.coffee
@@ -245,7 +245,10 @@ account =
     pending = pending.filter (r) -> r.TransactionStatus != 'Checked out in FOLIO'
 
     # Sort out the FOLIO request data into the format and category expected by the views
+    folioTitles = []
     folioData.forEach (entry) ->
+      console.log entry
+      folioTitles.push entry.item.title
       # Look up the service point based on its ID. getServicePoint() will try to dynamically update the status
       # in the table when the result is ready
       account.getServicePoint(entry.pickupLocationId, entry.requestId)
@@ -269,21 +272,26 @@ account =
     availableStatuses = ['REQ_CHECKED_IN']
 
     bdData.forEach (entry) ->
-      # Olin Library's pickup location is actually '*Olin Library', so strip off the *
-      location = entry.lo.replace '*', ''
-      requestObj = {
-        iid: entry.iid, # N.B. The ID used here for FOLIO requests is the REQUEST ID, not the item ID!
-        tl: entry.tl,
-        system: 'bd',
-        lo: location
-      }
-
-      # This is a bit of a hack. An ON_LOAN item is really an available request, but at that point in
-      # the process it shows up as a FOLIO loan item; if we include this one in available_requests,
-      # we'll get a duplicate entry. So we'll ignore items with status ON_LOAN here.
-      if entry.status != 'ON_LOAN'
-        pending.push requestObj if pendingStatuses.includes(entry.status)
-        available.push requestObj if availableStatuses.includes(entry.status)
+      # Check to see if the entry title matches one of the FOLIO entry titles. This indicates a BD
+      # item that has already been checked in at CUL and is being shown as a FOLIO request there,
+      # so we don't want to duplicate the entry. Title matching is iffy, but maybe it's OK if the
+      # FOLIO record is generated automatically?
+      if !folioTitles.includes entry.tl
+        # Olin Library's pickup location is actually '*Olin Library', so strip off the *
+        location = entry.lo.replace '*', ''
+        requestObj = {
+          iid: entry.iid, # N.B. The ID used here for FOLIO requests is the REQUEST ID, not the item ID!
+          tl: entry.tl,
+          system: 'bd',
+          lo: location
+        }
+
+        # This is a bit of a hack. An ON_LOAN item is really an available request, but at that point in
+        # the process it shows up as a FOLIO loan item; if we include this one in available_requests,
+        # we'll get a duplicate entry. So we'll ignore items with status ON_LOAN here.
+        if entry.status != 'ON_LOAN'
+          pending.push requestObj if pendingStatuses.includes(entry.status)
+          available.push requestObj if availableStatuses.includes(entry.status)
 
     # Available requests tab
     $.ajax({

diff --git a/app/controllers/my_account/account_controller.rb b/app/controllers/my_account/account_controller.rb
@@ -6,26 +6,24 @@
 require 'cul/folio/edge'
 
 module MyAccount
-
   class AccountController < ApplicationController
     #include Reshare
 
     before_action :heading
     before_action :authenticate_user, except: [:intro]
 
     def heading
-      @heading='My Account'
+      @heading = 'My Account'
     end
 
     def authenticate_user
       # Master disable -- this kicks the user out of My Account before anything gets going
       if ENV['DISABLE_MY_ACCOUNT']
         msg = 'My Account is currently unavailable. We apologize for the inconvenience. For more information, check the <a href="https://library.cornell.edu">CUL home page</a> for updates or <a href="https://library.cornell.edu/ask">ask a librarian</a>.'
-        redirect_to "/catalog#index", :notice => msg.html_safe
+        redirect_to '/catalog#index', notice: msg.html_safe
         return
       end
 
-      #Rails.logger.debug "mjc12test: authenticating"
       if user.present?
         index
       else
@@ -36,10 +34,10 @@ def authenticate_user
           # Omniauth gem requirements
           uri = URI(request.original_url)
           scheme_host = "#{uri.scheme}://#{uri.host}"
-          if uri.port.present? && uri.port !=  uri.default_port()
+          if uri.port.present? && uri.port != uri.default_port
             scheme_host = scheme_host + ':' + uri.port.to_s
           end
-          redirect_post("#{scheme_host}/users/auth/saml", options: {authenticity_token: :auto})
+          redirect_post("#{scheme_host}/users/auth/saml", options: { authenticity_token: :auto })
         end
       end
     end
@@ -50,14 +48,14 @@ def intro
     end
 
     def index
-      if request.headers["REQUEST_METHOD"] == "HEAD"
+      if request.headers['REQUEST_METHOD'] == 'HEAD'
         head :no_content
         return
       end
       # Master disable -- this kicks the user out of My Account before anything gets going
       if ENV['DISABLE_MY_ACCOUNT']
         msg = 'My Account is currently unavailable. We apologize for the inconvenience. For more information, check the <a href="https://library.cornell.edu">CUL home page</a> for updates or <a href="https://library.cornell.edu/ask">ask a librarian</a>.'
-        redirect_to "/catalog#index", :notice => msg.html_safe
+        redirect_to '/catalog#index', notice: msg.html_safe
       end
 
       @netid = user
@@ -81,12 +79,11 @@ def index
     # was prevoiusly created, or directly from FOLIO otherwise.
     def folio_token
       if session[:folio_token].nil?
-        Rails.logger.debug "mjc12test6: creating new token"
         url = ENV['OKAPI_URL']
         tenant = ENV['OKAPI_TENANT']
         response = CUL::FOLIO::Edge.authenticate(url, tenant, ENV['OKAPI_USER'], ENV['OKAPI_PW'])
         if response[:code] >= 300
-          Rails.logger.error "MyAccount error: Could not create a FOLIO token for #{netid}"
+          Rails.logger.error "MyAccount error: Could not create a FOLIO token for #{user}"
         else
           session[:folio_token] = response[:token]
         end
@@ -134,15 +131,15 @@ def ajax_renew
     #     flash[:notice] = 'Your requests have been cancelled.'
     #   end
 
-    # end 
-    
+    # end
+
     # Use the CUL::FOLIO::Edge gem to cancel a request. Operation is triggered via AJAX.
     def ajax_cancel
       url = ENV['OKAPI_URL']
       tenant = ENV['OKAPI_TENANT']
       # NOTE: The cancel_reason value set below is the UUID of the current 'Patron Cancelled'
       # request cancellation reason in FOLIO. Hard-coding it here is risky if that value ever changes,
-      # but the alternative would be to do a secondary call to 
+      # but the alternative would be to do a secondary call to
       # /cancellation-request-storage/cancellation-reasons and then parse out the correct reason
       # by text matching -- also a risky proposition.
       cancel_reason = 'ba60fd97-adcf-406e-97aa-6bf5e2a6243d'
@@ -162,13 +159,12 @@ def ajax_cancel
     # parsing below greatly. No need to try to figure out whether something is a charged item or a request
     def get_illiad_data
       record = nil
-      msg = ""
 
       netid = params['netid']
       # folio_account_data = get_folio_accountinfo netid
       # Rails.logger.debug "mjc12test: Start parsing"
 
-      begin 
+      begin
         # NOTE: the key MY_ACCOUNT_ISLAPI_URL is a misomer now, because we've eliminated the ilsapi CGI
         # script and are using the illiad6.cgi script, which ilsapi called, without the middleman. Probably
         # the key should be renamed at some point.
@@ -192,6 +188,10 @@ def get_illiad_data
         # need to do anything with this
         # TODO 2: Is this still relevant with FOLIO? Can ILL items have this status?
         next if i['status'] == 'finef'
+        Rails.logger.debug "mjc12a: item: #{i}"
+        # Skip if this is a ReShare submission that has been rerouted to BD -- otherwise it will
+        # show up twice in the user's requests (BD/ReShare and ILL)
+        next if i['TransactionStatus'] == 'Request Sent to BD'
 
         # This is a hold, recall, or ILL request. Rather than tracking the item ID, we need the request
         # id for potential cancellations.
@@ -200,18 +200,17 @@ def get_illiad_data
         i['iid'] = i['tid'] # not sure why 'tid' is used in the ILSAPI return - "transaction ID"?
         # add a special "item id" for ILLiad items
         if i['system'] == 'illiad'
-          i['iid'] = "illiad-#{i['TransactionNumber']}" 
-          i['requestDate'] = DateTime.parse(i['TransactionDate']).strftime("%-m/%-d/%y")
+          i['iid'] = "illiad-#{i['TransactionNumber']}"
+          i['requestDate'] = DateTime.parse(i['TransactionDate']).strftime('%-m/%-d/%y')
         end
-
-        if i['status'] == 'waiting'
+
+        case i['status']
+        when 'waiting'
           # If the due date is an empty string, this line in the haml file throws an exception:
           # c['DueDate']).to_date.strftime('%m/%d/%y'). (DISCOVERYACCESS-5822)
-          if i['DueDate'] == ''
-            i['DueDate'] = nil
-          end
+          i['DueDate'] = nil if i['DueDate'] == ''
           available_requests << i
-        elsif i['status'] == 'chrged'
+        when 'chrged'
           # TODO: Is this still relevant with FOLIO? Can an ILL item have this status?
           i['status'] = 'Charged'
           # checkouts << i
@@ -220,15 +219,11 @@ def get_illiad_data
         end
       end
 
-      #bd_items = get_bd_requests netid
-      bd_items = []
-      #Rails.logger.debug "mjc12test: got bd_items #{bd_items}"
-
       render json: { pending: pending_requests, available: available_requests }
     end
 
     # Use the FOLIO EdgePatron API to retrieve a user's user record from FOLIO (*not* the user's account,
-    # which here refers to his/her checkouts and fines/fees). 
+    # which here refers to his/her checkouts and fines/fees).
     def get_user_record
       netid = params['netid']
       url = ENV['OKAPI_URL']
@@ -244,16 +239,16 @@ def get_folio_data
       netid = params['netid']
       url = ENV['OKAPI_URL']
       tenant = ENV['OKAPI_TENANT']
-      account = CUL::FOLIO::Edge.patron_account(url, tenant, folio_token, {:username => netid})
-     # Rails.logger.debug("mjc12test: Got FOLIO account #{account.inspect}")
+      account = CUL::FOLIO::Edge.patron_account(url, tenant, folio_token, { username: netid })
+      # Rails.logger.debug("mjc12test: Got FOLIO account #{account.inspect}")
       render json: account
     end
 
     # Render the _checkouts partial in response to an AJAX call
     def ajax_checkouts
       @checkouts = params['checkouts']&.values.to_a
-      #@checkouts.sort_by! { |c| Date.parse(c['dueDate']) }
-      render json: { record: render_to_string('_checkouts', :layout => false), locals: { checkouts: @checkouts }}
+      # @checkouts.sort_by! { |c| Date.parse(c['dueDate']) }
+      render json: { record: render_to_string('_checkouts', layout: false), locals: { checkouts: @checkouts } }
     end
 
     # Retrieve a service point record from FOLIO
@@ -269,60 +264,38 @@ def ajax_service_point
     # as a parameter so that calling this repeatedly in a loop doesn't incur multiple authentication calls. The
     # source is needed later to derermine whether this is a BD or ILL or FOLIO item.
     def ajax_catalog_link_and_source
-      instanceId = params['instanceId']
+      instance_id = params['instanceId']
       url = ENV['OKAPI_URL']
       tenant = ENV['OKAPI_TENANT']
       # Get instance HRID (e.g., bibid) for the record
-      response = CUL::FOLIO::Edge.instance_record(url, tenant, folio_token, instanceId)
+      response = CUL::FOLIO::Edge.instance_record(url, tenant, folio_token, instance_id)
       link = nil
       source = nil
       if response[:code] < 300
         source = response[:instance]['source']
         # Ignore Borrow Direct records for the link -- they have an HRID that looks like a legit bibid, but
         # it's something else BD-related. We can't link to those.
-        if source != 'bd'
-          link = "https://newcatalog.library.cornell.edu/catalog/#{response[:instance]['hrid']}"
-        end
+        link = "https://newcatalog.library.cornell.edu/catalog/#{response[:instance]['hrid']}" if source != 'bd'
       end
       render json: { link: link, source: source }
     end
 
     # Render the _checkouts partial in response to an AJAX call
     def ajax_fines
       @fines = params['fines']&.values.to_a
-      render json: { record: render_to_string('_fines', :layout => false), locals: { fines: @fines }}
+      render json: { record: render_to_string('_fines', layout: false), locals: { fines: @fines } }
     end
 
     # Render the _available_requests partial in response to an AJAX call
     def ajax_illiad_available
       @available_requests = params['requests']&.values.to_a
-      render json: { record: render_to_string('_available_requests', :layout => false), locals: { available_requests: @available_requests }}
+      render json: { record: render_to_string('_available_requests', layout: false), locals: { available_requests: @available_requests } }
     end
 
     # Render the _pending_requests partial in response to an AJAX call
     def ajax_illiad_pending
       @pending_requests = params['requests']&.values.to_a
-      render json: { record: render_to_string('_pending_requests', :layout => false), locals: { pending_requests: @pending_requests }}
-    end
-
-    # TODO: Replace with FOLIO
-    def patron_id(netid)
-      response = RestClient.get "#{ENV['MY_ACCOUNT_PATRONINFO_URL']}/#{netid}"
-      record = JSON.parse(response.body)
-      record[netid]['patron_id']
-    rescue
-      Rails.logger.debug("tlw72 ****** could not retrieve patron id.")
-      return ""
-    end
-
-    # TODO: Replace with FOLIO
-    def patron_barcode(netid)
-      response = RestClient.get "#{ENV['MY_ACCOUNT_PATRONINFO_URL']}/#{netid}"
-      record = JSON.parse(response.body)
-      record[netid]['barcode']
-    rescue
-      Rails.logger.debug("tlw72 ****** could not retrieve patron barcode.")
-      return ""
+      render json: { record: render_to_string('_pending_requests', layout: false), locals: { pending_requests: @pending_requests } }
     end
 
     def get_bd_requests
@@ -331,10 +304,10 @@ def get_bd_requests
       # Using the BD API is an expensive operation, so use the Rails session to cache the
       # response the first time a user accesses her account
       Rails.logger.debug "mjc12a: Checking session  #{session['mjc12_bd_items']}"
-      #return session[netid + '_bd_items'] if session[netid + '_bd_items']
-      Rails.logger.debug "mjc12test: Can't use session value for BD items - doing full lookup #{}"
+      # return session[netid + '_bd_items'] if session[netid + '_bd_items']
+      Rails.logger.debug "mjc12test: Can't use session value for BD items - doing full lookup"
 
-      #barcode = patron_barcode(netid)
+      # barcode = patron_barcode(netid)
 
       # Set parameters for the Borrow Direct API
       # BorrowDirect::Defaults.library_symbol = 'CORNELL'
@@ -362,11 +335,12 @@ def get_bd_requests
         response = RestClient.get(url, headers)
         # TODO: check that response is in the proper form and there are no returned errors
         items = JSON.parse(response)
-        items.each do |i|
-          Rails.logger.debug "mjc12testa: got BD results #{i['title']} with state #{i['state']['code']}"
-        end
-     rescue RestClient::Exception => e
-       # items = BorrowDirect::RequestQuery.new(barcode).requests('open')
+        # The ReShare match query is too broad and will match any portion of the netid --
+        # e.g., the results for 'mjc12' will also include any that are found for 'mjc124'.
+        # So we need to ensure that the patronIdentifier of each result matches our netid.
+        items.select! { |i| i['patronIdentifier'] == netid }
+      rescue RestClient::Exception => e
+      # items = BorrowDirect::RequestQuery.new(barcode).requests('open')
       # rescue BorrowDirect::Error => e
       #   # The Borrow Direct gem doesn't differentiate among all of the BD API error types.
       #   # In this case, PUBQR004 is an exception raised when there are no results for the query
@@ -379,7 +353,7 @@ def get_bd_requests
       #     # TODO: Add better error handling. For now, BD is causing too many problems with flaky connections;
       #     # we have to do something other than raise the errors here.
       #     # raise unless e.message.include? 'PUBQR004'
-      #   end  
+      #   end
         items = []
         Rails.logger.error "MyAccount error: Couldn\'t retrieve patron requests from ReShare (#{e})."
       end
@@ -392,7 +366,7 @@ def get_bd_requests
         # HACK: This is a terrible way to obtain the item title. Unfortunately, this information isn't surfaced
         # in the API response, but only provided as part of a marcxml description of the entire item record.
         marc = XmlSimple.xml_in(item['bibRecord'])
-        f245 = marc['GetRecord'][0]['record'][0]['metadata'][0]['record'][0]['datafield'].find {|t| t['tag'] == '245'}
+        f245 = marc['GetRecord'][0]['record'][0]['metadata'][0]['record'][0]['datafield'].find { |t| t['tag'] == '245' }
         f245a = f245['subfield'].find { |sf| sf['code'] == 'a' }
         f245b = f245['subfield'].find { |sf| sf['code'] == 'b' }
         title = f245b ? "#{f245a['content']} #{f245b['content']}" : f245a['content']
@@ -422,15 +396,13 @@ def user
       if ENV['DEBUG_USER'] && Rails.env.development?
         netid = ENV['DEBUG_USER']
       else
-        netid = request.env['REMOTE_USER'] ? request.env['REMOTE_USER']  : session[:cu_authenticated_user]
+        netid = request.env['REMOTE_USER'] || session[:cu_authenticated_user]
       end
 
       netid = netid.sub('@CORNELL.EDU', '') unless netid.nil?
       netid = netid.sub('@cornell.edu', '') unless netid.nil?
 
       netid
     end
-
   end
-
 end
diff --git a/lib/my_account/version.rb b/lib/my_account/version.rb
@@ -1,3 +1,3 @@
 module MyAccount
-  VERSION = "2.2.0"
+  VERSION = "2.2.1"
 end
diff --git a/release_notes.md b/release_notes.md
@@ -1,5 +1,10 @@
 # Release Notes - my-account
 
+## v2.2.1
+- Filter out ReShare items that don't match user's NetID.
+- Skip ILL requests with status 'Request Sent to BD' (avoid duplicates).
+- Skip BD/ReShare requests that are also listed in FOLIO (avoid duplicates).
+
 ## v2.2.0
 - Replace Borrow Direct with ReShare
 - Eliminate reference to ilsapi.cgi script