From 935fdc6db1c9667df9cef44c4befa0fc5fb13364 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Wed, 29 Apr 2026 13:51:19 +0930 Subject: [PATCH 1/3] test(room_booking_approval): [PPT-2340] add tests --- drivers/place/room_booking_approval_spec.cr | 455 ++++++++++++++++++-- 1 file changed, 427 insertions(+), 28 deletions(-) diff --git a/drivers/place/room_booking_approval_spec.cr b/drivers/place/room_booking_approval_spec.cr index f30247a012..a182292af7 100644 --- a/drivers/place/room_booking_approval_spec.cr +++ b/drivers/place/room_booking_approval_spec.cr @@ -13,15 +13,141 @@ end # :nodoc: class CalendarMock < DriverSpecs::MockDriver + def on_load + self[:accept_event_calls] = 0 + self[:accept_event_args] = nil + + self[:decline_event_calls] = 0 + self[:decline_event_args] = nil + + self[:get_event_calls] = 0 + self[:get_event_args] = nil + end + + def accept_event(calendar_id : String, event_id : String, user_id : String? = nil, notify : Bool = true, comment : String? = nil) + self[:accept_event_calls] = self[:accept_event_calls].as_i + 1 + self[:accept_event_args] = {calendar_id: calendar_id, event_id: event_id, user_id: user_id, notify: notify, comment: comment} + nil + end + + def decline_event(calendar_id : String, event_id : String, user_id : String? = nil, notify : Bool = true, comment : String? = nil) + self[:decline_event_calls] = self[:decline_event_calls].as_i + 1 + self[:decline_event_args] = {calendar_id: calendar_id, event_id: event_id, user_id: user_id, notify: notify, comment: comment} + nil + end + + def get_event(user_id : String, id : String, calendar_id : String) + self[:get_event_calls] = self[:get_event_calls].as_i + 1 + self[:get_event_args] = {user_id: user_id, id: id, calendar_id: calendar_id} + + # Return event data that matches PlaceCalendar::Event JSON shape. + # If the requested id is an occurrence id ("occurrence-of-series-1"), + # return an event whose recurring_event_id differs from the requested id. + # Otherwise return the id as-is (it is the series root). + case id + when "occurrence-of-series-1" + { + event_start: 1.hour.from_now.to_unix, + event_end: 2.hours.from_now.to_unix, + id: "occurrence-of-series-1", + recurring_event_id: "series-root-1", + host: calendar_id, + title: "Occurrence Event", + attendees: [] of Nil, + hide_attendees: false, + private: false, + all_day: false, + attachments: [] of Nil, + status: "tentative", + } + else + { + event_start: 1.hour.from_now.to_unix, + event_end: 2.hours.from_now.to_unix, + id: id, + recurring_event_id: id, + host: calendar_id, + title: "Series Root Event", + attendees: [] of Nil, + hide_attendees: false, + private: false, + all_day: false, + attachments: [] of Nil, + status: "tentative", + } + end + end end # :nodoc: -# Mimics how the real Bookings module (see bookings.cr poll_events) stores events: -# - confirmed events go into self[:bookings] -# - tentative events go into self[:tentative] (and are excluded from :bookings -# unless @show_tentative_meetings is true, which is false by default) +# Provides tentative events for the approval cache. +# The data set covers: +# - a standalone tentative event +# - two events belonging to recurring series "series-root-1" +# - one event belonging to a different series "series-root-2" class BookingsMock < DriverSpecs::MockDriver + TENTATIVE_EVENTS = [ + { + event_start: 1.hour.from_now.to_unix, + event_end: 2.hours.from_now.to_unix, + id: "tentative-event-1", + recurring_event_id: nil, + host: "jane@example.com", + title: "Standalone Tentative", + attendees: [{name: "Room 5", email: "room5@example.com", response_status: "tentative", resource: true}], + hide_attendees: false, + private: false, + all_day: false, + attachments: [] of Nil, + status: "tentative", + }, + { + event_start: 1.day.from_now.to_unix, + event_end: (1.day.from_now + 1.hour).to_unix, + id: "tentative-event-2", + recurring_event_id: "series-root-1", + host: "bob@example.com", + title: "Weekly Standup (Mon)", + attendees: [] of Nil, + hide_attendees: false, + private: false, + all_day: false, + attachments: [] of Nil, + status: "tentative", + }, + { + event_start: 2.days.from_now.to_unix, + event_end: (2.days.from_now + 1.hour).to_unix, + id: "tentative-event-3", + recurring_event_id: "series-root-1", + host: "bob@example.com", + title: "Weekly Standup (Tue)", + attendees: [] of Nil, + hide_attendees: false, + private: false, + all_day: false, + attachments: [] of Nil, + status: "tentative", + }, + { + event_start: 3.days.from_now.to_unix, + event_end: (3.days.from_now + 1.hour).to_unix, + id: "tentative-event-4", + recurring_event_id: "series-root-2", + host: "alice@example.com", + title: "Design Review", + attendees: [] of Nil, + hide_attendees: false, + private: false, + all_day: false, + attachments: [] of Nil, + status: "tentative", + }, + ] + def on_load + self[:poll_events_calls] = 0 + self[:bookings] = [{ event_start: 1.hour.ago.to_unix, event_end: 1.hour.from_now.to_unix, @@ -36,24 +162,14 @@ class BookingsMock < DriverSpecs::MockDriver status: "confirmed", }] - self[:tentative] = [{ - event_start: 1.hour.ago.to_unix, - event_end: 1.hour.from_now.to_unix, - id: "tentative-event-id", - host: "jane@example.com", - title: "Approval Test", - attendees: [{ - name: "Room 5", - email: "room5@example.com", - response_status: "tentative", - resource: true, - }], - hide_attendees: false, - private: false, - all_day: false, - attachments: [] of Nil, - status: "tentative", - }] + self[:tentative] = TENTATIVE_EVENTS + end + + # Simulates the real Bookings#poll_events: re-polls the calendar and rebuilds + # the tentative list. In this mock, we filter out events whose id or + # recurring_event_id appears in the CalendarMock's accepted/declined event list. + def poll_events + self[:poll_events_calls] = self[:poll_events_calls].as_i + 1 end end @@ -64,17 +180,300 @@ DriverSpecs.mock_driver "Place::RoomBookingApproval" do Bookings: {BookingsMock}, }) + # Disable the debounced Bookings re-poll for all sections that don't + # test it. This avoids residual timers leaking between test sections. + settings({ + disable_refresh_bookings: true, + }) + sleep 0.5 + # =================================================================== + # find_bookings_for_approval — discovers tentative events + # =================================================================== + exec(:find_bookings_for_approval).get approval = status["approval_required"].as_h - - # Tentative events should appear in approval_required, - # read from the "tentative" status key (not "bookings"). approval.has_key?("spec_runner_system").should be_true + events = approval["spec_runner_system"].as_a - events.size.should eq(1) - events[0]["id"].should eq("tentative-event-id") - events[0]["status"].should eq("tentative") + events.size.should eq(4) + + event_ids = events.map { |e| e["id"].as_s } + event_ids.should contain("tentative-event-1") + event_ids.should contain("tentative-event-2") + event_ids.should contain("tentative-event-3") + event_ids.should contain("tentative-event-4") + + # Confirmed events must not appear in the approval cache. + event_ids.should_not contain("confirmed-event-id") + + # All discovered events should have tentative status. + events.each { |e| e["status"].should eq("tentative") } + + # =================================================================== + # clear_cache — remove a single standalone event + # =================================================================== + + exec(:clear_cache, "tentative-event-1").get + + approval = status["approval_required"].as_h + remaining = approval["spec_runner_system"].as_a + remaining.size.should eq(3) + remaining.map { |e| e["id"].as_s }.should_not contain("tentative-event-1") + + # =================================================================== + # clear_cache — remove all events in a recurring series + # =================================================================== + + # Re-populate from Bookings so we have the full set again. + exec(:find_bookings_for_approval).get + + # Clearing by recurring_event_id should remove every event in that series. + exec(:clear_cache, "series-root-1").get + + approval = status["approval_required"].as_h + remaining = approval["spec_runner_system"].as_a + remaining_ids = remaining.map { |e| e["id"].as_s } + + # Both occurrences of series-root-1 should be gone. + remaining_ids.should_not contain("tentative-event-2") + remaining_ids.should_not contain("tentative-event-3") + + # Events from other series and standalone events are untouched. + remaining_ids.should contain("tentative-event-1") + remaining_ids.should contain("tentative-event-4") + remaining.size.should eq(2) + + # =================================================================== + # clear_cache(nil) — clear entire cache + # =================================================================== + + exec(:find_bookings_for_approval).get + exec(:clear_cache).get + + approval = status["approval_required"].as_h + approval.size.should eq(0) + + # =================================================================== + # accept_event — accepts a single event and clears it from cache + # =================================================================== + + exec(:find_bookings_for_approval).get + + exec(:accept_event, + calendar_id: "room5@example.com", + event_id: "tentative-event-1", + ).get + + # Verify Calendar.accept_event was called with correct arguments. + system(:Calendar_1)[:accept_event_calls].should eq(1) + accept_args = system(:Calendar_1)[:accept_event_args].as_h + accept_args["calendar_id"].should eq("room5@example.com") + accept_args["event_id"].should eq("tentative-event-1") + + # The accepted event should be removed from the approval cache. + approval = status["approval_required"].as_h + remaining = approval["spec_runner_system"].as_a + remaining.map { |e| e["id"].as_s }.should_not contain("tentative-event-1") + remaining.size.should eq(3) + + # =================================================================== + # decline_event — declines a single event and clears it from cache + # =================================================================== + + exec(:decline_event, + calendar_id: "room5@example.com", + event_id: "tentative-event-4", + ).get + + # Verify Calendar.decline_event was called. + system(:Calendar_1)[:decline_event_calls].should eq(1) + decline_args = system(:Calendar_1)[:decline_event_args].as_h + decline_args["calendar_id"].should eq("room5@example.com") + decline_args["event_id"].should eq("tentative-event-4") + + # The declined event should be removed from the approval cache. + approval = status["approval_required"].as_h + remaining = approval["spec_runner_system"].as_a + remaining.map { |e| e["id"].as_s }.should_not contain("tentative-event-4") + remaining.size.should eq(2) + + # =================================================================== + # accept_recurring_event — with check_recurring_event_id: false (default) + # Accepts a series by recurring_event_id and clears all occurrences. + # =================================================================== + + exec(:find_bookings_for_approval).get + + exec(:accept_recurring_event, + calendar_id: "room5@example.com", + recurring_event_id: "series-root-1", + ).get + + # Calendar.accept_event should be called with the series root id. + accept_args = system(:Calendar_1)[:accept_event_args].as_h + accept_args["event_id"].should eq("series-root-1") + + # get_event should NOT have been called (check_recurring_event_id is false). + system(:Calendar_1)[:get_event_calls].should eq(0) + + # All events in the series should be removed from the approval cache. + approval = status["approval_required"].as_h + remaining = approval["spec_runner_system"].as_a + remaining_ids = remaining.map { |e| e["id"].as_s } + remaining_ids.should_not contain("tentative-event-2") + remaining_ids.should_not contain("tentative-event-3") + + # The standalone event and the other series are untouched. + remaining_ids.should contain("tentative-event-1") + remaining_ids.should contain("tentative-event-4") + + # =================================================================== + # decline_recurring_event — with check_recurring_event_id: false + # =================================================================== + + exec(:find_bookings_for_approval).get + + exec(:decline_recurring_event, + calendar_id: "room5@example.com", + recurring_event_id: "series-root-2", + ).get + + decline_args = system(:Calendar_1)[:decline_event_args].as_h + decline_args["event_id"].should eq("series-root-2") + + system(:Calendar_1)[:get_event_calls].should eq(0) + + approval = status["approval_required"].as_h + remaining = approval["spec_runner_system"].as_a + remaining.map { |e| e["id"].as_s }.should_not contain("tentative-event-4") + + # =================================================================== + # accept_recurring_event — with check_recurring_event_id: true + # When an occurrence id is passed, it should resolve to the series root + # via get_event before accepting. + # =================================================================== + + settings({ + check_recurring_event_id: true, + disable_refresh_bookings: true, + }) + + sleep 0.5 + + exec(:find_bookings_for_approval).get + + exec(:accept_recurring_event, + calendar_id: "room5@example.com", + recurring_event_id: "occurrence-of-series-1", + ).get + + # get_event should have been called to resolve the occurrence id. + system(:Calendar_1)[:get_event_calls].should eq(1) + get_args = system(:Calendar_1)[:get_event_args].as_h + get_args["id"].should eq("occurrence-of-series-1") + + # Calendar.accept_event should have been called with the RESOLVED series root id, + # not the occurrence id that was originally passed in. + accept_args = system(:Calendar_1)[:accept_event_args].as_h + accept_args["event_id"].should eq("series-root-1") + + # All events in the series should be removed from the cache. + approval = status["approval_required"].as_h + remaining = approval["spec_runner_system"].as_a + remaining_ids = remaining.map { |e| e["id"].as_s } + remaining_ids.should_not contain("tentative-event-2") + remaining_ids.should_not contain("tentative-event-3") + + # =================================================================== + # accept_recurring_event — with check_recurring_event_id: true + # When the id IS already the series root, it should pass it through. + # =================================================================== + + exec(:find_bookings_for_approval).get + + exec(:accept_recurring_event, + calendar_id: "room5@example.com", + recurring_event_id: "series-root-1", + ).get + + # get_event is called, but the returned recurring_event_id matches the input, + # so the original id is used as-is. + accept_args = system(:Calendar_1)[:accept_event_args].as_h + accept_args["event_id"].should eq("series-root-1") + + # =================================================================== + # decline_recurring_event — with check_recurring_event_id: true + # =================================================================== + + exec(:find_bookings_for_approval).get + + exec(:decline_recurring_event, + calendar_id: "room5@example.com", + recurring_event_id: "occurrence-of-series-1", + ).get + + # Should have resolved the occurrence to the series root before declining. + decline_args = system(:Calendar_1)[:decline_event_args].as_h + decline_args["event_id"].should eq("series-root-1") + + # =================================================================== + # Fix D — Bookings re-poll is debounced + # After accepting/declining a recurring series the driver schedules a + # Bookings poll_events call with a 10-second debounce. Multiple + # actions within the window should be batched into a single poll. + # =================================================================== + + settings({ + check_recurring_event_id: false, + disable_refresh_bookings: false, + }) + + sleep 0.5 + + exec(:find_bookings_for_approval).get + + poll_before = system(:Bookings_1)[:poll_events_calls].as_i + + # Accept and decline two different series in quick succession. + # Both should be batched by the debounce timer into a single poll. + exec(:accept_recurring_event, + calendar_id: "room5@example.com", + recurring_event_id: "series-root-1", + ).get + + exec(:decline_recurring_event, + calendar_id: "room5@example.com", + recurring_event_id: "series-root-2", + ).get + + # Wait for the debounce timer to fire (default 10 s + margin). + sleep 11 + + # The debounced refresh should have triggered poll_events at least once. + system(:Bookings_1)[:poll_events_calls].as_i.should be >= (poll_before + 1) + + # =================================================================== + # resolve_recurring_event_id — returns series root when ids differ + # =================================================================== + + result = exec(:resolve_recurring_event_id, + calendar_id: "room5@example.com", + event_id: "occurrence-of-series-1", + ).get + + result.should eq("series-root-1") + + # =================================================================== + # resolve_recurring_event_id — returns input when id is already root + # =================================================================== + + result = exec(:resolve_recurring_event_id, + calendar_id: "room5@example.com", + event_id: "series-root-1", + ).get + + result.should eq("series-root-1") end From a6e8d7122f46236aa5c2cb8d0bfad0ed8b96b4b4 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Wed, 29 Apr 2026 13:52:42 +0930 Subject: [PATCH 2/3] fix(room_booking_approval): [PPT-2340] cache update issues --- drivers/place/room_booking_approval.cr | 58 ++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/drivers/place/room_booking_approval.cr b/drivers/place/room_booking_approval.cr index fba0106ce8..d0fc971e35 100644 --- a/drivers/place/room_booking_approval.cr +++ b/drivers/place/room_booking_approval.cr @@ -8,13 +8,19 @@ class Place::RoomBookingApproval < PlaceOS::Driver default_settings({ check_recurring_event_id: false, # fetches the event to verify the provided id is the series root. - check_bookings_every_minutes: 5, + check_bookings_every_minutes: 2, + refresh_debounce_seconds: 10, # seconds to wait before triggering Bookings poll after an approval action. + disable_refresh_bookings: false, # set to true to skip the debounced Bookings re-poll entirely. }) accessor calendar : Calendar_1 @check_recurring_event_id : Bool = false - @booking_poll_rate : UInt32 = 5 + @booking_poll_rate : UInt32 = 2 + @refresh_debounce : UInt32 = 10 + @disable_refresh_bookings : Bool = false + @pending_refresh : Set(String) = Set(String).new + @refresh_scheduled : Bool = false # ameba:disable Lint/NotNil getter building_id : String { get_building_id.not_nil! } @@ -25,9 +31,13 @@ class Place::RoomBookingApproval < PlaceOS::Driver @building_id = nil @systems = nil @check_recurring_event_id = setting?(Bool, :check_recurring_event_id) || false - @booking_poll_rate = setting?(UInt32, :check_bookings_every_minutes) || 5_u32 + @booking_poll_rate = setting?(UInt32, :check_bookings_every_minutes) || 2_u32 + @refresh_debounce = setting?(UInt32, :refresh_debounce_seconds) || 10_u32 + @disable_refresh_bookings = setting?(Bool, :disable_refresh_bookings) || false schedule.clear + @pending_refresh.clear + @refresh_scheduled = false # used to detect changes in building configuration schedule.every(1.hour) { @systems = get_systems_list.not_nil! } # ameba:disable Lint/NotNil @@ -102,8 +112,10 @@ class Place::RoomBookingApproval < PlaceOS::Driver recurring_event_id = resolve_recurring_event_id(calendar_id, recurring_event_id, user_id) if @check_recurring_event_id logger.debug { "accepting recurring event #{recurring_event_id} on #{calendar_id}" } + affected_systems = find_affected_systems(recurring_event_id) calendar.accept_event(calendar_id: calendar_id, event_id: recurring_event_id, user_id: user_id, notify: notify, comment: comment) clear_cache(event_id: recurring_event_id) + refresh_bookings(affected_systems) end @[Security(Level::Support)] @@ -117,8 +129,48 @@ class Place::RoomBookingApproval < PlaceOS::Driver recurring_event_id = resolve_recurring_event_id(calendar_id, recurring_event_id, user_id) if @check_recurring_event_id logger.debug { "declining recurring event #{recurring_event_id} on #{calendar_id}" } + affected_systems = find_affected_systems(recurring_event_id) calendar.decline_event(calendar_id: calendar_id, event_id: recurring_event_id, user_id: user_id, notify: notify, comment: comment) clear_cache(event_id: recurring_event_id) + refresh_bookings(affected_systems) + end + + # Returns the system IDs whose cached tentative events match the given + # event_id (by event id or recurring_event_id). Must be called *before* + # clear_cache so the cache still contains the events. + protected def find_affected_systems(event_id : String) : Array(String) + cache = self[:approval_required]? ? ApprovalCache.from_json(self[:approval_required].to_json) : ApprovalCache.new + cache.compact_map do |system_id, events| + system_id if events.any? { |event| event.id == event_id || event.recurring_event_id == event_id } + end + end + + # Queues the given system IDs for a debounced Bookings re-poll. + # Multiple calls within the 10-second window are batched into a + # single poll_events call per affected system, giving the calendar + # provider time to propagate the status change. + protected def refresh_bookings(system_ids : Array(String)) + return if @disable_refresh_bookings + @pending_refresh.concat(system_ids) + return if @refresh_scheduled + @refresh_scheduled = true + schedule.in(@refresh_debounce.seconds) { perform_refresh } + end + + # Drains the pending set and triggers poll_events on each system. + protected def perform_refresh + system_ids = @pending_refresh.dup + @pending_refresh.clear + @refresh_scheduled = false + + system_ids.each do |system_id| + begin + sys = system(system_id) + sys.get("Bookings", 1).poll_events if sys.exists?("Bookings", 1) + rescue error + logger.warn(exception: error) { "unable to trigger poll_events on Bookings in #{system_id}" } + end + end end @[Security(Level::Support)] From 1284c736f5a3c8be54e9b9752e8f38901cde5d76 Mon Sep 17 00:00:00 2001 From: Mia Bennett Date: Wed, 29 Apr 2026 14:03:17 +0930 Subject: [PATCH 3/3] fix(room_booking_approval): [PPT-2340] add fix to single approve/decline --- drivers/place/room_booking_approval.cr | 4 +++ drivers/place/room_booking_approval_spec.cr | 36 +++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/drivers/place/room_booking_approval.cr b/drivers/place/room_booking_approval.cr index d0fc971e35..617b869325 100644 --- a/drivers/place/room_booking_approval.cr +++ b/drivers/place/room_booking_approval.cr @@ -103,8 +103,10 @@ class Place::RoomBookingApproval < PlaceOS::Driver @[Security(Level::Support)] def accept_event(calendar_id : String, event_id : String, user_id : String? = nil, notify : Bool = true, comment : String? = nil) + affected_systems = find_affected_systems(event_id) calendar.accept_event(calendar_id: calendar_id, event_id: event_id, user_id: user_id, notify: notify, comment: comment) clear_cache(event_id: event_id) + refresh_bookings(affected_systems) end @[Security(Level::Support)] @@ -120,8 +122,10 @@ class Place::RoomBookingApproval < PlaceOS::Driver @[Security(Level::Support)] def decline_event(calendar_id : String, event_id : String, user_id : String? = nil, notify : Bool = true, comment : String? = nil) + affected_systems = find_affected_systems(event_id) calendar.decline_event(calendar_id: calendar_id, event_id: event_id, user_id: user_id, notify: notify, comment: comment) clear_cache(event_id: event_id) + refresh_bookings(affected_systems) end @[Security(Level::Support)] diff --git a/drivers/place/room_booking_approval_spec.cr b/drivers/place/room_booking_approval_spec.cr index a182292af7..23e295a3b3 100644 --- a/drivers/place/room_booking_approval_spec.cr +++ b/drivers/place/room_booking_approval_spec.cr @@ -300,6 +300,42 @@ DriverSpecs.mock_driver "Place::RoomBookingApproval" do remaining.map { |e| e["id"].as_s }.should_not contain("tentative-event-4") remaining.size.should eq(2) + # =================================================================== + # accept_event / decline_event — also trigger debounced Bookings re-poll + # =================================================================== + + settings({ + disable_refresh_bookings: false, + }) + + sleep 0.5 + + exec(:find_bookings_for_approval).get + + poll_before = system(:Bookings_1)[:poll_events_calls].as_i + + exec(:accept_event, + calendar_id: "room5@example.com", + event_id: "tentative-event-1", + ).get + + exec(:decline_event, + calendar_id: "room5@example.com", + event_id: "tentative-event-4", + ).get + + sleep 11 + + # The debounced refresh should have triggered poll_events. + system(:Bookings_1)[:poll_events_calls].as_i.should be >= (poll_before + 1) + + # Re-disable for the remaining non-debounce tests. + settings({ + disable_refresh_bookings: true, + }) + + sleep 0.5 + # =================================================================== # accept_recurring_event — with check_recurring_event_id: false (default) # Accepts a series by recurring_event_id and clears all occurrences.