fix(gallagher/parking_approvals): prevent duplicate approval emails

drivers/gallagher/parking_approvals.cr

@@ -589,10 +589,34 @@ class Place::Parking::Approvals < PlaceOS::Driver
   # Gallagher access grant stays with the sweep — it is applied once the
   # booking enters the window, like every other allocation. Bookings inside
   # the window are skipped here: the sweep this same event triggers handles
-  # them (approval, email AND access) via handle_allocated_booking. Repeat
-  # events carry the persisted process_state / notification ledger, so the
-  # guards in approved_email keep this to a single email.
-  protected def notify_manual_allocation(booking : Booking) : Nil
+  # them (approval, email AND access) via handle_allocated_booking.
+  #
+  # The event payload is only a point-in-time snapshot and can be stale: a
+  # state write / user edit may have already advanced the booking, and the
+  # state-change event produced by this send can arrive without the ledger
+  # entry written afterwards. Always re-read the persisted booking and let its
+  # process_state / notification ledger decide whether the email is still owed.
+  # The lock keeps two events from both reading the pre-send state.
+  @manual_allocation_mutex : Mutex = Mutex.new
+
+  protected def notify_manual_allocation(event : Booking) : Nil
+    @manual_allocation_mutex.synchronize do
+      notify_manual_allocation_locked(event)
+    end
+  rescue error
+    logger.warn(exception: error) { "failed to process manually allocated booking #{event.id}" }
+  end
+
+  protected def notify_manual_allocation_locked(event : Booking) : Nil
+    return unless event.approved
+    return if event.rejected || event.deleted
+
+    booking = Booking.from_json(
+      with_retry("fetch booking #{event.id} for manual allocation") do
+        staff_api.get_booking(event.id, event.instance).get_json
+      end
+    )
+
     return unless booking.approved
     return if booking.rejected || booking.deleted
     return if booking.process_state == "access_granted_emailed"
@@ -616,7 +640,7 @@ class Place::Parking::Approvals < PlaceOS::Driver
     update_state(booking, "access_granted") unless booking.process_state == "access_granted"
     approved_email(booking, space)
   rescue error
-    logger.warn(exception: error) { "failed to process manually allocated booking #{booking.id}" }
+    logger.warn(exception: error) { "failed to process manually allocated booking #{event.id}" }
   end
 
   # ===================================
@@ -1856,10 +1880,10 @@ class Place::Parking::Approvals < PlaceOS::Driver
           booking_id: booking.id,
           asset_id: space.id,
           instance: booking.instance,
-          extension_data: booking.extension_data,
           zones: space.zones,
         ).get_json
       end
+      persist_allocation_metadata(booking)
     rescue error
       if clash_error?(error)
         logger.warn { "space #{space.id} is already booked for booking #{booking.id} (server clash); trying another space" }
@@ -2023,9 +2047,9 @@ class Place::Parking::Approvals < PlaceOS::Driver
         booking_id: booking.id,
         asset_id: space.id,
         instance: booking.instance,
-        extension_data: booking.extension_data
       ).get_json
     end
+    persist_allocation_metadata(booking)
 
     booking.asset_id = space.id
     booking.asset_ids = [space.id]
@@ -2034,6 +2058,31 @@ class Place::Parking::Approvals < PlaceOS::Driver
     logger.warn(exception: error) { "failed to restore booking #{booking.id} to space #{space.id}; leaving unallocated for re-allocation" }
   end
 
+  # Allocation only owns the space metadata. Patch these keys in isolation so a
+  # stale booking snapshot can never overwrite the notification ledger or the
+  # email counter — the staff API merges the supplied extension_data into the
+  # persisted booking, so sending the whole hash would undo concurrent writes.
+  protected def persist_allocation_metadata(booking : Booking) : Nil
+    metadata = {} of String => JSON::Any
+    if location = booking.extension_data["location"]?
+      metadata["location"] = location
+    end
+    if group = booking.extension_data["parking_group"]?
+      metadata["parking_group"] = group
+    end
+    return if metadata.empty?
+
+    with_retry("persist allocation metadata for booking #{booking.id}") do
+      staff_api.booking_extension_data(
+        booking.id,
+        metadata,
+        instance: booking.instance,
+      ).get
+    end
+  rescue error
+    logger.warn(exception: error) { "failed to persist allocation metadata for booking #{booking.id}" }
+  end
+
   # ===================================
   # Gallagher access management
   # ===================================

