From 8d3e8e91fd531d435537981be085073d21663155 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Tue, 4 Aug 2026 15:27:52 +0930 Subject: [PATCH 1/8] fix(visitor_mailer): skip change emails for visitors added by the same edit (PPT-2375) --- drivers/place/visitor_mailer.cr | 442 +++++++++++++++++------- drivers/place/visitor_mailer_readme.md | 49 ++- drivers/place/visitor_mailer_spec.cr | 458 ++++++++++++++++++++++--- 3 files changed, 764 insertions(+), 185 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index e31fe762c3..42e4fa2414 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -34,14 +34,19 @@ class Place::VisitorMailer < PlaceOS::Driver notify_induction_accepted_template: "induction_accepted", notify_induction_declined_template: "induction_declined", notify_original_host_template: "notify_original_host", - # sent to all visitors when details change (date, time, location, etc.): - # bookings (desk/resource) use booking_changed, calendar events (rooms) use event_changed + # sent to the existing visitors when details change (date, time, location, + # etc.): bookings (desk/resource) use booking_changed, calendar events + # (rooms) use event_changed. Visitors added by the same edit are left out — + # their invitation already carries the new details. booking_changed_template: "booking_changed", event_changed_template: "event_changed", group_event_template: "group_event", # Combine duplicate change emails sent within this many seconds; 0 disables. - event_change_debounce: 15, + event_change_debounce: 15, + # As above for bookings. This also buys the window needed to notice that a + # visitor was added by the same edit, so 0 will re-notify new visitors. + booking_change_debounce: 15, disable_qr_code: false, send_network_credentials: false, network_password_length: DEFAULT_PASSWORD_LENGTH, @@ -167,12 +172,20 @@ class Place::VisitorMailer < PlaceOS::Driver @skip_host_email : Bool = true @skip_internal_domain_email : Bool = false - # Coalescing buffer for staff/event/changed, swept once the window elapses. - # seconds to buffer a change; 0 emails on every signal + # Coalescing buffer for staff/{event,booking}/changed, swept once the window + # elapses. seconds to buffer a change; 0 emails on every signal @event_change_debounce : Int32 = 15 - # ical_uid (or event_id) => coalesced change awaiting its flush - @pending_event_changes : Hash(String, PendingEventChange) = {} of String => PendingEventChange - @pending_event_changes_lock : Mutex = Mutex.new + @booking_change_debounce : Int32 = 15 + # buffer_key => coalesced change awaiting its flush + @pending_changes : Hash(String, PendingChange) = {} of String => PendingChange + @pending_changes_lock : Mutex = Mutex.new + + # Visitors invited within the debounce window, so a change notification for + # the same visit can leave them out: the invitation they are receiving already + # carries the new details, and they never saw the old ones. + # invite_key => expires (monotonic) + @recent_invites : Hash(String, Time::Span) = {} of String => Time::Span + @recent_invites_lock : Mutex = Mutex.new @uri : URI = URI.new @jwt_private_key : String = PlaceOS::Model::JWTBase.private_key @@ -194,6 +207,7 @@ class Place::VisitorMailer < PlaceOS::Driver @event_changed_template = setting?(String, :event_changed_template) || "event_changed" @group_event_template = setting?(String, :group_event_template) || "group_event" @event_change_debounce = setting?(Int32, :event_change_debounce) || 15 + @booking_change_debounce = setting?(Int32, :booking_change_debounce) || 15 @disable_qr_code = setting?(Bool, :disable_qr_code) || false @determine_host_name_using = setting?(String, :determine_host_name_using) || "calendar-driver" @send_network_credentials = setting?(Bool, :send_network_credentials) || false @@ -226,16 +240,22 @@ class Place::VisitorMailer < PlaceOS::Driver zones = control_system_zone_list - # The sweep below picks up the rest; with the debounce off nothing would. - flush_event_changes("debounce disabled") if @event_change_debounce <= 0 + # Each buffered change carries the debounce it was accepted under, so a + # sweep still drains entries left over from the previous settings. + debounces = [@event_change_debounce, @booking_change_debounce].select(&.positive?) schedule.clear if reminders = @send_reminders schedule.cron(reminders, @time_zone) { send_reminder_emails } end - # Sweeps at most every 5s, so a change waits its debounce plus up to one interval. - schedule.every(@event_change_debounce.clamp(1, 5).seconds) { sweep_event_changes } if @event_change_debounce > 0 + if interval = debounces.min? + # Sweeps at most every 5s, so a change waits its debounce plus up to one interval. + schedule.every(interval.clamp(1, 5).seconds) { sweep_pending_changes } + else + # Nothing would sweep the buffer with every debounce switched off. + flush_pending_changes("debounce disabled") + end spawn { ensure_building_zone(zones) } end @@ -243,7 +263,7 @@ class Place::VisitorMailer < PlaceOS::Driver # The scheduler is dead by now, so nothing else would sweep the buffer. # Bounded to return within the driver manager's 6s unload budget. def on_unload - flush_event_changes("driver unloading", wait: 5.seconds) + flush_pending_changes("driver unloading", wait: 5.seconds) end def control_system_zone_list @@ -308,6 +328,14 @@ class Place::VisitorMailer < PlaceOS::Driver end end + # An invitation is our only notice that a visitor has just been added to a + # visit: staff-api signals attendance solely for attendees that weren't + # already attending. Recorded ahead of the filters below so a visitor whose + # invite email is suppressed (disable_event_visitors, + # skip_event_linked_booking_email) still counts as newly invited — they were + # invited, just via the other template. + record_invite(guest_details) if guest_details.is_a?(EventGuest) || guest_details.is_a?(BookingGuest) + # don't email staff members if !@host_domain_filter.empty? && guest_details.attendee_email.split('@', 2)[1].downcase.in?(@host_domain_filter) logger.debug { "ignoring event matches host domain filter" } @@ -397,18 +425,29 @@ class Place::VisitorMailer < PlaceOS::Driver return end - send_visitor_qr_email( - template, - guest_details.attendee_email, - guest_details.attendee_name, - guest_details.host, - guest_details.event_title || guest_details.event_summary, - guest_details.event_starting, - guest_details.resource_id, - guest_details.event_id, - area_name, - system_id: guest_details.responds_to?(:system_id) ? guest_details.system_id : nil, - ) + logger.debug { "emailing the #{template} invite to #{guest_details.attendee_email}" } + + begin + send_visitor_qr_email( + template, + guest_details.attendee_email, + guest_details.attendee_name, + guest_details.host, + guest_details.event_title || guest_details.event_summary, + guest_details.event_starting, + guest_details.resource_id, + guest_details.event_id, + area_name, + system_id: guest_details.responds_to?(:system_id) ? guest_details.system_id : nil, + ) + rescue error + # counted separately from error_count so a missing invite can be told + # apart from a failure anywhere else in this handler + self[:visitor_email_errors] = @visitor_email_errors += 1 + raise error + end + + self[:visitor_emails_sent] = @visitor_emails_sent += 1 rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -639,13 +678,13 @@ class Place::VisitorMailer < PlaceOS::Driver TemplateFields.new( trigger: {"visitor_invited", @booking_changed_template}, name: "Booking details changed notification", - description: "Notification sent to all visitors on a booking (desk/resource) when details change (date, time, etc.)", + description: "Notification sent to the existing visitors on a booking (desk/resource) when details change (date, time, etc.). Visitors added by the same edit are sent their invitation instead", fields: changed_fields ), TemplateFields.new( trigger: {"visitor_invited", @event_changed_template}, name: "Event details changed notification", - description: "Notification sent to all visitors on a calendar event (room) when details change (date, time, location, etc.)", + description: "Notification sent to the existing visitors on a calendar event (room) when details change (date, time, location, etc.). Visitors added by the same edit are sent their invitation instead", fields: changed_fields ), ] @@ -698,46 +737,17 @@ class Place::VisitorMailer < PlaceOS::Driver return unless fields_changed - # Resolve previous location names from previous zones - previous_building_name = building_zone.display_name.presence || building_zone.name - previous_room_name = @booking_space_name - - if prev_zones = details.previous_zones - found_building = false - found_room = false - prev_zones.each do |zone_id| - break if found_building && found_room - begin - zone = fetch_zone(zone_id) - if zone.tags.includes?(@invite_zone_tag) - previous_building_name = zone.display_name.presence || zone.name - found_building = true - else - previous_room_name = zone.display_name.presence || zone.name - found_room = true - end - rescue error - logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } - end - end - end - - # include_linked: true ensures guests from child bookings (e.g. per-visitor - # bookings under a group parent) are returned in a single request. - guests = staff_api.booking_guests(details.id, include_linked: details.booking_type == "group").get.as_a - - send_booking_changed_emails( - guests, - @booking_changed_template, - details.user_email, - details.booking_start, - details.title, - details.previous_booking_start, - previous_building_name, - previous_room_name, - event_id: details.id.to_s, - resource_id: details.resource_id, + # Buffer rather than send now: a group visitor edit updates the parent + # booking before adding this edit's new visitors in later requests, so the + # debounce is what lets us recognise those visitors and leave them out. + change = PendingBookingChange.new( + @booking_change_debounce, + details.id, details.booking_type, details.user_email, details.title, + details.resource_id, details.booking_start, details.booking_end, + details.previous_booking_start, details.previous_booking_end, + details.zones, details.previous_zones, ) + @booking_change_debounce > 0 ? buffer_change(change) : dispatch_booking_change(change) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -812,11 +822,12 @@ class Place::VisitorMailer < PlaceOS::Driver # Coalesce the burst of signals Office365 emits per edit into one email. change = PendingEventChange.new( + @event_change_debounce, details.event_id, details.system_id, details.event_ical_uid, host, details.title, event_start, event_end, details.previous_event_start, details.previous_event_end, details.previous_system_id, ) - @event_change_debounce > 0 ? buffer_event_change(change) : dispatch_event_change(change) + @event_change_debounce > 0 ? buffer_change(change) : dispatch_event_change(change) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -836,52 +847,102 @@ class Place::VisitorMailer < PlaceOS::Driver attendee_domain.downcase == host_domain.downcase end + # Remembers that a visitor was just invited, so a change notification for the + # same visit can leave them out. Expired entries are dropped on the way in, + # which keeps the map to a couple of minutes' worth of invites even with every + # debounce off (and so no sweep running to prune it). + protected def record_invite(guest_details : GuestNotification) : Nil + key = invite_key(guest_details.attendee_email, guest_details.host, guest_details.event_starting) + now = Time.monotonic + + @recent_invites_lock.synchronize do + @recent_invites.reject! { |_key, expires| expires <= now } + @recent_invites[key] = now + invite_memory + end + + logger.debug { "noted #{guest_details.attendee_email} as newly invited by #{guest_details.host}" } + end + + # Whether this visitor was invited to this visit within the memory window. + protected def recently_invited?(visitor_email : String, host_email : String, event_start : Int64) : Bool + key = invite_key(visitor_email, host_email, event_start) + now = Time.monotonic + + @recent_invites_lock.synchronize do + if expires = @recent_invites[key]? + next true if expires > now + @recent_invites.delete(key) + end + false + end + end + + # A change notification names the parent booking while the invitation names the + # visitor's own child booking, and for an event-linked visitor booking the ids + # don't correspond at all. Both do always describe the same visitor attending + # the same host's visit at the same (new) start time, so key on that instead. + private def invite_key(visitor_email : String, host_email : String?, event_start : Int64) : String + "#{visitor_email.strip.downcase}\t#{host_email.to_s.strip.downcase}\t#{event_start}" + end + + # Long enough to cover the debounce holding a change notification back, plus + # room for a front end that adds its visitors in requests which follow the one + # that made the change. + private def invite_memory : Time::Span + {@event_change_debounce, @booking_change_debounce}.max.clamp(0, 3600).seconds + 60.seconds + end + # Collapses the burst of signals for one edit into a single buffered change. - # Keyed by event instance, so the rooms either side of a move coalesce too; - # the one email then names a single room and uses that room's guest list. - private def buffer_event_change(change : PendingEventChange) : Nil - @pending_event_changes_lock.synchronize do - if pending = @pending_event_changes[change.buffer_key]? + # Events are keyed by instance, so the rooms either side of a move coalesce + # too; the one email then names a single room and uses that room's guest list. + private def buffer_change(change : PendingChange) : Nil + @pending_changes_lock.synchronize do + if pending = @pending_changes[change.buffer_key]? pending.merge(change) else - @pending_event_changes[change.buffer_key] = change + @pending_changes[change.buffer_key] = change end end end # Sends any change that has been buffered for its full debounce window. - private def sweep_event_changes : Nil - flush_event_changes("debounce window elapsed", older_than: Time.monotonic - @event_change_debounce.seconds) + private def sweep_pending_changes : Nil + flush_pending_changes("debounce window elapsed", ready_only: true) end - # Dispatches matching changes, each in its own fiber so a slow send can't stall - # the sweep. `older_than` limits the flush to entries buffered before that point - # (nil takes the lot), `wait` bounds how long we block for the sends to finish. - private def flush_event_changes(reason : String, older_than : Time::Span? = nil, wait : Time::Span? = nil) : Nil - flushing = @pending_event_changes_lock.synchronize do - ready = if cutoff = older_than - @pending_event_changes.values.select { |pending| pending.first_seen <= cutoff } + # Dispatches buffered changes, each in its own fiber so a slow send can't stall + # the sweep. `ready_only` limits the flush to entries that have served their + # debounce, `wait` bounds how long we block for the sends to finish. + private def flush_pending_changes(reason : String, ready_only : Bool = false, wait : Time::Span? = nil) : Nil + flushing = @pending_changes_lock.synchronize do + now = Time.monotonic + ready = if ready_only + @pending_changes.values.select(&.ready?(now)) else - @pending_event_changes.values + @pending_changes.values end - ready.each { |pending| @pending_event_changes.delete(pending.buffer_key) } + ready.each { |pending| @pending_changes.delete(pending.buffer_key) } ready end return if flushing.empty? - logger.debug { "flushing #{flushing.size} pending event change(s): #{reason}" } + logger.debug { "flushing #{flushing.size} pending change(s): #{reason}" } complete = Channel(Nil).new(flushing.size) flushing.each do |pending| spawn do - dispatch_event_change(pending) + case pending + in PendingEventChange then dispatch_event_change(pending) + in PendingBookingChange then dispatch_booking_change(pending) + in PendingChange then logger.error { "no dispatcher for pending change #{pending.buffer_key}" } + end rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 self[:last_error] = { error: error.message, time: Time.local.to_s, - user: "flushing event change #{pending.event_id}: #{reason}", + user: "flushing change #{pending.buffer_key}: #{reason}", } ensure complete.send(nil) @@ -897,23 +958,68 @@ class Place::VisitorMailer < PlaceOS::Driver select when complete.receive when timeout(remaining) - logger.warn { "timeout flushing pending event changes: #{reason}, #{flushing.size - index} of #{flushing.size} still in flight" } + logger.warn { "timeout flushing pending changes: #{reason}, #{flushing.size - index} of #{flushing.size} still in flight" } break end end end - # Resolves locations, fetches guests and emails visitors about a change. + # Resolves locations, fetches guests and emails visitors about a booking + # change. Shared by the immediate and debounced paths. + private def dispatch_booking_change(change : PendingBookingChange) + # Skip a coalesced no-op (e.g. an edit that was undone within the window). + return unless change.changed? + + # Resolve previous location names from previous zones + previous_building_name = building_zone.display_name.presence || building_zone.name + previous_room_name = @booking_space_name + + if prev_zones = change.previous_zones + found_building = false + found_room = false + prev_zones.each do |zone_id| + break if found_building && found_room + begin + zone = fetch_zone(zone_id) + if zone.tags.includes?(@invite_zone_tag) + previous_building_name = zone.display_name.presence || zone.name + found_building = true + else + previous_room_name = zone.display_name.presence || zone.name + found_room = true + end + rescue error + logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } + end + end + end + + # include_linked: true ensures guests from child bookings (e.g. per-visitor + # bookings under a group parent) are returned in a single request. + guests = staff_api.booking_guests(change.booking_id, include_linked: change.booking_type == "group").get.as_a + + send_booking_changed_emails( + guests, + @booking_changed_template, + change.host, + change.current_start, + change.title, + change.previous_start, + previous_building_name, + previous_room_name, + event_id: change.booking_id.to_s, + resource_id: change.resource_id, + ) + end + + # Resolves locations, fetches guests and emails visitors about an event change. # Shared by the immediate and debounced paths. private def dispatch_event_change(change : PendingEventChange) system_id = change.system_id previous_system_id = change.previous_system_id # Skip a coalesced no-op (e.g. an A->B->A flip-flop that nets to no change). - changed = change.moved_room? - changed = true if (previous_start = change.previous_start) && previous_start != change.current_start - changed = true if (previous_end = change.previous_end) && previous_end != change.current_end - return unless changed + return unless change.changed? current_building_name = building_zone.display_name.presence || building_zone.name current_room_name = @booking_space_name @@ -988,6 +1094,14 @@ class Place::VisitorMailer < PlaceOS::Driver # don't treat the host's colleagues as visitors next if @skip_internal_domain_email && colleague_of_host?(visitor_email, host_email) + # don't tell a visitor added by this very edit that their visit changed — + # the invitation they are receiving already carries these details, and + # there is nothing they knew of to have changed (PPT-2375) + if recently_invited?(visitor_email, host_email, event_start) + logger.debug { "skipping #{template} email to #{visitor_email} as they were just invited" } + next + end + local_start_time = Time.unix(event_start).in(@time_zone) previous_date = previous_start.try { |timestamp| Time.unix(timestamp).in(@time_zone).to_s(@date_format) } @@ -1242,39 +1356,77 @@ class Place::VisitorMailer < PlaceOS::Driver property parent_id : String? end - # A staff/event/changed change buffered awaiting a debounced flush. - # `current_*` follow the latest signal in the burst, `previous_*` and - # `first_seen` stay as they were when it started, so the email describes the - # net change of the whole edit. - class PendingEventChange - property event_id : String - property system_id : String # the room the event sits in - property event_ical_uid : String? + # A change buffered awaiting a debounced flush. `current_*` follow the latest + # signal in the burst, `previous_*` and `first_seen` stay as they were when it + # started, so the email describes the net change of the whole edit. + # + # Buffer keys are namespaced per subclass, so the two kinds of change can never + # collide even when a booking id and an event id coincide. + abstract class PendingChange property host : String property title : String? property current_start : Int64 property current_end : Int64 property previous_start : Int64? property previous_end : Int64? - property previous_system_id : String? # the room before the edit + getter first_seen : Time::Span = Time.monotonic - # ical_uid identifies the event instance across mailbox copies and rooms; - # event_id is only a fallback for a signal that omits it. - getter buffer_key : String + getter buffer_key : String = "" + # The debounce this change was accepted under, kept per entry so a sweep + # still drains anything left over from the previous settings. + getter debounce : Time::Span = Time::Span.zero + + def initialize(debounce_seconds : Int32, @host, @title, @current_start, @current_end, @previous_start, @previous_end) + @debounce = debounce_seconds.clamp(0, 3600).seconds + end + + # Whether this change has served its full debounce window. + def ready?(now : Time::Span) : Bool + (first_seen + debounce) <= now + end + + # Whether the coalesced result still describes a real change. + def changed? : Bool + return true if (previous = previous_start) && previous != current_start + return true if (previous = previous_end) && previous != current_end + false + end + + # Advance to the latest signal in the burst. + def merge(change : PendingChange) : Nil + @host = change.host + @title = change.title + @current_start = change.current_start + @current_end = change.current_end + end + end + + # A staff/event/changed change awaiting its flush. + class PendingEventChange < PendingChange + property event_id : String + property system_id : String # the room the event sits in + property event_ical_uid : String? + property previous_system_id : String? # the room before the edit def initialize( + debounce_seconds : Int32, @event_id, @system_id, @event_ical_uid, - @host, - @title, - @current_start, - @current_end, - @previous_start, - @previous_end, + host, + title, + current_start, + current_end, + previous_start, + previous_end, @previous_system_id, ) - @buffer_key = @event_ical_uid.presence || @event_id + super(debounce_seconds, host, title, current_start, current_end, previous_start, previous_end) + # ical_uid identifies the event instance across mailbox copies and rooms, + # so the rooms either side of a move coalesce too; the one email then names + # a single room and uses that room's guest list. event_id is only a + # fallback for a signal that omits it. + @buffer_key = "event\t#{@event_ical_uid.presence || @event_id}" end # Whether this signal reports the event changing rooms. @@ -1282,20 +1434,70 @@ class Place::VisitorMailer < PlaceOS::Driver !!previous_system_id.try { |previous| previous != system_id } end - # Advance to the latest signal in the burst. The room only moves when a - # signal reports the move, so a same-room echo from another mailbox can't - # steal it back. - def merge(change : PendingEventChange) : Nil + def changed? : Bool + moved_room? || super + end + + # The room only moves when a signal reports the move, so a same-room echo + # from another mailbox can't steal it back. + def merge(change : PendingChange) : Nil + super + return unless change.is_a?(PendingEventChange) + if change.moved_room? @event_id = change.event_id @system_id = change.system_id @previous_system_id ||= change.previous_system_id end @event_ical_uid = change.event_ical_uid || @event_ical_uid - @host = change.host - @title = change.title - @current_start = change.current_start - @current_end = change.current_end + end + end + + # A staff/booking/changed change awaiting its flush. + class PendingBookingChange < PendingChange + property booking_id : Int64 + property booking_type : String + property resource_id : String + property zones : Array(String)? + property previous_zones : Array(String)? + + def initialize( + debounce_seconds : Int32, + @booking_id, + @booking_type, + host, + title, + @resource_id, + current_start, + current_end, + previous_start, + previous_end, + @zones, + @previous_zones, + ) + super(debounce_seconds, host, title, current_start, current_end, previous_start, previous_end) + @buffer_key = "booking\t#{@booking_id}" + end + + # Whether this signal reports the booking changing location. + def moved_zones? : Bool + !!previous_zones.try { |previous| previous.sort != (zones || [] of String).sort } + end + + def changed? : Bool + moved_zones? || super + end + + def merge(change : PendingChange) : Nil + super + return unless change.is_a?(PendingBookingChange) + + @booking_type = change.booking_type + @resource_id = change.resource_id + if change.moved_zones? + @zones = change.zones + @previous_zones ||= change.previous_zones + end end end diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index b4c4d0a17a..d034d6e962 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -29,31 +29,58 @@ Requires the following drivers in the system: skip_internal_domain_email: false ``` -## Debouncing event changes +## Debouncing change notifications -A single calendar edit is rarely a single signal: Office365 emits a burst of +A single edit is rarely a single signal: Office365 emits a burst of `staff/event/changed` updates (the organizer copy, then each room mailbox catching up), which can briefly flip-flop between the old and new values. Sending an email per signal spams visitors with contradictory notifications. -`event_change_debounce` (seconds, default `15`) buffers the burst for one event and -sends a single email describing the net change once the window closes. Set it to `0` -to email on every signal. +`event_change_debounce` and `booking_change_debounce` (seconds, default `15`) buffer +the burst for one visit and send a single email describing the net change once the +window closes. Set one to `0` to email on every signal for that kind of change. ```yaml # Combine duplicate change emails sent within this many seconds; 0 disables. - event_change_debounce: 15 + event_change_debounce: 15 + booking_change_debounce: 15 ``` Buffered changes are swept on a timer rather than each having its own, so the actual delay is the configured debounce plus up to one sweep interval (at most 5s). Anything -still buffered is emailed immediately when the driver is unloaded, or when the +still buffered is emailed immediately when the driver is unloaded, or when every debounce is turned off, so a restart never silently drops a pending notification. -Signals are grouped by event instance (its ical uid), not by room, so an edit that -moves the meeting *and* changes the time sends one email describing both rather than -one per room. A move between buildings is handled by two separate mailer modules and -so still sends an email each. +Event signals are grouped by event instance (its ical uid), not by room, so an edit +that moves the meeting *and* changes the time sends one email describing both rather +than one per room. A move between buildings is handled by two separate mailer modules +and so still sends an email each. Booking signals are grouped by booking id. + +The booking debounce also buys the window needed to recognise a visitor added by the +same edit, so setting it to `0` will re-notify new visitors — see below. + +## New visitors are not told the visit changed + +A visitor added while the details are being changed does not need a change +notification: the invitation they are receiving already carries the new date, time and +location, and they never saw the old ones. Being told their visit "changed" before +they have registered being invited to it at all is worse than confusing — QA saw the +change notification arrive alongside, and sometimes in place of, the invite. + +The staff API announces attendance (`staff/guest/attending`) only for attendees that +were not already attending, which makes it an exact statement of "this visitor is +new". Each such announcement is remembered for the debounce window plus a minute, and +those visitors are left out of any change notification for the same visit in that +time. Recipients are matched on visitor, host and the new start time rather than on +the booking id, because a change notification names the *parent* booking while the +invitation names the visitor's own child booking — and for an event-linked visitor +booking the two ids do not correspond at all. + +This works regardless of where the edit came from (workplace, concierge, Outlook or +another driver), as it depends only on the signals rather than on the request that +caused them. The invitation is remembered even when the invite email itself is +suppressed by `disable_event_visitors` or `skip_event_linked_booking_email`, since in +both cases the visitor was still invited, just through the other template. ## Colleagues are not visitors diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 66c97c2aec..8cba05b51e 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -9,9 +9,14 @@ class MailerMock < DriverSpecs::MockDriver # produced rather than only counting them. @templates_sent : Array(String) = [] of String + # "recipient|template" for each email, so a test can assert exactly who + # received which one rather than only counting. + @emails_sent : Array(String) = [] of String + def on_load self[:send_count] = 0 self[:sent_templates] = @templates_sent + self[:emails_sent] = @emails_sent end def send_template( @@ -32,6 +37,8 @@ class MailerMock < DriverSpecs::MockDriver self[:last_attachments] = resource_attachments @templates_sent << template[1] self[:sent_templates] = @templates_sent + @emails_sent << "#{to.is_a?(Array) ? to.join(',') : to}|#{template[1]}" + self[:emails_sent] = @emails_sent self[:send_count] = self[:send_count].as_i + 1 true end @@ -170,6 +177,12 @@ class StaffAPIMock < DriverSpecs::MockDriver def event_guests(event_id : String, system_id : String, ical_uid : String? = nil) case event_id + when "evt-two-visitors" + # An existing visitor plus one added by the same edit. + [ + {email: "visitor-a@external.com", name: "Visitor A", checked_in: false, visit_expected: true}, + {email: "visitor-b@external.com", name: "Visitor B", checked_in: false, visit_expected: true}, + ] when "evt-host-in-guests" # Mirrors the production scenario where events.cr stores the host # as an attendee (visit_expected: true), so they appear in the @@ -271,6 +284,18 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # Allow on_load -> on_update -> ensure_building_zone to complete sleep 1.5 + # Most tests below assert an immediate send, so both debounces are disabled + # (they default to 15s). The debounce behaviour has dedicated tests that turn + # them back on. send_reminders/domain_uri mirror default_settings so nothing + # else changes. + settings({ + event_change_debounce: 0, + booking_change_debounce: 0, + send_reminders: "0 7 * * *", + domain_uri: "https://example.com/", + }) + sleep 1.0 + # ------------------------------------------------------------------ # Test 1: booking_changed with previous_zones resolves names correctly # ------------------------------------------------------------------ @@ -691,9 +716,10 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # These tests assert an immediate send, so disable the debounce (default 15s). # send_reminders/domain_uri mirror default_settings so nothing else changes. settings({ - event_change_debounce: 0, - send_reminders: "0 7 * * *", - domain_uri: "https://example.com/", + event_change_debounce: 0, + booking_change_debounce: 0, + send_reminders: "0 7 * * *", + domain_uri: "https://example.com/", }) sleep 1.0 @@ -1202,6 +1228,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do invite_zone_tag: "building", skip_event_linked_booking_email: false, event_change_debounce: 0, + booking_change_debounce: 0, }) sleep 1.0 @@ -1404,11 +1431,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - skip_host_email: false, - event_change_debounce: 0, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + skip_host_email: false, + event_change_debounce: 0, + booking_change_debounce: 0, }) sleep 1.0 @@ -1446,6 +1474,8 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do invite_zone_tag: "building", notify_induction_accepted_template: "custom_accepted", notify_induction_declined_template: "custom_declined", + event_change_debounce: 0, + booking_change_debounce: 0, }) sleep 1.0 @@ -1510,6 +1540,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_changed_template: "custom_booking_changed", event_changed_template: "custom_event_changed", event_change_debounce: 0, + booking_change_debounce: 0, }) sleep 1.0 @@ -1581,6 +1612,8 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_space_name: "Client Floor", invite_zone_tag: "building", # skip_event_linked_booking_email defaults to true (invite flow only) + event_change_debounce: 0, + booking_change_debounce: 0, }) sleep 1.0 @@ -1658,6 +1691,8 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_space_name: "Client Floor", invite_zone_tag: "building", skip_event_linked_booking_email: false, + event_change_debounce: 0, + booking_change_debounce: 0, }) sleep 1.0 @@ -1679,9 +1714,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # show a real date, so the driver looks the event up via the staff API. settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + booking_change_debounce: 0, }) sleep 1.0 @@ -1753,10 +1790,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # showing the true net change. settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 3, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 3, + booking_change_debounce: 0, }) sleep 1.0 @@ -1861,10 +1899,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do system(:Mailer)[:send_count].should eq count_before_survives settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 3, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 3, + booking_change_debounce: 0, }) # the update must not cut the window short @@ -2028,10 +2067,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 30, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 30, + booking_change_debounce: 0, }) sleep 1.0 @@ -2060,10 +2100,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # Disabling the debounce flushes the buffer instead of orphaning it. settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + booking_change_debounce: 0, }) sleep 1.5 @@ -2207,11 +2248,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do status[:users_checked_in].should eq checked_in_before_checkout settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + booking_change_debounce: 0, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2325,12 +2367,13 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - disable_qr_code: true, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + booking_change_debounce: 0, + disable_qr_code: true, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2383,12 +2426,13 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - host_domain_filter: ["example.com"], - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + booking_change_debounce: 0, + host_domain_filter: ["example.com"], + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2423,6 +2467,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_space_name: "Client Floor", invite_zone_tag: "building", event_change_debounce: 0, + booking_change_debounce: 0, skip_internal_domain_email: true, domain_uri: "https://example.com/", }) @@ -2500,11 +2545,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + booking_change_debounce: 0, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2548,11 +2594,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + booking_change_debounce: 0, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2582,4 +2629,307 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do system(:Mailer)[:sent_templates].as_a[-2].as_s.should eq "notify_original_host" system(:Mailer)[:last_template].should eq ["visitor_invited", "event_changed"] system(:Mailer)[:last_args]["host_email"].should eq "new-host@example.com" + + # ================================================================== + # A visitor added by the same edit is not told the visit changed + # ================================================================== + # + # PPT-2375. Editing a booking's time and adding a visitor in the one action + # left the new visitor receiving the change notification as well as (and + # sometimes seemingly instead of) their invitation. They never saw the old + # details, and their invitation already carries the new ones. + # + # The change and the invitation arrive as two separate signals, so the + # debounce is what gives the driver a chance to correlate them. Each test + # below uses its own host and start time so the invite memory (which outlives + # the debounce) can't leak between them. + + # each assertion below reads system(:Mailer)[:emails_sent] from the index it + # noted beforehand, giving "recipient|template" for just this test's emails + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + booking_change_debounce: 2, + domain_uri: "https://example.com/", + }) + sleep 1.0 + + # ------------------------------------------------------------------ + # Test 51: the QA reproduction — the group booking is updated first, + # then the new visitor is added in a later request. Booking + # 300 with include_linked returns both visitors. + # ------------------------------------------------------------------ + + sent_before_new_visitor = system(:Mailer)[:emails_sent].as_a.size + + publish("staff/booking/changed", { + action: "changed", + id: 300_i64, + booking_type: "group", + booking_start: now + 7200, + booking_end: now + 10800, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host-late@example.com", + title: "Group Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 3600, + previous_booking_end: now + 7200, + }.to_json) + + # still inside the debounce window, as the front end adds its visitors in the + # requests that follow the one which moved the booking + sleep 0.5 + + publish("staff/guest/attending", { + action: "booking_created", + id: 1_i64, + booking_id: 305_i64, + resource_id: "visitor-b@external.com", + resource_ids: ["visitor-b@external.com"], + event_title: "Group Visit", + event_summary: "Group Visit", + event_starting: now + 7200, + attendee_name: "Visitor B", + attendee_email: "visitor-b@external.com", + host: "host-late@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + # debounce plus up to one sweep interval + sleep 8.0 + + new_visitor_emails = system(:Mailer)[:emails_sent].as_a[sent_before_new_visitor..].map(&.as_s) + + # the visitor who was already coming is told the details changed + new_visitor_emails.should contain "visitor-a@external.com|booking_changed" + # the new visitor gets their invitation + new_visitor_emails.should contain "visitor-b@external.com|booking" + # and is not also told that a visit they only just heard about has changed + new_visitor_emails.should_not contain "visitor-b@external.com|booking_changed" + new_visitor_emails.size.should eq 2 + + # ------------------------------------------------------------------ + # Test 52: the same holds when the invitation lands first (adding the + # visitor before saving the new time) + # ------------------------------------------------------------------ + + sent_before_invite_first = system(:Mailer)[:emails_sent].as_a.size + + publish("staff/guest/attending", { + action: "booking_created", + id: 2_i64, + booking_id: 306_i64, + resource_id: "visitor-b@external.com", + resource_ids: ["visitor-b@external.com"], + event_title: "Group Visit", + event_summary: "Group Visit", + event_starting: now + 14400, + attendee_name: "Visitor B", + attendee_email: "visitor-b@external.com", + host: "host-early@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + publish("staff/booking/changed", { + action: "changed", + id: 300_i64, + booking_type: "group", + booking_start: now + 14400, + booking_end: now + 18000, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host-early@example.com", + title: "Group Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 10800, + previous_booking_end: now + 14400, + }.to_json) + + sleep 8.0 + + invite_first_emails = system(:Mailer)[:emails_sent].as_a[sent_before_invite_first..].map(&.as_s) + + invite_first_emails.should contain "visitor-a@external.com|booking_changed" + invite_first_emails.should contain "visitor-b@external.com|booking" + invite_first_emails.should_not contain "visitor-b@external.com|booking_changed" + invite_first_emails.size.should eq 2 + + # ------------------------------------------------------------------ + # Test 53: with no visitor added, every visitor is still notified + # ------------------------------------------------------------------ + + sent_before_no_invite = system(:Mailer)[:emails_sent].as_a.size + + publish("staff/booking/changed", { + action: "changed", + id: 300_i64, + booking_type: "group", + booking_start: now + 21600, + booking_end: now + 25200, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host-nobody-new@example.com", + title: "Group Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 18000, + previous_booking_end: now + 21600, + }.to_json) + + sleep 8.0 + + no_invite_emails = system(:Mailer)[:emails_sent].as_a[sent_before_no_invite..].map(&.as_s) + + no_invite_emails.should contain "visitor-a@external.com|booking_changed" + no_invite_emails.should contain "visitor-b@external.com|booking_changed" + no_invite_emails.size.should eq 2 + + # ------------------------------------------------------------------ + # Test 54: an invitation to a different visit doesn't suppress this + # one's change notification. Recipients are matched on the + # host and the new start, not on the visitor alone. + # ------------------------------------------------------------------ + + sent_before_other_visit = system(:Mailer)[:emails_sent].as_a.size + + publish("staff/guest/attending", { + action: "booking_created", + id: 3_i64, + booking_id: 307_i64, + resource_id: "visitor-b@external.com", + resource_ids: ["visitor-b@external.com"], + event_title: "An Unrelated Visit", + event_summary: "An Unrelated Visit", + event_starting: now + 90000, + attendee_name: "Visitor B", + attendee_email: "visitor-b@external.com", + host: "someone-else@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + publish("staff/booking/changed", { + action: "changed", + id: 300_i64, + booking_type: "group", + booking_start: now + 28800, + booking_end: now + 32400, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host-unrelated@example.com", + title: "Group Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 25200, + previous_booking_end: now + 28800, + }.to_json) + + sleep 8.0 + + other_visit_emails = system(:Mailer)[:emails_sent].as_a[sent_before_other_visit..].map(&.as_s) + + other_visit_emails.should contain "visitor-b@external.com|booking" + other_visit_emails.should contain "visitor-a@external.com|booking_changed" + other_visit_emails.should contain "visitor-b@external.com|booking_changed" + + # ------------------------------------------------------------------ + # Test 55: a burst of booking/changed signals for one booking is + # coalesced into a single email per visitor + # ------------------------------------------------------------------ + + sent_before_booking_burst = system(:Mailer)[:emails_sent].as_a.size + + 3.times do |index| + publish("staff/booking/changed", { + action: "changed", + id: 300_i64, + booking_type: "group", + booking_start: now + 36000 + index, + booking_end: now + 39600 + index, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host-burst@example.com", + title: "Group Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 32400, + previous_booking_end: now + 36000, + }.to_json) + sleep 0.2 + end + + sleep 8.0 + + booking_burst_emails = system(:Mailer)[:emails_sent].as_a[sent_before_booking_burst..].map(&.as_s) + + booking_burst_emails.size.should eq 2 + # the one email describes the latest values in the burst + system(:Mailer)[:last_args]["event_time"].should eq Time.unix(now + 36002).in(Time::Location.load("GMT")).to_s("%l:%M%p") + + # ------------------------------------------------------------------ + # Test 56: the same exclusion applies to calendar events + # ------------------------------------------------------------------ + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 2, + booking_change_debounce: 0, + disable_event_visitors: false, + domain_uri: "https://example.com/", + }) + sleep 1.0 + + sent_before_event_invite = system(:Mailer)[:emails_sent].as_a.size + + publish("staff/event/changed", { + action: "update", + system_id: "sys-room1", + event_id: "evt-two-visitors", + event_ical_uid: "ical-two-visitors", + host: "host-event@example.com", + resource: "room1@example.com", + title: "Two Visitors", + event_start: now + 46800, + event_end: now + 50400, + zones: ["zone-building", "zone-room"], + previous_event_start: now + 43200, + previous_event_end: now + 46800, + }.to_json) + + sleep 0.5 + + publish("staff/guest/attending", { + action: "meeting_update", + system_id: "sys-room1", + event_id: "evt-two-visitors", + event_ical_uid: "ical-two-visitors", + resource: "room1@example.com", + event_title: "Two Visitors", + event_summary: "Two Visitors", + event_starting: now + 46800, + attendee_name: "Visitor B", + attendee_email: "visitor-b@external.com", + host: "host-event@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 8.0 + + event_invite_emails = system(:Mailer)[:emails_sent].as_a[sent_before_event_invite..].map(&.as_s) + + event_invite_emails.should contain "visitor-a@external.com|event_changed" + event_invite_emails.should contain "visitor-b@external.com|event" + event_invite_emails.should_not contain "visitor-b@external.com|event_changed" + event_invite_emails.size.should eq 2 end From 9dd1bfe5fcd0ff2a76878db2f44ec708fdb33210 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Tue, 4 Aug 2026 15:40:30 +0930 Subject: [PATCH 2/8] fix(visitor_mailer): skip change emails for visitors added by the same edit (PPT-2375) --- drivers/place/visitor_mailer.cr | 73 ++++++++------------ drivers/place/visitor_mailer_readme.md | 92 ++++++++------------------ drivers/place/visitor_mailer_spec.cr | 18 ++--- 3 files changed, 62 insertions(+), 121 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 42e4fa2414..a28ae7acbe 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -34,18 +34,15 @@ class Place::VisitorMailer < PlaceOS::Driver notify_induction_accepted_template: "induction_accepted", notify_induction_declined_template: "induction_declined", notify_original_host_template: "notify_original_host", - # sent to the existing visitors when details change (date, time, location, - # etc.): bookings (desk/resource) use booking_changed, calendar events - # (rooms) use event_changed. Visitors added by the same edit are left out — - # their invitation already carries the new details. + # sent to the existing visitors when details change (date, time, location): + # bookings (desk/resource) use booking_changed, events (rooms) use event_changed booking_changed_template: "booking_changed", event_changed_template: "event_changed", group_event_template: "group_event", # Combine duplicate change emails sent within this many seconds; 0 disables. event_change_debounce: 15, - # As above for bookings. This also buys the window needed to notice that a - # visitor was added by the same edit, so 0 will re-notify new visitors. + # as above for bookings; 0 also notifies visitors added by the same edit booking_change_debounce: 15, disable_qr_code: false, send_network_credentials: false, @@ -180,9 +177,8 @@ class Place::VisitorMailer < PlaceOS::Driver @pending_changes : Hash(String, PendingChange) = {} of String => PendingChange @pending_changes_lock : Mutex = Mutex.new - # Visitors invited within the debounce window, so a change notification for - # the same visit can leave them out: the invitation they are receiving already - # carries the new details, and they never saw the old ones. + # Recently invited visitors, so a change notification for the same visit can + # leave them out: their invitation already carries the new details. # invite_key => expires (monotonic) @recent_invites : Hash(String, Time::Span) = {} of String => Time::Span @recent_invites_lock : Mutex = Mutex.new @@ -240,8 +236,8 @@ class Place::VisitorMailer < PlaceOS::Driver zones = control_system_zone_list - # Each buffered change carries the debounce it was accepted under, so a - # sweep still drains entries left over from the previous settings. + # Each change carries its own debounce, so a sweep still drains entries + # buffered under the previous settings. debounces = [@event_change_debounce, @booking_change_debounce].select(&.positive?) schedule.clear @@ -328,12 +324,9 @@ class Place::VisitorMailer < PlaceOS::Driver end end - # An invitation is our only notice that a visitor has just been added to a - # visit: staff-api signals attendance solely for attendees that weren't - # already attending. Recorded ahead of the filters below so a visitor whose - # invite email is suppressed (disable_event_visitors, - # skip_event_linked_booking_email) still counts as newly invited — they were - # invited, just via the other template. + # Attendance is only signalled for attendees that weren't already attending, + # making this our notice that a visitor is new. Recorded ahead of the filters + # below, as a visitor whose invite email is suppressed was still invited. record_invite(guest_details) if guest_details.is_a?(EventGuest) || guest_details.is_a?(BookingGuest) # don't email staff members @@ -441,8 +434,7 @@ class Place::VisitorMailer < PlaceOS::Driver system_id: guest_details.responds_to?(:system_id) ? guest_details.system_id : nil, ) rescue error - # counted separately from error_count so a missing invite can be told - # apart from a failure anywhere else in this handler + # tracked apart from error_count to pinpoint a missing invite self[:visitor_email_errors] = @visitor_email_errors += 1 raise error end @@ -737,9 +729,9 @@ class Place::VisitorMailer < PlaceOS::Driver return unless fields_changed - # Buffer rather than send now: a group visitor edit updates the parent - # booking before adding this edit's new visitors in later requests, so the - # debounce is what lets us recognise those visitors and leave them out. + # Buffered rather than sent now: a group visitor edit updates the parent + # booking before adding its new visitors, so the debounce is what lets us + # recognise them. change = PendingBookingChange.new( @booking_change_debounce, details.id, details.booking_type, details.user_email, details.title, @@ -848,9 +840,8 @@ class Place::VisitorMailer < PlaceOS::Driver end # Remembers that a visitor was just invited, so a change notification for the - # same visit can leave them out. Expired entries are dropped on the way in, - # which keeps the map to a couple of minutes' worth of invites even with every - # debounce off (and so no sweep running to prune it). + # same visit can leave them out. Expired entries go on the way in, as nothing + # else prunes them. protected def record_invite(guest_details : GuestNotification) : Nil key = invite_key(guest_details.attendee_email, guest_details.host, guest_details.event_starting) now = Time.monotonic @@ -877,17 +868,15 @@ class Place::VisitorMailer < PlaceOS::Driver end end - # A change notification names the parent booking while the invitation names the - # visitor's own child booking, and for an event-linked visitor booking the ids - # don't correspond at all. Both do always describe the same visitor attending - # the same host's visit at the same (new) start time, so key on that instead. + # Not keyed by id: a change names the parent booking while the invitation names + # the visitor's own child booking, and for an event-linked visitor booking the + # two don't correspond at all. Both do name the same visitor, host and start. private def invite_key(visitor_email : String, host_email : String?, event_start : Int64) : String "#{visitor_email.strip.downcase}\t#{host_email.to_s.strip.downcase}\t#{event_start}" end - # Long enough to cover the debounce holding a change notification back, plus - # room for a front end that adds its visitors in requests which follow the one - # that made the change. + # Covers the debounce holding a change back, plus room for a front end that + # adds its visitors in later requests. private def invite_memory : Time::Span {@event_change_debounce, @booking_change_debounce}.max.clamp(0, 3600).seconds + 60.seconds end @@ -1094,9 +1083,8 @@ class Place::VisitorMailer < PlaceOS::Driver # don't treat the host's colleagues as visitors next if @skip_internal_domain_email && colleague_of_host?(visitor_email, host_email) - # don't tell a visitor added by this very edit that their visit changed — - # the invitation they are receiving already carries these details, and - # there is nothing they knew of to have changed (PPT-2375) + # don't tell a visitor added by this edit that their visit changed — their + # invitation already carries these details (PPT-2375) if recently_invited?(visitor_email, host_email, event_start) logger.debug { "skipping #{template} email to #{visitor_email} as they were just invited" } next @@ -1358,10 +1346,8 @@ class Place::VisitorMailer < PlaceOS::Driver # A change buffered awaiting a debounced flush. `current_*` follow the latest # signal in the burst, `previous_*` and `first_seen` stay as they were when it - # started, so the email describes the net change of the whole edit. - # - # Buffer keys are namespaced per subclass, so the two kinds of change can never - # collide even when a booking id and an event id coincide. + # started, so the email describes the net change of the whole edit. Buffer keys + # are namespaced per subclass so the two kinds can't collide. abstract class PendingChange property host : String property title : String? @@ -1372,8 +1358,7 @@ class Place::VisitorMailer < PlaceOS::Driver getter first_seen : Time::Span = Time.monotonic getter buffer_key : String = "" - # The debounce this change was accepted under, kept per entry so a sweep - # still drains anything left over from the previous settings. + # kept per entry so a sweep drains changes buffered under earlier settings getter debounce : Time::Span = Time::Span.zero def initialize(debounce_seconds : Int32, @host, @title, @current_start, @current_end, @previous_start, @previous_end) @@ -1422,10 +1407,8 @@ class Place::VisitorMailer < PlaceOS::Driver @previous_system_id, ) super(debounce_seconds, host, title, current_start, current_end, previous_start, previous_end) - # ical_uid identifies the event instance across mailbox copies and rooms, - # so the rooms either side of a move coalesce too; the one email then names - # a single room and uses that room's guest list. event_id is only a - # fallback for a signal that omits it. + # ical_uid identifies the event instance across mailbox copies and rooms; + # event_id is only a fallback for a signal that omits it. @buffer_key = "event\t#{@event_ical_uid.presence || @event_id}" end diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index d034d6e962..e4ae1ede8f 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -2,7 +2,7 @@ Emails visitors when they are invited (including a QR code for check-in), notifies hosts when visitors check in, and notifies a previous host when a booking's host is -reassigned. Also handles induction and booking-changed notifications. +reassigned. Also handles induction and change notifications. ## Requirements @@ -29,64 +29,32 @@ Requires the following drivers in the system: skip_internal_domain_email: false ``` -## Debouncing change notifications +## Change notifications -A single edit is rarely a single signal: Office365 emits a burst of -`staff/event/changed` updates (the organizer copy, then each room mailbox catching -up), which can briefly flip-flop between the old and new values. Sending an email -per signal spams visitors with contradictory notifications. +When a visit's date, time or location changes, the visitors already on it are sent the +`booking_changed` or `event_changed` template. A visitor added by the same edit is left +out — the invitation they are receiving already carries the new details, and they never +saw the old ones. -`event_change_debounce` and `booking_change_debounce` (seconds, default `15`) buffer -the burst for one visit and send a single email describing the net change once the -window closes. Set one to `0` to email on every signal for that kind of change. +One edit usually produces several signals, so change emails are held briefly and +combined into a single email describing the net change. ```yaml - # Combine duplicate change emails sent within this many seconds; 0 disables. + # Combine change emails sent within this many seconds; 0 disables. event_change_debounce: 15 booking_change_debounce: 15 ``` -Buffered changes are swept on a timer rather than each having its own, so the actual -delay is the configured debounce plus up to one sweep interval (at most 5s). Anything -still buffered is emailed immediately when the driver is unloaded, or when every -debounce is turned off, so a restart never silently drops a pending notification. - -Event signals are grouped by event instance (its ical uid), not by room, so an edit -that moves the meeting *and* changes the time sends one email describing both rather -than one per room. A move between buildings is handled by two separate mailer modules -and so still sends an email each. Booking signals are grouped by booking id. - -The booking debounce also buys the window needed to recognise a visitor added by the -same edit, so setting it to `0` will re-notify new visitors — see below. - -## New visitors are not told the visit changed - -A visitor added while the details are being changed does not need a change -notification: the invitation they are receiving already carries the new date, time and -location, and they never saw the old ones. Being told their visit "changed" before -they have registered being invited to it at all is worse than confusing — QA saw the -change notification arrive alongside, and sometimes in place of, the invite. - -The staff API announces attendance (`staff/guest/attending`) only for attendees that -were not already attending, which makes it an exact statement of "this visitor is -new". Each such announcement is remembered for the debounce window plus a minute, and -those visitors are left out of any change notification for the same visit in that -time. Recipients are matched on visitor, host and the new start time rather than on -the booking id, because a change notification names the *parent* booking while the -invitation names the visitor's own child booking — and for an event-linked visitor -booking the two ids do not correspond at all. - -This works regardless of where the edit came from (workplace, concierge, Outlook or -another driver), as it depends only on the signals rather than on the request that -caused them. The invitation is remembered even when the invite email itself is -suppressed by `disable_event_visitors` or `skip_event_linked_booking_email`, since in -both cases the visitor was still invited, just through the other template. +The email goes out a few seconds after the window closes. Anything still waiting is +sent immediately if the driver restarts, so a notification is never dropped. + +Setting a debounce to `0` emails on every signal, which can mean duplicate and +contradictory notifications, and can also notify visitors added by the edit. ## Colleagues are not visitors Front ends tend to mark every attendee of a meeting as an expected visitor, so staff -invited to a meeting are announced by the staff API exactly like external guests and -would receive visitor invites and QR codes. +invited to a meeting would otherwise receive visitor invites and QR codes. Two settings filter them out: @@ -103,25 +71,21 @@ unaffected — they are filtered on the *attendee's* domain, not the recipient's ## QR code and kiosk link on change notifications -The `booking_changed` and `event_changed` templates receive `guest_jwt` and -`kiosk_url` fields and the same inline `qr.png` attachment as an invitation, because -a move invalidates the kiosk link issued with the original invite — its token is -scoped to the room the meeting has just left, and no fresh invitation is sent. +The `booking_changed` and `event_changed` templates receive `guest_jwt` and `kiosk_url` +fields and the same inline `qr.png` attachment as an invitation, because a move +invalidates the kiosk link issued with the original invite. -Reference the attachment from the template the same way the invite template does, or -set `disable_qr_code: true` to leave it off. Until a template uses them the fields are -simply unused, though an unreferenced attachment may still show up in some mail -clients. +Reference the attachment the same way the invite template does, or set +`disable_qr_code: true` to leave it off. An unreferenced attachment may still show up +in some mail clients. ## Reply-To -Visitor emails set a `Reply-To` header so replies reach a useful person rather than -the no-reply sender address. By default the reply-to is the visitor's **host** -(for the "original host changed" notification it is the new host). This means a -visitor replying to their invite reaches the person hosting them. This requires no -configuration. +Visitor emails set a `Reply-To` header so replies reach the visitor's host rather than +the no-reply sender address (for the "original host changed" notification it is the new +host). This requires no configuration. -This default can be overridden per-template (a `reply_to` field on the template -metadata), tenant-wide (the `reply_to` setting on the Template Mailer), or for all -mail (the `reply_to` setting on the SMTP Mailer). See the Template Mailer readme -for the full precedence cascade. +It can be overridden per-template (a `reply_to` field on the template metadata), +tenant-wide (the `reply_to` setting on the Template Mailer), or for all mail (the +`reply_to` setting on the SMTP Mailer). See the Template Mailer readme for the full +precedence cascade. diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 8cba05b51e..d31e026353 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -2634,18 +2634,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # A visitor added by the same edit is not told the visit changed # ================================================================== # - # PPT-2375. Editing a booking's time and adding a visitor in the one action - # left the new visitor receiving the change notification as well as (and - # sometimes seemingly instead of) their invitation. They never saw the old - # details, and their invitation already carries the new ones. + # The change and the invitation arrive as separate signals, so the debounce is + # what gives the driver a chance to correlate them (PPT-2375). Each test uses + # its own host and start time, as the invite memory outlives the debounce. # - # The change and the invitation arrive as two separate signals, so the - # debounce is what gives the driver a chance to correlate them. Each test - # below uses its own host and start time so the invite memory (which outlives - # the debounce) can't leak between them. - - # each assertion below reads system(:Mailer)[:emails_sent] from the index it - # noted beforehand, giving "recipient|template" for just this test's emails + # Assertions read system(:Mailer)[:emails_sent] from a noted index, giving + # "recipient|template" for just that test's emails. settings({ timezone: "GMT", @@ -2682,7 +2676,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do }.to_json) # still inside the debounce window, as the front end adds its visitors in the - # requests that follow the one which moved the booking + # requests following the one that moved the booking sleep 0.5 publish("staff/guest/attending", { From cc62a58ffeb74bf62010f5f9f911cdde9b8c4eec Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Tue, 4 Aug 2026 15:48:42 +0930 Subject: [PATCH 3/8] fix(visitor_mailer): skip change emails for visitors added by the same edit (PPT-2375) --- drivers/place/visitor_mailer.cr | 6 +++--- drivers/place/visitor_mailer_readme.md | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index a28ae7acbe..44df41b571 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -74,9 +74,9 @@ class Place::VisitorMailer < PlaceOS::Driver skip_host_email: true, # When true, attendees whose email domain matches the host's are treated as - # colleagues rather than visitors and are not emailed. Front ends tend to - # mark every attendee as an expected visitor, so staff invited to a meeting - # would otherwise receive visitor invites and QR codes. + # staff rather than visitors and are not emailed. The front end might mark + # any attendee as an expected visitor, so staff invited to a meeting can + # otherwise receive visitor invites and QR codes. skip_internal_domain_email: false, domain_uri: "https://example.com/", diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index e4ae1ede8f..7db4410a37 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -24,8 +24,8 @@ Requires the following drivers in the system: event_template: "event" # When true, the host is not sent visitor-targeted emails skip_host_email: true - # When true, attendees sharing the host's email domain are treated as - # colleagues rather than visitors and are not emailed + # When true, attendees sharing the host's email domain are treated as staff + # rather than visitors and are not emailed skip_internal_domain_email: false ``` @@ -51,10 +51,10 @@ sent immediately if the driver restarts, so a notification is never dropped. Setting a debounce to `0` emails on every signal, which can mean duplicate and contradictory notifications, and can also notify visitors added by the edit. -## Colleagues are not visitors +## Excluding staff attendees -Front ends tend to mark every attendee of a meeting as an expected visitor, so staff -invited to a meeting would otherwise receive visitor invites and QR codes. +The front end might mark any attendee as an expected visitor, so staff invited to a +meeting can receive visitor invites and QR codes. Two settings filter them out: From 64cce11eeff993ec66e1a376b6054b016c7b109d Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Tue, 4 Aug 2026 16:25:26 +0930 Subject: [PATCH 4/8] refactor(visitor_mailer): debounce (PPT-2375) --- drivers/place/visitor_mailer.cr | 66 +++------ drivers/place/visitor_mailer_readme.md | 7 +- drivers/place/visitor_mailer_spec.cr | 181 +++++++++++-------------- 3 files changed, 105 insertions(+), 149 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 44df41b571..cae6c8060b 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -40,10 +40,9 @@ class Place::VisitorMailer < PlaceOS::Driver event_changed_template: "event_changed", group_event_template: "group_event", - # Combine duplicate change emails sent within this many seconds; 0 disables. - event_change_debounce: 15, - # as above for bookings; 0 also notifies visitors added by the same edit - booking_change_debounce: 15, + # Combine duplicate change emails sent within this many seconds. 0 disables, + # which also notifies visitors added by the same edit. + change_debounce: 15, disable_qr_code: false, send_network_credentials: false, network_password_length: DEFAULT_PASSWORD_LENGTH, @@ -171,8 +170,7 @@ class Place::VisitorMailer < PlaceOS::Driver # Coalescing buffer for staff/{event,booking}/changed, swept once the window # elapses. seconds to buffer a change; 0 emails on every signal - @event_change_debounce : Int32 = 15 - @booking_change_debounce : Int32 = 15 + @change_debounce : Int32 = 15 # buffer_key => coalesced change awaiting its flush @pending_changes : Hash(String, PendingChange) = {} of String => PendingChange @pending_changes_lock : Mutex = Mutex.new @@ -202,8 +200,7 @@ class Place::VisitorMailer < PlaceOS::Driver @booking_changed_template = setting?(String, :booking_changed_template) || "booking_changed" @event_changed_template = setting?(String, :event_changed_template) || "event_changed" @group_event_template = setting?(String, :group_event_template) || "group_event" - @event_change_debounce = setting?(Int32, :event_change_debounce) || 15 - @booking_change_debounce = setting?(Int32, :booking_change_debounce) || 15 + @change_debounce = setting?(Int32, :change_debounce) || 15 @disable_qr_code = setting?(Bool, :disable_qr_code) || false @determine_host_name_using = setting?(String, :determine_host_name_using) || "calendar-driver" @send_network_credentials = setting?(Bool, :send_network_credentials) || false @@ -236,22 +233,16 @@ class Place::VisitorMailer < PlaceOS::Driver zones = control_system_zone_list - # Each change carries its own debounce, so a sweep still drains entries - # buffered under the previous settings. - debounces = [@event_change_debounce, @booking_change_debounce].select(&.positive?) + # The sweep below picks up the rest; with the debounce off nothing would. + flush_pending_changes("debounce disabled") if @change_debounce <= 0 schedule.clear if reminders = @send_reminders schedule.cron(reminders, @time_zone) { send_reminder_emails } end - if interval = debounces.min? - # Sweeps at most every 5s, so a change waits its debounce plus up to one interval. - schedule.every(interval.clamp(1, 5).seconds) { sweep_pending_changes } - else - # Nothing would sweep the buffer with every debounce switched off. - flush_pending_changes("debounce disabled") - end + # Sweeps at most every 5s, so a change waits its debounce plus up to one interval. + schedule.every(@change_debounce.clamp(1, 5).seconds) { sweep_pending_changes } if @change_debounce > 0 spawn { ensure_building_zone(zones) } end @@ -733,13 +724,12 @@ class Place::VisitorMailer < PlaceOS::Driver # booking before adding its new visitors, so the debounce is what lets us # recognise them. change = PendingBookingChange.new( - @booking_change_debounce, details.id, details.booking_type, details.user_email, details.title, details.resource_id, details.booking_start, details.booking_end, details.previous_booking_start, details.previous_booking_end, details.zones, details.previous_zones, ) - @booking_change_debounce > 0 ? buffer_change(change) : dispatch_booking_change(change) + @change_debounce > 0 ? buffer_change(change) : dispatch_booking_change(change) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -814,12 +804,11 @@ class Place::VisitorMailer < PlaceOS::Driver # Coalesce the burst of signals Office365 emits per edit into one email. change = PendingEventChange.new( - @event_change_debounce, details.event_id, details.system_id, details.event_ical_uid, host, details.title, event_start, event_end, details.previous_event_start, details.previous_event_end, details.previous_system_id, ) - @event_change_debounce > 0 ? buffer_change(change) : dispatch_event_change(change) + @change_debounce > 0 ? buffer_change(change) : dispatch_event_change(change) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -878,7 +867,7 @@ class Place::VisitorMailer < PlaceOS::Driver # Covers the debounce holding a change back, plus room for a front end that # adds its visitors in later requests. private def invite_memory : Time::Span - {@event_change_debounce, @booking_change_debounce}.max.clamp(0, 3600).seconds + 60.seconds + @change_debounce.clamp(0, 3600).seconds + 60.seconds end # Collapses the burst of signals for one edit into a single buffered change. @@ -896,17 +885,16 @@ class Place::VisitorMailer < PlaceOS::Driver # Sends any change that has been buffered for its full debounce window. private def sweep_pending_changes : Nil - flush_pending_changes("debounce window elapsed", ready_only: true) + flush_pending_changes("debounce window elapsed", older_than: Time.monotonic - @change_debounce.seconds) end - # Dispatches buffered changes, each in its own fiber so a slow send can't stall - # the sweep. `ready_only` limits the flush to entries that have served their - # debounce, `wait` bounds how long we block for the sends to finish. - private def flush_pending_changes(reason : String, ready_only : Bool = false, wait : Time::Span? = nil) : Nil + # Dispatches matching changes, each in its own fiber so a slow send can't stall + # the sweep. `older_than` limits the flush to entries buffered before that point + # (nil takes the lot), `wait` bounds how long we block for the sends to finish. + private def flush_pending_changes(reason : String, older_than : Time::Span? = nil, wait : Time::Span? = nil) : Nil flushing = @pending_changes_lock.synchronize do - now = Time.monotonic - ready = if ready_only - @pending_changes.values.select(&.ready?(now)) + ready = if cutoff = older_than + @pending_changes.values.select { |pending| pending.first_seen <= cutoff } else @pending_changes.values end @@ -1358,16 +1346,8 @@ class Place::VisitorMailer < PlaceOS::Driver getter first_seen : Time::Span = Time.monotonic getter buffer_key : String = "" - # kept per entry so a sweep drains changes buffered under earlier settings - getter debounce : Time::Span = Time::Span.zero - - def initialize(debounce_seconds : Int32, @host, @title, @current_start, @current_end, @previous_start, @previous_end) - @debounce = debounce_seconds.clamp(0, 3600).seconds - end - # Whether this change has served its full debounce window. - def ready?(now : Time::Span) : Bool - (first_seen + debounce) <= now + def initialize(@host, @title, @current_start, @current_end, @previous_start, @previous_end) end # Whether the coalesced result still describes a real change. @@ -1394,7 +1374,6 @@ class Place::VisitorMailer < PlaceOS::Driver property previous_system_id : String? # the room before the edit def initialize( - debounce_seconds : Int32, @event_id, @system_id, @event_ical_uid, @@ -1406,7 +1385,7 @@ class Place::VisitorMailer < PlaceOS::Driver previous_end, @previous_system_id, ) - super(debounce_seconds, host, title, current_start, current_end, previous_start, previous_end) + super(host, title, current_start, current_end, previous_start, previous_end) # ical_uid identifies the event instance across mailbox copies and rooms; # event_id is only a fallback for a signal that omits it. @buffer_key = "event\t#{@event_ical_uid.presence || @event_id}" @@ -1445,7 +1424,6 @@ class Place::VisitorMailer < PlaceOS::Driver property previous_zones : Array(String)? def initialize( - debounce_seconds : Int32, @booking_id, @booking_type, host, @@ -1458,7 +1436,7 @@ class Place::VisitorMailer < PlaceOS::Driver @zones, @previous_zones, ) - super(debounce_seconds, host, title, current_start, current_end, previous_start, previous_end) + super(host, title, current_start, current_end, previous_start, previous_end) @buffer_key = "booking\t#{@booking_id}" end diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index 7db4410a37..e8ab16114b 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -41,15 +41,14 @@ combined into a single email describing the net change. ```yaml # Combine change emails sent within this many seconds; 0 disables. - event_change_debounce: 15 - booking_change_debounce: 15 + change_debounce: 15 ``` The email goes out a few seconds after the window closes. Anything still waiting is sent immediately if the driver restarts, so a notification is never dropped. -Setting a debounce to `0` emails on every signal, which can mean duplicate and -contradictory notifications, and can also notify visitors added by the edit. +Setting this to `0` emails on every signal, which can mean duplicate and contradictory +notifications, and can also notify visitors added by the edit. ## Excluding staff attendees diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index d31e026353..7d56dc6d73 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -284,15 +284,14 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # Allow on_load -> on_update -> ensure_building_zone to complete sleep 1.5 - # Most tests below assert an immediate send, so both debounces are disabled - # (they default to 15s). The debounce behaviour has dedicated tests that turn - # them back on. send_reminders/domain_uri mirror default_settings so nothing - # else changes. + # Most tests below assert an immediate send, so the debounce is disabled (it + # defaults to 15s). The debounce behaviour has dedicated tests that turn it + # back on. send_reminders/domain_uri mirror default_settings so nothing else + # changes. settings({ - event_change_debounce: 0, - booking_change_debounce: 0, - send_reminders: "0 7 * * *", - domain_uri: "https://example.com/", + change_debounce: 0, + send_reminders: "0 7 * * *", + domain_uri: "https://example.com/", }) sleep 1.0 @@ -716,10 +715,9 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # These tests assert an immediate send, so disable the debounce (default 15s). # send_reminders/domain_uri mirror default_settings so nothing else changes. settings({ - event_change_debounce: 0, - booking_change_debounce: 0, - send_reminders: "0 7 * * *", - domain_uri: "https://example.com/", + change_debounce: 0, + send_reminders: "0 7 * * *", + domain_uri: "https://example.com/", }) sleep 1.0 @@ -1227,8 +1225,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_space_name: "Client Floor", invite_zone_tag: "building", skip_event_linked_booking_email: false, - event_change_debounce: 0, - booking_change_debounce: 0, + change_debounce: 0, }) sleep 1.0 @@ -1431,12 +1428,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - skip_host_email: false, - event_change_debounce: 0, - booking_change_debounce: 0, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + skip_host_email: false, + change_debounce: 0, }) sleep 1.0 @@ -1474,8 +1470,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do invite_zone_tag: "building", notify_induction_accepted_template: "custom_accepted", notify_induction_declined_template: "custom_declined", - event_change_debounce: 0, - booking_change_debounce: 0, + change_debounce: 0, }) sleep 1.0 @@ -1539,8 +1534,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do invite_zone_tag: "building", booking_changed_template: "custom_booking_changed", event_changed_template: "custom_event_changed", - event_change_debounce: 0, - booking_change_debounce: 0, + change_debounce: 0, }) sleep 1.0 @@ -1612,8 +1606,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_space_name: "Client Floor", invite_zone_tag: "building", # skip_event_linked_booking_email defaults to true (invite flow only) - event_change_debounce: 0, - booking_change_debounce: 0, + change_debounce: 0, }) sleep 1.0 @@ -1691,8 +1684,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_space_name: "Client Floor", invite_zone_tag: "building", skip_event_linked_booking_email: false, - event_change_debounce: 0, - booking_change_debounce: 0, + change_debounce: 0, }) sleep 1.0 @@ -1714,11 +1706,10 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # show a real date, so the driver looks the event up via the staff API. settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 0, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, }) sleep 1.0 @@ -1782,7 +1773,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do system(:Mailer)[:last_args]["event_date"].raw.should be_nil # ================================================================== - # event_change_debounce — coalesce the Office365 signal burst (PPT-2375) + # change_debounce — coalesce the Office365 signal burst (PPT-2375) # ================================================================== # # One edit arrives as an A -> B -> A flip-flop (Wed->Thu, Thu->Wed, Wed->Thu) @@ -1790,11 +1781,10 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # showing the true net change. settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 3, - booking_change_debounce: 0, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 3, }) sleep 1.0 @@ -1899,11 +1889,10 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do system(:Mailer)[:send_count].should eq count_before_survives settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 3, - booking_change_debounce: 0, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 3, }) # the update must not cut the window short @@ -2067,11 +2056,10 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 30, - booking_change_debounce: 0, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 30, }) sleep 1.0 @@ -2100,11 +2088,10 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # Disabling the debounce flushes the buffer instead of orphaning it. settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 0, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, }) sleep 1.5 @@ -2248,12 +2235,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do status[:users_checked_in].should eq checked_in_before_checkout settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 0, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2367,13 +2353,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 0, - disable_qr_code: true, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, + disable_qr_code: true, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2426,13 +2411,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 0, - host_domain_filter: ["example.com"], - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, + host_domain_filter: ["example.com"], + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2466,8 +2450,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do timezone: "GMT", booking_space_name: "Client Floor", invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 0, + change_debounce: 0, skip_internal_domain_email: true, domain_uri: "https://example.com/", }) @@ -2545,12 +2528,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 0, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2594,12 +2576,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 0, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2642,12 +2623,11 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # "recipient|template" for just that test's emails. settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 0, - booking_change_debounce: 2, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 2, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2874,13 +2854,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ settings({ - timezone: "GMT", - booking_space_name: "Client Floor", - invite_zone_tag: "building", - event_change_debounce: 2, - booking_change_debounce: 0, - disable_event_visitors: false, - domain_uri: "https://example.com/", + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 2, + disable_event_visitors: false, + domain_uri: "https://example.com/", }) sleep 1.0 From 07a4c1f5b8211e6bfeb3a93fa6eced2ccd2b9bb3 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Mon, 10 Aug 2026 10:52:41 +0930 Subject: [PATCH 5/8] fix(visitor_mailer): only a sent invitation marks a visitor as new (PPT-2375) Recording an invite on any staff/guest/attending signal was wrong. A booking create signals attendance for every attendee rather than only new ones, and the front end tears down and re-creates the visitor bookings behind a calendar event on every save. Moving the room leaves the start time untouched, so those signals matched the invite key exactly and suppressed the room-move notification. Record only once an invitation email has actually been sent, so a signal that produced no email says nothing about whether the visitor is new. --- drivers/place/visitor_mailer.cr | 13 ++--- drivers/place/visitor_mailer_readme.md | 6 +-- drivers/place/visitor_mailer_spec.cr | 70 ++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 9 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index cae6c8060b..68cef00b4b 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -315,11 +315,6 @@ class Place::VisitorMailer < PlaceOS::Driver end end - # Attendance is only signalled for attendees that weren't already attending, - # making this our notice that a visitor is new. Recorded ahead of the filters - # below, as a visitor whose invite email is suppressed was still invited. - record_invite(guest_details) if guest_details.is_a?(EventGuest) || guest_details.is_a?(BookingGuest) - # don't email staff members if !@host_domain_filter.empty? && guest_details.attendee_email.split('@', 2)[1].downcase.in?(@host_domain_filter) logger.debug { "ignoring event matches host domain filter" } @@ -431,6 +426,12 @@ class Place::VisitorMailer < PlaceOS::Driver end self[:visitor_emails_sent] = @visitor_emails_sent += 1 + + # Only an attendance signal that produced an invitation tells us a visitor + # is new. One that produced no email says nothing: the front end re-creates + # the visitor bookings behind an event on every save, and a booking create + # signals attendance for everyone on it (PPT-2375). + record_invite(guest_details) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -840,7 +841,7 @@ class Place::VisitorMailer < PlaceOS::Driver @recent_invites[key] = now + invite_memory end - logger.debug { "noted #{guest_details.attendee_email} as newly invited by #{guest_details.host}" } + logger.debug { "noted #{guest_details.attendee_email} as just invited by #{guest_details.host}" } end # Whether this visitor was invited to this visit within the memory window. diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index e8ab16114b..5efb26c1e2 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -32,9 +32,9 @@ Requires the following drivers in the system: ## Change notifications When a visit's date, time or location changes, the visitors already on it are sent the -`booking_changed` or `event_changed` template. A visitor added by the same edit is left -out — the invitation they are receiving already carries the new details, and they never -saw the old ones. +`booking_changed` or `event_changed` template. A visitor who is being invited by the +same edit is left out — the invitation they are receiving already carries the new +details, and they never saw the old ones. One edit usually produces several signals, so change emails are held briefly and combined into a single email describing the net change. diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 7d56dc6d73..789dd43671 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -2905,4 +2905,74 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do event_invite_emails.should contain "visitor-b@external.com|event" event_invite_emails.should_not contain "visitor-b@external.com|event_changed" event_invite_emails.size.should eq 2 + + # ------------------------------------------------------------------ + # Test 57: a re-created event-linked visitor booking is not an + # invitation, so it must not suppress the change email + # ------------------------------------------------------------------ + # + # The front end tears down and re-creates the visitor bookings behind a + # calendar event on every save, and a booking create signals attendance for + # every attendee regardless of whether they are new. Moving the room leaves + # the start time untouched, so such a signal looks exactly like an invitation + # for the visit being changed. It isn't one — no invite email is sent for it + # — and treating it as one silenced the room-move notification entirely. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 2, + skip_event_linked_booking_email: true, + domain_uri: "https://example.com/", + }) + sleep 1.0 + + sent_before_room_move = system(:Mailer)[:emails_sent].as_a.size + + # the room moves; the times are untouched + publish("staff/event/changed", { + action: "update", + system_id: "sys-room1", + event_id: "evt-room-move", + event_ical_uid: "ical-room-move", + host: "host-roommove@example.com", + resource: "room1@example.com", + title: "Room Moved", + event_start: now + 54000, + event_end: now + 57600, + zones: ["zone-building", "zone-room"], + previous_event_start: now + 54000, + previous_event_end: now + 57600, + previous_system_id: "sys-room2", + }.to_json) + + sleep 0.5 + + # booking 601 is event-linked (extension_data.parent_id), so no invite email + # is sent for it — the visitor was already invited when the event was created + publish("staff/guest/attending", { + action: "booking_created", + id: 4_i64, + booking_id: 601_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Room Moved", + event_summary: "Room Moved", + event_starting: now + 54000, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-roommove@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 8.0 + + room_move_emails = system(:Mailer)[:emails_sent].as_a[sent_before_room_move..].map(&.as_s) + + # the visitor is told their meeting moved rooms + room_move_emails.should contain "visitor@external.com|event_changed" + # and the event-linked booking still sends no invite of its own + room_move_emails.should_not contain "visitor@external.com|booking" + room_move_emails.size.should eq 1 end From cfecf8172f8edd981136cc162a2f03ff602b616d Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Mon, 10 Aug 2026 12:39:39 +0930 Subject: [PATCH 6/8] fix(visitor_mailer): relocating a visit still notifies its visitors (PPT-2375) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inviting a visitor and then moving their visit are two separate actions, however close together, but a location change leaves the times untouched so the two were indistinguishable by visitor, host and start time alone. Creating a visitor booking and then relocating it left the visitor unaware of the move. Record what each invitation was for, and whether that booking or event was created by the same action. An invitation to the very thing being changed is simply how the visit began; only an invitation to something else — the visitor's own child booking under a group parent — means this edit is what added them. Ordering cannot serve as the discriminator: the single-visitor edit form and the group container patch both emit the invitation before the change within one request. --- drivers/place/visitor_mailer.cr | 48 ++++++++++++++------ drivers/place/visitor_mailer_spec.cr | 68 ++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 13 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 68cef00b4b..448267550d 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -177,8 +177,8 @@ class Place::VisitorMailer < PlaceOS::Driver # Recently invited visitors, so a change notification for the same visit can # leave them out: their invitation already carries the new details. - # invite_key => expires (monotonic) - @recent_invites : Hash(String, Time::Span) = {} of String => Time::Span + # invite_key => the invitation + @recent_invites : Hash(String, Invite) = {} of String => Invite @recent_invites_lock : Mutex = Mutex.new @uri : URI = URI.new @@ -384,9 +384,12 @@ class Place::VisitorMailer < PlaceOS::Driver room = get_room_details(guest_details.system_id) area_name = room.display_name.presence || room.name template = @event_template + # what the visitor is being invited to, to compare against what changed + invited_to = guest_details.event_id in BookingGuest area_name = @booking_space_name template = @booking_template + invited_to = guest_details.booking_id.to_s booking = staff_api.get_booking(guest_details.booking_id).get if @skip_event_linked_booking_email @@ -431,7 +434,7 @@ class Place::VisitorMailer < PlaceOS::Driver # is new. One that produced no email says nothing: the front end re-creates # the visitor bookings behind an event on every save, and a booking create # signals attendance for everyone on it (PPT-2375). - record_invite(guest_details) + record_invite(guest_details, invited_to) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -829,32 +832,51 @@ class Place::VisitorMailer < PlaceOS::Driver attendee_domain.downcase == host_domain.downcase end + # `invited_to` is the booking or event the invitation was for, and `from_create` + # whether that booking or event was created by the same action. + record Invite, expires : Time::Span, from_create : Bool, invited_to : String + # Remembers that a visitor was just invited, so a change notification for the # same visit can leave them out. Expired entries go on the way in, as nothing # else prunes them. - protected def record_invite(guest_details : GuestNotification) : Nil + protected def record_invite(guest_details : GuestNotification, invited_to : String) : Nil key = invite_key(guest_details.attendee_email, guest_details.host, guest_details.event_starting) now = Time.monotonic + invite = Invite.new( + expires: now + invite_memory, + from_create: guest_details.action.in?("booking_created", "meeting_created"), + invited_to: invited_to, + ) @recent_invites_lock.synchronize do - @recent_invites.reject! { |_key, expires| expires <= now } - @recent_invites[key] = now + invite_memory + @recent_invites.reject! { |_key, recorded| recorded.expires <= now } + @recent_invites[key] = invite end - logger.debug { "noted #{guest_details.attendee_email} as just invited by #{guest_details.host}" } + logger.debug { "noted #{guest_details.attendee_email} as just invited to #{invited_to} by #{guest_details.host}" } end - # Whether this visitor was invited to this visit within the memory window. - protected def recently_invited?(visitor_email : String, host_email : String, event_start : Int64) : Bool + # Whether this visitor was invited to this visit within the memory window, and + # by something other than the creation of `changed` itself. + # + # Being invited to a visit *as it is created* is simply how that visit began — + # relocating it afterwards is a separate action the visitor still needs to hear + # about. Only an invitation to something else (the visitor's own child booking + # under a group parent, say) means this edit is what added them. + protected def recently_invited?(visitor_email : String, host_email : String, event_start : Int64, changed : String?) : Bool key = invite_key(visitor_email, host_email, event_start) now = Time.monotonic @recent_invites_lock.synchronize do - if expires = @recent_invites[key]? - next true if expires > now + invite = @recent_invites[key]? + next false unless invite + + if invite.expires <= now @recent_invites.delete(key) + next false end - false + + !(invite.from_create && invite.invited_to == changed) end end @@ -1074,7 +1096,7 @@ class Place::VisitorMailer < PlaceOS::Driver # don't tell a visitor added by this edit that their visit changed — their # invitation already carries these details (PPT-2375) - if recently_invited?(visitor_email, host_email, event_start) + if recently_invited?(visitor_email, host_email, event_start, event_id) logger.debug { "skipping #{template} email to #{visitor_email} as they were just invited" } next end diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 789dd43671..38b9552b1c 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -2975,4 +2975,72 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # and the event-linked booking still sends no invite of its own room_move_emails.should_not contain "visitor@external.com|booking" room_move_emails.size.should eq 1 + + # ------------------------------------------------------------------ + # Test 58: relocating a booking still notifies the visitor who was + # invited when that same booking was created + # ------------------------------------------------------------------ + # + # Inviting a visitor and then moving their visit are two separate actions, + # however close together. Only an invitation to something *other* than what + # changed means the visitor is being added by this edit — an invitation to the + # very booking now being changed is just how the visit began. + # + # A location change leaves the times alone, so this is indistinguishable from + # an invitation by visitor, host and start time alone. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 2, + skip_event_linked_booking_email: true, + domain_uri: "https://example.com/", + }) + sleep 1.0 + + sent_before_relocate = system(:Mailer)[:emails_sent].as_a.size + + publish("staff/guest/attending", { + action: "booking_created", + id: 5_i64, + booking_id: 600_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Relocated Visit", + event_summary: "Relocated Visit", + event_starting: now + 61200, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-relocate@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + # the same booking is then moved: the location changes, the times do not + publish("staff/booking/changed", { + action: "metadata_changed", + id: 600_i64, + booking_type: "visitor", + booking_start: now + 61200, + booking_end: now + 64800, + timezone: "GMT", + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + user_email: "host-relocate@example.com", + title: "Relocated Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 61200, + previous_booking_end: now + 64800, + previous_zones: ["zone-old-building", "zone-old-room"], + }.to_json) + + sleep 8.0 + + relocate_emails = system(:Mailer)[:emails_sent].as_a[sent_before_relocate..].map(&.as_s) + + relocate_emails.should contain "visitor@external.com|booking" + relocate_emails.should contain "visitor@external.com|booking_changed" + relocate_emails.size.should eq 2 end From 34a6a9a292bff86a9d9f7e70a19d9b684e569a58 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Mon, 10 Aug 2026 16:32:42 +0930 Subject: [PATCH 7/8] docs(visitor_mailer): tighten the invite correlation comments (PPT-2375) --- drivers/place/visitor_mailer.cr | 23 +++++++++-------------- drivers/place/visitor_mailer_spec.cr | 17 +++++------------ 2 files changed, 14 insertions(+), 26 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 448267550d..fc5ae7cea3 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -384,7 +384,6 @@ class Place::VisitorMailer < PlaceOS::Driver room = get_room_details(guest_details.system_id) area_name = room.display_name.presence || room.name template = @event_template - # what the visitor is being invited to, to compare against what changed invited_to = guest_details.event_id in BookingGuest area_name = @booking_space_name @@ -430,10 +429,9 @@ class Place::VisitorMailer < PlaceOS::Driver self[:visitor_emails_sent] = @visitor_emails_sent += 1 - # Only an attendance signal that produced an invitation tells us a visitor - # is new. One that produced no email says nothing: the front end re-creates - # the visitor bookings behind an event on every save, and a booking create - # signals attendance for everyone on it (PPT-2375). + # Only an invitation that was actually emailed marks a visitor as new: a + # booking create signals attendance for everyone on it, and the front end + # re-creates the bookings behind an event on every save (PPT-2375). record_invite(guest_details, invited_to) rescue error logger.error { error.inspect_with_backtrace } @@ -832,8 +830,8 @@ class Place::VisitorMailer < PlaceOS::Driver attendee_domain.downcase == host_domain.downcase end - # `invited_to` is the booking or event the invitation was for, and `from_create` - # whether that booking or event was created by the same action. + # `invited_to` is the booking or event invited to, `from_create` whether that + # booking or event was created by the same action. record Invite, expires : Time::Span, from_create : Bool, invited_to : String # Remembers that a visitor was just invited, so a change notification for the @@ -856,13 +854,10 @@ class Place::VisitorMailer < PlaceOS::Driver logger.debug { "noted #{guest_details.attendee_email} as just invited to #{invited_to} by #{guest_details.host}" } end - # Whether this visitor was invited to this visit within the memory window, and - # by something other than the creation of `changed` itself. - # - # Being invited to a visit *as it is created* is simply how that visit began — - # relocating it afterwards is a separate action the visitor still needs to hear - # about. Only an invitation to something else (the visitor's own child booking - # under a group parent, say) means this edit is what added them. + # Whether this visitor was just invited to something other than the creation of + # `changed`. Being invited as a visit is created is how that visit began, so + # relocating it afterwards still notifies; an invitation to something else (the + # visitor's own child booking under a group parent) is this edit adding them. protected def recently_invited?(visitor_email : String, host_email : String, event_start : Int64, changed : String?) : Bool key = invite_key(visitor_email, host_email, event_start) now = Time.monotonic diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 38b9552b1c..c335974781 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -2911,12 +2911,9 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # invitation, so it must not suppress the change email # ------------------------------------------------------------------ # - # The front end tears down and re-creates the visitor bookings behind a - # calendar event on every save, and a booking create signals attendance for - # every attendee regardless of whether they are new. Moving the room leaves - # the start time untouched, so such a signal looks exactly like an invitation - # for the visit being changed. It isn't one — no invite email is sent for it - # — and treating it as one silenced the room-move notification entirely. + # The front end re-creates the visitor bookings behind a calendar event on + # every save, and a booking create signals attendance for every attendee. No + # invite email is sent for an event-linked one, so it is not an invitation. settings({ timezone: "GMT", @@ -2982,12 +2979,8 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ------------------------------------------------------------------ # # Inviting a visitor and then moving their visit are two separate actions, - # however close together. Only an invitation to something *other* than what - # changed means the visitor is being added by this edit — an invitation to the - # very booking now being changed is just how the visit began. - # - # A location change leaves the times alone, so this is indistinguishable from - # an invitation by visitor, host and start time alone. + # however close together. A location change leaves the times alone, so the two + # are indistinguishable by visitor, host and start time alone. settings({ timezone: "GMT", From a803815be564c51c7162d92d0986ff83bfa51c91 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Tue, 11 Aug 2026 14:33:28 +0930 Subject: [PATCH 8/8] fix(visitor_mailer): honour the legacy debounce and track invitations apart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two review findings. event_change_debounce is read again when change_debounce is absent, so a deployment configured before the rename keeps its value instead of silently returning to the default, where notifications would be both delayed and newly filtered. Invitations are held individually rather than one per visitor. A visitor can be announced more than once for the one visit — their own child booking plus the group container the front end pushes them onto — and collapsing those let a later, unrelated invitation displace the one showing this edit added them. Each invitation now also carries the parent booking it sits under, so being added to a group is told apart from a visit at the same time under a different parent. The booking mock gained the parent ids the front end actually sets, without which the group children were modelled as standalone bookings and the collision could not surface. --- drivers/place/visitor_mailer.cr | 91 +++++---- drivers/place/visitor_mailer_spec.cr | 271 +++++++++++++++++++++++++++ 2 files changed, 327 insertions(+), 35 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index fc5ae7cea3..fa28e808ff 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -175,10 +175,10 @@ class Place::VisitorMailer < PlaceOS::Driver @pending_changes : Hash(String, PendingChange) = {} of String => PendingChange @pending_changes_lock : Mutex = Mutex.new - # Recently invited visitors, so a change notification for the same visit can - # leave them out: their invitation already carries the new details. - # invite_key => the invitation - @recent_invites : Hash(String, Invite) = {} of String => Invite + # Recent invitations, so a change notification for the same visit can leave the + # visitor out: their invitation already carries the new details. A visitor can + # hold more than one at a time, so each is kept separately. + @recent_invites : Array(Invite) = [] of Invite @recent_invites_lock : Mutex = Mutex.new @uri : URI = URI.new @@ -200,7 +200,9 @@ class Place::VisitorMailer < PlaceOS::Driver @booking_changed_template = setting?(String, :booking_changed_template) || "booking_changed" @event_changed_template = setting?(String, :event_changed_template) || "event_changed" @group_event_template = setting?(String, :group_event_template) || "group_event" - @change_debounce = setting?(Int32, :change_debounce) || 15 + # event_change_debounce is the pre-unification name, still read so an existing + # deployment doesn't silently fall back to the default + @change_debounce = setting?(Int32, :change_debounce) || setting?(Int32, :event_change_debounce) || 15 @disable_qr_code = setting?(Bool, :disable_qr_code) || false @determine_host_name_using = setting?(String, :determine_host_name_using) || "calendar-driver" @send_network_credentials = setting?(Bool, :send_network_credentials) || false @@ -385,11 +387,14 @@ class Place::VisitorMailer < PlaceOS::Driver area_name = room.display_name.presence || room.name template = @event_template invited_to = guest_details.event_id + invited_under = nil in BookingGuest area_name = @booking_space_name template = @booking_template invited_to = guest_details.booking_id.to_s booking = staff_api.get_booking(guest_details.booking_id).get + # a group visitor is invited to their own booking beneath the group parent + invited_under = booking["parent_id"]?.try { |id| id.as_i64?.try(&.to_s) || id.as_s? } if @skip_event_linked_booking_email parent_id = booking.dig?("extension_data", "parent_id").try(&.as_s?) @@ -432,7 +437,7 @@ class Place::VisitorMailer < PlaceOS::Driver # Only an invitation that was actually emailed marks a visitor as new: a # booking create signals attendance for everyone on it, and the front end # re-creates the bookings behind an event on every save (PPT-2375). - record_invite(guest_details, invited_to) + record_invite(guest_details, invited_to, invited_under) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -830,58 +835,74 @@ class Place::VisitorMailer < PlaceOS::Driver attendee_domain.downcase == host_domain.downcase end - # `invited_to` is the booking or event invited to, `from_create` whether that - # booking or event was created by the same action. - record Invite, expires : Time::Span, from_create : Bool, invited_to : String + # One invitation: `invited_to` is the booking or event invited to, + # `invited_under` its parent booking (group visitors get their own child + # booking), `from_create` whether it was created by the same action. + record Invite, + visitor : String, + host : String, + event_start : Int64, + invited_to : String, + invited_under : String?, + from_create : Bool, + expires : Time::Span do + def matches?(other_visitor : String, other_host : String, other_start : Int64) : Bool + visitor == other_visitor && host == other_host && event_start == other_start + end + + # Whether this invitation is the visitor being added to `changed`, rather + # than `changed` being the visit they were invited to in the first place. + def added_to?(changed : String?) : Bool + return true if invited_under && invited_under == changed + invited_to == changed && !from_create + end + end # Remembers that a visitor was just invited, so a change notification for the # same visit can leave them out. Expired entries go on the way in, as nothing # else prunes them. - protected def record_invite(guest_details : GuestNotification, invited_to : String) : Nil - key = invite_key(guest_details.attendee_email, guest_details.host, guest_details.event_starting) + protected def record_invite(guest_details : GuestNotification, invited_to : String, invited_under : String?) : Nil now = Time.monotonic invite = Invite.new( - expires: now + invite_memory, - from_create: guest_details.action.in?("booking_created", "meeting_created"), + visitor: guest_details.attendee_email.strip.downcase, + host: guest_details.host.strip.downcase, + event_start: guest_details.event_starting, invited_to: invited_to, + invited_under: invited_under, + from_create: guest_details.action.in?("booking_created", "meeting_created"), + expires: now + invite_memory, ) @recent_invites_lock.synchronize do - @recent_invites.reject! { |_key, recorded| recorded.expires <= now } - @recent_invites[key] = invite + @recent_invites.reject! do |recorded| + recorded.expires <= now || + (recorded.matches?(invite.visitor, invite.host, invite.event_start) && recorded.invited_to == invited_to) + end + @recent_invites << invite end logger.debug { "noted #{guest_details.attendee_email} as just invited to #{invited_to} by #{guest_details.host}" } end - # Whether this visitor was just invited to something other than the creation of - # `changed`. Being invited as a visit is created is how that visit began, so - # relocating it afterwards still notifies; an invitation to something else (the - # visitor's own child booking under a group parent) is this edit adding them. + # Whether this edit is what added the visitor to `changed`. Being invited to a + # visit as it is created is how that visit began, so relocating it afterwards + # still notifies; an invitation under `changed` (the visitor's own child booking + # beneath a group parent) is this edit adding them. + # + # Matched on visitor, host and start rather than on id, as a change names the + # parent booking while the invitation names the child. protected def recently_invited?(visitor_email : String, host_email : String, event_start : Int64, changed : String?) : Bool - key = invite_key(visitor_email, host_email, event_start) + visitor = visitor_email.strip.downcase + host = host_email.strip.downcase now = Time.monotonic @recent_invites_lock.synchronize do - invite = @recent_invites[key]? - next false unless invite - - if invite.expires <= now - @recent_invites.delete(key) - next false + @recent_invites.any? do |invite| + invite.expires > now && invite.matches?(visitor, host, event_start) && invite.added_to?(changed) end - - !(invite.from_create && invite.invited_to == changed) end end - # Not keyed by id: a change names the parent booking while the invitation names - # the visitor's own child booking, and for an event-linked visitor booking the - # two don't correspond at all. Both do name the same visitor, host and start. - private def invite_key(visitor_email : String, host_email : String?, event_start : Int64) : String - "#{visitor_email.strip.downcase}\t#{host_email.to_s.strip.downcase}\t#{event_start}" - end - # Covers the debounce holding a change back, plus room for a front end that # adds its visitors in later requests. private def invite_memory : Time::Span diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index c335974781..a4305e421f 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -222,7 +222,34 @@ class StaffAPIMock < DriverSpecs::MockDriver # 600 — standalone visitor booking # 601 — event-linked visitor booking (parent_id set) # 602 — group-event booking + # Group visitors get their own booking beneath a group parent, which is how the + # driver tells "added to the group by this edit" from "this is the visit". + # 305/306/307 — children of group parent 300 + # 600 — child of group parent 599 + # 801 — child of group container 800 + GROUP_CHILDREN = { + 305_i64 => 300_i64, + 306_i64 => 300_i64, + 307_i64 => 300_i64, + 600_i64 => 599_i64, + 801_i64 => 800_i64, + } + def get_booking(booking_id : Int64, instance : Int64? = nil) + if parent = GROUP_CHILDREN[booking_id]? + return { + id: booking_id, + parent_id: parent, + booking_type: "visitor", + booking_start: 0, + booking_end: 0, + resource_id: "visitor@external.com", + user_email: "host@example.com", + title: "Group Member Visit", + extension_data: {} of String => String, + } + end + case booking_id when 601_i64 { @@ -3036,4 +3063,248 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do relocate_emails.should contain "visitor@external.com|booking" relocate_emails.should contain "visitor@external.com|booking_changed" relocate_emails.size.should eq 2 + + # ------------------------------------------------------------------ + # Test 59: the pre-unification event_change_debounce is still read + # ------------------------------------------------------------------ + # + # A deployment that set the old name must not silently fall back to the + # default, which would both delay and start filtering its notifications. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + event_change_debounce: 0, + domain_uri: "https://example.com/", + }) + sleep 1.0 + + count_before_legacy = system(:Mailer)[:send_count].as_i + + publish("staff/booking/changed", { + action: "changed", + id: 100_i64, + booking_type: "desk", + booking_start: now + 86400, + booking_end: now + 90000, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host-legacy@example.com", + title: "Legacy Debounce", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 82800, + previous_booking_end: now + 86400, + }.to_json) + + sleep 1.0 + + system(:Mailer)[:send_count].should eq count_before_legacy + 1 + system(:Mailer)[:last_template].should eq ["visitor_invited", "booking_changed"] + + # ------------------------------------------------------------------ + # Test 60: two visits for the same visitor, host and start time + # ------------------------------------------------------------------ + # + # Each invitation has to be remembered on its own. Collapsing them onto one + # record let the later invitation stand in for the earlier one, so changing + # the first visit was mistaken for adding the visitor to it. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 2, + domain_uri: "https://example.com/", + }) + sleep 1.0 + + sent_before_two_visits = system(:Mailer)[:emails_sent].as_a.size + + [700_i64, 701_i64].each do |booking_id| + publish("staff/guest/attending", { + action: "booking_created", + id: 6_i64, + booking_id: booking_id, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Concurrent Visit", + event_summary: "Concurrent Visit", + event_starting: now + 93600, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-two-visits@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + sleep 1.0 + end + + # the first of the two visits is then moved + publish("staff/booking/changed", { + action: "metadata_changed", + id: 700_i64, + booking_type: "visitor", + booking_start: now + 93600, + booking_end: now + 97200, + timezone: "GMT", + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + user_email: "host-two-visits@example.com", + title: "Concurrent Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 93600, + previous_booking_end: now + 97200, + previous_zones: ["zone-old-building", "zone-old-room"], + }.to_json) + + sleep 8.0 + + two_visit_emails = system(:Mailer)[:emails_sent].as_a[sent_before_two_visits..].map(&.as_s) + + # the other visit's invitation must not stand in for this one + two_visit_emails.should contain "visitor@external.com|booking_changed" + two_visit_emails.count("visitor@external.com|booking").should eq 2 + + # ------------------------------------------------------------------ + # Test 61: a visitor invited to both a group container and their own + # child booking, in either order + # ------------------------------------------------------------------ + # + # Editing a group pushes the member's attendee onto the container as well, so + # the visitor is announced twice for the one visit. Which announcement landed + # last must not decide whether their child booking's move reaches them. + + sent_before_leak = system(:Mailer)[:emails_sent].as_a.size + + # the child booking first, then the container — the order that used to lose + publish("staff/guest/attending", { + action: "booking_created", + id: 7_i64, + booking_id: 801_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Group Leak", + event_summary: "Group Leak", + event_starting: now + 100800, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-leak@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + publish("staff/guest/attending", { + action: "booking_updated", + id: 8_i64, + booking_id: 800_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Group Leak", + event_summary: "Group Leak", + event_starting: now + 100800, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-leak@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + # the child booking is moved + publish("staff/booking/changed", { + action: "metadata_changed", + id: 801_i64, + booking_type: "visitor", + booking_start: now + 100800, + booking_end: now + 104400, + timezone: "GMT", + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + user_email: "host-leak@example.com", + title: "Group Leak", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 100800, + previous_booking_end: now + 104400, + previous_zones: ["zone-old-building", "zone-old-room"], + }.to_json) + + sleep 8.0 + + leak_emails = system(:Mailer)[:emails_sent].as_a[sent_before_leak..].map(&.as_s) + + leak_emails.should contain "visitor@external.com|booking_changed" + + # ------------------------------------------------------------------ + # Test 62: a later, unrelated invitation must not displace the one + # that shows the visitor being added to the group + # ------------------------------------------------------------------ + # + # Keeping a single invitation per visitor lost whichever arrived first, so an + # unrelated visit at the same time could mask the fact that this edit is what + # added them to the group being changed. + + sent_before_evict = system(:Mailer)[:emails_sent].as_a.size + + # added to group 300 by this edit, via their own child booking + publish("staff/guest/attending", { + action: "booking_created", + id: 9_i64, + booking_id: 305_i64, + resource_id: "visitor-b@external.com", + resource_ids: ["visitor-b@external.com"], + event_title: "Group Visit", + event_summary: "Group Visit", + event_starting: now + 108000, + attendee_name: "Visitor B", + attendee_email: "visitor-b@external.com", + host: "host-evict@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + # and separately invited to an unrelated visit at the same time + publish("staff/guest/attending", { + action: "booking_created", + id: 10_i64, + booking_id: 700_i64, + resource_id: "visitor-b@external.com", + resource_ids: ["visitor-b@external.com"], + event_title: "Unrelated Visit", + event_summary: "Unrelated Visit", + event_starting: now + 108000, + attendee_name: "Visitor B", + attendee_email: "visitor-b@external.com", + host: "host-evict@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + publish("staff/booking/changed", { + action: "changed", + id: 300_i64, + booking_type: "group", + booking_start: now + 108000, + booking_end: now + 111600, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host-evict@example.com", + title: "Group Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 104400, + previous_booking_end: now + 108000, + }.to_json) + + sleep 8.0 + + evict_emails = system(:Mailer)[:emails_sent].as_a[sent_before_evict..].map(&.as_s) + + # the visitor already on the group is told + evict_emails.should contain "visitor-a@external.com|booking_changed" + # the one this edit added is not, despite the later unrelated invitation + evict_emails.should_not contain "visitor-b@external.com|booking_changed" end