From cd5792c708a96ec9704e6af7c112edc8c91f9f7c Mon Sep 17 00:00:00 2001
From: Jim Benton <3331+jim@users.noreply.github.com>
Date: Mon, 25 Nov 2024 11:42:58 -0600
Subject: [PATCH] Return items on a reservation (#1764)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

# 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.
---
 .../reservations/check_ins_controller.rb      | 67 +++++++++++++++++++
 .../pending_reservation_items_controller.rb   |  2 +-
 .../admin/reservations/pickups_controller.rb  |  9 ---
 .../reservation_loans_controller.rb           |  4 ++
 app/forms/reservation_loan_lookup_form.rb     |  6 ++
 app/helpers/reservations_helper.rb            |  2 +-
 app/lib/reservation_state_machine.rb          |  3 +-
 .../account/reservations/_profile.html.erb    | 23 -------
 .../admin/reservations/_profile.html.erb      |  2 +-
 .../reservations/check_ins/_form.html.erb     |  6 ++
 .../check_ins/create.turbo_stream.erb         | 19 ++++++
 .../reservation_loans/_form.html.erb          |  2 +-
 .../_reservation_hold.html.erb}               | 19 +++++-
 .../reservation_loans/create.turbo_stream.erb |  2 +-
 .../index.html.erb}                           |  4 ++
 config/initializers/lograge.rb                |  2 +-
 config/routes.rb                              |  4 +-
 .../{ => reservations}/reservations_test.rb   | 64 ++++++++++++++----
 18 files changed, 183 insertions(+), 57 deletions(-)
 create mode 100644 app/controllers/admin/reservations/check_ins_controller.rb
 delete mode 100644 app/controllers/admin/reservations/pickups_controller.rb
 create mode 100644 app/forms/reservation_loan_lookup_form.rb
 delete mode 100644 app/views/account/reservations/_profile.html.erb
 create mode 100644 app/views/admin/reservations/check_ins/_form.html.erb
 create mode 100644 app/views/admin/reservations/check_ins/create.turbo_stream.erb
 rename app/views/admin/reservations/{pickups/_reservation_hold.erb => reservation_loans/_reservation_hold.html.erb} (61%)
 rename app/views/admin/reservations/{pickups/show.html.erb => reservation_loans/index.html.erb} (79%)
 rename test/system/admin/{ => reservations}/reservations_test.rb (84%)

diff --git a/app/controllers/admin/reservations/check_ins_controller.rb b/app/controllers/admin/reservations/check_ins_controller.rb
new file mode 100644
index 000000000..c5a6a24bc
--- /dev/null
+++ b/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
diff --git a/app/controllers/admin/reservations/pending_reservation_items_controller.rb b/app/controllers/admin/reservations/pending_reservation_items_controller.rb
index 12ee392aa..ed68d3cd8 100644
--- a/app/controllers/admin/reservations/pending_reservation_items_controller.rb
+++ b/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
diff --git a/app/controllers/admin/reservations/pickups_controller.rb b/app/controllers/admin/reservations/pickups_controller.rb
deleted file mode 100644
index a6852316d..000000000
--- a/app/controllers/admin/reservations/pickups_controller.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-module Admin
-  module Reservations
-    class PickupsController < BaseController
-      def show
-        # eager load reservation_holds: [:item_pool, reservation_loans: :reservable_item]
-      end
-    end
-  end
-end
diff --git a/app/controllers/admin/reservations/reservation_loans_controller.rb b/app/controllers/admin/reservations/reservation_loans_controller.rb
index 58eeafb59..dab837395 100644
--- a/app/controllers/admin/reservations/reservation_loans_controller.rb
+++ b/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.
diff --git a/app/forms/reservation_loan_lookup_form.rb b/app/forms/reservation_loan_lookup_form.rb
new file mode 100644
index 000000000..966835210
--- /dev/null
+++ b/app/forms/reservation_loan_lookup_form.rb
@@ -0,0 +1,6 @@
+class ReservationLoanLookupForm
+  include ActiveModel::Model
+  include ActiveModel::Attributes
+
+  attribute :reservable_item_id
+end
diff --git a/app/helpers/reservations_helper.rb b/app/helpers/reservations_helper.rb
index bb95b0171..5ec1c53dd 100644
--- a/app/helpers/reservations_helper.rb
+++ b/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
 