drivers/gallagher/parking_approvals_spec.cr

@@ -147,6 +147,10 @@ DriverSpecs.mock_driver "Place::Parking::Approvals" do
   # both car spaces share the carpriority zone, so the smaller-height space
   # (car_b, 1.95m) is preferred over car_a (2.1m) to keep taller spaces free
   staff.last_update_for(1001_i64).should eq("asset-car_b")
+  # allocation must not resend the full extension_data snapshot: it may be
+  # stale and would clobber the notification ledger / email counter
+  staff.update_extension_data_for(1001_i64).should be_nil
+  staff.patched_ext_value(1001_i64, "location").should eq(JSON::Any.new("BM2.002"))
   gallagher.access_for("ch-normal").should contain("gallagher-group1")
   # approval email uses the per-area trigger (Open Basement -> gallagher-group1)
   mailer.last_template.should eq(["parking_request", "approved_gallagher-group1"])
@@ -3732,8 +3736,8 @@ DriverSpecs.mock_driver "Place::Parking::Approvals" do
       booking_type:    "parking",
       booking_start:   cancel_start,
       booking_end:     cancel_end,
-      asset_id:        asset,
-      asset_ids:       [asset],
+      resource_id:     asset,
+      resource_ids:    [asset],
       user_id:         "user-85001",
       user_email:      "cancel.user@example.com",
       user_name:       "Cancel User",
@@ -3772,6 +3776,33 @@ DriverSpecs.mock_driver "Place::Parking::Approvals" do
   sleep 100.milliseconds
   mailer.times_sent("cancel.user@example.com", "parking_request", "cancelled").should eq(1)
 
