From 62cf92abeb574f25872ecc376c46a2d1f40f7c02 Mon Sep 17 00:00:00 2001 From: Stephanie Minn <12875392+stephanieminn@users.noreply.github.com> Date: Fri, 29 Jul 2022 11:54:56 -0500 Subject: [PATCH 1/2] Update hold association to have one appointment In reality, a hold can only be have one scheduled appointment at a time. Also makes sure that when a hold is destroyed, its associated appointment hold join record is destroyed. --- app/helpers/holds_helper.rb | 2 +- app/models/hold.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/helpers/holds_helper.rb b/app/helpers/holds_helper.rb index 7f58cdb25..9e59a07ec 100644 --- a/app/helpers/holds_helper.rb +++ b/app/helpers/holds_helper.rb @@ -36,7 +36,7 @@ def render_hold_status(hold) end def render_remove_link(hold) - unless hold.appointments.present? + unless hold.appointment.present? link_to("Remove Hold", account_hold_path(hold), class: "btn", method: :delete, data: {confirm: "Are you sure you want to remove this hold?"}) end end diff --git a/app/models/hold.rb b/app/models/hold.rb index 27d8232d9..6b4937793 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 From 58f41fc708ecd3a90c3ae1819d166241d7c57183 Mon Sep 17 00:00:00 2001 From: Stephanie Minn <12875392+stephanieminn@users.noreply.github.com> Date: Fri, 29 Jul 2022 16:21:46 -0500 Subject: [PATCH 2/2] Canceling a hold cleans up associated appointments If a hold is canceled by a member, it is removed from any associated appointments. If it was the only item in the appointment, the appointment is also canceled. --- app/controllers/account/holds_controller.rb | 4 +- app/helpers/holds_helper.rb | 10 +++- app/models/appointment.rb | 10 +++- app/models/hold.rb | 9 ++++ test/factories/appointments.rb | 4 ++ test/models/appointment_test.rb | 9 ++++ test/system/remove_holds_test.rb | 51 +++++++++++++++++++++ 7 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 test/system/remove_holds_test.rb 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 9e59a07ec..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.appointment.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 6b4937793..9fbb158b1 100644 --- a/app/models/hold.rb +++ b/app/models/hold.rb @@ -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