Revive PR#1067

app/controllers/account/holds_controller.rb

@@ -19,7 +19,9 @@ def create
     end
 
     def destroy
-      current_member.active_holds.find(params[:id]).destroy!
+      hold = current_member.active_holds.find(params[:id])
+      hold.cancel!
+
       redirect_to account_holds_path
     end
 

app/helpers/holds_helper.rb

@@ -36,9 +36,15 @@ def render_hold_status(hold)
   end
 
   def render_remove_link(hold)
-    unless hold.appointments.present?
-      link_to("Remove Hold", account_hold_path(hold), class: "btn", method: :delete, data: {confirm: "Are you sure you want to remove this hold?"})
+    message = "Are you sure you want to remove this hold?"
+
+    if hold.appointment.present?
+      message <<
+        " It will also be removed from your appointment."\
+        " If it's the only item in your appointment, your appointment will be canceled."
     end
+
+    link_to("Remove Hold", account_hold_path(hold), class: "btn", method: :delete, data: {confirm: message})
   end
 
   def place_in_line_for(hold)

app/models/appointment.rb

@@ -44,10 +44,18 @@ def merge!(other_appointment)
     end
   end
 
+  def cancel_if_no_items!
+    destroy! if no_items?
+  end
+
   private
 
+  def no_items?
+    holds.empty? && loans.empty?
+  end
+
   def item_present
-    if holds.empty? && loans.empty?
+    if no_items?
       errors.add(:base, "Please select an item to pick-up or return for your appointment")
     end
   end

app/models/hold.rb

@@ -1,8 +1,8 @@
 class Hold < ApplicationRecord
   HOLD_LENGTH = 7.days
 
-  has_many :appointment_holds
-  has_many :appointments, through: :appointment_holds
+  has_one :appointment_hold, dependent: :destroy
+  has_one :appointment, through: :appointment_hold
 
   belongs_to :member
   belongs_to :item, counter_cache: true
@@ -57,6 +57,15 @@ def start!(now = Time.current)
     )
   end
 
+  def cancel!
+    if appointment.present?
+      appointment_hold.destroy!
+      appointment.cancel_if_no_items!
+    end
+
+    destroy!
+  end
+
   # A hold that timed out
   def expired?(now = Time.current)
     started? && expires_at < now

test/factories/appointments.rb

@@ -4,5 +4,9 @@
     ends_at { "2020-09-23 12:14:27" }
     comment { "My Comment" }
     member
+
+    factory :appointment_with_holds do
+      holds { [association(:hold, creator: member.user)] }
+    end
   end
 end

test/models/appointment_test.rb

@@ -68,4 +68,13 @@ class AppointmentTest < ActiveSupport::TestCase
     assert_equal [loan, loan2], original.loans
     assert_equal "First notes\n\nSecond notes", original.comment
   end
+
+  test "#cancel_if_no_items!" do
+    appointment = create(:appointment_with_holds)
+    appointment.holds.destroy_all
+
+    appointment.cancel_if_no_items!
+
+    assert appointment.destroyed?
+  end
 end

test/system/remove_holds_test.rb

@@ -0,0 +1,51 @@
+require "application_system_test_case"
+
+class RemoveHoldsTest < ApplicationSystemTestCase
+  setup do
+    @member = create(:verified_member_with_membership)
+    login_as @member.user
+  end
+
+  test "removes hold if it doesn't have an appointment" do
+    @held_item = create(:item)
+    @hold = create(:hold, item: @held_item, member: @member)
+
+    visit account_holds_path
+    accept_confirm { click_link "Remove Hold" }
+
+    assert_text "You have no items on hold"
+  end
+
+  test "removes hold from associated appointment if appointment has more than one item" do
+    @held_item = create(:item)
+    @borrowed_item = create(:item)
+
+    @hold = create(:hold, item: @held_item, member: @member)
+    @loan = create(:loan, item: @borrowed_item, member: @member)
+
+    create(:event, calendar_id: Event.appointment_slot_calendar_id, start: 27.hours.since.beginning_of_hour, finish: 28.hours.since.beginning_of_hour)
+    @appointment = create(:appointment, member: @member, starts_at: Time.now + 1.day, ends_at: Time.now + 1.day + 2.hours, holds: [@hold], loans: [@loan])
+
+    visit account_holds_path
+    accept_confirm { click_link "Remove Hold" }
+    click_on "Appointments"
+
+    appointment_card = find("li.appointment")
+    within(appointment_card) do
+      assert_no_text @held_item.name
+      assert_text @borrowed_item.name
+    end
+  end
+
+  test "removes hold and cancels associated appointment if it's the only item" do
+    held_item = create(:item)
+    hold = create(:hold, item: held_item, member: @member)
+    create(:appointment, member: @member, starts_at: Time.now + 1.day, ends_at: Time.now + 1.day + 2.hours, holds: [hold])
+
+    visit account_holds_path
+    accept_confirm { click_link "Remove Hold" }
+    click_on "Appointments"
+
+    assert_text "You have no scheduled appointments"
+  end
+end