Surface unfinished appointments so dropped items get caught

app/controllers/admin/appointments_controller.rb

@@ -10,6 +10,8 @@ def index
         .chronologically
         .includes(:member, loans: {item: :borrow_policy}, holds: {item: :borrow_policy})
 
+      @unfinished_appointments_count = Appointment.unfinished.count
+
       @pending_appointments = []
       @pulled_appointments = []
       @completed_appointments = []

app/controllers/admin/reports/unfinished_appointments_controller.rb

@@ -0,0 +1,22 @@
+module Admin
+  module Reports
+    class UnfinishedAppointmentsController < BaseController
+      def index
+        @appointments = Appointment.unfinished
+          .chronologically
+          .includes(:member, holds: :item)
+      end
+
+      # Resolve a row straight from the worklist: mark the appointment complete
+      # so it drops off the list. Reversible from the appointment page if needed.
+      def complete
+        appointment = Appointment.find(params[:id])
+        appointment.update!(completed_at: Time.current, staff_updating: true)
+
+        redirect_to admin_reports_unfinished_appointments_path,
+          status: :see_other,
+          flash: {success: "Appointment marked complete."}
+      end
+    end
+  end
+end

app/models/appointment.rb

@@ -23,6 +23,31 @@ class Appointment < ApplicationRecord
   scope :chronologically, -> { order("starts_at ASC") }
   scope :simultaneous, ->(appointment) { where(starts_at: appointment.starts_at, ends_at: appointment.ends_at).where.not(id: appointment.id) }
   scope :not_pulled, -> { where(pulled_at: nil) }
+  scope :pulled, -> { where.not(pulled_at: nil) }
+  scope :not_completed, -> { where(completed_at: nil) }
+
+  # How far back the "unfinished appointments" report looks. Anything older is
+  # almost always a stale record whose item has long since circulated to other
+  # borrowers (an expired hold is a fine terminal state on its own), so we don't
+  # nag staff about it.
+  UNFINISHED_WINDOW = 30.days
+
+  # Appointments from the last UNFINISHED_WINDOW that were pulled, never marked
+  # complete, and still have at least one held item that was neither checked out
+  # nor explicitly ended (cancelled / retired). These are where an item came off
+  # the shelf for a member and nobody closed the loop (#2182). What's unresolved
+  # is the appointment itself, which is why we key on completion rather than on
+  # the hold's own clock.
+  scope :unfinished, ->(now = Time.current) {
+    pulled
+      .not_completed
+      .where(starts_at: (now - UNFINISHED_WINDOW).beginning_of_day...now.beginning_of_day)
+      .where(id: AppointmentHold.where(hold: Hold.where(loan_id: nil, ended_at: nil)).select(:appointment_id))
+  }
+
+  def self.unfinished_window_days
+    (UNFINISHED_WINDOW / 1.day).to_i
+  end
 
   attr_accessor :member_updating, :staff_updating
 
@@ -44,6 +69,11 @@ def completed?
     completed_at.present?
   end
 
+  # Held items on this appointment that never became a loan.
+  def holds_not_checked_out
+    holds.select { |hold| hold.loan_id.nil? }
+  end
+
   def dropoff_only?
     holds.empty? && !loans.empty?
   end

app/views/admin/appointments/_unfinished_banner.html.erb

@@ -0,0 +1,9 @@
+<% if @unfinished_appointments_count.to_i > 0 %>
+  <div class="toast toast-warning mb-2">
+    <%= link_to "Review", admin_reports_unfinished_appointments_path, class: "btn btn-sm float-right" %>
+    <%= pluralize(@unfinished_appointments_count, "appointment") %> from the last
+    <%= pluralize(Appointment.unfinished_window_days, "day") %>
+    <%= (@unfinished_appointments_count == 1) ? "was" : "were" %> pulled but never completed,
+    with items that were never checked out.
+  </div>
+<% end %>

app/views/admin/appointments/index.html.erb

@@ -2,6 +2,8 @@
   <%= index_header "Scheduled Appointments" %>
 <% end %>
 
+<%= render "unfinished_banner" %>
+
 <div class="columns">
   <div class="col-12 admin-appointments">
     <ul class="pagination">

app/views/admin/appointments/index_orig.html.erb

@@ -2,6 +2,8 @@
   <%= index_header "Scheduled Appointments" %>
 <% end %>
 
