Merge pull request #967 from sul-dlss/folio-requests

Change naming in RequestsController and Folio::Client to be more FOLIO-centric

app/controllers/requests_controller.rb

@@ -6,7 +6,7 @@ class RequestsController < ApplicationController
   before_action :authorize_update!, except: :index
   rescue_from RequestException, with: :deny_access
 
-  # Renders user requests from Symphony and/or BorrowDirect
+  # Renders user requests from FOLIO and/or BorrowDirect
   #
   # GET /requests
   # GET /requests.json
@@ -28,7 +28,7 @@ def edit
     end
   end
 
-  # Handles form submission for changing or canceling requests/holds/etc in Symphony
+  # Handles form submission for changing or canceling requests/holds/etc in FOLIO
   #
   # PATCH /requests/:id
   # PUT /requests/:id
@@ -38,23 +38,21 @@ def update
     flash[:success] = []
     flash[:error] = []
 
-    handle_pickup_change_request if params['service_point'].present?
-    handle_not_needed_after_request if params['not_needed_after'].present? &&
+    handle_change_pickup_service_point if params['service_point'].present?
+    handle_change_pickup_expiration if params['not_needed_after'].present? &&
                                        params['not_needed_after'] != params['current_fill_by_date']
 
     redirect_to requests_path(group: params[:group])
   end
 
-  # Handles form submission for canceling requests/holds/etc in Symphony
+  # Handles form submission for canceling requests/holds/etc in FOLIO
   #
   # DELETE /requests/:id
   def destroy
-    @response = ils_client.cancel_hold(*cancel_hold_params, patron_or_group.key)
+    @response = ils_client.cancel_request(*cancel_request_params, patron_or_group.key)
 
     case @response.status
-    # The FOLIO API returns 204
-    # TODO: after FOLIO launch remove the 200 case which was the Symphony response
-    when 200, 204
+    when 204
       flash[:success] = t 'mylibrary.request.cancel.success_html', title: params['title']
     else
       Rails.logger.error(@response.body)
@@ -66,38 +64,36 @@ def destroy
 
   private
 
-  def handle_pickup_change_request
-    response_flash_message(response: ils_client.change_pickup_library(*change_pickup_params),
-                           translation_key: 'update_pickup')
+  def handle_change_pickup_service_point
+    response_flash_message(response: ils_client.change_pickup_service_point(*change_pickup_service_point_params),
+                           translation_key: 'change_pickup_service_point')
   end
 
-  def handle_not_needed_after_request
-    response_flash_message(response: ils_client.not_needed_after(*not_needed_after_params),
-                           translation_key: 'update_not_needed_after')
+  def handle_change_pickup_expiration
+    response_flash_message(response: ils_client.change_pickup_expiration(*change_pickup_expiration_params),
+                           translation_key: 'change_pickup_expiration')
   end
 
   def response_flash_message(response:, translation_key:)
     case response.status
-    # The FOLIO API returns 204
-    # TODO: after updating feature tests for FOLIO remove the 200 case which was the Symphony response
-    when 200, 204
+    when 204
       flash[:success].push(t("mylibrary.request.#{translation_key}.success_html", title: params['title']))
     else
       Rails.logger.error(response.body)
       flash[:error].push(t("mylibrary.request.#{translation_key}.success_html", title: params['title']))
     end
   end
 
-  def cancel_hold_params
-    params.require(%I[resource id])
+  def cancel_request_params
+    params.require(%I[id])
   end
 
-  def change_pickup_params
-    params.require(%I[resource id service_point])
+  def change_pickup_service_point_params
+    params.require(%I[id service_point])
   end
 
-  def not_needed_after_params
-    params.require(%I[resource id not_needed_after])
+  def change_pickup_expiration_params
+    params.require(%I[id not_needed_after])
   end
 
   def item_details

app/models/folio/request.rb

@@ -32,10 +32,6 @@ def proxy_request?
       record.dig('details', 'proxyUserId').present?
     end
 
-    def resource
-      key
-    end
-
     def status
       record['status']
     end

app/services/folio_client.rb

@@ -118,21 +118,13 @@ def renew_item_request(user_id, item_id)
     response
   end
 
