From e616060c77344fe501ec72754cb2cb692137aa38 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 27 Aug 2026 12:10:41 +0930 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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