Skip to content

fix(visitor_mailer): don't re-invite visitors when an event changes room (PPT-2375) #620

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
chillfox wants to merge 1 commit into from
+834 −17
Draft

fix(visitor_mailer): don't re-invite visitors when an event changes room (PPT-2375) #620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
206 changes: 189 additions & 17 deletions drivers/place/visitor_mailer.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,20 @@ class Place::VisitorMailer < PlaceOS::Driver
@skip_event_linked_booking_email : Bool = true
@skip_host_email : Bool = true

# How long an event is remembered as changed after its notification was
# buffered, so a re-invite that arrives late is still recognised as one.
CHANGE_MEMORY = 60

# Coalescing buffer for staff/event/changed, swept once the window elapses.
# seconds to buffer a change; 0 emails on every signal
@event_change_debounce : Int32 = 15
# ical_uid (or event_id) => coalesced change awaiting its flush
@pending_event_changes : Hash(String, PendingEventChange) = {} of String => PendingEventChange
# "event key|attendee" => invite awaiting its flush
@pending_invites : Hash(String, PendingInvite) = {} of String => PendingInvite
# ical_uid and event_id of a changed event => expiry
@recent_event_changes : Hash(String, Time::Span) = {} of String => Time::Span
# guards all three of the above
@pending_event_changes_lock : Mutex = Mutex.new

@uri : URI = URI.new
Expand Down Expand Up @@ -219,23 +228,27 @@ class Place::VisitorMailer < PlaceOS::Driver
zones = control_system_zone_list

# The sweep below picks up the rest; with the debounce off nothing would.
flush_event_changes("debounce disabled") if @event_change_debounce <= 0
if @event_change_debounce <= 0
flush_event_changes("debounce disabled")
flush_pending_invites("debounce disabled")
end

schedule.clear
if reminders = @send_reminders
schedule.cron(reminders, @time_zone) { send_reminder_emails }
end

# Sweeps at most every 5s, so a change waits its debounce plus up to one interval.
schedule.every(@event_change_debounce.clamp(1, 5).seconds) { sweep_event_changes } if @event_change_debounce > 0
schedule.every(@event_change_debounce.clamp(1, 5).seconds) { sweep_buffers } if @event_change_debounce > 0

spawn { ensure_building_zone(zones) }
end

# The scheduler is dead by now, so nothing else would sweep the buffer.
# The scheduler is dead by now, so nothing else would sweep the buffers.
# Bounded to return within the driver manager's 6s unload budget.
def on_unload
flush_event_changes("driver unloading", wait: 5.seconds)
flush_event_changes("driver unloading", wait: 3.seconds)
flush_pending_invites("driver unloading", wait: 2.seconds)
end

def control_system_zone_list
Expand Down Expand Up @@ -360,9 +373,25 @@ class Place::VisitorMailer < PlaceOS::Driver
in EventGuest
return if @disable_event_visitors

room = get_room_details(guest_details.system_id)
area_name = room.display_name.presence || room.name
template = @event_template
# Moving an event to another room makes staff-api re-signal every visitor
# already attending it, which is a change rather than a new invitation.
# Hold the invite until the change window closes and drop it if a change
# for the same event turns up — the change email covers those visitors.
# The two signals are emitted concurrently, so either can arrive first.
if guest_details.action == "meeting_update"
if event_changed?(guest_details.event_ical_uid, guest_details.event_id)
logger.debug { "ignoring re-invite for #{guest_details.attendee_email} as event #{guest_details.event_id} changed" }
return
end

if @event_change_debounce > 0
buffer_pending_invite(guest_details)
return
end
end