-  # API compatibility shim with SymphonyClient
-  # @param [String] resource the UUID of the FOLIO hold
-  # @param [String] _item_key this was the item key in Symphony
-  # @param [String] patron_key the UUID of the user in FOLIO
-  def cancel_hold(resource, _item_key, patron_key)
-    cancel_hold_request(patron_key, resource)
-  end
-
-  # Cancel a hold request
+  # Cancel a request
+  # @param [String] request_id the UUID of the FOLIO request
   # @param [String] user_id the UUID of the user in FOLIO
-  # @param [String] hold_id the UUID of the FOLIO hold
-  def cancel_hold_request(user_id, hold_id)
-    # We formerly used the mod-patron API to cancel hold requests, but it is
-    # unable to cancel title level hold requests.
-    request_data = get_json("/circulation/requests/#{hold_id}")
+  def cancel_request(request_id, user_id)
+    # We formerly used the mod-patron API to cancel requests, but it is
+    # unable to cancel title level requests.
+    request_data = get_json("/circulation/requests/#{request_id}")
 
     # Ensure this is the user's request before trying to cancel it
     request_data = {} unless request_data['requesterId'] == user_id || request_data['proxyUserId'] == user_id
@@ -141,41 +133,27 @@ def cancel_hold_request(user_id, hold_id)
                         'cancelledByUserId' => user_id,
                         'cancelledDate' => Time.now.utc.iso8601,
                         'status' => 'Closed - Cancelled')
-    response = put("/circulation/requests/#{hold_id}", json: request_data)
-    check_response(response, title: 'Cancel', context: { user_id: user_id, hold_id: hold_id })
+    response = put("/circulation/requests/#{request_id}", json: request_data)
+    check_response(response, title: 'Cancel', context: { user_id: user_id, request_id: request_id })
 
     response
   end
 
-  # TODO: after FOLIO launch rename this method to reflect service point terminology (maybe change_pickup_service_point)
-  #
-  # Change hold request service point
-  # @example client.change_pickup_library(resource: '4a64eccd-3e44-4bb0-a0f7-9b4c487abf61',
-  #                                        _item_key: '4a64eccd-3e44-4bb0-a0f7-9b4c487abf61',
-  #                                        pickup_location_id: 'bd5fd8d9-72f3-4532-b68c-4db88063d16b')
-  # @param [String] resource the UUID of the FOLIO hold
-  # @param [String] _item_key this was the item key in Symphony
+  # Change request service point
+  # @example client.change_pickup_service_point(request_id: '4a64eccd-3e44-4bb0-a0f7-9b4c487abf61',
+  #                                             pickup_location_id: 'bd5fd8d9-72f3-4532-b68c-4db88063d16b')
+  # @param [String] request_id the UUID of the FOLIO request
   # @param [String] service_point the UUID of the new service point
-  def change_pickup_library(resource, _item_key, service_point)
-    update_request(resource, { 'pickupServicePointId' => service_point })
-  end
-
-  # TODO: after updating feature tests for FOLIO remove this shim and use #change_pickup_expiration directly
-  #
-  # API compatibility shim with SymphonyClient
-  # @param [String] resource the UUID of the FOLIO hold
-  # @param [String] _item_key this was the item key in Symphony
-  # @param [Date] not_needed_after date of the hold request
-  def not_needed_after(resource, _item_key, not_needed_after)
-    change_pickup_expiration(resource, not_needed_after)
+  def change_pickup_service_point(request_id, service_point)
+    update_request(request_id, { 'pickupServicePointId' => service_point })
   end
 
-  # @example client.change_pickup_expiration(hold_id: '4a64eccd-3e44-4bb0-a0f7-9b4c487abf61',
-  #                                        expiration: Date.parse('2023-05-18'))
-  # @param [String] hold_id the UUID of the FOLIO hold
-  # @param [Date] expiration date of the hold request
-  def change_pickup_expiration(hold_id, expiration)
-    update_request(hold_id, { 'requestExpirationDate' => expiration.to_time.utc.iso8601 })
+  # @example client.change_pickup_expiration(request_id: '4a64eccd-3e44-4bb0-a0f7-9b4c487abf61',
+  #                                          expiration: Date.parse('2023-05-18'))
+  # @param [String] request_id the UUID of the FOLIO request
+  # @param [Date] expiration date of the request
+  def change_pickup_expiration(request_id, expiration)
+    update_request(request_id, { 'requestExpirationDate' => expiration.to_time.utc.iso8601 })
   end
 
   # Validate a pin for a user
@@ -240,13 +218,13 @@ def pay_fines(user_id:, amount:)
 
   private
 
