From e616060c77344fe501ec72754cb2cb692137aa38 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 27 Aug 2026 12:10:41 +0930 Subject: [PATCH 01/16] fix(visitor_mailer): refresh the building zone instead of memoising it (PPT-2375) --- drivers/place/visitor_mailer.cr | 24 +++++++++++++++--- drivers/place/visitor_mailer_spec.cr | 38 +++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index fa28e808ff..509f8396a7 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -132,9 +132,8 @@ class Place::VisitorMailer < PlaceOS::Driver @time_format : String = "%l:%M%p" @date_format : String = "%A, %-d %B" - getter building_zone : ZoneDetails do - find_building(control_system_zone_list) - end + @building_zone : ZoneDetails? = nil + @building_zone_id : String? = nil getter parent_zone_ids : Array(String) = [] of String @booking_space_name : String = "Client Floor" @@ -266,11 +265,30 @@ class Place::VisitorMailer < PlaceOS::Driver schedule.in(5.seconds) { ensure_building_zone(zones) } end + # Resolved through the zone cache on every use, so a building renamed in + # backoffice reaches the emails within `zone_cache_timeout` rather than + # surviving until the driver next reloads. + def building_zone : ZoneDetails + if zone_id = @building_zone_id + begin + return fetch_zone(zone_id) + rescue error + logger.warn(exception: error) { "error refreshing building zone #{zone_id}" } + # last known good, an email is better than no email + if known = @building_zone + return known + end + end + end + find_building(control_system_zone_list) + end + protected def find_building(zones : Array(String)) : ZoneDetails zones.each do |zone_id| zone = fetch_zone(zone_id) if zone.tags.includes?(@invite_zone_tag) @building_zone = zone + @building_zone_id = zone.id if @is_parent_zone && (child_zones = Array(ZoneDetails).from_json(staff_api.zones(parent: zone_id).get_json)) @parent_zone_ids = child_zones.map(&.id) else diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index a4305e421f..50e9186e62 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -126,7 +126,8 @@ class StaffAPIMock < DriverSpecs::MockDriver self[:zone_lookups] = self[:zone_lookups].as_i + 1 case id when "zone-building" - BUILDING_ZONE + # a spec can rename the building the way backoffice would + BUILDING_ZONE.merge({display_name: self[:building_display_name]?.try(&.as_s) || "Main Building"}) when "zone-old-building" OLD_BUILDING_ZONE when "zone-room" @@ -3307,4 +3308,39 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do 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" + + # ------------------------------------------------------------------ + # Test 63: a building renamed in backoffice reaches the emails + # ------------------------------------------------------------------ + # + # The building zone was resolved once and memoised for the life of the driver, + # so a rename never reached an email until the driver was reloaded, and + # clear_zone_cache could not shift it either. + + system(:StaffAPI)[:building_display_name] = "Renamed Building" + exec(:clear_zone_cache).get + + publish("staff/guest/attending", { + action: "booking_created", + id: 11_i64, + booking_id: 900_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Renamed Building Visit", + event_summary: "Renamed Building Visit", + event_starting: now + 115200, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-rename@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + system(:Mailer)[:last_template].should eq ["visitor_invited", "booking"] + system(:Mailer)[:last_args]["building_name"].should eq "Renamed Building" + + # leave the mock as the rest of the suite expects it + system(:StaffAPI)[:building_display_name] = "Main Building" + exec(:clear_zone_cache).get end From 7b7f6f79273fecf544613cea9cd6e89e0dc284bd Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 27 Aug 2026 12:46:33 +0930 Subject: [PATCH 02/16] fix(visitor_mailer): name the building the visit is in (PPT-2375) --- drivers/place/visitor_mailer.cr | 79 +++++++++++++++++++--------- drivers/place/visitor_mailer_spec.cr | 65 +++++++++++++++++++++++ 2 files changed, 118 insertions(+), 26 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 509f8396a7..b3a069a73a 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -321,6 +321,25 @@ class Place::VisitorMailer < PlaceOS::Driver end end + # The building a visit is in, named from the zones the signal carries rather + # than the system's own zone, so a campus driver (and a visit that moved + # buildings) names the building the visitor is expected at. + protected def building_name_for(zones : Array(String)?) : String + if zones + # a campus building is the more specific answer than the campus itself + candidates = @parent_zone_ids.empty? ? zones : (zones & @parent_zone_ids) + zones + candidates.each do |zone_id| + begin + zone = fetch_zone(zone_id) + return zone.display_name.presence || zone.name if zone.tags.includes?(@invite_zone_tag) + rescue error + logger.warn(exception: error) { "error looking up zone #{zone_id}" } + end + end + end + building_zone.display_name.presence || building_zone.name + end + protected def guest_event(payload) logger.debug { "received guest event payload: #{payload}" } guest_details = GuestNotification.from_json payload @@ -368,7 +387,8 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.attendee_name, guest_details.host, guest_details.event_title || guest_details.event_summary, - guest_details.event_starting + guest_details.event_starting, + building_name_for(guest_details.zones) ) self[:users_checked_in] = @users_checked_in += 1 return @@ -381,7 +401,8 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.host, guest_details.event_title || guest_details.event_summary, guest_details.event_starting, - guest_details.induction + guest_details.induction, + building_name_for(guest_details.zones) ) self[:users_accepted_induction] = @users_accepted_induction += 1 elsif guest_details.induction.declined? @@ -392,7 +413,8 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.host, guest_details.event_title || guest_details.event_summary, guest_details.event_starting, - guest_details.induction + guest_details.induction, + building_name_for(guest_details.zones) ) self[:users_declined_induction] = @users_declined_induction += 1 end @@ -443,6 +465,7 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.event_id, area_name, system_id: guest_details.responds_to?(:system_id) ? guest_details.system_id : nil, + building_name: building_name_for(guest_details.zones), ) rescue error # tracked apart from error_count to pinpoint a missing invite @@ -474,6 +497,7 @@ class Place::VisitorMailer < PlaceOS::Driver host_email : String?, event_title : String?, event_start : Int64, + building_name : String? = nil, ) local_start_time = Time.unix(event_start).in(@time_zone) @@ -485,7 +509,7 @@ class Place::VisitorMailer < PlaceOS::Driver visitor_name: visitor_name, host_name: get_host_name(host_email), host_email: host_email, - building_name: building_zone.display_name.presence || building_zone.name, + building_name: building_name || building_name_for(nil), event_title: event_title, event_start: local_start_time.to_s(@time_format), event_date: local_start_time.to_s(@date_format), @@ -504,6 +528,7 @@ class Place::VisitorMailer < PlaceOS::Driver event_title : String?, event_start : Int64, induction_status : Induction, + building_name : String? = nil, ) local_start_time = Time.unix(event_start).in(@time_zone) @@ -515,7 +540,7 @@ class Place::VisitorMailer < PlaceOS::Driver visitor_name: visitor_name, host_name: get_host_name(host_email), host_email: host_email, - building_name: building_zone.display_name.presence || building_zone.name, + building_name: building_name || building_name_for(nil), event_title: event_title, event_start: local_start_time.to_s(@time_format), event_date: local_start_time.to_s(@date_format), @@ -546,6 +571,7 @@ class Place::VisitorMailer < PlaceOS::Driver details.new_host_email, details.event_title || details.event_summary, details.event_starting, + building_name_for(details.zones), ) rescue error logger.error { error.inspect_with_backtrace } @@ -564,6 +590,7 @@ class Place::VisitorMailer < PlaceOS::Driver new_host_email : String, event_title : String?, event_start : Int64?, + building_name : String? = nil, ) # A host can be reassigned via a metadata-only update that carries no event # timing, so render the date/time only when a start time is available. @@ -577,7 +604,7 @@ class Place::VisitorMailer < PlaceOS::Driver previous_host_name: get_host_name(previous_host_email), new_host_email: new_host_email, new_host_name: get_host_name(new_host_email), - building_name: building_zone.display_name.presence || building_zone.name, + building_name: building_name || building_name_for(nil), event_title: event_title, event_date: local_start_time.try(&.to_s(@date_format)), event_time: local_start_time.try(&.to_s(@time_format)), @@ -801,6 +828,7 @@ class Place::VisitorMailer < PlaceOS::Driver host, details.title, event_start, + building_name_for(details.zones), ) end @@ -1004,27 +1032,24 @@ class Place::VisitorMailer < PlaceOS::Driver # 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 + # named from the booking's own zones, so a booking moved to another building + # is announced as being in the building it moved to + building_name = building_name_for(change.zones) + + # Resolve previous location names from previous zones, defaulting to the + # current ones so a date/time-only edit reads as the same place. + previous_zones = change.previous_zones + previous_building_name = previous_zones ? building_name_for(previous_zones) : building_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 + previous_zones.try &.each do |zone_id| + begin + zone = fetch_zone(zone_id) + next if zone.tags.includes?(@invite_zone_tag) + previous_room_name = zone.display_name.presence || zone.name + break + rescue error + logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } end end @@ -1041,6 +1066,7 @@ class Place::VisitorMailer < PlaceOS::Driver change.previous_start, previous_building_name, previous_room_name, + building_name, event_id: change.booking_id.to_s, resource_id: change.resource_id, ) @@ -1235,6 +1261,7 @@ class Place::VisitorMailer < PlaceOS::Driver event_end : Int64? = nil, system_id : String? = nil, + building_name : String? = nil, ) local_start_time = Time.unix(event_start).in(@time_zone) @@ -1283,7 +1310,7 @@ class Place::VisitorMailer < PlaceOS::Driver host_name: get_host_name(host_email), host_email: host_email, room_name: area_name, - building_name: building_zone.display_name.presence || building_zone.name, + building_name: building_name || building_name_for(nil), event_title: event_title, event_start: local_start_time.to_s(@time_format), event_date: local_start_time.to_s(@date_format), diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 50e9186e62..e2bb9b5f72 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -91,6 +91,16 @@ class StaffAPIMock < DriverSpecs::MockDriver parent_id: "zone-org", } + # a second building under the same campus as BUILDING_ZONE + SECOND_BUILDING_ZONE = { + id: "zone-building2", + name: "Building Two", + display_name: "Second Building", + location: "", + tags: ["building"], + parent_id: "zone-building", + } + ROOM_ZONE = { id: "zone-room", name: "Room 101", @@ -130,6 +140,8 @@ class StaffAPIMock < DriverSpecs::MockDriver BUILDING_ZONE.merge({display_name: self[:building_display_name]?.try(&.as_s) || "Main Building"}) when "zone-old-building" OLD_BUILDING_ZONE + when "zone-building2" + SECOND_BUILDING_ZONE when "zone-room" ROOM_ZONE when "zone-old-room" @@ -142,6 +154,17 @@ class StaffAPIMock < DriverSpecs::MockDriver end end + # only used when the driver is configured as a campus + def zones( + q : String? = nil, + limit : Int32 = 1000, + offset : Int32 = 0, + parent : String? = nil, + tags : Array(String) | String? = nil, + ) + parent ? [SECOND_BUILDING_ZONE, OLD_BUILDING_ZONE] : [] of typeof(BUILDING_ZONE) + end + # When include_linked is true, parent group bookings (e.g. id 300) return # guests from all child bookings in a single response — just like the real # staff-api endpoint. @@ -3343,4 +3366,46 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # leave the mock as the rest of the suite expects it system(:StaffAPI)[:building_display_name] = "Main Building" exec(:clear_zone_cache).get + + # ------------------------------------------------------------------ + # Test 64: a booking moved to another building of the same campus + # ------------------------------------------------------------------ + # + # The new location was always described as the building the driver's own + # system sits in, so a campus wide driver announced the move using the + # building the visit had just left. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + is_campus: true, + change_debounce: 0, + domain_uri: "https://example.com/", + }) + sleep 1.5 + + publish("staff/booking/changed", { + action: "changed", + id: 950_i64, + booking_type: "visitor", + booking_start: now + 122400, + booking_end: now + 126000, + timezone: "GMT", + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + user_email: "host-campus@example.com", + title: "Campus Move", + zones: ["zone-building2", "zone-room"], + previous_booking_start: now + 118800, + previous_booking_end: now + 122400, + previous_zones: ["zone-old-building", "zone-old-room"], + }.to_json) + + sleep 1.5 + + move_building_args = system(:Mailer)[:last_args] + move_building_args["event_title"].should eq "Campus Move" + move_building_args["building_name"].should eq "Second Building" + move_building_args["previous_building_name"].should eq "Previous Building" end From 87a24c18599b81086ca6bfb6dab916a6e894659b Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 27 Aug 2026 12:58:36 +0930 Subject: [PATCH 03/16] fix(visitor_mailer): stop emailing visitors removed from the visit (PPT-2375) --- drivers/place/visitor_mailer.cr | 18 +++++++++ drivers/place/visitor_mailer_spec.cr | 59 ++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index b3a069a73a..56485b2cb5 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -1145,6 +1145,14 @@ class Place::VisitorMailer < PlaceOS::Driver visitor_email = guest["email"].as_s visitor_name = guest["name"].as_s? + # a visitor removed from the visit keeps their (soft deleted) booking, and + # the guest list of a group still aggregates it, so they would otherwise be + # told about a visit they are no longer part of (PPT-2375) + if no_longer_attending?(guest) + logger.debug { "skipping #{template} email to #{visitor_email} as they are no longer attending" } + next + end + # don't email the host their own booking_changed notification. next if @skip_host_email && visitor_email.downcase == host_email.downcase @@ -1217,6 +1225,16 @@ class Place::VisitorMailer < PlaceOS::Driver end end + # Whether a guest from a booking or event guest list is no longer attending: + # their attendance was withdrawn, or the booking they attend was cancelled. + private def no_longer_attending?(guest : JSON::Any) : Bool + return true if guest["visit_expected"]?.try(&.as_bool?) == false + + booking = guest["booking"]? + return false unless booking + !!(booking["deleted"]?.try(&.as_bool?) || booking["rejected"]?.try(&.as_bool?)) + end + # Returns `{room_name, building_name}` for `system_id`, falling back to the # supplied values (and logging a warning) if any lookup fails. private def resolve_system_location_names(system_id : String, fallback_room : String, fallback_building : String) : {String, String} diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index e2bb9b5f72..d9f16408de 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -179,6 +179,19 @@ class StaffAPIMock < DriverSpecs::MockDriver else [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool) end + when 310 + # A group where one visitor was removed: the front end deletes their child + # booking, which staff-api only marks as deleted, so the aggregated guest + # list still returns them. A third guest is no longer expected to visit. + if include_linked + [ + {email: "visitor-a@external.com", name: "Visitor A", checked_in: false, visit_expected: true, booking: {id: 311_i64, deleted: false}}, + {email: "visitor-gone@external.com", name: "Visitor Gone", checked_in: false, visit_expected: true, booking: {id: 312_i64, deleted: true}}, + {email: "visitor-unexpected@external.com", name: "Visitor Unexpected", checked_in: false, visit_expected: false, booking: {id: 313_i64, deleted: false}}, + ] + else + [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool, booking: NamedTuple(id: Int64, deleted: Bool)) + end when 301 # Simulates the host being stored as a visit_expected attendee # alongside a real external visitor (mirrors what events.cr does @@ -3408,4 +3421,50 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do move_building_args["event_title"].should eq "Campus Move" move_building_args["building_name"].should eq "Second Building" move_building_args["previous_building_name"].should eq "Previous Building" + + # ------------------------------------------------------------------ + # Test 65: a visitor removed by the same edit is not told about it + # ------------------------------------------------------------------ + # + # Removing a visitor deletes their child booking, but a soft deleted booking + # is still aggregated into the group's guest list, so the visitor kept being + # emailed about a visit they had been taken off. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, + domain_uri: "https://example.com/", + }) + sleep 1.5 + + sent_before_removed = system(:Mailer)[:emails_sent].as_a.size + + publish("staff/booking/changed", { + action: "changed", + id: 310_i64, + booking_type: "group", + booking_start: now + 129600, + booking_end: now + 133200, + timezone: "GMT", + resource_id: "host-removed@example.com[2026-05-15]", + resource_ids: ["host-removed@example.com[2026-05-15]"], + user_email: "host-removed@example.com", + title: "Visitor Removed", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 126000, + previous_booking_end: now + 129600, + }.to_json) + + sleep 1.5 + + removed_emails = system(:Mailer)[:emails_sent].as_a[sent_before_removed..].map(&.as_s) + + # the visitor still on the booking is told + removed_emails.should contain "visitor-a@external.com|booking_changed" + # the one whose booking was cancelled by this edit is not + removed_emails.should_not contain "visitor-gone@external.com|booking_changed" + # neither is one who is no longer expected to visit + removed_emails.should_not contain "visitor-unexpected@external.com|booking_changed" end From d65b8226460bdefc52c9beb41e707df5c2510fd0 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 27 Aug 2026 13:17:25 +0930 Subject: [PATCH 04/16] fix(visitor_mailer): email the original host once per reassignment (PPT-2375) --- drivers/place/visitor_mailer.cr | 58 ++++++++++++++++++++++++++-- drivers/place/visitor_mailer_spec.cr | 50 ++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 4 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 56485b2cb5..28da31da06 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -180,6 +180,11 @@ class Place::VisitorMailer < PlaceOS::Driver @recent_invites : Array(Invite) = [] of Invite @recent_invites_lock : Mutex = Mutex.new + # Emails already sent, so one edit doesn't repeat them: editing a group + # booking signals the container and every child booking of it separately. + @sent_notices : Array(SentNotice) = [] of SentNotice + @sent_notices_lock : Mutex = Mutex.new + @uri : URI = URI.new @jwt_private_key : String = PlaceOS::Model::JWTBase.private_key @@ -565,8 +570,7 @@ class Place::VisitorMailer < PlaceOS::Driver end end - send_original_host_email( - @notify_original_host_template, + notify_original_host( details.previous_host_email, details.new_host_email, details.event_title || details.event_summary, @@ -583,6 +587,36 @@ class Place::VisitorMailer < PlaceOS::Driver } end + # Tells the previous host their booking was reassigned, once per reassignment: + # a group booking reassigns its container and every child booking of it, each + # signalling the same change (PPT-2375). + protected def notify_original_host( + previous_host_email : String, + new_host_email : String, + event_title : String?, + event_start : Int64?, + building_name : String, + ) : Nil + key = { + @notify_original_host_template, previous_host_email.strip.downcase, + new_host_email.strip.downcase, event_title, event_start, building_name, + }.join('\t') + + unless first_send?(key) + logger.debug { "skipping host reassigned email to #{previous_host_email}, already sent" } + return + end + + send_original_host_email( + @notify_original_host_template, + previous_host_email, + new_host_email, + event_title, + event_start, + building_name, + ) + end + @[Security(Level::Support)] def send_original_host_email( template : String, @@ -822,8 +856,7 @@ class Place::VisitorMailer < PlaceOS::Driver # A host can be reassigned without any change to the event timing; the host # email still renders (date/time blank only if the lookup also came up empty). if (prev_host = details.previous_host_email) && prev_host.downcase != host.downcase - send_original_host_email( - @notify_original_host_template, + notify_original_host( prev_host, host, details.title, @@ -955,6 +988,23 @@ class Place::VisitorMailer < PlaceOS::Driver @change_debounce.clamp(0, 3600).seconds + 60.seconds end + # An email we've sent, keyed on what it says rather than on the booking that + # prompted it, as each signal of the same edit names a different booking. + record SentNotice, key : String, expires : Time::Span + + # Whether this is the first time we're sending it, remembering it if so. + # Expired entries go on the way in, as nothing else prunes them. + protected def first_send?(key : String) : Bool + now = Time.monotonic + + @sent_notices_lock.synchronize do + @sent_notices.reject! { |notice| notice.expires <= now } + return false if @sent_notices.any? { |notice| notice.key == key } + @sent_notices << SentNotice.new(key, now + invite_memory) + true + end + end + # Collapses the burst of signals for one edit into a single buffered change. # 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. diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index d9f16408de..27f7881307 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -3467,4 +3467,54 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do removed_emails.should_not contain "visitor-gone@external.com|booking_changed" # neither is one who is no longer expected to visit removed_emails.should_not contain "visitor-unexpected@external.com|booking_changed" + + # ------------------------------------------------------------------ + # Test 66: one reassignment sends the original host one email + # ------------------------------------------------------------------ + # + # Reassigning a group booking updates its container and every child booking of + # it, and each of those signals the same reassignment, so the previous host + # was emailed once per booking the edit touched. + + sent_before_host_dupe = system(:Mailer)[:emails_sent].as_a.size + + [960_i64, 961_i64, 962_i64].each do |booking_id| + publish("staff/booking/host_changed", { + action: "host_changed", + booking_id: booking_id, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Reassigned Group Visit", + event_summary: "Reassigned Group Visit", + event_starting: now + 136800, + previous_host_email: "old-host-group@example.com", + new_host_email: "new-host-group@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + sleep 0.5 + end + + sleep 1.0 + + host_dupe_emails = system(:Mailer)[:emails_sent].as_a[sent_before_host_dupe..].map(&.as_s) + host_dupe_emails.count("old-host-group@example.com|notify_original_host").should eq 1 + + # a different reassignment is still its own email + publish("staff/booking/host_changed", { + action: "host_changed", + booking_id: 963_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Reassigned Group Visit", + event_summary: "Reassigned Group Visit", + event_starting: now + 136800, + previous_host_email: "other-old-host@example.com", + new_host_email: "new-host-group@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + system(:Mailer)[:last_to].should eq "other-old-host@example.com" + system(:Mailer)[:last_template].should eq ["visitor_invited", "notify_original_host"] end From dc26c33d24c5071a0c1169fb30436c24deb29f10 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 27 Aug 2026 13:45:22 +0930 Subject: [PATCH 05/16] fix(visitor_mailer): tell a visitor about a change once (PPT-2375) --- drivers/place/visitor_mailer.cr | 42 +++++++++--- drivers/place/visitor_mailer_spec.cr | 99 +++++++++++++++++++++++++++- 2 files changed, 130 insertions(+), 11 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 28da31da06..582755e56f 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -607,14 +607,20 @@ class Place::VisitorMailer < PlaceOS::Driver return end - send_original_host_email( - @notify_original_host_template, - previous_host_email, - new_host_email, - event_title, - event_start, - building_name, - ) + begin + send_original_host_email( + @notify_original_host_template, + previous_host_email, + new_host_email, + event_title, + event_start, + building_name, + ) + rescue error + # a repeat signal is the only retry there is + forget_send(key) + raise error + end end @[Security(Level::Support)] @@ -1005,6 +1011,11 @@ class Place::VisitorMailer < PlaceOS::Driver end end + # Forget an email that turned out not to have been sent. + protected def forget_send(key : String) : Nil + @sent_notices_lock.synchronize { @sent_notices.reject! { |notice| notice.key == key } } + end + # Collapses the burst of signals for one edit into a single buffered change. # 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. @@ -1219,6 +1230,19 @@ class Place::VisitorMailer < PlaceOS::Driver next end + # one edit signals the group container and every child booking of it, each + # describing the same change to the same visitors (PPT-2375) + notice_key = { + template, visitor_email.strip.downcase, host_email.strip.downcase, event_title, + event_start, previous_start, resolved_room_name, resolved_building_name, + previous_room_name, previous_building_name, + }.join('\t') + + unless first_send?(notice_key) + logger.debug { "skipping #{template} email to #{visitor_email}, already sent" } + 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) } @@ -1271,6 +1295,8 @@ class Place::VisitorMailer < PlaceOS::Driver reply_to: host_email.presence, ) rescue error + # a repeat signal is the only retry there is + forget_send(notice_key) if notice_key logger.warn(exception: error) { "failed to send booking_changed email to #{visitor_email}" } end end diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 27f7881307..23da7ef817 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -179,6 +179,16 @@ class StaffAPIMock < DriverSpecs::MockDriver else [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool) end + when 320 + # a group container and, below, the child booking each of its visitors has + include_linked ? [ + {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}, + ] : [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool) + when 321 + [{email: "visitor-a@external.com", name: "Visitor A", checked_in: false, visit_expected: true}] + when 322 + [{email: "visitor-b@external.com", name: "Visitor B", checked_in: false, visit_expected: true}] when 310 # A group where one visitor was removed: the front end deletes their child # booking, which staff-api only marks as deleted, so the aggregated guest @@ -1513,7 +1523,22 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # in the guest list count_before_optout_bc = system(:Mailer)[:send_count].as_i - publish("staff/event/changed", event_changed_host_in_guests) + # a change of its own: repeating the one test 28 made would be a duplicate, + # and the driver only tells a visitor about a change once + publish("staff/event/changed", { + action: "update", + system_id: "sys-room1", + event_id: "evt-host-in-guests", + event_ical_uid: "ical-host-in-guests", + host: "host@example.com", + resource: "room1@example.com", + title: "Mixed Guests Meeting Rescheduled", + event_start: now + 14400, + event_end: now + 18000, + zones: ["zone-building", "zone-room"], + previous_event_start: now + 10800, + previous_event_end: now + 14400, + }.to_json) sleep 1.5 # Both host AND visitor receive the booking_changed email @@ -1754,7 +1779,24 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do count_before_optout_linked = system(:Mailer)[:send_count].as_i - publish("staff/booking/changed", linked_booking_changed) + # a change of its own: repeating the one test 34 made would be a duplicate, + # and the driver only tells a visitor about a change once + publish("staff/booking/changed", { + action: "changed", + id: 601_i64, + booking_type: "visitor", + booking_start: now + 14400, + booking_end: now + 18000, + timezone: "GMT", + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + user_email: "host@example.com", + title: "Linked Visit Changed Again", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 10800, + previous_booking_end: now + 14400, + extension_data: {parent_id: "event-evt-200"}, + }.to_json) sleep 1.5 system(:Mailer)[:send_count].should eq count_before_optout_linked + 1 @@ -2624,7 +2666,23 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # ... and receives change notifications, as before count_before_default_change = system(:Mailer)[:send_count].as_i - publish("staff/booking/changed", internal_guest_booking) + # a change of its own: repeating the one test 48 made would be a duplicate, + # and the driver only tells a visitor about a change once + publish("staff/booking/changed", { + action: "changed", + id: 302_i64, + booking_type: "desk", + booking_start: now + 14400, + booking_end: now + 18000, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host@example.com", + title: "Internal Guest Booking Rescheduled", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 10800, + previous_booking_end: now + 14400, + }.to_json) sleep 1.5 system(:Mailer)[:send_count].should eq count_before_default_change + 2 @@ -3517,4 +3575,39 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do system(:Mailer)[:last_to].should eq "other-old-host@example.com" system(:Mailer)[:last_template].should eq ["visitor_invited", "notify_original_host"] + + # ------------------------------------------------------------------ + # Test 67: one edit of a group booking, one email per visitor + # ------------------------------------------------------------------ + # + # Rescheduling a group saves the container booking and every child booking of + # it. The container's guest list covers all of them, so each visitor was told + # about the change twice: once by the container and once by their own booking. + + sent_before_group_dupe = system(:Mailer)[:emails_sent].as_a.size + + [{320_i64, "group"}, {321_i64, "visitor"}, {322_i64, "visitor"}].each do |(booking_id, booking_type)| + publish("staff/booking/changed", { + action: "changed", + id: booking_id, + booking_type: booking_type, + booking_start: now + 144000, + booking_end: now + 147600, + timezone: "GMT", + resource_id: "host-group@example.com[2026-05-15]", + resource_ids: ["host-group@example.com[2026-05-15]"], + user_email: "host-group@example.com", + title: "Group Reschedule", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 140400, + previous_booking_end: now + 144000, + }.to_json) + sleep 0.5 + end + + sleep 1.5 + + group_dupe_emails = system(:Mailer)[:emails_sent].as_a[sent_before_group_dupe..].map(&.as_s) + group_dupe_emails.count("visitor-a@external.com|booking_changed").should eq 1 + group_dupe_emails.count("visitor-b@external.com|booking_changed").should eq 1 end From cb017735d49b901626891ca7caa26ac9ce848b8b Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 27 Aug 2026 13:57:27 +0930 Subject: [PATCH 06/16] fix(visitor_mailer): notify group event registrations of changes (PPT-2375) --- drivers/place/visitor_mailer.cr | 5 +++-- drivers/place/visitor_mailer_spec.cr | 31 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 582755e56f..6b1589619a 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -1115,8 +1115,9 @@ class Place::VisitorMailer < PlaceOS::Driver 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 + # bookings under a group parent, or a group event's registrations) are + # returned in a single request. It is ignored for a child booking. + guests = staff_api.booking_guests(change.booking_id, include_linked: change.booking_type.in?("group", "group-event")).get.as_a send_booking_changed_emails( guests, diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 23da7ef817..9ec540e21e 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -3610,4 +3610,35 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do group_dupe_emails = system(:Mailer)[:emails_sent].as_a[sent_before_group_dupe..].map(&.as_s) group_dupe_emails.count("visitor-a@external.com|booking_changed").should eq 1 group_dupe_emails.count("visitor-b@external.com|booking_changed").should eq 1 + + # ------------------------------------------------------------------ + # Test 68: a group event change reaches everyone registered for it + # ------------------------------------------------------------------ + # + # Registrations are child bookings of the group event, and only a booking + # typed "group" asked for them, so nobody who had registered was told. + + sent_before_group_event = system(:Mailer)[:emails_sent].as_a.size + + publish("staff/booking/changed", { + action: "changed", + id: 320_i64, + booking_type: "group-event", + booking_start: now + 151200, + booking_end: now + 154800, + timezone: "GMT", + resource_id: "host-group@example.com[2026-05-15]", + resource_ids: ["host-group@example.com[2026-05-15]"], + user_email: "host-group@example.com", + title: "Group Event Reschedule", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 147600, + previous_booking_end: now + 151200, + }.to_json) + + sleep 1.5 + + group_event_emails = system(:Mailer)[:emails_sent].as_a[sent_before_group_event..].map(&.as_s) + group_event_emails.should contain "visitor-a@external.com|booking_changed" + group_event_emails.should contain "visitor-b@external.com|booking_changed" end From 9589132ec8737ae0b94a01d085ea83a43cd4efce Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 27 Aug 2026 13:58:07 +0930 Subject: [PATCH 07/16] docs(visitor_mailer): document building naming and change coalescing (PPT-2375) --- drivers/place/visitor_mailer_readme.md | 27 ++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index 5efb26c1e2..3d1dd102b0 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -47,8 +47,31 @@ combined into a single email describing the net change. 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 this 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 contradictory notifications, +and can also notify visitors added by the edit, or one removed by it: an event update +is signalled before the removed attendees have been dropped from the guest list. + +Regardless of the window, the same visitor is never told the same thing twice: one edit +of a group booking saves the group and every booking beneath it, each signalling the +same change. + +## Building name + +Emails name the building the visit is in, taken from the zones on the signal, so a +driver covering a campus names the building the visitor is expected at rather than the +campus itself. Where a visit names no building, the system's own building zone is used. + +```yaml + # the zone tag identifying a building + invite_zone_tag: "building" + # the driver's zone is a campus, its child zones are the buildings + is_campus: false + # how long zone details (i.e. the building name) are cached for + zone_cache_timeout: 300 +``` + +A building renamed in backoffice reaches the emails once its cache entry expires. Call +`clear_zone_cache` to pick the new name up immediately. ## Excluding staff attendees From 5fded8a37d2a4dc6d1a04efee74cf602cfca65c5 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Tue, 1 Sep 2026 11:12:39 +0930 Subject: [PATCH 08/16] fix(visitor_mailer): render times in the zone the visit is in (PPT-2375) --- drivers/place/visitor_mailer.cr | 187 ++++++++++++++++++++++----- drivers/place/visitor_mailer_spec.cr | 122 +++++++++++++++++ drivers/place/visitor_models.cr | 11 ++ 3 files changed, 290 insertions(+), 30 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 6b1589619a..5075e89010 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -330,19 +330,102 @@ class Place::VisitorMailer < PlaceOS::Driver # than the system's own zone, so a campus driver (and a visit that moved # buildings) names the building the visitor is expected at. protected def building_name_for(zones : Array(String)?) : String - if zones - # a campus building is the more specific answer than the campus itself - candidates = @parent_zone_ids.empty? ? zones : (zones & @parent_zone_ids) + zones - candidates.each do |zone_id| - begin - zone = fetch_zone(zone_id) - return zone.display_name.presence || zone.name if zone.tags.includes?(@invite_zone_tag) - rescue error - logger.warn(exception: error) { "error looking up zone #{zone_id}" } + found = building_zone_for(zones) + found ? (found.display_name.presence || found.name) : (building_zone.display_name.presence || building_zone.name) + end + + # The zone tagged as a building that a visit belongs to, from its own zones if + # one of them is tagged, else from the parent chain of its zones (a level or + # room zone), else nil for unknown. + private def building_zone_for(zones : Array(String)?) : ZoneDetails? + return if zones.nil? + tagged = [] of ZoneDetails + zones.each do |zone_id| + begin + zone = fetch_zone(zone_id) + tagged << zone if zone.tags.includes?(@invite_zone_tag) + rescue error + logger.warn(exception: error) { "error looking up zone #{zone_id}" } + end + end + + # an org or campus zone may also carry the building tag; whichever of the + # tagged zones the others sit beneath is the actual building + if tagged.size > 1 + ancestors = Set(String).new + tagged.each do |zone| + parent_id = zone.parent_id + while parent_id && !ancestors.includes?(parent_id) + ancestors << parent_id + parent_id = fetch_zone(parent_id).parent_id + end + rescue error + logger.warn(exception: error) { "error looking up zone #{zone.id}" } + end + descendants = tagged.reject { |zone| zone.id.in?(ancestors) } + return descendants.last if descendants.size == 1 + end + return tagged.last unless tagged.empty? + + # the zones of a room or level carry no building; follow their parents up + zones.reverse_each do |zone_id| + begin + parent_id = fetch_zone(zone_id).parent_id + visited = Set(String).new + while parent_id && !visited.includes?(parent_id) + visited << parent_id + zone = fetch_zone(parent_id) + return zone if zone.tags.includes?(@invite_zone_tag) + parent_id = zone.parent_id end + rescue error + logger.warn(exception: error) { "error looking up zone #{zone_id}" } + end + end + nil + end + + # The zone a visit is in, following the parent chain upwards so a visit whose + # own zones omit the building (it is level or room only) still names one. + private def zone_with_timezone(zones : Array(String)?) : ZoneDetails? + if zone = building_zone_for(zones) + return zone if zone.timezone.presence + end + zones.try &.each do |zone_id| + begin + zone = fetch_zone(zone_id) + return zone if zone.timezone.presence + rescue error + logger.warn(exception: error) { "error looking up zone #{zone_id}" } end end - building_zone.display_name.presence || building_zone.name + nil + end + + # The calendar event's own time zone, as signalled in the nested event. + private def event_timezone(details : EventChanged) : String? + details.event.try &.timezone + end + + # The system's zone list, used to locate the building a room belongs to. + protected def resolve_system_zones(system_id : String) : Array(String)? + get_room_details(system_id).zones + rescue error + logger.warn(exception: error) { "error looking up zones for system #{system_id}" } + nil + end + + # Renders in the time zone the visit is held in: a `timezone` field on the + # signal, else the visit's building zone's timezone, else the setting (a + # deployment default of "GMT" otherwise swamps where the visit actually is). + private def visit_time_zone(signal_timezone : String?, zones : Array(String)? = nil) : Time::Location + candidate = signal_timezone.presence || + zone_with_timezone(zones).try(&.timezone.presence) || + @time_zone.name + Time::Location.load(candidate) + rescue error + logger.warn(exception: error) { "error loading time zone #{signal_timezone}" } + @time_zone end protected def guest_event(payload) @@ -393,7 +476,8 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.host, guest_details.event_title || guest_details.event_summary, guest_details.event_starting, - building_name_for(guest_details.zones) + building_name_for(guest_details.zones), + visit_time_zone(nil, guest_details.zones) ) self[:users_checked_in] = @users_checked_in += 1 return @@ -407,7 +491,8 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.event_title || guest_details.event_summary, guest_details.event_starting, guest_details.induction, - building_name_for(guest_details.zones) + building_name_for(guest_details.zones), + visit_time_zone(nil, guest_details.zones) ) self[:users_accepted_induction] = @users_accepted_induction += 1 elsif guest_details.induction.declined? @@ -419,7 +504,8 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.event_title || guest_details.event_summary, guest_details.event_starting, guest_details.induction, - building_name_for(guest_details.zones) + building_name_for(guest_details.zones), + visit_time_zone(nil, guest_details.zones) ) self[:users_declined_induction] = @users_declined_induction += 1 end @@ -471,6 +557,7 @@ class Place::VisitorMailer < PlaceOS::Driver area_name, system_id: guest_details.responds_to?(:system_id) ? guest_details.system_id : nil, building_name: building_name_for(guest_details.zones), + time_zone: visit_time_zone(nil, guest_details.zones), ) rescue error # tracked apart from error_count to pinpoint a missing invite @@ -503,8 +590,9 @@ class Place::VisitorMailer < PlaceOS::Driver event_title : String?, event_start : Int64, building_name : String? = nil, + time_zone : Time::Location? = nil, ) - local_start_time = Time.unix(event_start).in(@time_zone) + local_start_time = Time.unix(event_start).in(time_zone || @time_zone) mailer.send_template( host_email, @@ -534,8 +622,9 @@ class Place::VisitorMailer < PlaceOS::Driver event_start : Int64, induction_status : Induction, building_name : String? = nil, + time_zone : Time::Location? = nil, ) - local_start_time = Time.unix(event_start).in(@time_zone) + local_start_time = Time.unix(event_start).in(time_zone || @time_zone) mailer.send_template( host_email, @@ -576,6 +665,8 @@ class Place::VisitorMailer < PlaceOS::Driver details.event_title || details.event_summary, details.event_starting, building_name_for(details.zones), + details.timezone, + details.zones, ) rescue error logger.error { error.inspect_with_backtrace } @@ -596,6 +687,8 @@ class Place::VisitorMailer < PlaceOS::Driver event_title : String?, event_start : Int64?, building_name : String, + signal_timezone : String? = nil, + zones : Array(String)? = nil, ) : Nil key = { @notify_original_host_template, previous_host_email.strip.downcase, @@ -615,6 +708,7 @@ class Place::VisitorMailer < PlaceOS::Driver event_title, event_start, building_name, + visit_time_zone(signal_timezone, zones), ) rescue error # a repeat signal is the only retry there is @@ -631,10 +725,11 @@ class Place::VisitorMailer < PlaceOS::Driver event_title : String?, event_start : Int64?, building_name : String? = nil, + time_zone : Time::Location? = nil, ) # A host can be reassigned via a metadata-only update that carries no event # timing, so render the date/time only when a start time is available. - local_start_time = event_start.try { |timestamp| Time.unix(timestamp).in(@time_zone) } + local_start_time = event_start.try { |timestamp| Time.unix(timestamp).in(time_zone || @time_zone) } mailer.send_template( previous_host_email, @@ -820,6 +915,7 @@ class Place::VisitorMailer < PlaceOS::Driver details.resource_id, details.booking_start, details.booking_end, details.previous_booking_start, details.previous_booking_end, details.zones, details.previous_zones, + details.timezone, ) @change_debounce > 0 ? buffer_change(change) : dispatch_booking_change(change) rescue error @@ -868,6 +964,8 @@ class Place::VisitorMailer < PlaceOS::Driver details.title, event_start, building_name_for(details.zones), + event_timezone(details), + details.zones, ) end @@ -899,6 +997,7 @@ class Place::VisitorMailer < PlaceOS::Driver 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_timezone(details), ) @change_debounce > 0 ? buffer_change(change) : dispatch_event_change(change) rescue error @@ -1095,20 +1194,26 @@ class Place::VisitorMailer < PlaceOS::Driver # named from the booking's own zones, so a booking moved to another building # is announced as being in the building it moved to - building_name = building_name_for(change.zones) + if current_zone = building_zone_for(change.zones) + building_name = current_zone.display_name.presence || current_zone.name + else + building_name = building_zone.display_name.presence || building_zone.name + end + previous_building_name = building_name + previous_room_name = @booking_space_name # Resolve previous location names from previous zones, defaulting to the # current ones so a date/time-only edit reads as the same place. previous_zones = change.previous_zones - previous_building_name = previous_zones ? building_name_for(previous_zones) : building_name - previous_room_name = @booking_space_name - previous_zones.try &.each do |zone_id| begin zone = fetch_zone(zone_id) - next if zone.tags.includes?(@invite_zone_tag) - previous_room_name = zone.display_name.presence || zone.name - break + if zone.tags.includes?(@invite_zone_tag) + previous_building_name = zone.display_name.presence || zone.name + else + previous_room_name = zone.display_name.presence || zone.name + end + break if previous_building_name != building_name && previous_room_name != @booking_space_name rescue error logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } end @@ -1131,6 +1236,7 @@ class Place::VisitorMailer < PlaceOS::Driver building_name, event_id: change.booking_id.to_s, resource_id: change.resource_id, + time_zone: visit_time_zone(change.timezone, change.zones), ) end @@ -1175,6 +1281,7 @@ class Place::VisitorMailer < PlaceOS::Driver event_id: change.event_id, resource_id: system_id, system_id: system_id, + time_zone: visit_time_zone(change.timezone, resolve_system_zones(system_id)), ) end @@ -1199,9 +1306,17 @@ class Place::VisitorMailer < PlaceOS::Driver event_id : String? = nil, resource_id : String? = nil, system_id : String? = nil, + time_zone : Time::Location? = nil, ) resolved_building_name = building_name || (building_zone.display_name.presence || building_zone.name) resolved_room_name = room_name || @booking_space_name + location = time_zone || @time_zone + + # a guest removed from a child booking is still returned against the group + # container (the attendee rows are per booking), so anyone withdrawn on ANY + # booking in the response is out, whichever row surfaced them + withdrawn = guests.select { |guest| no_longer_attending?(guest) } + .compact_map { |guest| guest["email"]?.try(&.as_s.downcase) } guests.each do |guest| visitor_email = guest["email"].as_s @@ -1210,7 +1325,7 @@ class Place::VisitorMailer < PlaceOS::Driver # a visitor removed from the visit keeps their (soft deleted) booking, and # the guest list of a group still aggregates it, so they would otherwise be # told about a visit they are no longer part of (PPT-2375) - if no_longer_attending?(guest) + if visitor_email.downcase.in?(withdrawn) logger.debug { "skipping #{template} email to #{visitor_email} as they are no longer attending" } next end @@ -1244,10 +1359,10 @@ class Place::VisitorMailer < PlaceOS::Driver next end - local_start_time = Time.unix(event_start).in(@time_zone) + local_start_time = Time.unix(event_start).in(location) - previous_date = previous_start.try { |timestamp| Time.unix(timestamp).in(@time_zone).to_s(@date_format) } - previous_time = previous_start.try { |timestamp| Time.unix(timestamp).in(@time_zone).to_s(@time_format) } + previous_date = previous_start.try { |timestamp| Time.unix(timestamp).in(location).to_s(@date_format) } + previous_time = previous_start.try { |timestamp| Time.unix(timestamp).in(location).to_s(@time_format) } guest_jwt = kiosk_url = "" attach = [] of NamedTuple(file_name: String, content: String, content_id: String) @@ -1357,8 +1472,10 @@ class Place::VisitorMailer < PlaceOS::Driver event_end : Int64? = nil, system_id : String? = nil, building_name : String? = nil, + time_zone : Time::Location? = nil, ) - local_start_time = Time.unix(event_start).in(@time_zone) + location = time_zone || @time_zone + local_start_time = Time.unix(event_start).in(location) attach = if @disable_qr_code [] of NamedTuple(file_name: String, content: String, content_id: String) @@ -1435,6 +1552,7 @@ class Place::VisitorMailer < PlaceOS::Driver guests.each do |guest| begin if event = guest["event"]? + event_zones = event.dig?("system", "zones").try(&.as_a.try(&.map(&.as_s))) send_visitor_qr_email( @reminder_template, guest["email"].as_s, @@ -1445,7 +1563,8 @@ class Place::VisitorMailer < PlaceOS::Driver event.dig("system", "id").as_s, event["id"].as_s, (event.dig?("system", "display_name") || event.dig("system", "name")).as_s, - event_end: event["event_end"].as_i64 + event_end: event["event_end"].as_i64, + time_zone: visit_time_zone(event["timezone"]?.try(&.as_s?), event_zones) ) elsif booking = guest["booking"]? send_visitor_qr_email( @@ -1458,7 +1577,8 @@ class Place::VisitorMailer < PlaceOS::Driver booking["asset_id"].as_s, booking["id"].as_i64.to_s, @booking_space_name, - event_end: booking["booking_end"].as_i64 + event_end: booking["booking_end"].as_i64, + time_zone: visit_time_zone(booking["timezone"]?.try(&.as_s?)) ) end rescue error @@ -1509,6 +1629,7 @@ class Place::VisitorMailer < PlaceOS::Driver property location : String? property tags : Array(String) property parent_id : String? + property timezone : String? end # A change buffered awaiting a debounced flush. `current_*` follow the latest @@ -1551,6 +1672,7 @@ class Place::VisitorMailer < PlaceOS::Driver 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 + property timezone : String? # the event's own time zone def initialize( @event_id, @@ -1563,6 +1685,7 @@ class Place::VisitorMailer < PlaceOS::Driver previous_start, previous_end, @previous_system_id, + @timezone = nil, ) super(host, title, current_start, current_end, previous_start, previous_end) # ical_uid identifies the event instance across mailbox copies and rooms; @@ -1591,6 +1714,7 @@ class Place::VisitorMailer < PlaceOS::Driver @previous_system_id ||= change.previous_system_id end @event_ical_uid = change.event_ical_uid || @event_ical_uid + @timezone = change.timezone || @timezone end end @@ -1601,6 +1725,7 @@ class Place::VisitorMailer < PlaceOS::Driver property resource_id : String property zones : Array(String)? property previous_zones : Array(String)? + property timezone : String? def initialize( @booking_id, @@ -1614,6 +1739,7 @@ class Place::VisitorMailer < PlaceOS::Driver previous_end, @zones, @previous_zones, + @timezone = nil, ) super(host, title, current_start, current_end, previous_start, previous_end) @buffer_key = "booking\t#{@booking_id}" @@ -1638,6 +1764,7 @@ class Place::VisitorMailer < PlaceOS::Driver @zones = change.zones @previous_zones ||= change.previous_zones end + @timezone = change.timezone || @timezone end end diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 9ec540e21e..279cca2f03 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -99,6 +99,17 @@ class StaffAPIMock < DriverSpecs::MockDriver location: "", tags: ["building"], parent_id: "zone-building", + timezone: "Australia/Sydney", + } + + # a campus zone, which some deployments also tag as a building + CAMPUS_ZONE = { + id: "zone-campus", + name: "Campus", + display_name: "PlaceOS Sydney Dev", + location: "", + tags: ["building"], + parent_id: "zone-org", } ROOM_ZONE = { @@ -142,6 +153,8 @@ class StaffAPIMock < DriverSpecs::MockDriver OLD_BUILDING_ZONE when "zone-building2" SECOND_BUILDING_ZONE + when "zone-campus" + CAMPUS_ZONE when "zone-room" ROOM_ZONE when "zone-old-room" @@ -3641,4 +3654,113 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do group_event_emails = system(:Mailer)[:emails_sent].as_a[sent_before_group_event..].map(&.as_s) group_event_emails.should contain "visitor-a@external.com|booking_changed" group_event_emails.should contain "visitor-b@external.com|booking_changed" + + # ================================================================== + # Times render in the time zone the visit is held in + # ================================================================== + # + # The driver's own timezone setting is a deployment default that is often + # left at "GMT", so the signal's timezone (or the building zone's) has to + # take precedence, or a 3pm meeting in Sydney reads as 5am. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + is_campus: true, + change_debounce: 0, + domain_uri: "https://example.com/", + }) + sleep 1.5 + + sydney = Time::Location.load("Australia/Sydney") + + # ------------------------------------------------------------------ + # Test 69: the booking's timezone field sets the rendered time + # ------------------------------------------------------------------ + + publish("staff/booking/changed", { + action: "changed", + id: 330_i64, + booking_type: "visitor", + booking_start: now + 25200, + booking_end: now + 28800, + timezone: "Australia/Sydney", + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + user_email: "host-sydney@example.com", + title: "Sydney Time", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 21600, + previous_booking_end: now + 25200, + timezone_override: nil, + }.to_json) + + sleep 1.5 + + sydney_args = system(:Mailer)[:last_args] + sydney_args["event_title"].should eq "Sydney Time" + sydney_args["event_time"].should eq Time.unix(now + 25200).in(sydney).to_s("%l:%M%p") + sydney_args["previous_event_time"].should eq Time.unix(now + 21600).in(sydney).to_s("%l:%M%p") + + # ------------------------------------------------------------------ + # Test 70: otherwise the time zone of the building the visit is in + # ------------------------------------------------------------------ + + publish("staff/guest/attending", { + action: "booking_created", + id: 12_i64, + booking_id: 340_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Sydney Building Time", + event_summary: "Sydney Building Time", + event_starting: now + 25200, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-sydney2@example.com", + zones: ["zone-building2", "zone-room"], + }.to_json) + + sleep 1.5 + + building_time_args = system(:Mailer)[:last_args] + building_time_args["building_name"].should eq "Second Building" + building_time_args["event_time"].should eq Time.unix(now + 25200).in(sydney).to_s("%l:%M%p") + + # ------------------------------------------------------------------ + # Test 71: a campus zone that is also tagged as a building does not + # shadow the building itself + # ------------------------------------------------------------------ + # + # The zones a visit reports read [org, campus, building], and a campus tagged + # as a building used to win over the building it contains, so both the + # previous and new details showed the campus name after a building changed. + + publish("staff/booking/changed", { + action: "changed", + id: 350_i64, + booking_type: "visitor", + booking_start: now + 25200, + booking_end: now + 28800, + timezone: "GMT", + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + user_email: "host-level@example.com", + title: "Campus Shadow", + zones: ["zone-campus", "zone-building", "zone-room"], + previous_booking_start: now + 21600, + previous_booking_end: now + 25200, + previous_zones: ["zone-campus", "zone-old-building", "zone-old-room"], + }.to_json) + + sleep 1.5 + + level_args = system(:Mailer)[:last_args] + level_args["event_title"].should eq "Campus Shadow" + # the building, not the campus it sits in + level_args["building_name"].should eq "Main Building" + level_args["previous_building_name"].should eq "Previous Building" + # a payload timezone is still honoured over the building zone's + level_args["event_time"].should eq Time.unix(now + 25200).in(Time::Location.load("GMT")).to_s("%l:%M%p") end diff --git a/drivers/place/visitor_models.cr b/drivers/place/visitor_models.cr index f7fb7b125a..974d312abf 100644 --- a/drivers/place/visitor_models.cr +++ b/drivers/place/visitor_models.cr @@ -98,6 +98,7 @@ module Place property previous_host_email : String property new_host_email : String property zones : Array(String)? + property timezone : String? def event_id booking_id.to_s @@ -150,6 +151,8 @@ module Place property event_start : Int64? property event_end : Int64? property zones : Array(String)? + # the calendar event as the provider sees it; only its timezone is read + property event : PlaceCalendarEvent? # Previous values — only present when action is "update" and the meta was persisted. property previous_event_start : Int64? @@ -157,4 +160,12 @@ module Place property previous_system_id : String? property previous_host_email : String? end + + # Mimics PlaceCalendar::Event so an event_changed payload can be parsed. + # Only the fields below exist, and everything else is ignored by JSON. + class PlaceCalendarEvent + include JSON::Serializable + + property timezone : String? + end end From c3470c1ba9f475c6d9afe10005dcf277e8ca5b0d Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Tue, 1 Sep 2026 12:32:28 +0930 Subject: [PATCH 09/16] docs(visitor_mailer): document time zone precedence (PPT-2375) --- drivers/place/visitor_mailer_readme.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index 3d1dd102b0..5d0adcd58c 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -55,12 +55,36 @@ Regardless of the window, the same visitor is never told the same thing twice: o of a group booking saves the group and every booking beneath it, each signalling the same change. +## Time zone + +Every email renders its times in the time zone the visit is held in, chosen in this +order: + +1. a `timezone` field on the signal itself (bookings carry one, calendar events carry + theirs in the nested event), +2. the time zone of the building the visit is in, taken from its zone, +3. the `timezone` setting, as a deployment default. + +This matters because the setting is often left at `"GMT"`, which would otherwise +announce a 3pm meeting as 5am. The `previous_event_date` / `previous_event_time` +fields are rendered in the same zone, so both halves of a change email read +consistently. + +```yaml + # %l:%M%p renders 15:00 as " 3:00pm"; %-H:%M renders it as "15:00" + time_format: "%l:%M%p" + date_format: "%A, %-d %B" +``` + ## Building name Emails name the building the visit is in, taken from the zones on the signal, so a driver covering a campus names the building the visitor is expected at rather than the campus itself. Where a visit names no building, the system's own building zone is used. +An org or campus zone that is itself tagged as a building does not shadow the building +it contains: the tagged zone the others sit beneath is the one named. + ```yaml # the zone tag identifying a building invite_zone_tag: "building" From 816c1119282c2053b26856cedcb7b7ba18a39415 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Tue, 1 Sep 2026 13:14:22 +0930 Subject: [PATCH 10/16] fix(visitor_mailer): prefer the building time zone for emails (PPT-2375) --- drivers/place/visitor_mailer.cr | 49 +++++++++----------------- drivers/place/visitor_mailer_readme.md | 25 ++++++------- drivers/place/visitor_mailer_spec.cr | 22 +++++++----- drivers/place/visitor_models.cr | 11 ------ 4 files changed, 40 insertions(+), 67 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 5075e89010..64683fd217 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -402,11 +402,6 @@ class Place::VisitorMailer < PlaceOS::Driver nil end - # The calendar event's own time zone, as signalled in the nested event. - private def event_timezone(details : EventChanged) : String? - details.event.try &.timezone - end - # The system's zone list, used to locate the building a room belongs to. protected def resolve_system_zones(system_id : String) : Array(String)? get_room_details(system_id).zones @@ -415,16 +410,15 @@ class Place::VisitorMailer < PlaceOS::Driver nil end - # Renders in the time zone the visit is held in: a `timezone` field on the - # signal, else the visit's building zone's timezone, else the setting (a - # deployment default of "GMT" otherwise swamps where the visit actually is). - private def visit_time_zone(signal_timezone : String?, zones : Array(String)? = nil) : Time::Location - candidate = signal_timezone.presence || - zone_with_timezone(zones).try(&.timezone.presence) || - @time_zone.name + # Renders in the time zone of the building the visit is in, falling back to + # the driver's timezone setting. The timezone a signal carries is ignored: a + # booking records the editing browser's zone unless the front end is set to + # use the building's, so it is not a reliable answer for where the visit is. + private def visit_time_zone(zones : Array(String)?) : Time::Location + candidate = zone_with_timezone(zones).try(&.timezone.presence) || @time_zone.name Time::Location.load(candidate) rescue error - logger.warn(exception: error) { "error loading time zone #{signal_timezone}" } + logger.warn(exception: error) { "error loading time zone" } @time_zone end @@ -477,7 +471,7 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.event_title || guest_details.event_summary, guest_details.event_starting, building_name_for(guest_details.zones), - visit_time_zone(nil, guest_details.zones) + visit_time_zone(guest_details.zones) ) self[:users_checked_in] = @users_checked_in += 1 return @@ -492,7 +486,7 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.event_starting, guest_details.induction, building_name_for(guest_details.zones), - visit_time_zone(nil, guest_details.zones) + visit_time_zone(guest_details.zones) ) self[:users_accepted_induction] = @users_accepted_induction += 1 elsif guest_details.induction.declined? @@ -505,7 +499,7 @@ class Place::VisitorMailer < PlaceOS::Driver guest_details.event_starting, guest_details.induction, building_name_for(guest_details.zones), - visit_time_zone(nil, guest_details.zones) + visit_time_zone(guest_details.zones) ) self[:users_declined_induction] = @users_declined_induction += 1 end @@ -557,7 +551,7 @@ class Place::VisitorMailer < PlaceOS::Driver area_name, system_id: guest_details.responds_to?(:system_id) ? guest_details.system_id : nil, building_name: building_name_for(guest_details.zones), - time_zone: visit_time_zone(nil, guest_details.zones), + time_zone: visit_time_zone(guest_details.zones), ) rescue error # tracked apart from error_count to pinpoint a missing invite @@ -665,7 +659,6 @@ class Place::VisitorMailer < PlaceOS::Driver details.event_title || details.event_summary, details.event_starting, building_name_for(details.zones), - details.timezone, details.zones, ) rescue error @@ -687,7 +680,6 @@ class Place::VisitorMailer < PlaceOS::Driver event_title : String?, event_start : Int64?, building_name : String, - signal_timezone : String? = nil, zones : Array(String)? = nil, ) : Nil key = { @@ -708,7 +700,7 @@ class Place::VisitorMailer < PlaceOS::Driver event_title, event_start, building_name, - visit_time_zone(signal_timezone, zones), + visit_time_zone(zones), ) rescue error # a repeat signal is the only retry there is @@ -915,7 +907,6 @@ class Place::VisitorMailer < PlaceOS::Driver details.resource_id, details.booking_start, details.booking_end, details.previous_booking_start, details.previous_booking_end, details.zones, details.previous_zones, - details.timezone, ) @change_debounce > 0 ? buffer_change(change) : dispatch_booking_change(change) rescue error @@ -964,7 +955,6 @@ class Place::VisitorMailer < PlaceOS::Driver details.title, event_start, building_name_for(details.zones), - event_timezone(details), details.zones, ) end @@ -997,7 +987,6 @@ class Place::VisitorMailer < PlaceOS::Driver 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_timezone(details), ) @change_debounce > 0 ? buffer_change(change) : dispatch_event_change(change) rescue error @@ -1236,7 +1225,7 @@ class Place::VisitorMailer < PlaceOS::Driver building_name, event_id: change.booking_id.to_s, resource_id: change.resource_id, - time_zone: visit_time_zone(change.timezone, change.zones), + time_zone: visit_time_zone(change.zones), ) end @@ -1281,7 +1270,7 @@ class Place::VisitorMailer < PlaceOS::Driver event_id: change.event_id, resource_id: system_id, system_id: system_id, - time_zone: visit_time_zone(change.timezone, resolve_system_zones(system_id)), + time_zone: visit_time_zone(resolve_system_zones(system_id)), ) end @@ -1564,7 +1553,7 @@ class Place::VisitorMailer < PlaceOS::Driver event["id"].as_s, (event.dig?("system", "display_name") || event.dig("system", "name")).as_s, event_end: event["event_end"].as_i64, - time_zone: visit_time_zone(event["timezone"]?.try(&.as_s?), event_zones) + time_zone: visit_time_zone(event_zones) ) elsif booking = guest["booking"]? send_visitor_qr_email( @@ -1578,7 +1567,7 @@ class Place::VisitorMailer < PlaceOS::Driver booking["id"].as_i64.to_s, @booking_space_name, event_end: booking["booking_end"].as_i64, - time_zone: visit_time_zone(booking["timezone"]?.try(&.as_s?)) + time_zone: visit_time_zone(nil) ) end rescue error @@ -1672,7 +1661,6 @@ class Place::VisitorMailer < PlaceOS::Driver 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 - property timezone : String? # the event's own time zone def initialize( @event_id, @@ -1685,7 +1673,6 @@ class Place::VisitorMailer < PlaceOS::Driver previous_start, previous_end, @previous_system_id, - @timezone = nil, ) super(host, title, current_start, current_end, previous_start, previous_end) # ical_uid identifies the event instance across mailbox copies and rooms; @@ -1714,7 +1701,6 @@ class Place::VisitorMailer < PlaceOS::Driver @previous_system_id ||= change.previous_system_id end @event_ical_uid = change.event_ical_uid || @event_ical_uid - @timezone = change.timezone || @timezone end end @@ -1725,7 +1711,6 @@ class Place::VisitorMailer < PlaceOS::Driver property resource_id : String property zones : Array(String)? property previous_zones : Array(String)? - property timezone : String? def initialize( @booking_id, @@ -1739,7 +1724,6 @@ class Place::VisitorMailer < PlaceOS::Driver previous_end, @zones, @previous_zones, - @timezone = nil, ) super(host, title, current_start, current_end, previous_start, previous_end) @buffer_key = "booking\t#{@booking_id}" @@ -1764,7 +1748,6 @@ class Place::VisitorMailer < PlaceOS::Driver @zones = change.zones @previous_zones ||= change.previous_zones end - @timezone = change.timezone || @timezone end end diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index 5d0adcd58c..761db2f169 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -57,25 +57,22 @@ same change. ## Time zone -Every email renders its times in the time zone the visit is held in, chosen in this -order: +Every email renders its times in the time zone of the building the visit is in, taken +from the building zone's `timezone`. A zone without one falls back to the driver's +`timezone` setting. -1. a `timezone` field on the signal itself (bookings carry one, calendar events carry - theirs in the nested event), -2. the time zone of the building the visit is in, taken from its zone, -3. the `timezone` setting, as a deployment default. - -This matters because the setting is often left at `"GMT"`, which would otherwise -announce a 3pm meeting as 5am. The `previous_event_date` / `previous_event_time` -fields are rendered in the same zone, so both halves of a change email read -consistently. +The timezone recorded on a booking or event is deliberately not consulted: a booking +carries the zone of whoever last edited it (the browser's) unless the front end is set +to use the building's zone, so it does not reliably answer where the visit is held. ```yaml - # %l:%M%p renders 15:00 as " 3:00pm"; %-H:%M renders it as "15:00" - time_format: "%l:%M%p" - date_format: "%A, %-d %B" + # the deployment default where a zone has no timezone + timezone: "GMT" ``` +The `previous_event_date` / `previous_event_time` fields are rendered in the same zone, +so both halves of a change email read consistently. + ## Building name Emails name the building the visit is in, taken from the zones on the signal, so a diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 279cca2f03..d95a2eaf7a 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -3656,12 +3656,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do group_event_emails.should contain "visitor-b@external.com|booking_changed" # ================================================================== - # Times render in the time zone the visit is held in + # Times render in the time zone of the building the visit is in # ================================================================== # - # The driver's own timezone setting is a deployment default that is often - # left at "GMT", so the signal's timezone (or the building zone's) has to - # take precedence, or a 3pm meeting in Sydney reads as 5am. + # The driver's timezone setting is a deployment default (often left at + # "GMT"), and the timezone a booking signals can be the editing browser's + # zone, so the building zone has the final say, then the setting. settings({ timezone: "GMT", @@ -3676,8 +3676,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do sydney = Time::Location.load("Australia/Sydney") # ------------------------------------------------------------------ - # Test 69: the booking's timezone field sets the rendered time + # Test 69: the building's zone beats a timezone a booking carries # ------------------------------------------------------------------ + # + # A booking's timezone field records whoever edited it (browser zone) unless + # the front end is set to use the building's, so it must not override where + # the visit actually is. publish("staff/booking/changed", { action: "changed", @@ -3685,15 +3689,15 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do booking_type: "visitor", booking_start: now + 25200, booking_end: now + 28800, - timezone: "Australia/Sydney", + timezone: "Europe/London", resource_id: "visitor@external.com", resource_ids: ["visitor@external.com"], user_email: "host-sydney@example.com", title: "Sydney Time", - zones: ["zone-building", "zone-room"], + zones: ["zone-building2", "zone-room"], previous_booking_start: now + 21600, previous_booking_end: now + 25200, - timezone_override: nil, + previous_zones: ["zone-building2", "zone-room"], }.to_json) sleep 1.5 @@ -3761,6 +3765,6 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # the building, not the campus it sits in level_args["building_name"].should eq "Main Building" level_args["previous_building_name"].should eq "Previous Building" - # a payload timezone is still honoured over the building zone's + # the zone carries no timezone, so the driver's setting applies level_args["event_time"].should eq Time.unix(now + 25200).in(Time::Location.load("GMT")).to_s("%l:%M%p") end diff --git a/drivers/place/visitor_models.cr b/drivers/place/visitor_models.cr index 974d312abf..f7fb7b125a 100644 --- a/drivers/place/visitor_models.cr +++ b/drivers/place/visitor_models.cr @@ -98,7 +98,6 @@ module Place property previous_host_email : String property new_host_email : String property zones : Array(String)? - property timezone : String? def event_id booking_id.to_s @@ -151,8 +150,6 @@ module Place property event_start : Int64? property event_end : Int64? property zones : Array(String)? - # the calendar event as the provider sees it; only its timezone is read - property event : PlaceCalendarEvent? # Previous values — only present when action is "update" and the meta was persisted. property previous_event_start : Int64? @@ -160,12 +157,4 @@ module Place property previous_system_id : String? property previous_host_email : String? end - - # Mimics PlaceCalendar::Event so an event_changed payload can be parsed. - # Only the fields below exist, and everything else is ignored by JSON. - class PlaceCalendarEvent - include JSON::Serializable - - property timezone : String? - end end From 1b34e4f0da1ac3b2bd1421c35408696e58440740 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Wed, 23 Sep 2026 16:57:35 +1000 Subject: [PATCH 11/16] feat(visitor_mailer): end time and time zone placeholders (PPT-2667, PPT-2375) Every visitor email now carries event_timezone, the abbreviation of the zone its times are rendered in, so a template can label the times it shows. The invitation, reminder and change emails carry event_end_time / event_end_date, and the change emails also carry previous_event_end_time / previous_event_end_date, so a booking-changed template can show the whole window before and after an edit whichever of the start or end moved. --- drivers/place/visitor_mailer.cr | 84 +++++++++++++++++--------- drivers/place/visitor_mailer_readme.md | 15 +++++ drivers/place/visitor_mailer_spec.cr | 17 +++++- 3 files changed, 88 insertions(+), 28 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 64683fd217..8c9d4ad48a 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -130,6 +130,9 @@ class Place::VisitorMailer < PlaceOS::Driver # See: https://crystal-lang.org/api/0.35.1/Time/Format.html @date_time_format : String = "%c" @time_format : String = "%l:%M%p" + + # Zone abbreviation for the `event_timezone` placeholder (e.g. AWST) + TIMEZONE_FORMAT = "%Z" @date_format : String = "%A, %-d %B" @building_zone : ZoneDetails? = nil @@ -592,15 +595,16 @@ class Place::VisitorMailer < PlaceOS::Driver host_email, {"visitor_invited", template}, # Template selection: "visitor_invited" "notify_checkin" { - visitor_email: visitor_email, - visitor_name: visitor_name, - host_name: get_host_name(host_email), - host_email: host_email, - building_name: building_name || building_name_for(nil), - event_title: event_title, - event_start: local_start_time.to_s(@time_format), - event_date: local_start_time.to_s(@date_format), - event_time: local_start_time.to_s(@time_format), + visitor_email: visitor_email, + visitor_name: visitor_name, + host_name: get_host_name(host_email), + host_email: host_email, + building_name: building_name || building_name_for(nil), + event_title: event_title, + event_start: local_start_time.to_s(@time_format), + event_date: local_start_time.to_s(@date_format), + event_time: local_start_time.to_s(@time_format), + event_timezone: local_start_time.to_s(TIMEZONE_FORMAT), }, reply_to: host_email.presence, ) @@ -633,6 +637,7 @@ class Place::VisitorMailer < PlaceOS::Driver event_start: local_start_time.to_s(@time_format), event_date: local_start_time.to_s(@date_format), event_time: local_start_time.to_s(@time_format), + event_timezone: local_start_time.to_s(TIMEZONE_FORMAT), induction_status: induction_status.to_s, }, reply_to: host_email.presence, @@ -735,6 +740,7 @@ class Place::VisitorMailer < PlaceOS::Driver event_title: event_title, event_date: local_start_time.try(&.to_s(@date_format)), event_time: local_start_time.try(&.to_s(@time_format)), + event_timezone: local_start_time.try(&.to_s(TIMEZONE_FORMAT)), }, reply_to: new_host_email.presence, ) @@ -752,10 +758,13 @@ class Place::VisitorMailer < PlaceOS::Driver {name: "event_start", description: "Start time (e.g., #{time_now.to_s(@time_format)})"}, {name: "event_date", description: "Date of the visit (e.g., #{time_now.to_s(@date_format)})"}, {name: "event_time", description: "Time of the visit (or 'all day' for 24-hour events)"}, + {name: "event_timezone", description: "Abbreviation of the time zone the times are shown in (e.g., #{time_now.to_s(TIMEZONE_FORMAT)})"}, ] invitation_fields = common_fields + [ {name: "room_name", description: "Name of the room or area being visited"}, + {name: "event_end_time", description: "End time of the visit (e.g., #{time_now.to_s(@time_format)})"}, + {name: "event_end_date", description: "End date of the visit (e.g., #{time_now.to_s(@date_format)})"}, {name: "network_username", description: "Network access username (if network credentials enabled)"}, {name: "network_password", description: "Generated network access password (if network credentials enabled)"}, ] @@ -773,8 +782,12 @@ class Place::VisitorMailer < PlaceOS::Driver # the same data but render through separate templates. changed_fields = common_fields + [ {name: "room_name", description: "Name of the room or area being visited"}, + {name: "event_end_time", description: "The new end time"}, + {name: "event_end_date", description: "The new end date"}, {name: "previous_event_date", description: "The original date before it was changed"}, - {name: "previous_event_time", description: "The original time before it was changed"}, + {name: "previous_event_time", description: "The original start time before it was changed"}, + {name: "previous_event_end_time", description: "The original end time before it was changed"}, + {name: "previous_event_end_date", description: "The original end date before it was changed"}, {name: "previous_room_name", description: "The original room or area name before it was moved"}, {name: "previous_building_name", description: "The original building name before it was moved"}, ] + jwt_fields @@ -1226,6 +1239,8 @@ class Place::VisitorMailer < PlaceOS::Driver event_id: change.booking_id.to_s, resource_id: change.resource_id, time_zone: visit_time_zone(change.zones), + event_end: change.current_end, + previous_end: change.previous_end, ) end @@ -1271,6 +1286,8 @@ class Place::VisitorMailer < PlaceOS::Driver resource_id: system_id, system_id: system_id, time_zone: visit_time_zone(resolve_system_zones(system_id)), + event_end: change.current_end, + previous_end: change.previous_end, ) end @@ -1296,6 +1313,8 @@ class Place::VisitorMailer < PlaceOS::Driver resource_id : String? = nil, system_id : String? = nil, time_zone : Time::Location? = nil, + event_end : Int64? = nil, + previous_end : Int64? = nil, ) resolved_building_name = building_name || (building_zone.display_name.presence || building_zone.name) resolved_room_name = room_name || @booking_space_name @@ -1339,7 +1358,7 @@ class Place::VisitorMailer < PlaceOS::Driver # describing the same change to the same visitors (PPT-2375) notice_key = { template, visitor_email.strip.downcase, host_email.strip.downcase, event_title, - event_start, previous_start, resolved_room_name, resolved_building_name, + event_start, event_end, previous_start, previous_end, resolved_room_name, resolved_building_name, previous_room_name, previous_building_name, }.join('\t') @@ -1349,9 +1368,11 @@ class Place::VisitorMailer < PlaceOS::Driver end local_start_time = Time.unix(event_start).in(location) + local_end_time = event_end.try { |timestamp| Time.unix(timestamp).in(location) } previous_date = previous_start.try { |timestamp| Time.unix(timestamp).in(location).to_s(@date_format) } previous_time = previous_start.try { |timestamp| Time.unix(timestamp).in(location).to_s(@time_format) } + previous_end_local = previous_end.try { |timestamp| Time.unix(timestamp).in(location) } guest_jwt = kiosk_url = "" attach = [] of NamedTuple(file_name: String, content: String, content_id: String) @@ -1379,22 +1400,27 @@ class Place::VisitorMailer < PlaceOS::Driver visitor_email, {"visitor_invited", template}, { - visitor_email: visitor_email, - visitor_name: visitor_name, - host_name: get_host_name(host_email), - host_email: host_email, - room_name: resolved_room_name, - building_name: resolved_building_name, - event_title: event_title, - event_start: local_start_time.to_s(@time_format), - event_date: local_start_time.to_s(@date_format), - event_time: local_start_time.to_s(@time_format), - previous_event_date: previous_date, - previous_event_time: previous_time, - previous_room_name: previous_room_name, - previous_building_name: previous_building_name, - guest_jwt: guest_jwt, - kiosk_url: kiosk_url, + visitor_email: visitor_email, + visitor_name: visitor_name, + host_name: get_host_name(host_email), + host_email: host_email, + room_name: resolved_room_name, + building_name: resolved_building_name, + event_title: event_title, + event_start: local_start_time.to_s(@time_format), + event_date: local_start_time.to_s(@date_format), + event_time: local_start_time.to_s(@time_format), + event_end_time: local_end_time.try(&.to_s(@time_format)), + event_end_date: local_end_time.try(&.to_s(@date_format)), + event_timezone: local_start_time.to_s(TIMEZONE_FORMAT), + previous_event_date: previous_date, + previous_event_time: previous_time, + previous_event_end_time: previous_end_local.try(&.to_s(@time_format)), + previous_event_end_date: previous_end_local.try(&.to_s(@date_format)), + previous_room_name: previous_room_name, + previous_building_name: previous_building_name, + guest_jwt: guest_jwt, + kiosk_url: kiosk_url, }, attach, reply_to: host_email.presence, @@ -1465,6 +1491,7 @@ class Place::VisitorMailer < PlaceOS::Driver ) location = time_zone || @time_zone local_start_time = Time.unix(event_start).in(location) + local_end_time = event_end.try { |timestamp| Time.unix(timestamp).in(location) } attach = if @disable_qr_code [] of NamedTuple(file_name: String, content: String, content_id: String) @@ -1516,6 +1543,9 @@ class Place::VisitorMailer < PlaceOS::Driver event_start: local_start_time.to_s(@time_format), event_date: local_start_time.to_s(@date_format), event_time: event_time, + event_end_time: local_end_time.try(&.to_s(@time_format)), + event_end_date: local_end_time.try(&.to_s(@date_format)), + event_timezone: local_start_time.to_s(TIMEZONE_FORMAT), network_username: network_username, network_password: network_password, guest_jwt: guest_jwt, diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index 761db2f169..e8dae50786 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -73,6 +73,21 @@ to use the building's zone, so it does not reliably answer where the visit is he The `previous_event_date` / `previous_event_time` fields are rendered in the same zone, so both halves of a change email read consistently. +Every email also carries `event_timezone`, the abbreviation of the zone its times are in +(for example `AWST`), so a template can label the times it shows. A `time_format` that +includes `%Z` labels every time placeholder instead: + +```yaml + time_format: "%l:%M%p %Z" +``` + +## End times + +The invitation, reminder and change emails carry the end of the visit as `event_end_time` +and `event_end_date`. The change emails also carry `previous_event_end_time` and +`previous_event_end_date`, so a template can show the whole window before and after an +edit, whichever of the start or the end moved. + ## Building name Emails name the building the visit is in, taken from the zones on the signal, so a diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index d95a2eaf7a..cd9ba45444 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -468,6 +468,13 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # previous_event_date and previous_event_time should be present (time did change) args2["previous_event_date"].should_not be_nil args2["previous_event_time"].should_not be_nil + # the end of the visit is rendered alongside the start (PPT-2667) + args2["event_end_time"].should_not be_nil + args2["event_end_date"].should_not be_nil + args2["previous_event_end_time"].should_not be_nil + args2["previous_event_end_date"].should_not be_nil + # and the zone the times are in is named (the building zone's, not the driver default) + args2["event_timezone"].should eq "UTC" # ------------------------------------------------------------------ # Test 3: action != "changed" is ignored (no extra email sent) @@ -650,7 +657,12 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do system(:Mailer)[:send_count].should eq 5 system(:Mailer)[:last_to].should eq "visitor@external.com" system(:Mailer)[:last_template].should eq ["visitor_invited", "booking_changed"] - system(:Mailer)[:last_args]["event_title"].should eq "End Time Only Change" + args6c = system(:Mailer)[:last_args] + args6c["event_title"].should eq "End Time Only Change" + args6c["event_end_time"].should_not be_nil + args6c["previous_event_end_time"].should_not be_nil + args6c["previous_event_end_time"].should_not eq args6c["event_end_time"] + args6c["previous_event_time"].should eq args6c["event_time"] # ================================================================== # booking_host_changed_event tests @@ -844,6 +856,9 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # previous dates should be present args11["previous_event_date"].should_not be_nil args11["previous_event_time"].should_not be_nil + args11["event_end_time"].should_not be_nil + args11["previous_event_end_time"].should_not be_nil + args11["event_timezone"].should eq "UTC" # The location did NOT change, so the "previous" room/building must mirror # the (unchanged) current room — resolved from system_id — rather than the # static @booking_space_name fallback. Otherwise the email shows a bogus From 90de29032b4fccb4cfe7dca5aca701efe70a46d3 Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Wed, 23 Sep 2026 17:36:49 +1000 Subject: [PATCH 12/16] fix(visitor_mailer): event_timezone is the zone abbreviation Crystal's %Z prints the location name (Australia/Perth); %^Z prints the zone abbreviation (AWST), which is what the placeholder documents. --- drivers/place/visitor_mailer.cr | 2 +- drivers/place/visitor_mailer_readme.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 8c9d4ad48a..3a2aacde4b 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -132,7 +132,7 @@ class Place::VisitorMailer < PlaceOS::Driver @time_format : String = "%l:%M%p" # Zone abbreviation for the `event_timezone` placeholder (e.g. AWST) - TIMEZONE_FORMAT = "%Z" + TIMEZONE_FORMAT = "%^Z" @date_format : String = "%A, %-d %B" @building_zone : ZoneDetails? = nil diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index e8dae50786..b97c411c23 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -75,10 +75,10 @@ so both halves of a change email read consistently. Every email also carries `event_timezone`, the abbreviation of the zone its times are in (for example `AWST`), so a template can label the times it shows. A `time_format` that -includes `%Z` labels every time placeholder instead: +includes `%^Z` labels every time placeholder instead: ```yaml - time_format: "%l:%M%p %Z" + time_format: "%l:%M%p %^Z" ``` ## End times From df4d6b1d28e89dcaa6017baeecf8872ed4f215fe Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Wed, 23 Sep 2026 17:41:30 +1000 Subject: [PATCH 13/16] fix(visitor_mailer): previous room ignores the zones above the building A visitor booking lists its org, region and campus zones beside the building, and the change notice took the first of them for the previous room ("Room: Australia" on HIO). Only a zone beneath the previous building can name its room now. --- drivers/place/visitor_mailer.cr | 35 ++++++++++++++++++++-------- drivers/place/visitor_mailer_spec.cr | 32 +++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 3a2aacde4b..005bb45974 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -390,6 +390,18 @@ class Place::VisitorMailer < PlaceOS::Driver # The zone a visit is in, following the parent chain upwards so a visit whose # own zones omit the building (it is level or room only) still names one. + # Whether `zone` sits beneath the zone `ancestor_id` in the hierarchy. + private def zone_within?(zone : ZoneDetails, ancestor_id : String) : Bool + parent_id = zone.parent_id + visited = Set(String).new + while parent_id && !visited.includes?(parent_id) + return true if parent_id == ancestor_id + visited << parent_id + parent_id = fetch_zone(parent_id).parent_id + end + false + end + private def zone_with_timezone(zones : Array(String)?) : ZoneDetails? if zone = building_zone_for(zones) return zone if zone.timezone.presence @@ -1206,18 +1218,21 @@ class Place::VisitorMailer < PlaceOS::Driver # Resolve previous location names from previous zones, defaulting to the # current ones so a date/time-only edit reads as the same place. - previous_zones = change.previous_zones - previous_zones.try &.each do |zone_id| - begin - zone = fetch_zone(zone_id) - if zone.tags.includes?(@invite_zone_tag) - previous_building_name = zone.display_name.presence || zone.name - else + if (previous_zones = change.previous_zones) && (previous_building = building_zone_for(previous_zones)) + previous_building_name = previous_building.display_name.presence || previous_building.name + + # a booking also lists the org, region and campus zones above its building, + # so only a zone beneath the building names its room + previous_zones.each do |zone_id| + begin + zone = fetch_zone(zone_id) + next if zone.id == previous_building.id || zone.tags.includes?(@invite_zone_tag) + next unless zone_within?(zone, previous_building.id) previous_room_name = zone.display_name.presence || zone.name + break + rescue error + logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } end - break if previous_building_name != building_name && previous_room_name != @booking_space_name - rescue error - logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } end end diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index cd9ba45444..493b6196ac 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -3782,4 +3782,36 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do level_args["previous_building_name"].should eq "Previous Building" # the zone carries no timezone, so the driver's setting applies level_args["event_time"].should eq Time.unix(now + 25200).in(Time::Location.load("GMT")).to_s("%l:%M%p") + + # ------------------------------------------------------------------ + # Test 72: the org zone a booking lists beside its building is not its room + # ------------------------------------------------------------------ + # + # A visitor booking reports [org, building]; the org zone used to be taken + # for the previous room, so a time change read "Room: Australia". + + publish("staff/booking/changed", { + action: "changed", + id: 351_i64, + booking_type: "visitor", + booking_start: now + 32400, + booking_end: now + 36000, + timezone: "GMT", + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + user_email: "host-level@example.com", + title: "Org Zone Room", + zones: ["zone-extra", "zone-building"], + previous_booking_start: now + 28800, + previous_booking_end: now + 32400, + previous_zones: ["zone-extra", "zone-building"], + }.to_json) + + sleep 1.5 + + org_args = system(:Mailer)[:last_args] + org_args["event_title"].should eq "Org Zone Room" + org_args["previous_building_name"].should eq "Main Building" + org_args["previous_room_name"].should eq "Client Floor" + org_args["room_name"].should eq "Client Floor" end From b9bbed6a2ac105dd94fdd7ce4eec606e08444cc5 Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Wed, 23 Sep 2026 17:44:58 +1000 Subject: [PATCH 14/16] fix(visitor_mailer): keep the previous-zone short circuit Only the room branch needs the guard; walking every previous zone up front cost an extra zone lookup per change. --- drivers/place/visitor_mailer.cr | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 005bb45974..5d09aa1332 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -390,14 +390,16 @@ class Place::VisitorMailer < PlaceOS::Driver # The zone a visit is in, following the parent chain upwards so a visit whose # own zones omit the building (it is level or room only) still names one. - # Whether `zone` sits beneath the zone `ancestor_id` in the hierarchy. - private def zone_within?(zone : ZoneDetails, ancestor_id : String) : Bool + # Whether `zone` sits beneath a zone tagged as the building. A booking lists + # the org, region and campus zones above its building as well. + private def within_building?(zone : ZoneDetails) : Bool parent_id = zone.parent_id visited = Set(String).new while parent_id && !visited.includes?(parent_id) - return true if parent_id == ancestor_id + parent = fetch_zone(parent_id) + return true if parent.tags.includes?(@invite_zone_tag) visited << parent_id - parent_id = fetch_zone(parent_id).parent_id + parent_id = parent.parent_id end false end @@ -1218,21 +1220,18 @@ class Place::VisitorMailer < PlaceOS::Driver # Resolve previous location names from previous zones, defaulting to the # current ones so a date/time-only edit reads as the same place. - if (previous_zones = change.previous_zones) && (previous_building = building_zone_for(previous_zones)) - previous_building_name = previous_building.display_name.presence || previous_building.name - - # a booking also lists the org, region and campus zones above its building, - # so only a zone beneath the building names its room - previous_zones.each do |zone_id| - begin - zone = fetch_zone(zone_id) - next if zone.id == previous_building.id || zone.tags.includes?(@invite_zone_tag) - next unless zone_within?(zone, previous_building.id) + previous_zones = change.previous_zones + previous_zones.try &.each do |zone_id| + begin + zone = fetch_zone(zone_id) + if zone.tags.includes?(@invite_zone_tag) + previous_building_name = zone.display_name.presence || zone.name + elsif within_building?(zone) previous_room_name = zone.display_name.presence || zone.name - break - rescue error - logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } end + break if previous_building_name != building_name && previous_room_name != @booking_space_name + rescue error + logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } end end From 5aed289c9359029947126bc50c08993b435dcfa6 Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Wed, 23 Sep 2026 22:47:23 +1000 Subject: [PATCH 15/16] fix(visitor_mailer): linked booking invites follow the event path by default Unset, skip_event_linked_booking_email now tracks disable_event_visitors, so a site with event invites off keeps the linked booking's invitation instead of sending the attendee nothing. Also keeps the last good building zone after a refresh, tidies the format constant and a stray comment, makes the spec's zone-label expectations environment independent and covers the new default (test 73). Readme documents the two attendee invite paths. --- drivers/place/visitor_mailer.cr | 20 +++---- drivers/place/visitor_mailer_readme.md | 21 +++++++ drivers/place/visitor_mailer_spec.cr | 76 +++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 12 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 5d09aa1332..f3671cd2f4 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -62,7 +62,9 @@ class Place::VisitorMailer < PlaceOS::Driver # Suppresses the `booking` template when the booking has an # extension_data.parent_id (i.e. auto-created from a calendar event that - # already triggers the `event` template). + # already triggers the `event` template). Unset, it follows + # disable_event_visitors: the linked booking's invite is skipped only while + # the event path sends one. skip_event_linked_booking_email: true, # When true, the host will not receive any visitor-targeted emails @@ -128,11 +130,10 @@ class Place::VisitorMailer < PlaceOS::Driver @host_domain_filter : Array(String) = [] of String # See: https://crystal-lang.org/api/0.35.1/Time/Format.html + # `%^Z` is the zone abbreviation (e.g. AWST); `%Z` would be the location name + TIMEZONE_FORMAT = "%^Z" @date_time_format : String = "%c" @time_format : String = "%l:%M%p" - - # Zone abbreviation for the `event_timezone` placeholder (e.g. AWST) - TIMEZONE_FORMAT = "%^Z" @date_format : String = "%A, %-d %B" @building_zone : ZoneDetails? = nil @@ -222,8 +223,9 @@ class Place::VisitorMailer < PlaceOS::Driver @network_group_ids = setting?(Array(String), :network_group_ids) || [] of String @host_domain_filter = setting?(Array(String), :host_domain_filter) || [] of String @disable_event_visitors = setting?(Bool, :disable_event_visitors) || false + # unset: skip the linked booking's invite only when the event path sends one skip_event_linked = setting?(Bool, :skip_event_linked_booking_email) - @skip_event_linked_booking_email = skip_event_linked.nil? ? true : skip_event_linked + @skip_event_linked_booking_email = skip_event_linked.nil? ? !@disable_event_visitors : skip_event_linked skip_host_email = setting?(Bool, :skip_host_email) @skip_host_email = skip_host_email.nil? ? true : skip_host_email @skip_internal_domain_email = setting?(Bool, :skip_internal_domain_email) || false @@ -279,7 +281,7 @@ class Place::VisitorMailer < PlaceOS::Driver def building_zone : ZoneDetails if zone_id = @building_zone_id begin - return fetch_zone(zone_id) + return @building_zone = fetch_zone(zone_id) rescue error logger.warn(exception: error) { "error refreshing building zone #{zone_id}" } # last known good, an email is better than no email @@ -388,10 +390,8 @@ class Place::VisitorMailer < PlaceOS::Driver nil end - # The zone a visit is in, following the parent chain upwards so a visit whose - # own zones omit the building (it is level or room only) still names one. - # Whether `zone` sits beneath a zone tagged as the building. A booking lists - # the org, region and campus zones above its building as well. + # Whether `zone` sits beneath a zone tagged as the building (a booking also + # lists the org, region and campus zones above its building). private def within_building?(zone : ZoneDetails) : Bool parent_id = zone.parent_id visited = Set(String).new diff --git a/drivers/place/visitor_mailer_readme.md b/drivers/place/visitor_mailer_readme.md index b97c411c23..ab25ade5c1 100644 --- a/drivers/place/visitor_mailer_readme.md +++ b/drivers/place/visitor_mailer_readme.md @@ -109,6 +109,27 @@ it contains: the tagged zone the others sit beneath is the one named. A building renamed in backoffice reaches the emails once its cache entry expires. Call `clear_zone_cache` to pick the new name up immediately. +## Room bookings and their attendees + +An external attendee on a calendar event can be invited two ways: by the event itself +(`disable_event_visitors: false`, the `event` template, which names the room) or by the +visitor booking the front end creates beneath the event (the `booking` template). +`skip_event_linked_booking_email` suppresses the second so the attendee is not invited +twice. Unset, it follows `disable_event_visitors`: a site that has turned event invites +off keeps the linked booking's invitation. + +The front end recreates the linked booking on every save of the event, which the driver +sees as a new booking. With event invites off, an edit therefore re-invites the attendee +rather than describing the change; with them on, the event's own change signal sends the +`event_changed` email instead. + +```yaml + # invitations and change notices come from the calendar event + disable_event_visitors: false + # the visitor booking beneath the event is not invited again + skip_event_linked_booking_email: true +``` + ## Excluding staff attendees The front end might mark any attendee as an expected visitor, so staff invited to a diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 493b6196ac..9ca1aafe31 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -474,7 +474,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do args2["previous_event_end_time"].should_not be_nil args2["previous_event_end_date"].should_not be_nil # and the zone the times are in is named (the building zone's, not the driver default) - args2["event_timezone"].should eq "UTC" + args2["event_timezone"].should eq Time.unix(now).in(Time::Location.load("GMT")).to_s("%^Z") # ------------------------------------------------------------------ # Test 3: action != "changed" is ignored (no extra email sent) @@ -858,7 +858,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do args11["previous_event_time"].should_not be_nil args11["event_end_time"].should_not be_nil args11["previous_event_end_time"].should_not be_nil - args11["event_timezone"].should eq "UTC" + args11["event_timezone"].should eq Time.unix(now).in(Time::Location.load("GMT")).to_s("%^Z") # The location did NOT change, so the "previous" room/building must mirror # the (unchanged) current room — resolved from system_id — rather than the # static @booking_space_name fallback. Otherwise the email shows a bogus @@ -3814,4 +3814,76 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do org_args["previous_building_name"].should eq "Main Building" org_args["previous_room_name"].should eq "Client Floor" org_args["room_name"].should eq "Client Floor" + + # ------------------------------------------------------------------ + # Test 73: with event invites disabled, an event-linked booking still + # invites unless skip_event_linked_booking_email says otherwise + # ------------------------------------------------------------------ + # + # Unset, the skip follows disable_event_visitors; a site with the event + # path off relies on the linked booking for the attendee's only invitation. + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, + disable_event_visitors: true, + }) + sleep 1.0 + + linked_before = system(:Mailer)[:send_count].as_i + + publish("staff/guest/attending", { + action: "booking_created", + id: 12_i64, + booking_id: 601_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Linked Visit", + event_summary: "Linked Visit", + event_starting: now + 118800, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-linked@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + system(:Mailer)[:send_count].should eq linked_before + 1 + system(:Mailer)[:last_to].should eq "visitor@external.com" + system(:Mailer)[:last_template].should eq ["visitor_invited", "booking"] + + # explicitly opted out, the same invite is skipped + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + change_debounce: 0, + disable_event_visitors: true, + skip_event_linked_booking_email: true, + }) + sleep 1.0 + + linked_before = system(:Mailer)[:send_count].as_i + + publish("staff/guest/attending", { + action: "booking_created", + id: 13_i64, + booking_id: 601_i64, + resource_id: "visitor@external.com", + resource_ids: ["visitor@external.com"], + event_title: "Linked Visit", + event_summary: "Linked Visit", + event_starting: now + 122400, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host-linked@example.com", + zones: ["zone-building", "zone-room"], + }.to_json) + + sleep 1.0 + + system(:Mailer)[:send_count].should eq linked_before end From 91c691fb27840510a584d4074b72618e8cf5a30d Mon Sep 17 00:00:00 2001 From: Cam Reeves Date: Wed, 23 Sep 2026 22:58:01 +1000 Subject: [PATCH 16/16] fix(visitor_mailer): cap change_debounce where the invite memory is A debounce above 3600s waited the full window while invitations were only remembered for 3660s, so a visitor added by the edit could be told about it after all. --- drivers/place/visitor_mailer.cr | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index f3671cd2f4..eb2ecc122b 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -210,7 +210,9 @@ class Place::VisitorMailer < PlaceOS::Driver @group_event_template = setting?(String, :group_event_template) || "group_event" # 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 + # capped where the invite memory is, so a change never outlives the memory + # of the invitations it must not repeat + @change_debounce = (setting?(Int32, :change_debounce) || setting?(Int32, :event_change_debounce) || 15).clamp(0, 3600) @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 @@ -1106,7 +1108,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 - @change_debounce.clamp(0, 3600).seconds + 60.seconds + @change_debounce.seconds + 60.seconds end # An email we've sent, keyed on what it says rather than on the booking that