From a7d60ae3988d76774cb748174faaba9f75b2d2de Mon Sep 17 00:00:00 2001 From: KesterJJ <62260424+camreeves@users.noreply.github.com> Date: Tue, 6 Oct 2026 17:45:57 +0100 Subject: [PATCH 1/3] feat(place/booking-notifier): email booking reminders before booking start Adds a bookings.booking_reminder template trigger plus a scheduled sweep that emails the booking owner ahead of the booking starting. Reminders are configured as a list of offsets in minutes before the start, checked by a cron sweep, since a cron entry alone cannot express "3 days before an arbitrary booking start". A grace window stops stale reminders being sent after downtime, bookings created after their reminder moment and checked-in bookings are skipped, and each reminder is sent once with the state persisted in the driver status. --- drivers/place/booking_notifier.cr | 177 +++++++++++++++++++++++ drivers/place/booking_notifier_readme.md | 49 ++++++- drivers/place/booking_notifier_spec.cr | 120 ++++++++++++++- 3 files changed, 344 insertions(+), 2 deletions(-) diff --git a/drivers/place/booking_notifier.cr b/drivers/place/booking_notifier.cr index 4a7cb2aca7..7451e0010c 100644 --- a/drivers/place/booking_notifier.cr +++ b/drivers/place/booking_notifier.cr @@ -31,6 +31,19 @@ class Place::BookingNotifier < PlaceOS::Driver poll_bookings: false, poll_every_minutes: 5, + # Reminders emailed to the booking owner before their booking starts. + # One reminder per entry, each entry is the number of minutes before the + # booking start time. An empty list disables reminders. + # i.e. [4320, 60, 15] => 3 days, 1 hour and 15 minutes before the booking + reminders_before_booking: [] of Int32, + # Cron schedule for checking bookings against the reminder offsets above. + # Run it more often than reminder_grace_minutes so no reminder is missed. + reminder_schedule: "*/15 * * * *", + # How long after an offset a reminder may still be sent. Covers the sweep + # interval and any downtime (a reminder outside this window is skipped + # rather than sent late). + reminder_grace_minutes: 30, + notify: { zone_id1: { name: "Sydney Building 1", @@ -99,6 +112,12 @@ class Place::BookingNotifier < PlaceOS::Driver @poll_bookings : Bool = false @poll_every_minutes : Int32 = 5 + # minutes before a booking start, sorted ascending, empty disables reminders + @reminders_before_booking : Array(Int32) = [] of Int32 + @reminder_schedule : String? = nil + @reminder_grace_minutes : Int32 = 30 + @reminders_sent_count : UInt64 = 0_u64 + # Zone_id => notify settings @notify_lookup : Hash(String, SiteDetails) = {} of String => SiteDetails @@ -142,8 +161,19 @@ class Place::BookingNotifier < PlaceOS::Driver @poll_bookings = setting(Bool, :poll_bookings) @poll_every_minutes = setting(Int32, :poll_every_minutes) + @reminders_before_booking = (setting?(Array(Int32), :reminders_before_booking) || [] of Int32) + .reject { |minutes| minutes <= 0 } + .uniq + .sort + @reminder_schedule = setting?(String, :reminder_schedule).presence + @reminder_grace_minutes = (setting?(Int32, :reminder_grace_minutes) || 30).clamp(1, 10_080) + schedule.clear schedule.every(@poll_every_minutes.minutes) { check_bookings } if @poll_bookings + + if !@reminders_before_booking.empty? && (cron = @reminder_schedule) + schedule.cron(cron, @time_zone) { send_booking_reminders } + end end def template_fields : Array(TemplateFields) @@ -201,6 +231,14 @@ class Place::BookingNotifier < PlaceOS::Driver description: "Notification when a booking is cancelled", fields: common_fields ), + TemplateFields.new( + trigger: {"bookings", "booking_reminder#{@template_suffix}"}, + name: "Booking reminder#{@template_fields_suffix}", + description: "Reminder sent to the booking owner ahead of the booking starting", + fields: common_fields + [ + {name: "reminder_offset_minutes", description: "Minutes before the booking start this reminder was sent (e.g., 60)"}, + ] + ), ] end @@ -542,6 +580,145 @@ class Place::BookingNotifier < PlaceOS::Driver end end + # Emails a reminder to the booking owner for every configured offset that the + # booking is currently inside of. Scheduled by on_update, and can be invoked + # directly to check for reminders without waiting for the schedule. + @[Security(Level::Support)] + def send_booking_reminders + offsets = @reminders_before_booking + return if offsets.empty? + + now = Time.utc + now_unix = now.to_unix + query_end = (now + (offsets.max + @reminder_grace_minutes).minutes).to_unix + + sent = load_sent_reminders + upcoming = Set(String).new + sent_this_sweep = 0_u64 + + @check_bookings_mutex.synchronize do + @notify_lookup.each do |building_zone, details| + query_reminder_bookings(building_zone, now_unix, query_end).each do |booking_details| + # already on site, so there is nothing left to remind them about + next if booking_details.checked_in + + booking_key = "#{booking_details.id}.#{booking_details.instance || 0}" + upcoming << booking_key + + offsets.each do |offset| + seconds_before = booking_details.booking_start - now_unix + + # the booking has started, or this offset is still in the future + next unless seconds_before > 0 && seconds_before <= offset + + # the offset passed without this driver running - sending it now + # would be a stale reminder + next if seconds_before <= offset - @reminder_grace_minutes + + # booked after the reminder was meant to go out + if created = booking_details.created + next if created > booking_details.booking_start - offset + end + + next if sent[booking_key]?.try(&.includes?(offset)) + + begin + mailer.send_template( + to: booking_details.user_email, + template: {"bookings", "booking_reminder#{@template_suffix}"}, + args: reminder_args(booking_details, building_zone, details.name, offset), + reply_to: booking_details.booked_by_email.presence, + ).get + rescue error + logger.error(exception: error) { "while sending reminder for booking #{booking_details.id}" } + next + end + + logger.debug { "sent #{offset} minute reminder for booking #{booking_details.id} to #{booking_details.user_email}" } + (sent[booking_key] ||= [] of Int32) << offset + sent_this_sweep += 1 + end + end + end + end + + # forget reminders for bookings that are no longer upcoming (cancelled, or started) + sent.select! { |booking_key, _| upcoming.includes?(booking_key) } + self[:reminders_sent] = sent + + @reminders_sent_count += sent_this_sweep + self[:reminders_sent_count] = @reminders_sent_count + logger.debug { "sent #{sent_this_sweep} booking reminders, #{sent.size} reminders tracked" } + rescue error + logger.error { error.inspect_with_backtrace } + self[:last_error] = { + error: error.message, + time: Time.local.to_s, + function: "send_booking_reminders", + } + end + + protected def query_reminder_bookings(building_zone : String, period_start : Int64, period_end : Int64) : Array(Booking) + unapproved = staff_api.query_bookings( + type: @booking_type, + period_start: period_start, + period_end: period_end, + zones: [building_zone], + approved: false, + rejected: false + ).get.as_a + + approved = staff_api.query_bookings( + type: @booking_type, + period_start: period_start, + period_end: period_end, + zones: [building_zone], + approved: true, + rejected: false + ).get.as_a + + Array(Booking).from_json((unapproved + approved).to_json) + end + + protected def reminder_args(booking_details : Booking, building_zone : String, building_name : String, offset : Int32) + timezone = booking_details.timezone.presence || @time_zone.name + location = Time::Location.load(timezone) + starting = Time.unix(booking_details.booking_start).in(location) + ending = Time.unix(booking_details.booking_end).in(location) + + { + booking_id: booking_details.id, + start_time: starting.to_s(@time_format), + start_date: starting.to_s(@date_format), + start_datetime: starting.to_s(@date_time_format), + end_time: ending.to_s(@time_format), + end_date: ending.to_s(@date_format), + end_datetime: ending.to_s(@date_time_format), + starting_unix: booking_details.booking_start, + + asset_id: booking_details.asset_id, + asset_name: lookup_asset(asset_id: booking_details.asset_id, type: booking_details.booking_type, zones: booking_details.zones), + user_id: booking_details.user_id, + user_email: booking_details.user_email, + user_name: booking_details.user_name, + reason: booking_details.title, + + level_zone: booking_details.zones.reject { |z| z == building_zone }.first?, + building_zone: building_zone, + building_name: building_name, + + booked_by_name: booking_details.booked_by_name, + booked_by_email: booking_details.booked_by_email, + + reminder_offset_minutes: offset.to_i64, + } + end + + protected def load_sent_reminders : Hash(String, Array(Int32)) + raw = self[:reminders_sent]? + raw ? Hash(String, Array(Int32)).from_json(raw.to_json) : Hash(String, Array(Int32)).new + end + @[Security(Level::Support)] def get_manager(staff_email : String) manager = calendar.get_user_manager(staff_email).get diff --git a/drivers/place/booking_notifier_readme.md b/drivers/place/booking_notifier_readme.md index 65ccf81936..6e218c94b5 100644 --- a/drivers/place/booking_notifier_readme.md +++ b/drivers/place/booking_notifier_readme.md @@ -66,12 +66,51 @@ mail (the `reply_to` setting on the SMTP Mailer). See the Template Mailer readme for the full precedence cascade. +## Booking reminders + +The driver can email the booking owner a reminder before their booking starts. +Reminders are driven by a list of offsets and a scheduled sweep, because a plain +cron entry cannot express "3 days before a booking that starts on Thursday". + +```yaml + # Minutes before the booking start time to email the booking owner. + # One reminder is sent per entry, an empty list disables reminders. + # i.e. [4320, 60, 15] => 3 days, 1 hour and 15 minutes before the booking + reminders_before_booking: [4320, 60] + + # Cron schedule for the sweep that checks bookings against the offsets above. + # Run it more often than reminder_grace_minutes so no reminder is missed. + reminder_schedule: "*/15 * * * *" + + # How long after an offset a reminder may still be sent. Covers the sweep + # interval and any driver downtime - a reminder outside this window is + # skipped rather than sent late. + reminder_grace_minutes: 30 +``` + +* Recipients: only the booking owner (`user_email`), replies go to the booking + creator as per the Reply-To section above. +* Already checked-in bookings are skipped, as are bookings created after their + reminder time had passed (booking them would not have triggered a reminder). +* Each reminder is sent once per booking. The sent state is stored in the + driver status under `reminders_sent`, so a restart of the driver does not + re-send reminders. +* Reminder windows are calculated from the booking's Unix start time, so the + booking timezone does not change when a reminder fires. + +A sweep can also be triggered manually with the `send_booking_reminders` +function (level `Support`) while configuring or troubleshooting. + + ## Template configuration on Mailer -There are two templates that are expected: +The templates expected are: * `booking_notify` (the booking owner booked the asset) * `booked_by_notify` (someone booked on the owners behalf) +* `rejected` (the booking was rejected) +* `cancelled` (the booking was cancelled) +* `booking_reminder` (reminder ahead of the booking starting) ```yaml email_templates: @@ -82,6 +121,12 @@ email_templates: your desk %{asset_id} has been booked for %{start_date} + booking_reminder: + subject: Reminder - your desk booking + html: > + + your desk %{asset_id} starts at %{start_time} on %{start_date} + ``` The variables available to mix into the email template are: @@ -107,3 +152,5 @@ The variables available to mix into the email template are: booked_by_email attachment_name attachment_url + reminder_offset_minutes (booking_reminder template only - the offset + in minutes that triggered this reminder) diff --git a/drivers/place/booking_notifier_spec.cr b/drivers/place/booking_notifier_spec.cr index 50ff9d6e3c..e96f43db90 100644 --- a/drivers/place/booking_notifier_spec.cr +++ b/drivers/place/booking_notifier_spec.cr @@ -1,7 +1,35 @@ require "placeos-driver/spec" require "placeos-driver/interface/mailer" -DriverSpecs.mock_driver "Place::BookingCheckInHelper" do +# Desk booking fixture anchored to the current time, so the reminder offsets +# (which the driver compares against Time.utc) can be exercised deterministically +def reminder_booking( + id : Int32, + starts_in : Time::Span, + email : String, + created : Time? = nil, + checked_in : Bool = false, +) + starting = Time.utc + starts_in + { + id: id, + booking_type: "desk", + booking_start: starting.to_unix, + booking_end: (starting + 8.hours).to_unix, + asset_id: "desk-#{id}", + user_id: "user-#{id}", + user_email: email, + user_name: "Person #{id}", + zones: ["zone-building"], + checked_in: checked_in, + rejected: false, + created: created.try(&.to_unix), + booked_by_name: "Booker #{id}", + booked_by_email: "booker-#{id}@org.com", + } +end + +DriverSpecs.mock_driver "Place::BookingNotifier" do system({ Mailer: {MailerMock}, Calendar: {CalendarMock}, @@ -16,12 +44,83 @@ DriverSpecs.mock_driver "Place::BookingCheckInHelper" do system(:Mailer)[:to].should eq ["concierge@place.com", "user1234@org.com", "manager@site.com"] # replies should go to the person who created the booking, not the PlaceOS sender system(:Mailer)[:reply_to].should eq "user1234@org.com" + + # ============================ booking reminders ============================ + + reminder_settings = { + booking_type: "desk", + poll_bookings: false, + poll_every_minutes: 5, + reminder_schedule: "", # driven by exec below rather than the cron + reminder_grace_minutes: 30, + reminders_before_booking: [60], + notify: { + zone_id1: { + name: "Sydney Building 1", + email: ["concierge@place.com"], + notify_manager: true, + notify_booking_owner: true, + }, + }, + } + + # starts inside the 60 minute offset (45 seconds before the start, the grace + # window is 30 minutes) => reminder due + due = reminder_booking(id: 11, starts_in: 45.seconds, email: "due@org.com") + # starts outside the 60 minute offset => no reminder yet + not_yet_due = reminder_booking(id: 12, starts_in: 2.hours, email: "later@org.com") + # booked 10 seconds ago for a start 45 seconds away, so the 60 minute + # reminder moment had already passed when it was booked => skipped + booked_too_late = reminder_booking( + id: 13, + starts_in: 45.seconds, + email: "late@org.com", + created: Time.utc - 10.seconds + ) + # already on site => skipped + on_site = reminder_booking(id: 14, starts_in: 45.seconds, email: "onsite@org.com", checked_in: true) + + system(:StaffAPI_1).as(StaffAPIMock).set_reminders( + [due, not_yet_due, booked_too_late, on_site].map { |booking| JSON.parse(booking.to_json) } + ) + + settings(reminder_settings) + + exec(:send_booking_reminders).get + + system(:Mailer)[:reminders].should eq 1 + system(:Mailer)[:template].should eq ["bookings", "booking_reminder"] + system(:Mailer)[:to].should eq "due@org.com" + # replies should go to the person who created the booking, not the PlaceOS sender + system(:Mailer)[:reply_to].should eq "booker-11@org.com" + status[:reminders_sent_count].should eq 1 + + # a second sweep does not send the same reminder again + exec(:send_booking_reminders).get + system(:Mailer)[:reminders].should eq 1 + status[:reminders_sent_count].should eq 1 + + # moving the offset out to 2 hours catches the booking that wasn't due yet, + # and the booking that was skipped is still skipped + settings(reminder_settings.merge({reminders_before_booking: [7200]})) + exec(:send_booking_reminders).get + system(:Mailer)[:reminders].should eq 2 + system(:Mailer)[:to].should eq "later@org.com" + system(:Mailer)[:reply_to].should eq "booker-12@org.com" + status[:reminders_sent_count].should eq 2 + + # the reminder trigger is advertised to the template mailer + triggers = exec(:template_fields).get.not_nil!.as_a.map { |fields| fields["trigger"].as_a.map(&.as_s).join(".") } + triggers.should contain "bookings.booking_notify" + triggers.should contain "bookings.booking_reminder" end # :nodoc: class MailerMock < DriverSpecs::MockDriver include PlaceOS::Driver::Interface::Mailer + @reminders : Int32 = 0 + # need this for the interface def send_mail( to : String | Array(String), @@ -53,6 +152,11 @@ class MailerMock < DriverSpecs::MockDriver self[:template] = template self[:to] = to self[:reply_to] = reply_to + + if template[1].starts_with?("booking_reminder") + @reminders += 1 + self[:reminders] = @reminders + end end end @@ -68,6 +172,12 @@ end # :nodoc: class StaffAPIMock < DriverSpecs::MockDriver @called : Int32 = 0 + @reminders : Array(JSON::Any) = [] of JSON::Any + + # bookings returned by query_bookings once reminders have been configured + def set_reminders(bookings : Array(JSON::Any)) + @reminders = bookings + end def query_bookings( type : String, @@ -87,6 +197,14 @@ class StaffAPIMock < DriverSpecs::MockDriver @called += 1 self[:queries] = @called + + # a reminder sweep queries each zone twice (approved and not yet approved), + # so the fixtures come back on the first query of each pair + unless @reminders.empty? + return @reminders if @called.odd? + return [] of JSON::Any + end + return [] of String if @called >= 2 now = Time.local From 8cc6c421e881e2d063ef1f46458268fa2cb5f6f5 Mon Sep 17 00:00:00 2001 From: KesterJJ <62260424+camreeves@users.noreply.github.com> Date: Wed, 7 Oct 2026 17:01:01 +0100 Subject: [PATCH 2/3] feat(place/booking-notifier): immediate reminder sweep and pre-push coverage - resolve the zone attachments and approver merge fields for reminders, honouring disable_attachments, so no placeholder is left unsubstituted - skip zones without notify_booking_owner and bookings with no owner email - drop network_username / network_password from the reminder trigger's advertised fields - a reminder must not rotate the booking's credential - sweep immediately when the driver starts or its settings change, so a restart cannot push a reminder past reminder_grace_minutes - spec: cron driven sweep, two offsets on one booking, a locker booking, and the advertised fields must equal the args actually sent --- drivers/place/booking_notifier.cr | 34 +++++- drivers/place/booking_notifier_readme.md | 24 +++- drivers/place/booking_notifier_spec.cr | 136 +++++++++++++++++++++-- 3 files changed, 177 insertions(+), 17 deletions(-) diff --git a/drivers/place/booking_notifier.cr b/drivers/place/booking_notifier.cr index 7451e0010c..f093d3dd1a 100644 --- a/drivers/place/booking_notifier.cr +++ b/drivers/place/booking_notifier.cr @@ -172,7 +172,9 @@ class Place::BookingNotifier < PlaceOS::Driver schedule.every(@poll_every_minutes.minutes) { check_bookings } if @poll_bookings if !@reminders_before_booking.empty? && (cron = @reminder_schedule) - schedule.cron(cron, @time_zone) { send_booking_reminders } + # immediate: the sweep runs as soon as the driver starts or its settings + # change, so a restart cannot push reminders past their grace window + schedule.cron(cron, @time_zone, immediate: true) { send_booking_reminders } end end @@ -235,7 +237,9 @@ class Place::BookingNotifier < PlaceOS::Driver trigger: {"bookings", "booking_reminder#{@template_suffix}"}, name: "Booking reminder#{@template_fields_suffix}", description: "Reminder sent to the booking owner ahead of the booking starting", - fields: common_fields + [ + # no network credentials - a reminder must not rotate the password that + # the booking notification already handed out + fields: common_fields.reject { |field| {"network_username", "network_password"}.includes?(field[:name]) } + [ {name: "reminder_offset_minutes", description: "Minutes before the booking start this reminder was sent (e.g., 60)"}, ] ), @@ -598,9 +602,16 @@ class Place::BookingNotifier < PlaceOS::Driver @check_bookings_mutex.synchronize do @notify_lookup.each do |building_zone, details| + # a zone that never emails the booking owner gets no reminders either + next unless details.notify_booking_owner + + attachments = details.attachments.compact_map { |n, l| get_attachment(n, l) } + attach = attachments.first? + attachments.clear if @disable_attachments + query_reminder_bookings(building_zone, now_unix, query_end).each do |booking_details| # already on site, so there is nothing left to remind them about - next if booking_details.checked_in + next if booking_details.checked_in || booking_details.user_email.presence.nil? booking_key = "#{booking_details.id}.#{booking_details.instance || 0}" upcoming << booking_key @@ -626,7 +637,8 @@ class Place::BookingNotifier < PlaceOS::Driver mailer.send_template( to: booking_details.user_email, template: {"bookings", "booking_reminder#{@template_suffix}"}, - args: reminder_args(booking_details, building_zone, details.name, offset), + args: reminder_args(booking_details, building_zone, details.name, offset, attach), + attachments: attachments, reply_to: booking_details.booked_by_email.presence, ).get rescue error @@ -680,7 +692,13 @@ class Place::BookingNotifier < PlaceOS::Driver Array(Booking).from_json((unapproved + approved).to_json) end - protected def reminder_args(booking_details : Booking, building_zone : String, building_name : String, offset : Int32) + protected def reminder_args( + booking_details : Booking, + building_zone : String, + building_name : String, + offset : Int32, + attach : NamedTuple(file_name: String, content: String, uri: String)? = nil, + ) timezone = booking_details.timezone.presence || @time_zone.name location = Time::Location.load(timezone) starting = Time.unix(booking_details.booking_start).in(location) @@ -707,9 +725,15 @@ class Place::BookingNotifier < PlaceOS::Driver building_zone: building_zone, building_name: building_name, + approver_name: booking_details.approver_name, + approver_email: booking_details.approver_email, + booked_by_name: booking_details.booked_by_name, booked_by_email: booking_details.booked_by_email, + attachment_name: attach.try &.[](:file_name), + attachment_url: attach.try &.[](:uri), + reminder_offset_minutes: offset.to_i64, } end diff --git a/drivers/place/booking_notifier_readme.md b/drivers/place/booking_notifier_readme.md index 6e218c94b5..9b64e220a3 100644 --- a/drivers/place/booking_notifier_readme.md +++ b/drivers/place/booking_notifier_readme.md @@ -89,14 +89,32 @@ cron entry cannot express "3 days before a booking that starts on Thursday". ``` * Recipients: only the booking owner (`user_email`), replies go to the booking - creator as per the Reply-To section above. + creator as per the Reply-To section above. A zone without + `notify_booking_owner: true` gets no reminders, matching the booking + notification emails. +* The sweep queries bookings of the driver's `booking_type`, so a reminder + reaches whichever kind of booking the instance is configured for - deploy an + instance per type (`desk`, `locker`, `visitor`, ...). For a visitor booking + `user_email` is the host, so the reminder goes to the host. With + `unique_templates: true` the trigger and template become + `bookings.booking_reminder_` (e.g. `booking_reminder_visitor`), + so each type can have its own template. * Already checked-in bookings are skipped, as are bookings created after their reminder time had passed (booking them would not have triggered a reminder). * Each reminder is sent once per booking. The sent state is stored in the driver status under `reminders_sent`, so a restart of the driver does not re-send reminders. +* The schedule sweeps immediately when the driver starts or its settings + change, then on the cron interval. A restart therefore checks for due + reminders straight away instead of waiting up to `reminder_schedule`. * Reminder windows are calculated from the booking's Unix start time, so the booking timezone does not change when a reminder fires. +* Zone `attachments` behave as they do on booking notifications: the file is + attached when `disable_attachments` is false, and `attachment_name` / + `attachment_url` merge fields are always available for linking to it. +* The reminder template does not provide `network_username` / + `network_password` - a reminder must not rotate the password that the + booking notification already handed out. A sweep can also be triggered manually with the `send_booking_reminders` function (level `Support`) while configuring or troubleshooting. @@ -154,3 +172,7 @@ The variables available to mix into the email template are: attachment_url reminder_offset_minutes (booking_reminder template only - the offset in minutes that triggered this reminder) + +`network_username` and `network_password` are available to the booking +notification templates when the zone enables `include_network_credentials`, +but are deliberately not offered on `booking_reminder`. diff --git a/drivers/place/booking_notifier_spec.cr b/drivers/place/booking_notifier_spec.cr index e96f43db90..8980646e08 100644 --- a/drivers/place/booking_notifier_spec.cr +++ b/drivers/place/booking_notifier_spec.cr @@ -9,14 +9,16 @@ def reminder_booking( email : String, created : Time? = nil, checked_in : Bool = false, + type : String = "desk", + asset_id : String? = nil, ) starting = Time.utc + starts_in { id: id, - booking_type: "desk", + booking_type: type, booking_start: starting.to_unix, booking_end: (starting + 8.hours).to_unix, - asset_id: "desk-#{id}", + asset_id: asset_id || "desk-#{id}", user_id: "user-#{id}", user_email: email, user_name: "Person #{id}", @@ -31,9 +33,10 @@ end DriverSpecs.mock_driver "Place::BookingNotifier" do system({ - Mailer: {MailerMock}, - Calendar: {CalendarMock}, - StaffAPI: {StaffAPIMock}, + Mailer: {MailerMock}, + Calendar: {CalendarMock}, + StaffAPI: {StaffAPIMock}, + LocationServices: {LocationServicesMock}, }) exec(:check_bookings).get @@ -60,6 +63,7 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do email: ["concierge@place.com"], notify_manager: true, notify_booking_owner: true, + attachments: {"welcome.pdf" => "https://s3.example.com/welcome.pdf"}, }, }, } @@ -84,9 +88,14 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do [due, not_yet_due, booked_too_late, on_site].map { |booking| JSON.parse(booking.to_json) } ) - settings(reminder_settings) + # arming the schedule in settings sweeps immediately, so the reminders due + # right after a restart are picked up without waiting for the cron interval + settings(reminder_settings.merge({reminder_schedule: "*/15 * * * *"})) - exec(:send_booking_reminders).get + 50.times do + break if status["reminders_sent_count"]?.try(&.as_i) == 1 + sleep 100.milliseconds + end system(:Mailer)[:reminders].should eq 1 system(:Mailer)[:template].should eq ["bookings", "booking_reminder"] @@ -95,7 +104,18 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do system(:Mailer)[:reply_to].should eq "booker-11@org.com" status[:reminders_sent_count].should eq 1 - # a second sweep does not send the same reminder again + # every merge field advertised for the reminder is supplied + reminder_args = system(:Mailer)[:args].as_h + reminder_args["reminder_offset_minutes"].as_i.should eq 60 + reminder_args["attachment_name"].as_s.should eq "welcome.pdf" + reminder_args["attachment_url"].as_s.should eq "https://s3.example.com/welcome.pdf" + reminder_args["approver_name"].raw.should be_nil + # zone attachments follow disable_attachments (default true) => no files attached + system(:Mailer)[:attachments].as_a.should be_empty + + # disarm the schedule so only exec drives the sweeps below, and a manual + # sweep after the scheduled one does not send the same reminder again + settings(reminder_settings) exec(:send_booking_reminders).get system(:Mailer)[:reminders].should eq 1 status[:reminders_sent_count].should eq 1 @@ -109,10 +129,83 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do system(:Mailer)[:reply_to].should eq "booker-12@org.com" status[:reminders_sent_count].should eq 2 + # a zone that does not notify the booking owner gets no reminders either + ownerless_due = reminder_booking(id: 15, starts_in: 45.seconds, email: "no-flag@org.com") + system(:StaffAPI_1).as(StaffAPIMock).set_reminders([ownerless_due].map { |booking| JSON.parse(booking.to_json) }) + settings(reminder_settings.merge({ + notify: {zone_id1: {name: "Sydney Building 1", email: ["concierge@place.com"]}}, + })) + exec(:send_booking_reminders).get + system(:Mailer)[:reminders].should eq 2 + status[:reminders_sent_count].should eq 2 + + # a visitor booking is queried as a visitor booking and reminded at the + # host's address, using the booking type specific template name + visitor_due = reminder_booking( + id: 16, + starts_in: 45.seconds, + email: "host@org.com", + type: "visitor", + asset_id: "guest@visitor.com" + ) + system(:StaffAPI_1).as(StaffAPIMock).set_reminders([visitor_due].map { |booking| JSON.parse(booking.to_json) }) + settings(reminder_settings.merge({booking_type: "visitor", unique_templates: true})) + exec(:send_booking_reminders).get + + system(:StaffAPI)[:last_type].should eq "visitor" + system(:Mailer)[:reminders].should eq 3 + system(:Mailer)[:template].should eq ["bookings", "booking_reminder_visitor"] + system(:Mailer)[:to].should eq "host@org.com" + system(:Mailer)[:reply_to].should eq "booker-16@org.com" + status[:reminders_sent_count].should eq 3 + # the reminder trigger is advertised to the template mailer - triggers = exec(:template_fields).get.not_nil!.as_a.map { |fields| fields["trigger"].as_a.map(&.as_s).join(".") } - triggers.should contain "bookings.booking_notify" - triggers.should contain "bookings.booking_reminder" + triggers = exec(:template_fields).get.not_nil!.as_a + trigger_names = triggers.map { |fields| fields["trigger"].as_a.map(&.as_s).join(".") } + trigger_names.should contain "bookings.booking_notify_visitor" + trigger_names.should contain "bookings.booking_reminder_visitor" + + reminder_fields = triggers.find { |fields| fields["trigger"].as_a.map(&.as_s).join(".") == "bookings.booking_reminder_visitor" } + field_names = reminder_fields.not_nil!["fields"].as_a.map { |field| field["name"].as_s } + field_names.should contain "reminder_offset_minutes" + field_names.should contain "attachment_name" + # a reminder must never rotate the network password handed out by the booking email + field_names.includes?("network_username").should be_false + field_names.includes?("network_password").should be_false + # every merge field advertised for the reminder is present in the arguments + # that were actually sent, so no placeholder can reach the email + # unsubstituted (the field list is shared by every booking type) + field_names.sort.should eq system(:Mailer)[:args].as_h.keys.sort + + # two offsets open at the same time on one booking => one reminder each, + # each carrying its own offset, sent in ascending order of the offsets + multi_due = reminder_booking(id: 17, starts_in: 40.seconds, email: "multi@org.com") + system(:StaffAPI_1).as(StaffAPIMock).set_reminders([multi_due].map { |booking| JSON.parse(booking.to_json) }) + settings(reminder_settings.merge({reminders_before_booking: [60, 45]})) + exec(:send_booking_reminders).get + + system(:Mailer)[:reminders].should eq 5 + system(:Mailer)[:to].should eq "multi@org.com" + system(:Mailer)[:offsets].as_a.last(2).map(&.as_i).should eq [45, 60] + status[:reminders_sent_count].should eq 5 + + # a locker booking is queried as a locker booking and reminded at the + # owner's address with the locker specific template name + locker_due = reminder_booking(id: 18, starts_in: 45.seconds, email: "locker@org.com", type: "locker", asset_id: "lock-1") + system(:StaffAPI_1).as(StaffAPIMock).set_reminders([locker_due].map { |booking| JSON.parse(booking.to_json) }) + settings(reminder_settings.merge({ + booking_type: "locker", + unique_templates: true, + reminders_before_booking: [60], + })) + exec(:send_booking_reminders).get + + system(:StaffAPI)[:last_type].should eq "locker" + system(:Mailer)[:reminders].should eq 6 + system(:Mailer)[:template].should eq ["bookings", "booking_reminder_locker"] + system(:Mailer)[:to].should eq "locker@org.com" + system(:Mailer)[:offsets].as_a.last.as_i.should eq 60 + status[:reminders_sent_count].should eq 6 end # :nodoc: @@ -120,6 +213,7 @@ class MailerMock < DriverSpecs::MockDriver include PlaceOS::Driver::Interface::Mailer @reminders : Int32 = 0 + @offsets : Array(Int64) = [] of Int64 # need this for the interface def send_mail( @@ -152,10 +246,15 @@ class MailerMock < DriverSpecs::MockDriver self[:template] = template self[:to] = to self[:reply_to] = reply_to + self[:args] = args + self[:attachments] = attachments if template[1].starts_with?("booking_reminder") @reminders += 1 self[:reminders] = @reminders + offset = args["reminder_offset_minutes"]? + @offsets << offset.as(Int64) if offset.is_a?(Int64) + self[:offsets] = @offsets end end end @@ -169,6 +268,14 @@ class CalendarMock < DriverSpecs::MockDriver end end +# :nodoc: +class LocationServicesMock < DriverSpecs::MockDriver + # the locker asset name lookup resolves the building through locations + def building_id : String + "zone-building" + end +end + # :nodoc: class StaffAPIMock < DriverSpecs::MockDriver @called : Int32 = 0 @@ -195,6 +302,7 @@ class StaffAPIMock < DriverSpecs::MockDriver ) logger.debug { "Querying desk bookings!" } + self[:last_type] = type @called += 1 self[:queries] = @called @@ -231,4 +339,10 @@ class StaffAPIMock < DriverSpecs::MockDriver self[:booking_state] = "#{booking_id}--#{state}" true end + + # no lockers are configured for these tests, so the locker asset name lookup + # falls back to the asset id just as it does for desks + def metadata(id : String, key : String? = nil) + Hash(String, JSON::Any).new + end end From e4ae33c3b71f9322d9e952e15d343242e95a9bdf Mon Sep 17 00:00:00 2001 From: Stephen von Takach Date: Wed, 14 Oct 2026 00:04:51 +1100 Subject: [PATCH 3/3] fix(place/booking-notifier): reminder offsets are minutes, harden sent ledger - offsets and grace window were compared against seconds, so a 60 minute reminder only fired within 60 seconds of the booking start - serialise sweeps with their own mutex and load the ledger inside it, so overlapping sweeps cannot send the same reminder twice - a zone query failure no longer aborts the sweep; the ledger is always persisted and only pruned when every zone was queried - key the ledger on the booking start so rescheduled bookings are reminded about their new time Co-Authored-By: Claude Opus 5.5 --- drivers/place/booking_notifier.cr | 60 +++++++++++++++------- drivers/place/booking_notifier_readme.md | 7 +-- drivers/place/booking_notifier_spec.cr | 65 +++++++++++++++++------- 3 files changed, 94 insertions(+), 38 deletions(-) diff --git a/drivers/place/booking_notifier.cr b/drivers/place/booking_notifier.cr index f093d3dd1a..106f122f7a 100644 --- a/drivers/place/booking_notifier.cr +++ b/drivers/place/booking_notifier.cr @@ -584,6 +584,8 @@ class Place::BookingNotifier < PlaceOS::Driver end end + @reminders_mutex = Mutex.new + # Emails a reminder to the booking owner for every configured offset that the # booking is currently inside of. Scheduled by on_update, and can be invoked # directly to check for reminders without waiting for the schedule. @@ -592,15 +594,24 @@ class Place::BookingNotifier < PlaceOS::Driver offsets = @reminders_before_booking return if offsets.empty? - now = Time.utc - now_unix = now.to_unix - query_end = (now + (offsets.max + @reminder_grace_minutes).minutes).to_unix + # serialise sweeps (cron, the immediate sweep after a settings change and + # manual execs) so each one sees the reminders the previous one sent + @reminders_mutex.synchronize { sweep_booking_reminders(offsets) } + end + + protected def sweep_booking_reminders(offsets : Array(Int32)) + now_unix = Time.utc.to_unix + grace_seconds = @reminder_grace_minutes.to_i64 * 60 + query_end = now_unix + offsets.max.to_i64 * 60 + grace_seconds sent = load_sent_reminders upcoming = Set(String).new sent_this_sweep = 0_u64 + # entries are only forgotten when every zone was queried successfully, + # otherwise a failed zone would look like its bookings were cancelled + all_zones_queried = true - @check_bookings_mutex.synchronize do + begin @notify_lookup.each do |building_zone, details| # a zone that never emails the booking owner gets no reminders either next unless details.notify_booking_owner @@ -609,26 +620,37 @@ class Place::BookingNotifier < PlaceOS::Driver attach = attachments.first? attachments.clear if @disable_attachments - query_reminder_bookings(building_zone, now_unix, query_end).each do |booking_details| + begin + bookings = query_reminder_bookings(building_zone, now_unix, query_end) + rescue error + all_zones_queried = false + logger.error(exception: error) { "while querying reminder bookings in #{building_zone}" } + next + end + + bookings.each do |booking_details| # already on site, so there is nothing left to remind them about next if booking_details.checked_in || booking_details.user_email.presence.nil? - booking_key = "#{booking_details.id}.#{booking_details.instance || 0}" + # the start time is part of the key so a rescheduled booking is + # reminded about its new time + booking_key = "#{booking_details.id}.#{booking_details.instance || 0}.#{booking_details.booking_start}" upcoming << booking_key + seconds_before = booking_details.booking_start - now_unix offsets.each do |offset| - seconds_before = booking_details.booking_start - now_unix + offset_seconds = offset.to_i64 * 60 # the booking has started, or this offset is still in the future - next unless seconds_before > 0 && seconds_before <= offset + next unless seconds_before > 0 && seconds_before <= offset_seconds # the offset passed without this driver running - sending it now # would be a stale reminder - next if seconds_before <= offset - @reminder_grace_minutes + next if seconds_before <= offset_seconds - grace_seconds # booked after the reminder was meant to go out if created = booking_details.created - next if created > booking_details.booking_start - offset + next if created > booking_details.booking_start - offset_seconds end next if sent[booking_key]?.try(&.includes?(offset)) @@ -652,15 +674,17 @@ class Place::BookingNotifier < PlaceOS::Driver end end end + ensure + # forget reminders for bookings that are no longer upcoming (cancelled, + # rescheduled or started). Always persisted, so reminders that went out + # before an error are not sent again by the next sweep + sent.select! { |booking_key, _| upcoming.includes?(booking_key) } if all_zones_queried + self[:reminders_sent] = sent + + @reminders_sent_count += sent_this_sweep + self[:reminders_sent_count] = @reminders_sent_count + logger.debug { "sent #{sent_this_sweep} booking reminders, #{sent.size} bookings tracked" } end - - # forget reminders for bookings that are no longer upcoming (cancelled, or started) - sent.select! { |booking_key, _| upcoming.includes?(booking_key) } - self[:reminders_sent] = sent - - @reminders_sent_count += sent_this_sweep - self[:reminders_sent_count] = @reminders_sent_count - logger.debug { "sent #{sent_this_sweep} booking reminders, #{sent.size} reminders tracked" } rescue error logger.error { error.inspect_with_backtrace } self[:last_error] = { diff --git a/drivers/place/booking_notifier_readme.md b/drivers/place/booking_notifier_readme.md index 9b64e220a3..6d0e1ec111 100644 --- a/drivers/place/booking_notifier_readme.md +++ b/drivers/place/booking_notifier_readme.md @@ -101,9 +101,10 @@ cron entry cannot express "3 days before a booking that starts on Thursday". so each type can have its own template. * Already checked-in bookings are skipped, as are bookings created after their reminder time had passed (booking them would not have triggered a reminder). -* Each reminder is sent once per booking. The sent state is stored in the - driver status under `reminders_sent`, so a restart of the driver does not - re-send reminders. +* Each reminder is sent once per booking start time. The sent state is stored + in the driver status under `reminders_sent`, so a restart of the driver does + not re-send reminders. A booking moved to a new start time is reminded again + about the new time. * The schedule sweeps immediately when the driver starts or its settings change, then on the cron interval. A restart therefore checks for due reminders straight away instead of waiting up to `reminder_schedule`. diff --git a/drivers/place/booking_notifier_spec.cr b/drivers/place/booking_notifier_spec.cr index 8980646e08..e8ab18440f 100644 --- a/drivers/place/booking_notifier_spec.cr +++ b/drivers/place/booking_notifier_spec.cr @@ -68,24 +68,27 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do }, } - # starts inside the 60 minute offset (45 seconds before the start, the grace - # window is 30 minutes) => reminder due - due = reminder_booking(id: 11, starts_in: 45.seconds, email: "due@org.com") + # starts 45 minutes from now, inside the 60 minute offset and its 30 minute + # grace window => reminder due + due = reminder_booking(id: 11, starts_in: 45.minutes, email: "due@org.com") # starts outside the 60 minute offset => no reminder yet - not_yet_due = reminder_booking(id: 12, starts_in: 2.hours, email: "later@org.com") - # booked 10 seconds ago for a start 45 seconds away, so the 60 minute + not_yet_due = reminder_booking(id: 12, starts_in: 100.minutes, email: "later@org.com") + # booked 10 minutes ago for a start 45 minutes away, so the 60 minute # reminder moment had already passed when it was booked => skipped booked_too_late = reminder_booking( id: 13, - starts_in: 45.seconds, + starts_in: 45.minutes, email: "late@org.com", - created: Time.utc - 10.seconds + created: Time.utc - 10.minutes ) # already on site => skipped - on_site = reminder_booking(id: 14, starts_in: 45.seconds, email: "onsite@org.com", checked_in: true) + on_site = reminder_booking(id: 14, starts_in: 45.minutes, email: "onsite@org.com", checked_in: true) + # the 60 minute offset passed 40 minutes ago, beyond the 30 minute grace + # window (i.e. the driver was offline) => skipped rather than sent late + stale = reminder_booking(id: 19, starts_in: 20.minutes, email: "stale@org.com") system(:StaffAPI_1).as(StaffAPIMock).set_reminders( - [due, not_yet_due, booked_too_late, on_site].map { |booking| JSON.parse(booking.to_json) } + [due, not_yet_due, booked_too_late, on_site, stale].map { |booking| JSON.parse(booking.to_json) } ) # arming the schedule in settings sweeps immediately, so the reminders due @@ -121,8 +124,8 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do status[:reminders_sent_count].should eq 1 # moving the offset out to 2 hours catches the booking that wasn't due yet, - # and the booking that was skipped is still skipped - settings(reminder_settings.merge({reminders_before_booking: [7200]})) + # and the bookings that were skipped are still skipped + settings(reminder_settings.merge({reminders_before_booking: [120]})) exec(:send_booking_reminders).get system(:Mailer)[:reminders].should eq 2 system(:Mailer)[:to].should eq "later@org.com" @@ -130,7 +133,7 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do status[:reminders_sent_count].should eq 2 # a zone that does not notify the booking owner gets no reminders either - ownerless_due = reminder_booking(id: 15, starts_in: 45.seconds, email: "no-flag@org.com") + ownerless_due = reminder_booking(id: 15, starts_in: 45.minutes, email: "no-flag@org.com") system(:StaffAPI_1).as(StaffAPIMock).set_reminders([ownerless_due].map { |booking| JSON.parse(booking.to_json) }) settings(reminder_settings.merge({ notify: {zone_id1: {name: "Sydney Building 1", email: ["concierge@place.com"]}}, @@ -143,7 +146,7 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do # host's address, using the booking type specific template name visitor_due = reminder_booking( id: 16, - starts_in: 45.seconds, + starts_in: 45.minutes, email: "host@org.com", type: "visitor", asset_id: "guest@visitor.com" @@ -179,7 +182,7 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do # two offsets open at the same time on one booking => one reminder each, # each carrying its own offset, sent in ascending order of the offsets - multi_due = reminder_booking(id: 17, starts_in: 40.seconds, email: "multi@org.com") + multi_due = reminder_booking(id: 17, starts_in: 40.minutes, email: "multi@org.com") system(:StaffAPI_1).as(StaffAPIMock).set_reminders([multi_due].map { |booking| JSON.parse(booking.to_json) }) settings(reminder_settings.merge({reminders_before_booking: [60, 45]})) exec(:send_booking_reminders).get @@ -191,7 +194,7 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do # a locker booking is queried as a locker booking and reminded at the # owner's address with the locker specific template name - locker_due = reminder_booking(id: 18, starts_in: 45.seconds, email: "locker@org.com", type: "locker", asset_id: "lock-1") + locker_due = reminder_booking(id: 18, starts_in: 45.minutes, email: "locker@org.com", type: "locker", asset_id: "lock-1") system(:StaffAPI_1).as(StaffAPIMock).set_reminders([locker_due].map { |booking| JSON.parse(booking.to_json) }) settings(reminder_settings.merge({ booking_type: "locker", @@ -206,6 +209,33 @@ DriverSpecs.mock_driver "Place::BookingNotifier" do system(:Mailer)[:to].should eq "locker@org.com" system(:Mailer)[:offsets].as_a.last.as_i.should eq 60 status[:reminders_sent_count].should eq 6 + + # the locker booking moved to a new start time => reminded about the new time + locker_moved = reminder_booking(id: 18, starts_in: 50.minutes, email: "locker@org.com", type: "locker", asset_id: "lock-1") + system(:StaffAPI_1).as(StaffAPIMock).set_reminders([locker_moved].map { |booking| JSON.parse(booking.to_json) }) + exec(:send_booking_reminders).get + system(:Mailer)[:reminders].should eq 7 + system(:Mailer)[:args].as_h["starting_unix"].as_i64.should eq locker_moved[:booking_start] + exec(:send_booking_reminders).get + system(:Mailer)[:reminders].should eq 7 + + # a zone whose bookings cannot be queried does not stop the other zones + # being reminded, and the reminders sent are remembered for the next sweep + failing_due = reminder_booking(id: 20, starts_in: 45.minutes, email: "failing@org.com") + system(:StaffAPI_1).as(StaffAPIMock).set_reminders([failing_due].map { |booking| JSON.parse(booking.to_json) }) + failing_zones = reminder_settings.merge({ + notify: { + zone_id1: {name: "Sydney Building 1", notify_booking_owner: true}, + zone_offline: {name: "Offline Building", notify_booking_owner: true}, + }, + }) + settings(failing_zones) + exec(:send_booking_reminders).get + system(:Mailer)[:reminders].should eq 8 + system(:Mailer)[:to].should eq "failing@org.com" + status[:reminders_sent].as_h.keys.should contain "20.0.#{failing_due[:booking_start]}" + exec(:send_booking_reminders).get + system(:Mailer)[:reminders].should eq 8 end # :nodoc: @@ -307,9 +337,10 @@ class StaffAPIMock < DriverSpecs::MockDriver self[:queries] = @called # a reminder sweep queries each zone twice (approved and not yet approved), - # so the fixtures come back on the first query of each pair + # so the fixtures come back on the not yet approved query unless @reminders.empty? - return @reminders if @called.odd? + raise "staff api unavailable for #{zones}" if zones.includes?("zone_offline") + return @reminders if approved == false return [] of JSON::Any end