+  # events that only provide the plural resource_ids are handled too
+  mailer.reset
+  publish("staff/booking/changed", {
+    action:          "cancelled",
+    id:              85003_i64,
+    booking_type:    "parking",
+    booking_start:   cancel_start,
+    booking_end:     cancel_end,
+    resource_ids:    ["asset-inv1"],
+    user_id:         "user-85003",
+    user_email:      "cancel2.user@example.com",
+    user_name:       "Cancel User 2",
+    booked_by_email: "cancel2.user@example.com",
+    booked_by_name:  "Cancel User 2",
+    zones:           ["zone-building"],
+    created:         now - 500_i64,
+    last_changed:    now,
+    approved:        true,
+    rejected:        false,
+    deleted:         false,
+    process_state:   "access_granted_emailed",
+    extension_data:  {"location" => "INV1"},
+  }.to_json)
+  sleep 100.milliseconds
+  mailer.sent?("cancel2.user@example.com", "parking_request", "cancelled").should eq(true)
+  staff.last_state(85003_i64).should eq("cancelled_emailed")
+
   # a cancelled booking that was only ever wait-listed -> nothing is sent
   mailer.reset
   publish("staff/booking/changed", {
@@ -3780,8 +3811,8 @@ DriverSpecs.mock_driver "Place::Parking::Approvals" do
     booking_type:    "parking",
     booking_start:   cancel_start,
     booking_end:     cancel_end,
-    asset_id:        "unallocated-85002",
-    asset_ids:       ["unallocated-85002"],
+    resource_id:     "unallocated-85002",
+    resource_ids:    ["unallocated-85002"],
     user_id:         "user-85002",
     user_email:      "waitlist.user@example.com",
     user_name:       "Wait List User",
@@ -5236,15 +5267,17 @@ DriverSpecs.mock_driver "Place::Parking::Approvals" do
   staff.set_bookings("[]")
 
   t111_far_start = now + 3600_i64 * 24 * 30
+  # events report resources rather than assets; the driver must still recognise
+  # the allocation and re-read the persisted booking before emailing
   manual_event = ->(state : String?) do
     {
       action:          "approved",
       id:              111003_i64,
       booking_type:    "parking",
       booking_start:   t111_far_start,
       booking_end:     t111_far_start + 3600_i64,
-      asset_id:        "asset-t111c",
-      asset_ids:       ["asset-t111c"],
+      resource_id:     "asset-t111c",
+      resource_ids:    ["asset-t111c"],
       user_id:         "user-111003",
       user_email:      "after.hours@example.com",
       user_name:       "After Hours",
@@ -5260,14 +5293,17 @@ DriverSpecs.mock_driver "Place::Parking::Approvals" do
       extension_data:  ext_manual_car,
     }
   end
+  # reachable by get_booking, but not by the sweep (outside its window)
+  staff.set_lookup_bookings([manual_event.call(nil)].to_json)
 
   publish("staff/booking/changed", manual_event.call(nil).to_json)
   sleep 200.milliseconds
   mailer.times_sent("after.hours@example.com", "parking_request", "approved_gallagher-group1").should eq(1)
   staff.last_state(111003_i64).should eq("access_granted_emailed")
 
-  # a repeat event (carrying the now-persisted state) does not re-send
-  publish("staff/booking/changed", manual_event.call("access_granted_emailed").to_json)
+  # a repeat event with a STALE process_state still re-reads the persisted
+  # state/ledger and does not re-send
+  publish("staff/booking/changed", manual_event.call(nil).to_json)
   sleep 200.milliseconds
   mailer.times_sent("after.hours@example.com", "parking_request", "approved_gallagher-group1").should eq(1)
 
@@ -5423,6 +5459,9 @@ end
 class StaffAPIMock < DriverSpecs::MockDriver
   @assets_json : String = "[]"
   @bookings_json : String = "[]"
+  # bookings findable by get_booking even when they fall outside the sweep
+  # window (query_bookings fixture above does not filter by period)
+  @lookup_bookings : Array(JSON::Any) = [] of JSON::Any
   @updates : Hash(Int64, String) = {} of Int64 => String
   @approved_set : Array(Int64) = [] of Int64
   # process_state keyed by "id:instance" so per-recurring-instance state is
@@ -5446,6 +5485,11 @@ class StaffAPIMock < DriverSpecs::MockDriver
     @bookings_json = json
   end
 
+  # Make bookings available to get_booking without exposing them to the sweep.
+  def set_lookup_bookings(json : String)
+    @lookup_bookings = JSON.parse(json).as_a
+  end
+
   # bookings returned for a query against a zone OTHER than the building (the
   # overflow car park), keyed by zone id — so a test proves the driver queried
   # the zone it was configured with, not just "some other zone"
@@ -5480,6 +5524,7 @@ class StaffAPIMock < DriverSpecs::MockDriver
     @update_attempts = {} of Int64 => Int32
     @created_bookings = [] of JSON::Any
     @created_ids = {} of String => Int64
+    @lookup_bookings = [] of JSON::Any
   end
 
   # when true, query_bookings raises (simulating the staff API erroring)
@@ -5539,7 +5584,8 @@ class StaffAPIMock < DriverSpecs::MockDriver
   # a string value out of the extension_data the driver last wrote for a booking.
   # nil when the booking was never updated, or the key was not set
   def extension_data_key(booking_id : Int64, key : String, instance : Int64? = nil) : String?
-    @update_extension_data["#{booking_id}:#{instance}"]?.try(&.[key]?).try(&.as_s?)
+    @update_extension_data["#{booking_id}:#{instance}"]?.try(&.[key]?).try(&.as_s?) ||
+      @ext_patches[state_key(booking_id, instance)]?.try(&.[key]?).try(&.as_s?)
   end
 
   def approved_instance?(booking_id : Int64, instance : Int64?) : Bool
@@ -5581,6 +5627,13 @@ class StaffAPIMock < DriverSpecs::MockDriver
     @updates[booking_id]?
   end
 
+  def update_extension_data_for(booking_id : Int64) : JSON::Any?
+    @update_extension_data.each do |key, value|
+      return value if key.starts_with?("#{booking_id}:")
+    end
+    nil
+  end
+
   def approved : Array(Int64)
     @approved_set
   end
@@ -5667,28 +5720,35 @@ class StaffAPIMock < DriverSpecs::MockDriver
     # extension_data, mirroring how the backend reflects booking_state /
     # booking_extension_data writes on the next fetch
     source = source.select { |b| b["user_email"]?.try(&.as_s?) == email } if email
-    bookings = source.map do |booking|
-      id = booking["id"].as_i64
-      inst = booking["instance"]?.try(&.as_i64?)
-      state = @states[state_key(id, inst)]?
-      patches = @ext_patches[state_key(id, inst)]?
-      next booking unless state || patches
-      hash = booking.as_h.dup
-      hash["process_state"] = JSON::Any.new(state) if state
-      if patches
-        ext = hash["extension_data"]?.try(&.as_h?).try(&.dup) || {} of String => JSON::Any
-        patches.each { |patch_key, value| ext[patch_key] = value }
-        hash["extension_data"] = JSON::Any.new(ext)
-      end
-      JSON::Any.new(hash)
-    end
-    JSON::Any.new(bookings)
+    JSON::Any.new(source.map { |booking| with_persisted_state(booking) })
   end
 
   def get_booking(booking_id : String | Int64, instance : Int64? = nil)
-    bookings = JSON.parse(@bookings_json).as_a
-    found = bookings.find { |b| b["id"].as_i64 == booking_id.to_s.to_i64 }
-    found || JSON::Any.new({} of String => JSON::Any)
+    source = @lookup_bookings + JSON.parse(@bookings_json).as_a + @created_bookings
+    found = source.find do |booking|
+      booking["id"]?.try(&.as_i64?) == booking_id.to_s.to_i64 &&
+        (instance.nil? || booking["instance"]?.try(&.as_i64?) == instance)
+    end
+    found ? with_persisted_state(found, instance) : JSON::Any.new({} of String => JSON::Any)
+  end
+
+  # Overlay the persisted state / extension data the driver has written, as the
+  # backend would on a re-fetch.
+  protected def with_persisted_state(booking : JSON::Any, instance : Int64? = nil) : JSON::Any
+    id = booking["id"].as_i64
+    inst = instance || booking["instance"]?.try(&.as_i64?)
+    state = @states[state_key(id, inst)]?
+    patches = @ext_patches[state_key(id, inst)]?
+    return booking unless state || patches
+
+    hash = booking.as_h.dup
+    hash["process_state"] = JSON::Any.new(state) if state
+    if patches
+      ext = hash["extension_data"]?.try(&.as_h?).try(&.dup) || {} of String => JSON::Any
+      patches.each { |patch_key, value| ext[patch_key] = value }
+      hash["extension_data"] = JSON::Any.new(ext)
+    end
+    JSON::Any.new(hash)
   end
 
   # bookings created via create_booking this test (mirrors them into queries)

drivers/place/booking_model.cr

@@ -13,13 +13,29 @@ class Place::Booking
   property booking_end : Int64
   property timezone : String?
 
-  # events use resource_id instead of asset_id
+  # events use resource_id/resource_ids instead of asset_id/asset_ids
   property asset_id : String?
   property asset_ids : Array(String) = [] of String
   property resource_id : String?
+  property resource_ids : Array(String) = [] of String
 
   def asset_id : String
-    (@asset_id || @resource_id).as(String)
+    (@asset_id || @resource_id || @asset_ids.first?).as(String)
+  end
+
+  # Staff booking events report the allocated resource(s) as resource_id /
+  # resource_ids. Normalise them onto asset_ids so callers can use the same
+  # accessors for API responses and events.
+  def after_initialize
+    return unless @asset_ids.empty?
+
+    @asset_ids = if !@resource_ids.empty?
+                   @resource_ids
+                 elsif asset = (@asset_id || @resource_id)
+                   [asset]
+                 else
+                   [] of String
+                 end
   end
 
   property user_id : String