Return items on a reservation (#1764)

# What it does

* Adds the ability for a staff member to mark items on a reservation as
returned at the end of a loan.
* Restructures the "Pickups" tab to be the "Items" tab on a reservation.
Now all item manipulation (both while building up the specific items to
be borrowed and when they are returned later on) happen on the same
page/tab.
* Deletes an unused partial:
`app/views/account/reservations/_profile.html.erb`.

# Why it is important

We need to have a accept items that are returned to us 😄 

# UI Change Screenshot

![2024-11-19 18 36
54](https://github.com/user-attachments/assets/a278e1ab-9213-4b10-95b1-f9a79b863284)

# Implementation notes

* It's possible we could avoid the turbo stream shenanigans in this
flow, but I started working on this before we merged in the page refresh
morphing stuff in #1738. I will see if that simplification is possible
while tackling future parts of the reservation flow.

app/controllers/admin/reservations/check_ins_controller.rb

@@ -0,0 +1,67 @@
+module Admin
+  module Reservations
+    class CheckInsController < BaseController
+      def create
+        if (reservation_item_id = reservation_loan_lookup_params[:reservable_item_id])
+          @reservable_item = ReservableItem.find_by(id: reservation_item_id)
+          if !@reservable_item
+            render_form_with_error("no item found with this ID")
+            return
+          end
+          @reservation_loan = @reservation.reservation_loans.find_by(reservable_item_id: @reservable_item.id)
+        elsif (reservation_loan_id = reservation_loan_lookup_params[:reservation_loan_id])
+          @reservation_loan = @reservation.reservation_loans.find_by(id: reservation_loan_id)
+        end
+
+        if !@reservation_loan
+          render_form_with_error("not found on this reservation")
+          return
+        end
+
+        if @reservation_loan.checked_in_at.present?
+          render_form_with_error("already marked as returned")
+          return
+        end
+
+        @reservation_loan.update(checked_in_at: Time.current)
+        @reservation_hold = @reservation_loan.reservation_hold
+      end
+
+      def destroy
+        if (reservation_loan_id = params[:id])
+          @reservation_loan = @reservation.reservation_loans.find_by(id: reservation_loan_id)
+        end
+
+        if !@reservation_loan
+          render_form_with_error("not found on this reservation")
+          return
+        end
+
+        if @reservation_loan.checked_in_at.blank?
+          render_form_with_error("not marked as returned")
+          return
+        end
+
+        @reservation_loan.update(checked_in_at: nil)
+        @reservation_hold = @reservation_loan.reservation_hold
+        render :create
+      end
+
+      private
+
+      def render_form_with_error(message)
+        @reservation_loan_lookup_form = ReservationLoanLookupForm.new
+        @reservation_loan_lookup_form.errors.add(:reservable_item_id, message)
+        render_form
+      end
+
+      def render_form
+        render partial: "admin/reservations/check_ins/form", locals: {reservation: @reservation, reservation_loan_lookup_form: @reservation_loan_lookup_form}, status: :unprocessable_entity
+      end
+
+      def reservation_loan_lookup_params
+        params.require(:reservation_loan_lookup_form).permit(:reservable_item_id, :reservation_loan_id)
+      end
+    end
+  end
+end

app/controllers/admin/reservations/pending_reservation_items_controller.rb

@@ -9,7 +9,7 @@ def update
         if result.success?
           render_turbo_response(
             turbo_stream: turbo_stream.action(:redirect,
-              admin_reservation_pickup_path(@pending_reservation_item.reservation))
+              admin_reservation_loans_path(@pending_reservation_item.reservation))
           )
         else
           render_turbo_response :error

app/controllers/admin/reservations/reservation_loans_controller.rb

@@ -3,6 +3,10 @@ module Reservations
     class ReservationLoansController < BaseController
       before_action :set_reservation_loan, only: :destroy
 
+      def index
+        # TODO some eager loading?
+      end
+
       # There are two wolves inside this method. If :reservation_hold_id is passed, it means
       # that we're creating a ReservationLoan for an ItemPool without uniquely numbered items.
       # Otherwise, we're creating a ReservationLoan for an individual ReservableItem.

app/forms/reservation_loan_lookup_form.rb

@@ -0,0 +1,6 @@
+class ReservationLoanLookupForm
+  include ActiveModel::Model
+  include ActiveModel::Attributes
+
+  attribute :reservable_item_id
+end

app/helpers/reservations_helper.rb

@@ -18,7 +18,7 @@ def default_reservation_tab_path(reservation)
     if manager.requested? || manager.pending?
       admin_reservation_path(reservation)
     else
-      admin_reservation_pickup_path(reservation)
+      admin_reservation_loans_path(reservation)
     end
   end
 

app/lib/reservation_state_machine.rb

@@ -14,8 +14,7 @@
   event :ready, from: :building, to: :ready
 
   event :borrow, from: :ready, to: :borrowed
-  event :return, from: :borrowed, to: :returned
-  event :unresolve, from: [:borrowed, :returned], to: :unresolved
+  event :unresolve, from: :borrowed, to: :unresolved
   event :resolve, from: :unresolved, to: :returned
 
   event :cancel, from: any_state, to: :cancelled

app/views/admin/reservations/_profile.html.erb

@@ -52,7 +52,7 @@
 
     <ul class="tab">
       <%= tab_link "Reservation", admin_reservation_path(@reservation) %>
-      <%= tab_link "Items", admin_reservation_pickup_path(@reservation) %>
+      <%= tab_link "Items", admin_reservation_loans_path(@reservation) %>
       <%= tab_link "Questions", admin_reservation_questions_path(@reservation) %>
       <%= tab_link("Review", edit_admin_reservation_review_path(@reservation), badge: true) if @reservation.manager.can?(:approve) %>
       <%= tab_link("Review Notes", admin_reservation_review_path(@reservation)) if @reservation.notes? %>

app/views/admin/reservations/check_ins/_form.html.erb

@@ -0,0 +1,6 @@
+<%= turbo_frame_tag "reservation-loan-form" do %>
+  <%= form_with(model: reservation_loan_lookup_form, url: admin_reservation_check_ins_path(reservation.id), builder: SpectreFormBuilder) do |form| %>
+    <%= form.text_field :reservable_item_id, label: "Item ID", autocomplete: "off", autofocus: true %>
+    <%= form.submit "Return Item" %>
+  <% end %>
+<% end %>

app/views/admin/reservations/check_ins/create.turbo_stream.erb

@@ -0,0 +1,19 @@
+<turbo-stream action="replace" target="reservation-loan-form">
+  <template>
+    <%= render partial: "admin/reservations/check_ins/form", formats: [:html], locals: {reservation: @reservation, reservation_loan_lookup_form: ReservationLoanLookupForm.new} %>
+  </template>
+</turbo-stream>
+
+<% if @reservation_hold %>
+  <turbo-stream action="replace" target="<%= dom_id(@reservation_hold) %>">
+    <template>
+      <%= render partial: "admin/reservations/reservation_loans/reservation_hold", formats: [:html], locals: {reservation: @reservation, reservation_hold: @reservation_hold} %>
+    </template>
+  </turbo-stream>
+<% end %>
+
+<turbo-stream action="replace" target="pending-reservation-items">
+  <template>
+    <%= render partial: "admin/reservations/pending_reservation_items", formats: [:html], locals: {reservation: @reservation} %>
+  </template>
+</turbo-stream>

app/views/admin/reservations/reservation_loans/_form.html.erb

@@ -1,5 +1,5 @@
 <%= turbo_frame_tag "reservation-loan-form" do %>
-  <%= form_with(model: reservation_loan, url: [:admin, reservation, reservation_loan], builder: SpectreFormBuilder) do |form| %>
+  <%= form_with(model: reservation_loan, url: admin_reservation_loans_path(reservation.id), builder: SpectreFormBuilder) do |form| %>
     <%= form.text_field :reservable_item_id, label: "Item ID", autocomplete: "off", autofocus: true %>
     <%= form.submit "Add Item" %>
   <% end %>

app/views/admin/reservations/pickups/_reservation_hold.erb → app/views/admin/reservations/reservation_loans/_reservation_hold.html.erb

@@ -8,7 +8,7 @@
     </th>
   </tr>
   <% reservation_hold.reservation_loans.each do |reservation_loan| %>
-    <tr>
+    <tr id="<%= dom_id(reservation_loan) %>">
       <% if reservation_loan.quantity %>
         <td><%= reservation_hold.item_pool.name %></td>
         <td><%= reservation_loan.quantity %></td>
@@ -18,7 +18,20 @@
       <% end %>
       <td>
         <% if reservation.manager.state == :building %>
-            <%= button_to "Remove", admin_reservation_reservation_loan_path(reservation, reservation_loan), method: :delete, class: "btn btn-sm" %>
+            <%= button_to "Remove", admin_reservation_loan_path(reservation, reservation_loan), method: :delete, class: "btn btn-sm" %>
+        <% end %>
+        <% if reservation_loan.checked_in_at.present? %>
+          returned
+          <%= button_to "Undo return",
+                admin_reservation_check_in_path(reservation, reservation_loan.id),
+                method: :delete,
+                class: "btn btn-sm" %>
+        <% elsif reservation.status == Reservation.statuses[:borrowed] && !reservation_hold.item_pool.uniquely_numbered? %>
+          <%= button_to "Return all",
+                admin_reservation_check_ins_path(reservation),
+                method: :post,
+                params: {reservation_loan_lookup_form: {reservation_loan_id: reservation_loan.id}},
+                class: "btn btn-sm" %>
         <% end %>
       </td>
     <tr>
@@ -32,7 +45,7 @@
       <td>
         <% if !reservation_hold.item_pool.uniquely_numbered? %>
           <%= button_to "Add all",
-                admin_reservation_reservation_loans_path(reservation),
+                admin_reservation_loans_path(reservation),
                 method: :post,
                 params: {reservation_loan: {reservation_hold_id: reservation_hold.id}},
                 class: "btn btn-sm" %>

app/views/admin/reservations/reservation_loans/create.turbo_stream.erb

@@ -7,7 +7,7 @@
 <% if @reservation_hold %>
   <turbo-stream action="replace" target="<%= dom_id(@reservation_hold) %>">
     <template>
-      <%= render partial: "admin/reservations/pickups/reservation_hold", formats: [:html], locals: {reservation: @reservation, reservation_hold: @reservation_hold} %>
+      <%= render partial: "admin/reservations/reservation_loans/reservation_hold", formats: [:html], locals: {reservation: @reservation, reservation_hold: @reservation_hold} %>
     </template>
   </turbo-stream>
 <% end %>

app/views/admin/reservations/pickups/show.html.erb → app/views/admin/reservations/reservation_loans/index.html.erb

@@ -7,6 +7,10 @@
   <% if @reservation.manager.state == :building %>
     <%= render partial: "admin/reservations/reservation_loans/form", locals: {reservation: @reservation, reservation_loan: ReservationLoan.new} %>
   <% end %>
+
+  <% if @reservation.manager.state == :borrowed %>
+    <%= render partial: "admin/reservations/check_ins/form", locals: {reservation: @reservation, reservation_loan_lookup_form: ReservationLoanLookupForm.new} %>
+  <% end %>
   <br>
 
   <%= render partial: "admin/reservations/pending_reservation_items", locals: {reservation: @reservation} %>

config/initializers/lograge.rb

@@ -1,5 +1,5 @@
 Rails.application.configure do
-  config.lograge.enabled = Rails.env.production?
+  config.lograge.enabled = true unless Rails.env.development?
 
   # Set a few custom parameters on log lines
   config.lograge.custom_payload do |controller|

config/routes.rb

@@ -200,12 +200,12 @@
       end
       resources :reservations do
         scope module: "reservations" do
-          resources :reservation_loans, only: [:create, :destroy]
+          resources :items, as: "loans", controller: "reservation_loans", only: [:index, :create, :destroy]
+          resources :check_ins, only: [:create, :destroy]
           resources :reservation_holds
           resources :pending_reservation_items, only: [:update, :destroy]
           resource :status, only: :update
           resource :review, only: [:edit, :update, :show]
-          resource :pickup, only: :show
           resource :item_pool_search, only: :show
           resource :questions, only: :show
         end

test/system/admin/reservations_test.rb → test/system/admin/reservations/reservations_test.rb

@@ -256,7 +256,7 @@ def assert_hold_quantity(item_pool, quantity)
 
     reservation = create(:reservation, :building)
     create(:reservation_hold, reservation: reservation, item_pool: hammer_pool)
-    visit admin_reservation_pickup_path(reservation)
+    visit admin_reservation_loans_path(reservation)
 
     assert_active_tab "Items"
     fill_in "Item ID", with: hammer.id
@@ -277,19 +277,22 @@ def assert_hold_quantity(item_pool, quantity)
     hammer = create(:reservable_item, item_pool: hammer_pool)
 
     reservation = create(:reservation, :building)
-    visit admin_reservation_pickup_path(reservation)
+    visit admin_reservation_loans_path(reservation)
 
-    assert_no_difference "PendingReservationItem.count" do
-      assert_difference "PendingReservationItem.count", 1 do
-        assert_active_tab "Items"
-        fill_in "Item ID", with: hammer.id
-        click_on "Add Item"
+    2.times do # do this twice to ensure that the form continues to work after handling an error
+      assert_no_difference "PendingReservationItem.count" do
+        assert_difference "PendingReservationItem.count", 1 do
+          assert_active_tab "Items"
+          fill_in "Item ID", with: hammer.id
+          click_on "Add Item"
 
-        assert_text "1 item scanned that did not match the reservation"
-      end
+          assert_text "1 item scanned that did not match the reservation"
+          assert_selector "button", text: "Add Item"
+        end
 
-      click_on "Remove"
-      refute_text "1 item scanned that did not match the reservation"
+        click_on "Remove"
+        refute_text "1 item scanned that did not match the reservation"
+      end
     end
   end
 
@@ -298,7 +301,7 @@ def assert_hold_quantity(item_pool, quantity)
     hammer = create(:reservable_item, item_pool: hammer_pool)
 
     reservation = create(:reservation, :building)
-    visit admin_reservation_pickup_path(reservation)
+    visit admin_reservation_loans_path(reservation)
 
     assert_difference -> { reservation.reservation_holds.count } => 1,
       -> { reservation.pending_reservation_items.count } => 0 do
@@ -314,4 +317,41 @@ def assert_hold_quantity(item_pool, quantity)
       assert_hold_quantity hammer_pool, "1/1"
     end
   end
+
+  test "returning items" do
+    hammer_pool = create(:item_pool, name: "Hammer")
+    hammer = create(:reservable_item, item_pool: hammer_pool)
+    glove_pool = create(:item_pool, name: "Gloves", uniquely_numbered: false, unnumbered_count: 10)
+
+    reservation = create(:reservation, :borrowed)
+
+    hammer_hold = create(:reservation_hold, item_pool: hammer_pool, reservation:)
+    hammer_loan = reservation.reservation_loans.create!(reservation_hold: hammer_hold, reservable_item: hammer)
+    glove_hold = create(:reservation_hold, item_pool: glove_pool, quantity: 2, reservation:)
+    glove_loan = reservation.reservation_loans.create!(reservation_hold: glove_hold, quantity: 2)
+
+    visit admin_reservation_loans_path(reservation)
+
+    within_dom_id(hammer_loan) do
+      refute_text "returned"
+    end
+
+    # return hammer
+    fill_in "Item ID", with: hammer.id
+    click_on "Return Item"
+
+    within_dom_id(hammer_loan) do
+      assert_text "returned"
+    end
+
+    # return gloves
+    within_dom_id(glove_loan) do
+      refute_text "returned"
+      click_on "Return all"
+      assert_text "returned"
+    end
+
+    assert hammer_loan.reload.checked_in_at
+    assert glove_loan.reload.checked_in_at
+  end
 end