-  def update_request(hold_id, request_data_updates)
-    request_data = get_json("/circulation/requests/#{hold_id}")
+  def update_request(request_id, request_data_updates)
+    request_data = get_json("/circulation/requests/#{request_id}")
 
     request_data.merge!(request_data_updates)
-    response = put("/circulation/requests/#{hold_id}", json: request_data)
+    response = put("/circulation/requests/#{request_id}", json: request_data)
     check_response(response, title: 'Update request',
-                             context: { hold_id: hold_id }.merge(request_data_updates))
+                             context: { request_id: request_id }.merge(request_data_updates))
 
     response
   end

app/views/requests/_request.html.erb

@@ -42,7 +42,6 @@
         <% if patron.can_modify_requests? %>
           <% if request.ready_for_pickup? %>
             <%= form_tag request_path(request.key), method: :delete do %>
-              <%= hidden_field_tag :resource, request.resource %>
               <%= hidden_field_tag :title, request.title %>
               <%= hidden_field_tag :group, params[:group] if params[:group] %>
               <%= button_tag class: 'btn btn-link btn-request-cancel btn-icon-prefix', type: 'submit' do %>

app/views/requests/edit.html.erb

@@ -4,7 +4,6 @@
 <div class="modal-body request-edit">
   <h3 class="record-title"><%=@request.title %></h3>
   <%= form_tag request_path(@request.key), method: :patch do %>
-    <%= hidden_field_tag :resource, @request.resource %>
     <%= hidden_field_tag :id, @request.key %>
     <%= hidden_field_tag :title, @request.title %>
     <%= hidden_field_tag :group, params[:group] if params[:group] %>

config/locales/en.yml

@@ -53,10 +53,10 @@ en:
       cancel:
         success_html: <span class="font-weight-bold">Success!</span> "%{title}" was canceled.
         error_html: <span class="font-weight-bold">Sorry!</span> Something went wrong and "%{title}" was not canceled.
-      update_not_needed_after:
+      change_pickup_expiration:
         success_html: <span class="font-weight-bold">Success!</span> "%{title}" not needed after date was updated.
         error_html: <span class="font-weight-bold">Sorry!</span> Something went wrong and "%{title}" not needed after date was not updated.
-      update_pickup:
+      change_pickup_service_point:
         success_html: <span class="font-weight-bold">Success!</span> "%{title}" pickup location was updated.
         error_html: <span class="font-weight-bold">Sorry!</span> Something went wrong and "%{title}" pickup location was not updated.
       deny_access: An unexpected error has occurred

spec/controllers/requests_controller_spec.rb

@@ -75,62 +75,62 @@
       end
 
       context 'when cancel param is sent' do
-        let(:mock_client) { instance_double(FolioClient, cancel_hold: api_response, ping: true) }
+        let(:mock_client) { instance_double(FolioClient, cancel_request: api_response, ping: true) }
 
         it 'cancels the hold and sets the flash message' do
-          patch :update, params: { resource: '123', id: '123', cancel: true }
+          patch :update, params: { id: '123', cancel: true }
 
           expect(flash[:success]).to match(/Success!.*was canceled/)
         end
       end
 
       context 'when service_point param is sent' do
-        let(:mock_client) { instance_double(FolioClient, change_pickup_library: api_response, ping: true) }
+        let(:mock_client) { instance_double(FolioClient, change_pickup_service_point: api_response, ping: true) }
 
         it 'updates the pickup library and sets the flash message' do
-          patch :update, params: { resource: '123', id: '123', service_point: 'Other library' }
+          patch :update, params: { id: '123', service_point: 'Other library' }
 
           expect(flash[:success].first).to match(/Success!.*pickup location was updated/)
         end
       end
 
       context 'when not_needed_after param is sent' do
-        let(:mock_client) { instance_double(FolioClient, not_needed_after: api_response, ping: true) }
+        let(:mock_client) { instance_double(FolioClient, change_pickup_expiration: api_response, ping: true) }
 
         it 'updates the not needed after and sets the flash message' do
-          patch :update, params: { resource: '123', id: '123', not_needed_after: '1999/01/01' }
+          patch :update, params: { id: '123', not_needed_after: '1999/01/01' }
 
           expect(flash[:success].first).to match(/Success!.*not needed after date was updated/)
         end
 
         it 'does not update the not needed after if dates are not changed' do
           patch :update, params: {
-            resource: '123', id: '123', not_needed_after: '1999/01/01', current_fill_by_date: '1999/01/01'
+            id: '123', not_needed_after: '1999/01/01', current_fill_by_date: '1999/01/01'
           }
 
           expect(flash[:success]).to eq []
         end
       end
 
       context 'with a group request' do
