From 90a61f70527c96c8c96e100e85bf87c5a7eb62b3 Mon Sep 17 00:00:00 2001 From: William Le Date: Mon, 2 Nov 2026 23:22:35 +0800 Subject: [PATCH 1/2] fix(place/booking_model): Booking signals use .resource_ids but Booking model and gallagher parking approval driver use .asset_ids. Update the shared Booking model to be compatible with the Booking signal params --- drivers/gallagher/parking_approvals.cr | 1 + drivers/gallagher/parking_approvals_spec.cr | 35 ++++++++++++++++++--- drivers/place/booking_model.cr | 20 ++++++++++-- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/drivers/gallagher/parking_approvals.cr b/drivers/gallagher/parking_approvals.cr index 94ebd142631..8682451b75d 100644 --- a/drivers/gallagher/parking_approvals.cr +++ b/drivers/gallagher/parking_approvals.cr @@ -2658,3 +2658,4 @@ class Place::Parking::Approvals < PlaceOS::Driver logger.warn(exception: error) { "failed to update process_state #{state} for booking #{booking.id}" } end end + \ No newline at end of file diff --git a/drivers/gallagher/parking_approvals_spec.cr b/drivers/gallagher/parking_approvals_spec.cr index 5efb50d994c..82c83730599 100644 --- a/drivers/gallagher/parking_approvals_spec.cr +++ b/drivers/gallagher/parking_approvals_spec.cr @@ -3732,8 +3732,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 +3772,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 +3807,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", diff --git a/drivers/place/booking_model.cr b/drivers/place/booking_model.cr index 14f6f0bb122..4e7436e63bc 100644 --- a/drivers/place/booking_model.cr +++ b/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 From 3decd8b299442385b05739e2b94c4133bf2f47a4 Mon Sep 17 00:00:00 2001 From: William Le Date: Thu, 12 Nov 2026 14:34:42 +0800 Subject: [PATCH 2/2] fix(gallagher/parking_approval): prevent duplicate confirmation emails --- drivers/gallagher/parking_approvals.cr | 64 ++++++++++++++-- drivers/gallagher/parking_approvals_spec.cr | 81 +++++++++++++++------ 2 files changed, 113 insertions(+), 32 deletions(-) diff --git a/drivers/gallagher/parking_approvals.cr b/drivers/gallagher/parking_approvals.cr index 8682451b75d..c8ad7034fea 100644 --- a/drivers/gallagher/parking_approvals.cr +++ b/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 # =================================== @@ -2658,4 +2707,3 @@ class Place::Parking::Approvals < PlaceOS::Driver logger.warn(exception: error) { "failed to update process_state #{state} for booking #{booking.id}" } end end - \ No newline at end of file diff --git a/drivers/gallagher/parking_approvals_spec.cr b/drivers/gallagher/parking_approvals_spec.cr index 82c83730599..93fbff55a68 100644 --- a/drivers/gallagher/parking_approvals_spec.cr +++ b/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"]) @@ -5263,6 +5267,8 @@ 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", @@ -5270,8 +5276,8 @@ DriverSpecs.mock_driver "Place::Parking::Approvals" do 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", @@ -5287,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) @@ -5450,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 @@ -5473,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" @@ -5507,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) @@ -5566,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 @@ -5608,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 @@ -5694,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)