+<%= render "unfinished_banner" %>
+
 <div class="columns">
   <div class="col-12 admin-appointments-orig">
     <ul class="pagination">

app/views/admin/reports/unfinished_appointments/index.html.erb

@@ -0,0 +1,56 @@
+<%= content_for :header do %>
+  <%= index_header "Unfinished Appointments" %>
+<% end %>
+
+<div class="columns">
+  <div class="column col-12">
+    <p class="text-gray">
+      Appointments from the last <%= pluralize(Appointment.unfinished_window_days, "day") %>
+      that were pulled but never marked complete, where at least one held item was
+      never checked out. Older appointments aren't shown here, since their items have
+      almost always circulated on to other borrowers by now.
+    </p>
+    <p class="text-gray">
+      Use the appointment or member links to figure out what happened to an item, then
+      <strong>Mark complete</strong> to clear the row. (You can undo that from the
+      appointment page if you need to.)
+    </p>
+
+    <% if @appointments.empty? %>
+      <%= empty_state "No unfinished appointments. Nice and tidy." %>
+    <% else %>
+      <table class="table">
+        <tr>
+          <th>Appointment</th>
+          <th>Member</th>
+          <th>Items not checked out</th>
+          <th></th>
+        </tr>
+        <% @appointments.each do |appointment| %>
+          <tr>
+            <td>
+              <%= link_to l(appointment.starts_at, format: :date), admin_appointment_path(appointment) %>
+            </td>
+            <td>
+              <%= link_to preferred_or_default_name(appointment.member), admin_member_path(appointment.member) %>
+            </td>
+            <td>
+              <ul class="simple-list">
+                <% appointment.holds_not_checked_out.each do |hold| %>
+                  <li>
+                    <%= link_to "#{hold.item.name} (#{full_item_number(hold.item)})", admin_item_path(hold.item) %>
+                  </li>
+                <% end %>
+              </ul>
+            </td>
+            <td class="text-right">
+              <%= button_to "Mark complete", complete_admin_reports_unfinished_appointment_path(appointment),
+                    method: :post, class: "btn btn-sm",
+                    data: {turbo_submits_with: "Marking complete..."} %>
+            </td>
+          </tr>
+        <% end %>
+      </table>
+    <% end %>
+  </div>
+</div>

app/views/layouts/admin.html.erb

@@ -59,6 +59,7 @@
               <li class="nav-item"><%= link_to "Activity", admin_reports_monthly_activities_path %></li>
               <li class="nav-item"><%= link_to "Member Requests", admin_reports_member_requests_path %></li>
               <li class="nav-item"><%= link_to "Members with Overdue Loans", admin_reports_members_with_overdue_loans_path %></li>
+              <li class="nav-item"><%= link_to "Unfinished Appointments", admin_reports_unfinished_appointments_path %></li>
               <li class="nav-item"><%= link_to "Memberships", admin_reports_memberships_path %></li>
               <li class="nav-item"><%= link_to "Money", admin_reports_money_path %></li>
               <li class="nav-item"><%= link_to "Notifications", admin_reports_notifications_path %></li>
@@ -166,6 +167,7 @@
                 <li class="menu-item"><%= link_to "Activity", admin_reports_monthly_activities_path %></li>
                 <li class="menu-item"><%= link_to "Member Requests", admin_reports_member_requests_path %></li>
                 <li class="menu-item"><%= link_to "Members with Overdue Loans", admin_reports_members_with_overdue_loans_path %></li>
+                <li class="menu-item"><%= link_to "Unfinished Appointments", admin_reports_unfinished_appointments_path %></li>
                 <li class="menu-item"><%= link_to "Memberships", admin_reports_memberships_path %></li>
                 <li class="menu-item"><%= link_to "Money", admin_reports_money_path %></li>
                 <li class="menu-item"><%= link_to "Notifications", admin_reports_notifications_path %></li>

config/routes.rb

@@ -171,6 +171,11 @@
       resources :shifts, only: :index
       resources :items_without_image, only: :index
       resources :items_with_holds, only: :index
+      resources :unfinished_appointments, only: :index do
+        member do
+          post :complete
+        end
+      end
       resources :zipcodes, only: :index
       get "money", to: "money#index"
       get "members-with-overdue-loans", to: "members_with_overdue_loans#index"

test/controllers/admin/appointments_controller_test.rb

