From 665bf1634731d60d6ef625a7ff5ee9ffe812b88e Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 14 May 2026 14:08:57 +0930 Subject: [PATCH 01/12] fix(visitor_mailer): not all signals have all fields, so make them nilable --- drivers/place/visitor_mailer.cr | 21 ++++++++++++++------- drivers/place/visitor_models.cr | 6 +++--- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 601c7c5a0a9..54c6da6593e 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -641,6 +641,13 @@ class Place::VisitorMailer < PlaceOS::Driver # only respond to updates, not creates or cancellations return unless details.action == "update" + # These fields may be missing from some payloads (e.g. cancelled events, + # metadata-only updates) so the model marks them nilable. + host = details.host + event_start = details.event_start + event_end = details.event_end + return unless host && event_start && event_end + # ensure the event is for this building if zones = details.zones check = [building_zone.id] + @parent_zone_ids @@ -653,13 +660,13 @@ class Place::VisitorMailer < PlaceOS::Driver # --- Host change notification if prev_host = details.previous_host_email - if prev_host.downcase != details.host.downcase + if prev_host.downcase != host.downcase send_original_host_email( @notify_original_host_template, prev_host, - details.host, + host, details.title, - details.event_start, + event_start, ) end end @@ -669,10 +676,10 @@ class Place::VisitorMailer < PlaceOS::Driver # Date or time changed if prev_start = details.previous_event_start - fields_changed = true if prev_start != details.event_start + fields_changed = true if prev_start != event_start end if prev_end = details.previous_event_end - fields_changed = true if prev_end != details.event_end + fields_changed = true if prev_end != event_end end # Location changed (system_id represents the room) @@ -716,8 +723,8 @@ class Place::VisitorMailer < PlaceOS::Driver guests = staff_api.event_guests(details.event_id, details.system_id).get.as_a send_booking_changed_emails( guests, - details.host, - details.event_start, + host, + event_start, details.title, details.previous_event_start, previous_building_name, diff --git a/drivers/place/visitor_models.cr b/drivers/place/visitor_models.cr index fb9cff831c0..6f60e2f01e3 100644 --- a/drivers/place/visitor_models.cr +++ b/drivers/place/visitor_models.cr @@ -136,11 +136,11 @@ module Place property system_id : String property event_id : String property event_ical_uid : String? - property host : String + property host : String? property resource : String? property title : String? - property event_start : Int64 - property event_end : Int64 + property event_start : Int64? + property event_end : Int64? property zones : Array(String)? # Previous values — only present when action is "update" and the meta was persisted. From b5c5e40c1aac704936c49f28717a17c26c3cc385 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 14 May 2026 17:46:03 +0930 Subject: [PATCH 02/12] fix(staff_api): support #guest_list ical_uid lookup (PPT-2375) --- drivers/place/staff_api.cr | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/place/staff_api.cr b/drivers/place/staff_api.cr index 5f2cf29f19d..381f1274312 100644 --- a/drivers/place/staff_api.cr +++ b/drivers/place/staff_api.cr @@ -993,9 +993,13 @@ class Place::StaffAPI < PlaceOS::Driver JSON.parse(response.body) end - def event_guests(event_id : String, system_id : String) + def event_guests(event_id : String, system_id : String, ical_uid : String? = nil) logger.debug { "getting guests for event #{event_id} in system #{system_id}" } - response = get("/api/staff/v1/events/#{event_id}/guests?system_id=#{system_id}", headers: authentication) + params = URI::Params.build do |form| + form.add "system_id", system_id + form.add "ical_uid", ical_uid if ical_uid + end + response = get("/api/staff/v1/events/#{event_id}/guests?#{params}", headers: authentication) raise "issue getting guests for event #{event_id}: #{response.status_code}" unless response.success? JSON.parse(response.body) end From d02b95a586f3e5ae36b55fb589a8fe98d9f31ad8 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 14 May 2026 17:46:25 +0930 Subject: [PATCH 03/12] fix(visitor_mailer): support #guest_list ical_uid lookup (PPT-2375) --- drivers/place/visitor_mailer.cr | 2 +- drivers/place/visitor_mailer_spec.cr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 54c6da6593e..02fec19102f 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -720,7 +720,7 @@ class Place::VisitorMailer < PlaceOS::Driver end end - guests = staff_api.event_guests(details.event_id, details.system_id).get.as_a + guests = staff_api.event_guests(details.event_id, details.system_id, details.event_ical_uid).get.as_a send_booking_changed_emails( guests, host, diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 257da8c3cf1..33d34fa7ee8 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -131,7 +131,7 @@ class StaffAPIMock < DriverSpecs::MockDriver ] end - def event_guests(event_id : String, system_id : String) + def event_guests(event_id : String, system_id : String, ical_uid : String? = nil) [ { email: "visitor@external.com", From 6f73b270dfeaba2ccc96d04a1e6eb4f39a71b99d Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 14 May 2026 17:52:12 +0930 Subject: [PATCH 04/12] refactor(staff_api): match style of other ical_uid lookups --- drivers/place/staff_api.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/place/staff_api.cr b/drivers/place/staff_api.cr index 381f1274312..f7700616ac8 100644 --- a/drivers/place/staff_api.cr +++ b/drivers/place/staff_api.cr @@ -997,7 +997,7 @@ class Place::StaffAPI < PlaceOS::Driver logger.debug { "getting guests for event #{event_id} in system #{system_id}" } params = URI::Params.build do |form| form.add "system_id", system_id - form.add "ical_uid", ical_uid if ical_uid + form.add "ical_uid", ical_uid.to_s if ical_uid.presence end response = get("/api/staff/v1/events/#{event_id}/guests?#{params}", headers: authentication) raise "issue getting guests for event #{event_id}: #{response.status_code}" unless response.success? From cfd604af1a231cc5e78218d2dc2e0d6fc23dd13d Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Fri, 15 May 2026 13:44:29 +0930 Subject: [PATCH 05/12] feat(staff_api): add include_linked guests (PPT-2375) --- drivers/place/staff_api.cr | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/place/staff_api.cr b/drivers/place/staff_api.cr index f7700616ac8..18c50893461 100644 --- a/drivers/place/staff_api.cr +++ b/drivers/place/staff_api.cr @@ -986,9 +986,10 @@ class Place::StaffAPI < PlaceOS::Driver JSON.parse(response.body) end - def booking_guests(booking_id : String | Int64) + def booking_guests(booking_id : String | Int64, include_linked : Bool = false) logger.debug { "getting guests for booking #{booking_id}" } - response = get("/api/staff/v1/bookings/#{booking_id}/guests", headers: authentication) + params = include_linked ? "?include_linked=true" : "" + response = get("/api/staff/v1/bookings/#{booking_id}/guests#{params}", headers: authentication) raise "issue getting guests for booking #{booking_id}: #{response.status_code}" unless response.success? JSON.parse(response.body) end From 497ad538b3fefc313164c4a95275424d97f882f4 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Fri, 15 May 2026 13:44:59 +0930 Subject: [PATCH 06/12] fix(visitor_mailer): support group bookings (PPT-2375) --- drivers/place/visitor_mailer.cr | 8 +++- drivers/place/visitor_mailer_spec.cr | 70 ++++++++++++++++++++++++---- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 02fec19102f..861f08048f5 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -614,7 +614,13 @@ class Place::VisitorMailer < PlaceOS::Driver end end - guests = staff_api.booking_guests(details.id).get.as_a + # include_linked: true ensures guests from child bookings (e.g. per-visitor + # bookings under a group parent) are returned in a single request. + guests = staff_api.booking_guests(details.id, include_linked: true).get.as_a + + # Deduplicate by email in case a guest appears on multiple child bookings + guests.uniq! { |guest| guest["email"].as_s.downcase } + send_booking_changed_emails( guests, details.user_email, diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 33d34fa7ee8..8be11b3a66a 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -120,15 +120,23 @@ class StaffAPIMock < DriverSpecs::MockDriver end end - def booking_guests(booking_id : Int64) - [ - { - email: "visitor@external.com", - name: "Visitor One", - checked_in: false, - visit_expected: true, - }, - ] + # 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. + def booking_guests(booking_id : Int64, include_linked : Bool = false) + case booking_id + when 300 + if 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}, + ] + else + [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool) + end + else + [{email: "visitor@external.com", name: "Visitor One", checked_in: false, visit_expected: true}] + end end def event_guests(event_id : String, system_id : String, ical_uid : String? = nil) @@ -874,4 +882,48 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # "approved" is not a visitor-notification action — no email should be sent system(:Mailer)[:send_count].should eq count_before_approved + + # ================================================================== + # Group booking child-guest fallback tests + # ================================================================== + + # ------------------------------------------------------------------ + # Test 20: booking_changed for a parent "group" booking whose guests + # live on child "visitor" bookings. The driver should fall + # back to fetching guests from linked_bookings when + # booking_guests returns empty for the parent. + # ------------------------------------------------------------------ + + count_before_group = system(:Mailer)[:send_count].as_i + + group_changed_payload = { + action: "changed", + id: 300_i64, + booking_type: "group", + booking_start: now + 7200, + booking_end: now + 10800, + timezone: "GMT", + resource_id: "host@example.com[2026-05-15]", + resource_ids: ["host@example.com[2026-05-15]"], + user_email: "host@example.com", + title: "Group Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 3600, + previous_booking_end: now + 7200, + }.to_json + + publish("staff/booking/changed", group_changed_payload) + sleep 1.5 + + # Two child bookings (301, 302) each have one unique guest, so two + # emails should be sent. + system(:Mailer)[:send_count].should eq count_before_group + 2 + + # Last email should be to visitor-b (second child processed) + system(:Mailer)[:last_to].should eq "visitor-b@external.com" + system(:Mailer)[:last_template].should eq ["visitor_invited", "booking_changed"] + + args20 = system(:Mailer)[:last_args] + args20["event_title"].should eq "Group Visit" + args20["host_email"].should eq "host@example.com" end From 4d18baeba79dd88386415690bf365cde2ca6df5c Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Fri, 15 May 2026 14:13:35 +0930 Subject: [PATCH 07/12] refactor(visitor_mailer): (PPT-2375) --- drivers/place/visitor_mailer_spec.cr | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 8be11b3a66a..c5dbb780cd2 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -884,14 +884,15 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do system(:Mailer)[:send_count].should eq count_before_approved # ================================================================== - # Group booking child-guest fallback tests + # Group booking linked-guest tests # ================================================================== # ------------------------------------------------------------------ - # Test 20: booking_changed for a parent "group" booking whose guests - # live on child "visitor" bookings. The driver should fall - # back to fetching guests from linked_bookings when - # booking_guests returns empty for the parent. + # Test 20: booking_changed for a parent "group" booking. The driver + # passes include_linked: true so the API aggregates guests + # from child bookings into a single response. The mock + # returns 2 unique guests for booking 300 when the flag is + # set, simulating this aggregation. # ------------------------------------------------------------------ count_before_group = system(:Mailer)[:send_count].as_i @@ -915,8 +916,8 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do publish("staff/booking/changed", group_changed_payload) sleep 1.5 - # Two child bookings (301, 302) each have one unique guest, so two - # emails should be sent. + # The mock returns 2 unique guests for booking 300 with + # include_linked: true, so 2 emails should be sent. system(:Mailer)[:send_count].should eq count_before_group + 2 # Last email should be to visitor-b (second child processed) From 483392d7afc20cc24583322fadf640e8515ae693 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Fri, 15 May 2026 15:22:59 +0930 Subject: [PATCH 08/12] refactor(visitor_mailer): (PPT-2375) --- drivers/place/visitor_mailer.cr | 2 +- drivers/place/visitor_mailer_spec.cr | 76 ++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 861f08048f5..ceb9fd10282 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -616,7 +616,7 @@ class Place::VisitorMailer < PlaceOS::Driver # include_linked: true ensures guests from child bookings (e.g. per-visitor # bookings under a group parent) are returned in a single request. - guests = staff_api.booking_guests(details.id, include_linked: true).get.as_a + guests = staff_api.booking_guests(details.id, include_linked: details.booking_type == "group").get.as_a # Deduplicate by email in case a guest appears on multiple child bookings guests.uniq! { |guest| guest["email"].as_s.downcase } diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index c5dbb780cd2..4136acecf56 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -134,6 +134,15 @@ class StaffAPIMock < DriverSpecs::MockDriver else [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool) end + when 301 + if include_linked + [ + {email: "visitor-dup@external.com", name: "Visitor Dup A", checked_in: false, visit_expected: true}, + {email: "VISITOR-DUP@external.com", name: "Visitor Dup B", checked_in: false, visit_expected: true}, + ] + else + [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool) + end else [{email: "visitor@external.com", name: "Visitor One", checked_in: false, visit_expected: true}] end @@ -927,4 +936,71 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do args20 = system(:Mailer)[:last_args] args20["event_title"].should eq "Group Visit" args20["host_email"].should eq "host@example.com" + + # ------------------------------------------------------------------ + # Test 21: deduplication — the mock for booking 301 returns two guests + # with the same email address (different cases) when + # include_linked is true. The driver's uniq! should collapse + # them into a single email. + # ------------------------------------------------------------------ + + count_before_dedup = system(:Mailer)[:send_count].as_i + + dedup_payload = { + action: "changed", + id: 301_i64, + booking_type: "group", + booking_start: now + 7200, + booking_end: now + 10800, + timezone: "GMT", + resource_id: "host@example.com[2026-05-15]", + resource_ids: ["host@example.com[2026-05-15]"], + user_email: "host@example.com", + title: "Dedup Visit", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 3600, + previous_booking_end: now + 7200, + }.to_json + + publish("staff/booking/changed", dedup_payload) + sleep 1.5 + + # Two guests with the same email (case-insensitive) should be + # deduplicated to a single notification. + system(:Mailer)[:send_count].should eq count_before_dedup + 1 + system(:Mailer)[:last_to].should eq "visitor-dup@external.com" + + # ------------------------------------------------------------------ + # Test 22: non-group booking should NOT pass include_linked. The + # mock for booking 300 returns an empty guest list when + # include_linked is false, so if the driver incorrectly + # passes include_linked: true for a non-group type the + # assertion below would fail (2 emails would be sent). + # ------------------------------------------------------------------ + + count_before_non_group = system(:Mailer)[:send_count].as_i + + non_group_payload = { + action: "changed", + id: 300_i64, + booking_type: "desk", + booking_start: now + 7200, + booking_end: now + 10800, + timezone: "GMT", + resource_id: "desk-1", + resource_ids: ["desk-1"], + user_email: "host@example.com", + title: "Desk Booking", + zones: ["zone-building", "zone-room"], + previous_booking_start: now + 3600, + previous_booking_end: now + 7200, + }.to_json + + publish("staff/booking/changed", non_group_payload) + sleep 0.5 + + # Booking 300 with include_linked: false returns no guests, so no + # emails should be sent. This proves the driver does not pass + # include_linked: true for non-group booking types. + system(:Mailer)[:send_count].should eq count_before_non_group end From eac234772164b9f0de947ea5c3f2ceeadacf43ca Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Fri, 15 May 2026 15:44:02 +0930 Subject: [PATCH 09/12] refactor(visitor_mailer): (PPT-2375) --- drivers/place/staff_api.cr | 2 +- drivers/place/visitor_mailer_spec.cr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/place/staff_api.cr b/drivers/place/staff_api.cr index 18c50893461..0fab97f3043 100644 --- a/drivers/place/staff_api.cr +++ b/drivers/place/staff_api.cr @@ -986,7 +986,7 @@ class Place::StaffAPI < PlaceOS::Driver JSON.parse(response.body) end - def booking_guests(booking_id : String | Int64, include_linked : Bool = false) + def booking_guests(booking_id : String | Int64, include_linked : Bool? = nil) logger.debug { "getting guests for booking #{booking_id}" } params = include_linked ? "?include_linked=true" : "" response = get("/api/staff/v1/bookings/#{booking_id}/guests#{params}", headers: authentication) diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index 4136acecf56..b99432716fa 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -123,7 +123,7 @@ class StaffAPIMock < DriverSpecs::MockDriver # 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. - def booking_guests(booking_id : Int64, include_linked : Bool = false) + def booking_guests(booking_id : Int64, include_linked : Bool? = nil) case booking_id when 300 if include_linked From 349f3c69ec7fbcf7d3fcec23fe2833e329b2a18c Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Fri, 15 May 2026 15:50:13 +0930 Subject: [PATCH 10/12] refactor(visitor_mailer): (PPT-2375) --- drivers/place/visitor_mailer.cr | 3 -- drivers/place/visitor_mailer_spec.cr | 44 +--------------------------- 2 files changed, 1 insertion(+), 46 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index ceb9fd10282..d4839a16502 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -618,9 +618,6 @@ class Place::VisitorMailer < PlaceOS::Driver # bookings under a group parent) are returned in a single request. guests = staff_api.booking_guests(details.id, include_linked: details.booking_type == "group").get.as_a - # Deduplicate by email in case a guest appears on multiple child bookings - guests.uniq! { |guest| guest["email"].as_s.downcase } - send_booking_changed_emails( guests, details.user_email, diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index b99432716fa..c1e74f6986f 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -134,15 +134,6 @@ class StaffAPIMock < DriverSpecs::MockDriver else [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool) end - when 301 - if include_linked - [ - {email: "visitor-dup@external.com", name: "Visitor Dup A", checked_in: false, visit_expected: true}, - {email: "VISITOR-DUP@external.com", name: "Visitor Dup B", checked_in: false, visit_expected: true}, - ] - else - [] of NamedTuple(email: String, name: String, checked_in: Bool, visit_expected: Bool) - end else [{email: "visitor@external.com", name: "Visitor One", checked_in: false, visit_expected: true}] end @@ -938,40 +929,7 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do args20["host_email"].should eq "host@example.com" # ------------------------------------------------------------------ - # Test 21: deduplication — the mock for booking 301 returns two guests - # with the same email address (different cases) when - # include_linked is true. The driver's uniq! should collapse - # them into a single email. - # ------------------------------------------------------------------ - - count_before_dedup = system(:Mailer)[:send_count].as_i - - dedup_payload = { - action: "changed", - id: 301_i64, - booking_type: "group", - booking_start: now + 7200, - booking_end: now + 10800, - timezone: "GMT", - resource_id: "host@example.com[2026-05-15]", - resource_ids: ["host@example.com[2026-05-15]"], - user_email: "host@example.com", - title: "Dedup Visit", - zones: ["zone-building", "zone-room"], - previous_booking_start: now + 3600, - previous_booking_end: now + 7200, - }.to_json - - publish("staff/booking/changed", dedup_payload) - sleep 1.5 - - # Two guests with the same email (case-insensitive) should be - # deduplicated to a single notification. - system(:Mailer)[:send_count].should eq count_before_dedup + 1 - system(:Mailer)[:last_to].should eq "visitor-dup@external.com" - - # ------------------------------------------------------------------ - # Test 22: non-group booking should NOT pass include_linked. The + # Test 21: non-group booking should NOT pass include_linked. The # mock for booking 300 returns an empty guest list when # include_linked is false, so if the driver incorrectly # passes include_linked: true for a non-group type the From 60e6767c6e13e279e1ca55d84ff2a3b9e7a28fdf Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 21 May 2026 13:35:45 +0930 Subject: [PATCH 11/12] fix(visitor_mailer): correctly handle room only change (PPT-2375) --- drivers/place/visitor_mailer.cr | 95 +++++++++++++++------- drivers/place/visitor_mailer_spec.cr | 117 ++++++++++++++++++++++++++- 2 files changed, 182 insertions(+), 30 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index d4839a16502..8ace54fefcd 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -54,6 +54,11 @@ class Place::VisitorMailer < PlaceOS::Driver invite_zone_tag: "building", is_campus: false, + # 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). + skip_event_linked_booking_email: true, + domain_uri: "https://example.com/", jwt_private_key: PlaceOS::Model::JWTBase.private_key, }) @@ -139,6 +144,7 @@ class Place::VisitorMailer < PlaceOS::Driver @network_password_minimum_symbols : Int32 = DEFAULT_PASSWORD_MINIMUM_SYMBOLS @network_group_ids = [] of String @disable_event_visitors : Bool = true + @skip_event_linked_booking_email : Bool = true @uri : URI = URI.new @jwt_private_key : String = PlaceOS::Model::JWTBase.private_key @@ -170,6 +176,8 @@ 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 + skip_event_linked = setting?(Bool, :skip_event_linked_booking_email) + @skip_event_linked_booking_email = skip_event_linked.nil? ? true : skip_event_linked @invite_zone_tag = setting?(String, :invite_zone_tag) || "building" @is_parent_zone = setting?(Bool, :is_campus) || false @@ -306,7 +314,17 @@ class Place::VisitorMailer < PlaceOS::Driver in BookingGuest area_name = @booking_space_name template = @booking_template - booking_type = staff_api.get_booking(guest_details.booking_id).get["booking_type"].as_s + booking = staff_api.get_booking(guest_details.booking_id).get + + if @skip_event_linked_booking_email + parent_id = booking.dig?("extension_data", "parent_id").try(&.as_s?) + if parent_id && !parent_id.empty? + logger.debug { "skipping booking_created email for booking #{guest_details.booking_id} as it is linked to event #{parent_id}" } + return + end + end + + booking_type = booking["booking_type"].as_s template = @group_event_template if booking_type == "group-event" in GuestNotification # should never get here @@ -692,37 +710,20 @@ class Place::VisitorMailer < PlaceOS::Driver return unless fields_changed - # Resolve previous location names from previous_system_id when room changed. - # Default previous_room_name to "unknown" when we know there was a different - # previous system — if the lookup succeeds it will be overwritten with the - # real name; if it fails (rescue) the "unknown" default is preserved and the - # recipient can see that the original location could not be determined. + # Previous location — "unknown" fallback is preserved if the lookup fails + # so recipients can see the original location couldn't be determined. previous_building_name = building_zone.display_name.presence || building_zone.name previous_room_name = @booking_space_name if (prev_sys_id = details.previous_system_id) && prev_sys_id != details.system_id previous_room_name = "unknown" - begin - prev_sys = get_room_details(prev_sys_id) - previous_room_name = prev_sys.display_name.presence || prev_sys.name - if prev_zones = prev_sys.zones - prev_zones.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 - break - end - rescue error - logger.warn(exception: error) { "error looking up previous zone #{zone_id}" } - end - end - end - rescue error - logger.warn(exception: error) { "error looking up previous system #{prev_sys_id}" } - end + previous_room_name, previous_building_name = resolve_system_location_names(prev_sys_id, previous_room_name, previous_building_name) end + current_building_name = building_zone.display_name.presence || building_zone.name + current_room_name = @booking_space_name + current_room_name, current_building_name = resolve_system_location_names(details.system_id, current_room_name, current_building_name) + guests = staff_api.event_guests(details.event_id, details.system_id, details.event_ical_uid).get.as_a send_booking_changed_emails( guests, @@ -732,6 +733,8 @@ class Place::VisitorMailer < PlaceOS::Driver details.previous_event_start, previous_building_name, previous_room_name, + current_building_name, + current_room_name, ) rescue error logger.error { error.inspect_with_backtrace } @@ -743,7 +746,9 @@ class Place::VisitorMailer < PlaceOS::Driver } end - # Sends booking-changed notification emails to each visitor in the guest list. + # `building_name` / `room_name` override the current location names; when + # omitted they fall back to `building_zone` / `@booking_space_name` (used by + # the booking flow, which has no system_id to resolve from). private def send_booking_changed_emails( guests : Array(JSON::Any), host_email : String, @@ -752,7 +757,12 @@ class Place::VisitorMailer < PlaceOS::Driver previous_start : Int64?, previous_building_name : String, previous_room_name : String, + building_name : String? = nil, + room_name : String? = nil, ) + resolved_building_name = building_name || (building_zone.display_name.presence || building_zone.name) + resolved_room_name = room_name || @booking_space_name + guests.each do |guest| visitor_email = guest["email"].as_s visitor_name = guest["name"].as_s? @@ -773,8 +783,8 @@ class Place::VisitorMailer < PlaceOS::Driver visitor_name: visitor_name, host_name: get_host_name(host_email), host_email: host_email, - room_name: @booking_space_name, - building_name: building_zone.display_name.presence || building_zone.name, + 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), @@ -790,6 +800,35 @@ class Place::VisitorMailer < PlaceOS::Driver end 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} + room_name = fallback_room + building_name = fallback_building + + begin + sys = get_room_details(system_id) + room_name = sys.display_name.presence || sys.name + if zones = sys.zones + zones.each do |zone_id| + begin + zone = fetch_zone(zone_id) + if zone.tags.includes?(@invite_zone_tag) + building_name = zone.display_name.presence || zone.name + break + end + rescue error + logger.warn(exception: error) { "error looking up zone #{zone_id} for system #{system_id}" } + end + end + end + rescue error + logger.warn(exception: error) { "error looking up system #{system_id}" } + end + + {room_name, building_name} + end + @[Security(Level::Support)] def send_visitor_qr_email( template : String, diff --git a/drivers/place/visitor_mailer_spec.cr b/drivers/place/visitor_mailer_spec.cr index c1e74f6986f..29f5b316813 100644 --- a/drivers/place/visitor_mailer_spec.cr +++ b/drivers/place/visitor_mailer_spec.cr @@ -41,6 +41,11 @@ class MailerMock < DriverSpecs::MockDriver ) : Bool true end + + # Used by send_visitor_qr_email when @disable_qr_code is false (the default). + def generate_png_qrcode(text : String, size : Int32 = 256) + "PNG-#{text}-#{size}" + end end # :nodoc: @@ -162,6 +167,47 @@ class StaffAPIMock < DriverSpecs::MockDriver {id: id, name: "Unknown Room", display_name: nil, map_id: nil, zones: [] of String} end end + + # 600 — standalone visitor booking + # 601 — event-linked visitor booking (parent_id set) + # 602 — group-event booking + def get_booking(booking_id : Int64, instance : Int64? = nil) + case booking_id + when 601_i64 + { + id: 601, + booking_type: "visitor", + booking_start: 0, + booking_end: 0, + resource_id: "visitor@external.com", + user_email: "host@example.com", + title: "Linked Visit", + extension_data: {parent_id: "event-evt-200"}, + } + when 602_i64 + { + id: 602, + booking_type: "group-event", + booking_start: 0, + booking_end: 0, + resource_id: "visitor@external.com", + user_email: "host@example.com", + title: "Group Event Visit", + extension_data: {} of String => String, + } + else + { + id: booking_id, + booking_type: "visitor", + booking_start: 0, + booking_end: 0, + resource_id: "visitor@external.com", + user_email: "host@example.com", + title: "Standalone Visit", + extension_data: {} of String => String, + } + end + end end DriverSpecs.mock_driver "Place::VisitorMailer" do @@ -818,8 +864,10 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # previous location should come from the previous system (sys-old-room), NOT the current args18["previous_room_name"].should eq "Old Conference Room 202" args18["previous_building_name"].should eq "Previous Building" - # current location should remain correct - args18["room_name"].should eq "Client Floor" + # current location should be resolved from the signal's system_id (sys-room1) + # rather than the static @booking_space_name setting, so visitors can see + # which room the meeting moved to. + args18["room_name"].should eq "Conference Room 1" args18["building_name"].should eq "Main Building" # ------------------------------------------------------------------ @@ -961,4 +1009,69 @@ DriverSpecs.mock_driver "Place::VisitorMailer" do # emails should be sent. This proves the driver does not pass # include_linked: true for non-group booking types. system(:Mailer)[:send_count].should eq count_before_non_group + + # Test 22: event-linked booking_created is skipped by default + + count_before_linked = system(:Mailer)[:send_count].as_i + + linked_booking_created_payload = { + action: "booking_created", + booking_id: 601_i64, + resource_id: "visitor@external.com", + event_title: "Linked Visit", + event_summary: "Linked Visit", + event_starting: now + 3600, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host@example.com", + zones: ["zone-building", "zone-room"], + }.to_json + + publish("staff/guest/attending", linked_booking_created_payload) + sleep 0.5 + + system(:Mailer)[:send_count].should eq count_before_linked + + # Test 23: standalone booking_created still emits the booking template + + count_before_standalone = system(:Mailer)[:send_count].as_i + + standalone_booking_created_payload = { + action: "booking_created", + booking_id: 600_i64, + resource_id: "visitor@external.com", + event_title: "Standalone Visit", + event_summary: "Standalone Visit", + event_starting: now + 3600, + attendee_name: "Visitor One", + attendee_email: "visitor@external.com", + host: "host@example.com", + zones: ["zone-building", "zone-room"], + }.to_json + + publish("staff/guest/attending", standalone_booking_created_payload) + sleep 0.5 + + system(:Mailer)[:send_count].should eq count_before_standalone + 1 + system(:Mailer)[:last_template].should eq ["visitor_invited", "booking"] + system(:Mailer)[:last_to].should eq "visitor@external.com" + + # Test 24: opt-out restores the legacy dual-email behaviour + + settings({ + timezone: "GMT", + booking_space_name: "Client Floor", + invite_zone_tag: "building", + skip_event_linked_booking_email: false, + }) + sleep 1.0 + + count_before_opt_out = system(:Mailer)[:send_count].as_i + + publish("staff/guest/attending", linked_booking_created_payload) + sleep 0.5 + + system(:Mailer)[:send_count].should eq count_before_opt_out + 1 + system(:Mailer)[:last_template].should eq ["visitor_invited", "booking"] + system(:Mailer)[:last_to].should eq "visitor@external.com" end From 35bcf269074c1a583252f692074b7e6fdfff4fdd Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Thu, 21 May 2026 13:47:53 +0930 Subject: [PATCH 12/12] refactor(visitor_mailer): (PPT-2375) --- drivers/place/visitor_mailer.cr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/place/visitor_mailer.cr b/drivers/place/visitor_mailer.cr index 8ace54fefcd..94f3938b600 100644 --- a/drivers/place/visitor_mailer.cr +++ b/drivers/place/visitor_mailer.cr @@ -710,12 +710,12 @@ class Place::VisitorMailer < PlaceOS::Driver return unless fields_changed - # Previous location — "unknown" fallback is preserved if the lookup fails - # so recipients can see the original location couldn't be determined. previous_building_name = building_zone.display_name.presence || building_zone.name previous_room_name = @booking_space_name if (prev_sys_id = details.previous_system_id) && prev_sys_id != details.system_id + # Use "unknown" as the fallback so a failed lookup surfaces in the email + # rather than silently showing the current room name. previous_room_name = "unknown" previous_room_name, previous_building_name = resolve_system_location_names(prev_sys_id, previous_room_name, previous_building_name) end