diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index e31fe762c3..fa28e808ff 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -34,14 +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 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): + # 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, + # 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, @@ -72,9 +73,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/", @@ -167,12 +168,18 @@ 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 - @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 + # Coalescing buffer for staff/{event,booking}/changed, swept once the window + # elapses. seconds to buffer a change; 0 emails on every signal + @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 + + # 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 @jwt_private_key : String = PlaceOS::Model::JWTBase.private_key @@ -193,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" - @event_change_debounce = setting?(Int32, :event_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 @@ -227,7 +236,7 @@ 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 + flush_pending_changes("debounce disabled") if @change_debounce <= 0 schedule.clear if reminders = @send_reminders @@ -235,7 +244,7 @@ class Place::VisitorMailer < PlaceOS::Driver 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 + schedule.every(@change_debounce.clamp(1, 5).seconds) { sweep_pending_changes } if @change_debounce > 0 spawn { ensure_building_zone(zones) } end @@ -243,7 +252,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 @@ -377,10 +386,15 @@ class Place::VisitorMailer < PlaceOS::Driver room = get_room_details(guest_details.system_id) 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?) @@ -397,18 +411,33 @@ 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 + # tracked apart from error_count to pinpoint a missing invite + self[:visitor_email_errors] = @visitor_email_errors += 1 + raise error + end + + self[:visitor_emails_sent] = @visitor_emails_sent += 1 + + # 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, invited_under) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -639,13 +668,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 +727,16 @@ 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, + # 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( + 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, ) + @change_debounce > 0 ? buffer_change(change) : dispatch_booking_change(change) rescue error logger.error { error.inspect_with_backtrace } self[:error_count] = @error_count += 1 @@ -816,7 +815,7 @@ class Place::VisitorMailer < PlaceOS::Driver 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) + @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 +835,130 @@ class Place::VisitorMailer < PlaceOS::Driver attendee_domain.downcase == host_domain.downcase end + # 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, invited_under : String?) : Nil + now = Time.monotonic + invite = Invite.new( + 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! 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 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 + visitor = visitor_email.strip.downcase + host = host_email.strip.downcase + now = Time.monotonic + + @recent_invites_lock.synchronize do + @recent_invites.any? do |invite| + invite.expires > now && invite.matches?(visitor, host, event_start) && invite.added_to?(changed) + end + end + 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 + @change_debounce.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", older_than: Time.monotonic - @change_debounce.seconds) 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 + private def flush_pending_changes(reason : String, older_than : Time::Span? = nil, wait : Time::Span? = nil) : Nil + flushing = @pending_changes_lock.synchronize do ready = if cutoff = older_than - @pending_event_changes.values.select { |pending| pending.first_seen <= cutoff } + @pending_changes.values.select { |pending| pending.first_seen <= cutoff } 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 +974,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 +1110,13 @@ 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 edit that their visit changed — their + # invitation already carries these details (PPT-2375) + 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 + 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 +1371,63 @@ 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 can't collide. + 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 = "" + + def initialize(@host, @title, @current_start, @current_end, @previous_start, @previous_end) + 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( @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(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}" end # Whether this signal reports the event changing rooms. @@ -1282,20 +1435,69 @@ 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( + @booking_id, + @booking_type, + host, + title, + @resource_id, + current_start, + current_end, + previous_start, + previous_end, + @zones, + @previous_zones, + ) + super(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..5efb26c1e2 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 @@ -24,42 +24,36 @@ 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 ``` -## Debouncing event changes +## Change notifications -A single calendar 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 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. -`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. +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. - event_change_debounce: 15 + # Combine change emails sent within this many seconds; 0 disables. + 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 -debounce is turned off, so a restart never silently drops a pending notification. +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. -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. +Setting this 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 are announced by the staff API exactly like external guests and -would 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: @@ -76,25 +70,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 66c97c2aec..a4305e421f 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 @@ -209,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 { @@ -271,6 +311,17 @@ 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 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({ + 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 +742,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, - send_reminders: "0 7 * * *", - domain_uri: "https://example.com/", + change_debounce: 0, + send_reminders: "0 7 * * *", + domain_uri: "https://example.com/", }) sleep 1.0 @@ -1201,7 +1252,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, + change_debounce: 0, }) sleep 1.0 @@ -1404,11 +1455,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, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + skip_host_email: false, + change_debounce: 0, }) sleep 1.0 @@ -1446,6 +1497,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do invite_zone_tag: "building", notify_induction_accepted_template: "custom_accepted", notify_induction_declined_template: "custom_declined", + change_debounce: 0, }) sleep 1.0 @@ -1509,7 +1561,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, + change_debounce: 0, }) sleep 1.0 @@ -1581,6 +1633,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) + change_debounce: 0, }) sleep 1.0 @@ -1658,6 +1711,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_space_name: "Client Floor", invite_zone_tag: "building", skip_event_linked_booking_email: false, + change_debounce: 0, }) sleep 1.0 @@ -1682,6 +1736,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do timezone: "GMT", booking_space_name: "Client Floor", invite_zone_tag: "building", + change_debounce: 0, }) sleep 1.0 @@ -1745,7 +1800,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) @@ -1753,10 +1808,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, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 3, }) sleep 1.0 @@ -1861,10 +1916,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, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 3, }) # the update must not cut the window short @@ -2028,10 +2083,10 @@ 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", + change_debounce: 30, }) sleep 1.0 @@ -2060,10 +2115,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, + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, }) sleep 1.5 @@ -2207,11 +2262,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, - 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 @@ -2325,12 +2380,12 @@ 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", + change_debounce: 0, + disable_qr_code: true, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2383,12 +2438,12 @@ 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", + change_debounce: 0, + host_domain_filter: ["example.com"], + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2422,7 +2477,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do timezone: "GMT", booking_space_name: "Client Floor", invite_zone_tag: "building", - event_change_debounce: 0, + change_debounce: 0, skip_internal_domain_email: true, domain_uri: "https://example.com/", }) @@ -2500,11 +2555,11 @@ 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", + change_debounce: 0, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2548,11 +2603,11 @@ 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", + change_debounce: 0, + domain_uri: "https://example.com/", }) sleep 1.0 @@ -2582,4 +2637,674 @@ 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 + # ================================================================== + # + # 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. + # + # Assertions read system(:Mailer)[:emails_sent] from a noted index, giving + # "recipient|template" for just that test's emails. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + 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 following the one that 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", + change_debounce: 2, + 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 + + # ------------------------------------------------------------------ + # Test 57: a re-created event-linked visitor booking is not an + # invitation, so it must not suppress the change email + # ------------------------------------------------------------------ + # + # 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", + 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 + + # ------------------------------------------------------------------ + # 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. A location change leaves the times alone, so the two + # are indistinguishable 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 + + # ------------------------------------------------------------------ + # 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