diff --git a/app/lib/reservation_state_machine.rb b/app/lib/reservation_state_machine.rb
index 5e2fc5391..67dabe503 100644
--- a/app/lib/reservation_state_machine.rb
+++ b/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
diff --git a/app/views/account/reservations/_profile.html.erb b/app/views/account/reservations/_profile.html.erb
deleted file mode 100644
index add088ac2..000000000
--- a/app/views/account/reservations/_profile.html.erb
+++ /dev/null
@@ -1,23 +0,0 @@
-<div class="columns">
-  <div class="column col-4 col-sm-12">
-    <div class="panel item-panel">
-      <div class="panel-body">
-        <ul class="item-stats">
-          <%= icon_stat "user", "org", title: "organization" %>
-          <%= icon_stat "clock", @reservation.started_at.to_formatted_s(:long), title: "pickup" %>
-          <%= icon_stat "corner-down-left", @reservation.ended_at.to_formatted_s(:long), title: "return" %>
-          <%= icon_stat "info", @reservation.status, title: "status" %>
-        </ul>
-      </div>
-    </div>
-  </div>
-
-  <div class="column col-sm-12 col-8">
-
-    <ul class="tab">
-      <%= tab_link "Items", admin_reservation_path(@reservation) %>
-      <%= tab_link "Pickup", admin_reservation_pickup_path(@reservation) %>
-    </ul>
-
-    <%= yield %>
-</div>
diff --git a/app/views/admin/reservations/_profile.html.erb b/app/views/admin/reservations/_profile.html.erb
index 66407a81a..3ad715d0a 100644
--- a/app/views/admin/reservations/_profile.html.erb
+++ b/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? %>
diff --git a/app/views/admin/reservations/check_ins/_form.html.erb b/app/views/admin/reservations/check_ins/_form.html.erb
new file mode 100644
index 000000000..b984004bf
--- /dev/null
+++ b/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 %>
diff --git a/app/views/admin/reservations/check_ins/create.turbo_stream.erb b/app/views/admin/reservations/check_ins/create.turbo_stream.erb
new file mode 100644
index 000000000..3dfd36862
--- /dev/null
+++ b/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>
diff --git a/app/views/admin/reservations/reservation_loans/_form.html.erb b/app/views/admin/reservations/reservation_loans/_form.html.erb
index 0316729d4..8b761bec6 100644
--- a/app/views/admin/reservations/reservation_loans/_form.html.erb
+++ b/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 %>
diff --git a/app/views/admin/reservations/pickups/_reservation_hold.erb b/app/views/admin/reservations/reservation_loans/_reservation_hold.html.erb
similarity index 61%
rename from app/views/admin/reservations/pickups/_reservation_hold.erb
rename to app/views/admin/reservations/reservation_loans/_reservation_hold.html.erb
index 150fceb25..d5823f900 100644
--- a/app/views/admin/reservations/pickups/_reservation_hold.erb
+++ b/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" %>
diff --git a/app/views/admin/reservations/reservation_loans/create.turbo_stream.erb b/app/views/admin/reservations/reservation_loans/create.turbo_stream.erb
index dccc17269..d1e1f9dd0 100644
--- a/app/views/admin/reservations/reservation_loans/create.turbo_stream.erb
+++ b/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 %>
diff --git a/app/views/admin/reservations/pickups/show.html.erb b/app/views/admin/reservations/reservation_loans/index.html.erb
similarity index 79%
rename from app/views/admin/reservations/pickups/show.html.erb
rename to app/views/admin/reservations/reservation_loans/index.html.erb
index e3d2c2ed1..dbe1759ec 100644
--- a/app/views/admin/reservations/pickups/show.html.erb
+++ b/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} %>
diff --git a/config/initializers/lograge.rb b/config/initializers/lograge.rb
index e5f618ef5..79b25a3d1 100644
--- a/config/initializers/lograge.rb
+++ b/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|
diff --git a/config/routes.rb b/config/routes.rb
index 4e1bbe6d1..b5a1cc68e 100644
--- a/config/routes.rb
+++ b/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
diff --git a/test/system/admin/reservations_test.rb b/test/system/admin/reservations/reservations_test.rb
similarity index 84%
rename from test/system/admin/reservations_test.rb
rename to test/system/admin/reservations/reservations_test.rb
index 616e4efd3..1077f3669 100644
--- a/test/system/admin/reservations_test.rb
+++ b/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