Fix appointment merging bug (#1155)

# What it does

This PR fixes an issue that we've seen in production where an ActiveJob
instance fails to deserialize due to an Appointment not being found with
a certain ID. I looked through the code, and I think the likely cause of
the issue is that in some cases we merge multiple appointments that have
been scheduled for the same time slot together, but we were still
enqueuing a confirmation email for an Appointment that didn't exist
after this merge was performed.

The errors we're seeing in production look like this:

```
Error while trying to deserialize arguments: Couldn't find Appointment with 'id'=11238
```

# Why it is important

Some members weren't getting confirmation emails, and it's nice to not
have recurring errors due to bugs.

# Implementation notes

* I added a couple tests for appointment scheduling in the process of
reproducing the bug.
* I refactored the code that handled merges a bit to consolidate that
logic in one place.
* This change does mean we'll be sending out more emails for
appointments than we previously were. We're now sending one for every
appointment change, which might be noisy for members. But it seems good
for them to always have the latest info in their email, so let's try
this and see how it goes.

# Your bandwidth for additional changes to this PR

_Please choose one of the following to help the project maintainers
provide the appropriate level of support:_

- [x] I have the time and interest to make additional changes to this PR
based on feedback.
- [ ] I am interested in feedback but don't need to make the changes
myself.
- [ ] I don't have time or interest in making additional changes to this
work.
- [ ] Other or not sure (please describe):

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