@@ -68,5 +68,14 @@ class AppointmentsControllerTest < ActionDispatch::IntegrationTest
 
       assert_select "em", text: "Item checked-out"
     end
+
+    test "banners unfinished appointments on the index when any exist" do
+      create(:appointment, holds: [create(:hold)], pulled_at: 2.days.ago,
+        starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+
+      get admin_appointments_path(day: Date.current.strftime("%F"))
+
+      assert_select "a[href=?]", admin_reports_unfinished_appointments_path
+    end
   end
 end

test/controllers/admin/reports/unfinished_appointments_controller_test.rb

@@ -0,0 +1,47 @@
+require "test_helper"
+
+module Admin
+  module Reports
+    class UnfinishedAppointmentsControllerTest < ActionDispatch::IntegrationTest
+      include Devise::Test::IntegrationHelpers
+
+      setup do
+        @user = create(:admin_user)
+        sign_in @user
+      end
+
+      test "lists pulled-but-never-completed appointments with items not checked out" do
+        unfinished = create(:appointment, holds: [create(:hold)], pulled_at: 2.days.ago,
+          starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+        completed = create(:appointment, holds: [create(:hold)], pulled_at: 2.days.ago,
+          completed_at: 1.day.ago, starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+
+        get admin_reports_unfinished_appointments_url
+        assert_response :success
+
+        assert_select "a[href=?]", admin_appointment_path(unfinished)
+        assert_select "a[href=?]", admin_appointment_path(completed), false,
+          "completed appointments should not appear"
+      end
+
+      test "shows an empty state when nothing is unfinished" do
+        get admin_reports_unfinished_appointments_url
+        assert_response :success
+
+        assert_select "table", false
+      end
+
+      test "marking an appointment complete resolves it and drops it from the list" do
+        appointment = create(:appointment, holds: [create(:hold)], pulled_at: 2.days.ago,
+          starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+        assert_includes Appointment.unfinished, appointment
+
+        post complete_admin_reports_unfinished_appointment_url(appointment)
+
+        assert_redirected_to admin_reports_unfinished_appointments_url
+        assert appointment.reload.completed?
+        refute_includes Appointment.unfinished, appointment
+      end
+    end
+  end
+end

test/models/appointment_test.rb

@@ -97,6 +97,38 @@ class AppointmentTest < ActiveSupport::TestCase
     assert_equal not_pulled_appointments, Appointment.all.not_pulled
   end
 
+  test ".unfinished surfaces recent pulled, never-completed appointments with items still not checked out" do
+    unfinished = create(:appointment, holds: [create(:hold)], pulled_at: 2.days.ago,
+      starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+
+    # completed -> resolved, excluded
+    create(:appointment, holds: [create(:hold)], pulled_at: 2.days.ago, completed_at: 1.day.ago,
+      starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+    # never pulled -> not in progress, excluded
+    create(:appointment, holds: [create(:hold)], pulled_at: nil,
+      starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+    # pulled today -> still legitimately in progress, excluded
+    create(:appointment, holds: [create(:hold)], pulled_at: Time.current,
+      starts_at: Time.current, ends_at: 1.hour.from_now)
+    # older than the window -> stale, excluded
+    create(:appointment, holds: [create(:hold)],
+      pulled_at: (Appointment::UNFINISHED_WINDOW + 5.days).ago,
+      starts_at: (Appointment::UNFINISHED_WINDOW + 5.days).ago,
+      ends_at: (Appointment::UNFINISHED_WINDOW + 5.days).ago + 1.hour)
+    # pulled, but every held item was checked out -> nothing dangling, excluded
+    member = create(:member)
+    checked_out = create(:hold, member: member, loan: create(:loan, member: member))
+    create(:appointment, holds: [checked_out], pulled_at: 2.days.ago, member: member,
+      starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+    # held item resolved by an explicit end (cancelled / retired) -> excluded
+    ended_member = create(:member)
+    ended_hold = create(:hold, :ended, member: ended_member)
+    create(:appointment, holds: [ended_hold], pulled_at: 2.days.ago, member: ended_member,
+      starts_at: 2.days.ago, ends_at: 2.days.ago + 1.hour)
+
+    assert_equal [unfinished], Appointment.unfinished
+  end
+
   test "#cancel_if_no_items!" do
     appointment = create(:appointment_with_holds)
     appointment.holds.destroy_all