send_event_invite(guest_details)
return
in BookingGuest
area_name = @booking_space_name
template = @booking_template
Expand Down Expand Up @@ -562,7 +591,7 @@ class Place::VisitorMailer < PlaceOS::Driver
{name: "previous_event_time", description: "The original time 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

[
TemplateFields.new(
Expand Down Expand Up @@ -721,6 +750,8 @@ class Place::VisitorMailer < PlaceOS::Driver
details.previous_booking_start,
previous_building_name,
previous_room_name,
event_id: details.id.to_s,
resource_id: details.resource_id,
)
rescue error
logger.error { error.inspect_with_backtrace }
Expand Down Expand Up @@ -794,6 +825,11 @@ class Place::VisitorMailer < PlaceOS::Driver

return unless fields_changed

# Remember the edit before buffering it: staff-api re-invites the visitors
# already on the event when it moves rooms, and that signal may arrive
# either side of this one.
register_event_change(details.event_ical_uid, details.event_id)

# Coalesce the burst of signals Office365 emits per edit into one email.
change = PendingEventChange.new(
details.event_id, details.system_id, details.event_ical_uid,
Expand All @@ -811,6 +847,25 @@ class Place::VisitorMailer < PlaceOS::Driver
}
end

# Emails a visitor their invitation to a calendar event. The room is resolved
# here rather than on receipt so a held invite names the settled room.
private def send_event_invite(guest : EventGuest) : Nil
room = get_room_details(guest.system_id)

send_visitor_qr_email(
@event_template,
guest.attendee_email,
guest.attendee_name,
guest.host,
guest.event_title || guest.event_summary,
guest.event_starting,
guest.resource_id,
guest.event_id,
room.display_name.presence || room.name,
system_id: guest.system_id,
)
end

# Collapses the burst of signals for one edit into a single buffered change.
# Keyed by event instance, so the rooms either side of a move coalesce too;
# the one email then names a single room and uses that room's guest list.
Expand All @@ -824,9 +879,71 @@ class Place::VisitorMailer < PlaceOS::Driver
end
end

# Sends any change that has been buffered for its full debounce window.
private def sweep_event_changes : Nil
flush_event_changes("debounce window elapsed", older_than: Time.monotonic - @event_change_debounce.seconds)
# Holds an invite back until we know whether the same edit also produced a
# change notification, which supersedes it.
private def buffer_pending_invite(guest : EventGuest) : Nil
invite = PendingInvite.new(guest)
@pending_event_changes_lock.synchronize do
@pending_invites[invite.buffer_key] ||= invite
end
end

# Records that visitors are being emailed about an edit of this event. Both
# identifiers are stored as a signal may carry either one alone.
private def register_event_change(ical_uid : String?, event_id : String?) : Nil
expiry = Time.monotonic + (@event_change_debounce + CHANGE_MEMORY).seconds
@pending_event_changes_lock.synchronize do
expire_event_changes
{ical_uid, event_id}.each do |key|
@recent_event_changes[key] = expiry if key && !key.blank?
end
end
end

private def event_changed?(ical_uid : String?, event_id : String?) : Bool
@pending_event_changes_lock.synchronize do
expire_event_changes
{ical_uid, event_id}.any? { |key| key && !key.blank? && @recent_event_changes.has_key?(key) }
end
end

# Callers must hold `@pending_event_changes_lock`.
private def expire_event_changes : Nil
now = Time.monotonic
@recent_event_changes.reject! { |_key, expiry| expiry <= now }
end

# Sends anything that has been buffered for its full debounce window.
private def sweep_buffers : Nil
cutoff = Time.monotonic - @event_change_debounce.seconds
flush_event_changes("debounce window elapsed", older_than: cutoff)
flush_pending_invites("debounce window elapsed", older_than: cutoff)
end

# Sends the invites whose window has closed, dropping any the same edit's
# change notification has since covered.
private def flush_pending_invites(reason : String, older_than : Time::Span? = nil, wait : Time::Span? = nil) : Nil
flushing = @pending_event_changes_lock.synchronize do
ready = if cutoff = older_than
@pending_invites.values.select { |pending| pending.first_seen <= cutoff }
else
@pending_invites.values
end
ready.each { |pending| @pending_invites.delete(pending.buffer_key) }
ready
end
return if flushing.empty?

logger.debug { "flushing #{flushing.size} held invite(s): #{reason}" }

dispatch_flushed(flushing, "held invites: #{reason}", wait) do |pending|
guest = pending.guest
if event_changed?(guest.event_ical_uid, guest.event_id)
logger.debug { "dropping held invite for #{guest.attendee_email} as event #{guest.event_id} changed" }
else
send_event_invite(guest)
end
end
end

# Dispatches matching changes, each in its own fiber so a slow send can't stall
Expand All @@ -846,17 +963,23 @@ class Place::VisitorMailer < PlaceOS::Driver

logger.debug { "flushing #{flushing.size} pending event change(s): #{reason}" }

complete = Channel(Nil).new(flushing.size)
flushing.each do |pending|
dispatch_flushed(flushing, "event changes: #{reason}", wait) { |pending| dispatch_event_change(pending) }
end

# Runs the handler for each flushed entry in its own fiber so a slow send can't
# stall the sweep. `wait` bounds how long we block for them to finish.
private def dispatch_flushed(items : Array(T), reason : String, wait : Time::Span?, &handler : T -> Nil) : Nil forall T
complete = Channel(Nil).new(items.size)
items.each do |item|
spawn do
dispatch_event_change(pending)
handler.call(item)
rescue error
logger.error { error.inspect_with_backtrace }
self[:error_count] = @error_count += 1
self[:last_error] = {
error: error.message,
time: Time.local.to_s,
user: "flushing event change #{pending.event_id}: #{reason}",
user: "flushing #{reason}",
}
ensure
complete.send(nil)
Expand All @@ -865,14 +988,14 @@ class Place::VisitorMailer < PlaceOS::Driver
return unless wait

deadline = Time.monotonic + wait
flushing.size.times do |index|
items.size.times do |index|
remaining = deadline - Time.monotonic
remaining = Time::Span.zero if remaining < Time::Span.zero

select
when complete.receive
when timeout(remaining)
logger.warn { "timeout flushing pending event changes: #{reason}, #{flushing.size - index} of #{flushing.size} still in flight" }
logger.warn { "timeout flushing #{reason}, #{items.size - index} of #{items.size} still in flight" }
break
end
end
Expand Down Expand Up @@ -919,12 +1042,19 @@ class Place::VisitorMailer < PlaceOS::Driver
previous_room_name,
current_building_name,
current_room_name,
event_id: change.event_id,
resource_id: system_id,
system_id: system_id,
)
end

# `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).
#
# `event_id` / `resource_id` / `system_id` identify the visit for the QR code
# and kiosk link; supplying them mirrors what the invitation email carries, so
# a visitor whose meeting moved has a check-in code for the new room.
private def send_booking_changed_emails(
guests : Array(JSON::Any),
template : String,
Expand All @@ -936,6 +1066,9 @@ class Place::VisitorMailer < PlaceOS::Driver
previous_room_name : String,
building_name : String? = nil,
room_name : String? = nil,
event_id : String? = nil,
resource_id : String? = nil,
system_id : String? = nil,
)
resolved_building_name = building_name || (building_zone.display_name.presence || building_zone.name)
resolved_room_name = room_name || @booking_space_name
Expand All @@ -955,6 +1088,28 @@ class Place::VisitorMailer < PlaceOS::Driver
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) }

