diff --git a/app/controllers/account/holds_controller.rb b/app/controllers/account/holds_controller.rb index 6e9a9d813..27154b762 100644 --- a/app/controllers/account/holds_controller.rb +++ b/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 diff --git a/app/helpers/holds_helper.rb b/app/helpers/holds_helper.rb index 7f58cdb25..92e86b038 100644 --- a/app/helpers/holds_helper.rb +++ b/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) diff --git a/app/models/appointment.rb b/app/models/appointment.rb index 9001ac221..f3c018315 100644 --- a/app/models/appointment.rb +++ b/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 diff --git a/app/models/hold.rb b/app/models/hold.rb index 27d8232d9..9fbb158b1 100644 --- a/app/models/hold.rb +++ b/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 diff --git a/test/factories/appointments.rb b/test/factories/appointments.rb index ee8d70e12..03b3b5dfd 100644 --- a/test/factories/appointments.rb +++ b/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 diff --git a/test/models/appointment_test.rb b/test/models/appointment_test.rb index a80b56ccd..0c83bf805 100644 --- a/test/models/appointment_test.rb +++ b/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 diff --git a/test/system/remove_holds_test.rb b/test/system/remove_holds_test.rb new file mode 100644 index 000000000..dd9706570 --- /dev/null +++ b/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