-        let(:mock_client) { instance_double(FolioClient, change_pickup_library: api_response, ping: true) }
+        let(:mock_client) { instance_double(FolioClient, change_pickup_service_point: api_response, ping: true) }
 
         it 'renews the item and redirects to requests_path' do
-          patch :update, params: { resource: '123', id: '123', service_point: 'Other library', group: true }
+          patch :update, params: { id: '123', service_point: 'Other library', group: true }
 
           expect(response).to redirect_to requests_path(group: true)
         end
       end
 
       context "when the request key does not match any of the patron's requests" do
         it 'does not renew the item and sets flash messages' do
-          patch :update, params: { resource: 'some_other_request_key', id: 'some_other_request_key' }
+          patch :update, params: { id: 'some_other_request_key' }
 
           expect(flash[:error]).to match('An unexpected error has occurred')
         end
 
         it 'does not renew the item and redirects to requests_path' do
-          patch :update, params: { resource: 'some_other_request_key', id: 'some_other_request_key' }
+          patch :update, params: { id: 'some_other_request_key' }
 
           expect(response).to redirect_to requests_path
         end
@@ -139,7 +139,7 @@
 
     describe '#destroy' do
       let(:api_response) { instance_double('Response', status: 204, content_type: :json) }
-      let(:mock_client) { instance_double(FolioClient, cancel_hold: api_response, ping: true) }
+      let(:mock_client) { instance_double(FolioClient, cancel_request: api_response, ping: true) }
 
       let(:requests) do
         [instance_double(Folio::Request, key: '123')]
@@ -151,12 +151,12 @@
 
       context 'when everything is good' do
         it 'cancels the hold and sets flash messages' do
-          delete :destroy, params: { resource: '123', id: '123' }
+          delete :destroy, params: { id: '123' }
           expect(flash[:success]).to match(/Success!/)
         end
 
         it 'cancels the hold and redirects to requests_path' do
-          delete :destroy, params: { resource: '123', id: '123' }
+          delete :destroy, params: { id: '123' }
           expect(response).to redirect_to requests_path
         end
       end
@@ -165,34 +165,34 @@
         let(:api_response) { instance_double('Response', status: 401, content_type: :json, body: 'foo') }
 
         it 'does not cancel the hold and sets flash messages' do
-          delete :destroy, params: { resource: '123', id: '123' }
+          delete :destroy, params: { id: '123' }
           expect(flash[:error]).to match(/Sorry!/)
         end
 
         it 'does not cancel the hold and redirects to requests_path' do
-          delete :destroy, params: { resource: '123', id: '123' }
+          delete :destroy, params: { id: '123' }
           expect(response).to redirect_to requests_path
         end
       end
     end
 
     context 'with a group request' do
       it 'renews the item and redirects to requests_path' do
-        delete :destroy, params: { resource: '123', id: '123', group: true }
+        delete :destroy, params: { id: '123', group: true }
 
         expect(response).to redirect_to requests_path(group: true)
       end
     end
 
     context "when the request key does not match any of the patron's requests" do
       it 'does not renew the item and sets flash messages' do
-        delete :destroy, params: { resource: 'some_other_request_key', id: 'some_other_request_key' }
+        delete :destroy, params: { id: 'some_other_request_key' }
 
         expect(flash[:error]).to match('An unexpected error has occurred')
       end
 
       it 'does not renew the item and redirects to requests_path' do
-        delete :destroy, params: { resource: 'some_other_request_key', id: 'some_other_request_key' }
+        delete :destroy, params: { id: 'some_other_request_key' }
 
         expect(response).to redirect_to requests_path
       end

spec/features/requests_spec.rb

@@ -17,9 +17,9 @@
   before do
     allow(FolioClient).to receive(:new).and_return(mock_client)
     allow(mock_client).to receive_messages(patron_info: patron_info,
-                                           cancel_hold: api_response,
-                                           change_pickup_library: api_response,
-                                           not_needed_after: api_response)
+                                           cancel_request: api_response,
+                                           change_pickup_service_point: api_response,
+                                           change_pickup_expiration: api_response)
     allow(Folio::ServicePoint).to receive_messages(
       all: service_points
     )