guest_jwt = kiosk_url = ""
attach = [] of NamedTuple(file_name: String, content: String, content_id: String)

if event_id
qr_resource = resource_id.presence || system_id.presence || ""
jwt_resource = system_id.presence || resource_id.presence || ""

guest_jwt = generate_guest_jwt(visitor_name || visitor_email, visitor_email, visitor_email, event_id, jwt_resource)
kiosk_url = "/visitor-kiosk/?email=#{visitor_email}&token=#{guest_jwt}&event_id=#{event_id}#/checkin/preferences"

unless @disable_qr_code
qr_png = mailer.generate_png_qrcode(text: "VISIT:#{visitor_email},#{qr_resource},#{event_id},#{host_email}", size: 256).get.as_s
attach = [
{
file_name: "qr.png",
content: qr_png,
content_id: visitor_email,
},
]
end
end

mailer.send_template(
visitor_email,
{"visitor_invited", template},
Expand All @@ -973,7 +1128,10 @@ class Place::VisitorMailer < PlaceOS::Driver
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,
},
attach,
reply_to: host_email.presence,
)
rescue error
Expand Down Expand Up @@ -1236,6 +1394,20 @@ class Place::VisitorMailer < PlaceOS::Driver
end
end

# A visitor invitation held back until the change window for its event closes.
class PendingInvite
getter guest : EventGuest
getter first_seen : Time::Span = Time.monotonic
# one invite per attendee per event instance; a repeated signal for the same
# pair is the same invitation, not a second one.
getter buffer_key : String

def initialize(@guest)
event_key = @guest.event_ical_uid.presence || @guest.event_id
@buffer_key = "#{event_key}|#{@guest.attendee_email.downcase}"
end
end

# zone_id, timeout, zone
alias ZoneCache = Hash(String, Tuple(Int64, ZoneDetails))

Expand Down
28 changes: 28 additions & 0 deletions drivers/place/visitor_mailer_readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,34 @@ moves the meeting *and* changes the time sends one email describing both rather
one per room. A move between buildings is handled by two separate mailer modules and
so still sends an email each.

### Re-invites during a room move

Moving an event to another room makes the staff API re-announce every visitor already
attending it, exactly as though each had just been invited. That is a change, not a
new invitation, so those invites are held for the same debounce window and dropped
when a change notification for the same event turns up — the change email covers
those visitors. The two signals are emitted concurrently, so either order works.

Consequences worth knowing:

* invitations for visitors added to an *existing* event are delayed by the debounce
window (invitations for brand new events are always sent immediately),
* a visitor added by the same edit that moved the room receives the change
notification instead of an invitation. They still get a QR code and kiosk link (see
below), so nothing is lost.

## QR code and kiosk link on change notifications

The `booking_changed` and `event_changed` templates receive `guest_jwt` and
`kiosk_url` fields and the same inline `qr.png` attachment as an invitation, because
a move invalidates the kiosk link issued with the original invite (its token is scoped
to the old room) and the invite is no longer re-sent.

Reference the attachment from the template the same way the invite template does, or
set `disable_qr_code: true` to leave it off. Until a template uses them the fields are
simply unused, though an unreferenced attachment may still show up in some mail
clients.

## Reply-To

Visitor emails set a `Reply-To` header so replies reach a useful person rather than
Expand Down
Loading
Loading