Fix appointment merging bug

README.md

@@ -4,26 +4,26 @@
 
 <!-- toc -->
 
-- [Circulate](#circulate)
-  - [About](#about)
-    - [Project Considerations](#project-considerations)
-  - [Requirements](#requirements)
-  - [Integrations](#integrations)
-  - [Development](#development)
-    - [Setting up Circulate on your machine](#setting-up-circulate-on-your-machine)
-    - [Multi-tenancy](#multi-tenancy)
-    - [Configuring your database](#configuring-your-database)
-    - [Resetting the application](#resetting-the-application)
-    - [Running tests](#running-tests)
-    - [Code formatting and linting](#code-formatting-and-linting)
-    - [Setup pre-commit checks](#setup-pre-commit-checks)
-    - [Documentation](#documentation)
-    - [Who to log in as](#who-to-log-in-as)
-  - [Deployment](#deployment)
-    - [Buildpacks](#buildpacks)
-    - [Release Command](#release-command)
-    - [Daily Summary Emails](#daily-summary-emails)
-  - [Alternatives](#alternatives)
+- [Welcome contributors!](#welcome-contributors)
+- [About](#about)
+  * [Project Considerations](#project-considerations)
+- [Requirements](#requirements)
+- [Integrations](#integrations)
+- [Development](#development)
+  * [Setting up Circulate on your machine](#setting-up-circulate-on-your-machine)
+  * [Multi-tenancy](#multi-tenancy)
+  * [Configuring your database](#configuring-your-database)
+  * [Resetting the application](#resetting-the-application)
+  * [Running tests](#running-tests)
+  * [Code formatting and linting](#code-formatting-and-linting)
+  * [Setup pre-commit checks](#setup-pre-commit-checks)
+  * [Documentation](#documentation)
+  * [Who to log in as](#who-to-log-in-as)
+- [Deployment](#deployment)
+  * [Buildpacks](#buildpacks)
+  * [Release Command](#release-command)
+  * [Daily Summary Emails](#daily-summary-emails)
+- [Alternatives](#alternatives)
 
 <!-- tocstop -->
 

app/controllers/account/appointments_controller.rb

@@ -22,13 +22,7 @@ def create
       @appointment = @member.appointments.new
 
       if @appointment.update(appointment_params)
-        message = if merge_simultaneous_appointments
-          "Your existing appointment scheduled for #{helpers.appointment_date_and_time(@appointment)} has been updated."
-        else
-          "Your appointment was scheduled for #{helpers.appointment_date_and_time(@appointment)}."
-        end
-        MemberMailer.with(member: @member, appointment: @appointment).appointment_confirmation.deliver_later
-        redirect_to account_appointments_path, success: message
+        merge_and_complete_appointment_update
       else
         load_holds_and_loans
         load_appointment_slots
@@ -47,12 +41,7 @@ def update
       @member = current_user.member
 
       if @appointment.update(appointment_params)
-        message = if merge_simultaneous_appointments
-          "Your existing appointment scheduled for #{helpers.appointment_date_and_time(@appointment)} has been updated."
-        else
-          "Your appointment scheduled for #{helpers.appointment_date_and_time(@appointment)} was updated."
-        end
-        redirect_to account_appointments_path, success: message
+        merge_and_complete_appointment_update
       else
         load_holds_and_loans
         load_appointment_slots
@@ -84,12 +73,19 @@ def load_holds_and_loans
       @loans = @member.loans.includes(:item, member: {appointments: :loans}).checked_out
     end
 
-    def merge_simultaneous_appointments
+    # In cases where a member already has an appointment during the selected time slot for the current appointment,
+    # merge those appointments together to avoid confusion for members or staff.
+    def merge_and_complete_appointment_update
       simultaneous_appointment = @member.appointments.simultaneous(@appointment).first
       if simultaneous_appointment
         simultaneous_appointment.merge!(@appointment)
-        true
+        message = "Your existing appointment scheduled for #{helpers.appointment_date_and_time(simultaneous_appointment)} has been updated."
+        MemberMailer.with(member: @member, appointment: simultaneous_appointment).appointment_confirmation.deliver_later
+      else
+        message = "Your appointment was scheduled for #{helpers.appointment_date_and_time(@appointment)}."
+        MemberMailer.with(member: @member, appointment: @appointment).appointment_confirmation.deliver_later
       end
+      redirect_to account_appointments_path, success: message
     end
 
     def load_appointment_for_editing

app/views/renewal/payments/new.html.erb

@@ -12,7 +12,6 @@
       <p>
         <strong>Last year you paid <%= @amount.format(no_cents: true) %>.</strong>
         This year we’re encouraging our library members who can afford to do so to give a little more. As always, though, we don’t turn anyone away for an inability to pay.
-
       </p>
     </div>
   </div>

test/controllers/account/appointments_controller_test.rb

@@ -5,31 +5,79 @@ class AppointmentsControllerTest < ActionDispatch::IntegrationTest
     include Devise::Test::IntegrationHelpers
 
     setup do
-      @appointment = FactoryBot.build(:appointment)
       @user = FactoryBot.create(:user)
       @member = FactoryBot.create(:member, user: @user)
+      sign_in @user
+    end
+
+    private def create_appointment
       @hold = FactoryBot.create(:started_hold, member: @member)
+      @appointment = FactoryBot.build(:appointment)
       @appointment.holds << @hold
       @appointment.member = @member
       @appointment.starts_at = "2020-10-05 7:00AM"
       @appointment.ends_at = "2020-10-05 8:00AM"
       @appointment.save
+    end
 
-      sign_in @user
+    private def assert_enqueued_email(mailer, method, params: {})
+      assert_enqueued_with(job: ActionMailer::MailDeliveryJob, args: ->(job_arguments) {
+        job_mailer, job_method, _delivery, rest = *job_arguments
+        assert_equal mailer.to_s, job_mailer
+        assert_equal method.to_s, job_method
+        assert_equal(params, rest[:params])
+      })
+    end
+
+    test "creates a new appointment to pickup items on hold" do
+      @hold = FactoryBot.create(:started_hold, member: @member)
+      @event = FactoryBot.create(:appointment_slot_event)
+
+      post account_appointments_path, params: {appointment: {
+        hold_ids: [@hold.id],
+        comment: "Excited to start on my project!",
+        time_range_string: "#{@event.start}..#{@event.finish}"
+      }}
+
+      @appointment = @member.appointments.last
+      assert_enqueued_email(MemberMailer, :appointment_confirmation, params: {member: @member, appointment: @appointment})
+
+      assert_redirected_to account_appointments_path
+      assert_equal 1, @member.appointments.count
+    end
+
+    test "adds additional items to an existing appointment via creation form" do
+      create_appointment
+      @hold = FactoryBot.create(:started_hold, member: @member)
+
+      post account_appointments_path, params: {appointment: {
+        hold_ids: [@hold.id],
+        comment: "Excited to start on my project!",
+        time_range_string: "#{@appointment.starts_at}..#{@appointment.ends_at}"
+      }}
+
+      @appointment = @member.appointments.last
+      assert_enqueued_email(MemberMailer, :appointment_confirmation, params: {member: @member, appointment: @appointment})
+
+      assert_redirected_to account_appointments_path
+      assert_equal 1, @member.appointments.count
     end
 
     test "should cancel appointment without affecting holds" do
+      create_appointment
       delete account_appointment_path(@appointment)
       assert_equal 1, @member.holds.count, "Member holds should not be deleted when an appointment is cancelled"
       assert_redirected_to account_appointments_path
     end
 
     test "should get edit appointment" do
+      create_appointment
       get edit_account_appointment_path(@appointment)
       assert_response :success
     end
 
     test "should update appointment" do
+      create_appointment
       assert_equal 1, @appointment.holds.count
 
       @hold2 = FactoryBot.create(:hold, member: @member)
@@ -41,6 +89,7 @@ class AppointmentsControllerTest < ActionDispatch::IntegrationTest
     end
 
     test "should not update appointment with invalid params" do
+      create_appointment
       put account_appointment_path(@appointment), params: {appointment: {hold_ids: [], time_range_string: @appointment.time_range_string, comment: @appointment.comment}}
 
       assert_template :edit
@@ -51,6 +100,7 @@ class AppointmentsControllerTest < ActionDispatch::IntegrationTest
     end
 
     test "should not update appointment scheduled after any holds expire" do
+      create_appointment
       @expired_hold = create(:hold, member: @member)
       @expired_hold.start!(@appointment.starts_at - Hold::HOLD_LENGTH - 1.days)
       put account_appointment_path(@appointment), params: {appointment: {hold_ids: [@hold.id, @expired_hold.id], time_range_string: @appointment.time_range_string, comment: @appointment.comment}}

test/factories/events.rb

@@ -22,4 +22,14 @@
     summary { "Librarian" }
     description { "More information about the event" }
   end
+
+  factory :appointment_slot_event, class: Event do
+    library { Library.first || create(:library) }
+    calendar_id { Event.appointment_slot_calendar_id }
+    calendar_event_id
+    start { 3.days.since }
+    finish { 3.days.since + 2.hours }
+    summary { "Appointment slot" }
+    description { "Pick up or drop off items" }
+  end
 end