Repository navigation
fix(bookings): visitor timezone setting overrides the bookings setting - #665
Conversation
The visitor invite forms read `bookings.use_building_timezone || visitors.use_building_timezone`, so a generic `true` overrode an explicit visitors `false`. They now read the visitors key first and fall back to the bookings key only when it is unset, as desks, parking and lockers do since #655. - Workplace visitor flow details, the shared invite visitor form and the Concierge visitor invite form. - Settings schema and docs describe the visitors key as an override. - Unit tests for each form, and an override case in the bookings.use_building_timezone e2e spec.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
…ng-timezone-override
MrYuion
left a comment
MrYuion
left a comment
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of Alex Sorafumo
The visitors ?? bookings change is correct for the first settings load, and the docs and schema match it. I found two issues:
- Fallback stops working after a building switch (both shared forms). The
settingSignalcache keeps the old value when a key becomes unset, so after a building that setvisitors.use_building_timezone, the forms never fall back tobookings.use_building_timezoneagain.visitor-flow-detailsreads_settings.getand does not have this problem. - QA doc totals not updated. The visitor-invite rows now add up to 31, but the header still says 29, and the summary still says 445 although the spec now has 2 more tests.
Not run: I did not run the unit or e2e suites. I checked the findings by reading settings.service.ts and organisation.service.ts.
| public readonly timezone = computed(() => | ||
| this._booking_use_building_timezone() || | ||
| this._visitor_use_building_timezone() | ||
| (this._visitor_use_building_timezone() ?? |
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of Alex Sorafumo
After the first set, this ?? cannot fall back to bookings again. The signal comes from the global settingSignal cache, and SettingsService._updateSignals() (libs/common/src/lib/settings.service.ts:431-434) does this.get(key) ?? old. So when the key becomes unset, the signal keeps its last value and never goes back to undefined.
How to see it: building A has zone metadata visitors.use_building_timezone: false. Building B does not set the visitors key and has bookings.use_building_timezone: true. A building switch calls OrganisationService._updateSettingOverrides() → setOverrides() → _updateSignals(), and the visitors signal stays false. On building B the form shows local time, but the docs say it should use bookings (true). The opposite case works the same way with a stale true.
Before this PR, || hid half of this. Now that an unset key has its own meaning, it matters more. visitor-flow-details.component.ts and the desk, parking and locker flows read _settings.get(...) each time, so they are not affected. A simple fix is to read the keys in a computed that uses this._settings.get(...) with a signal dependency on overrides, or to change _updateSignals so an unset key goes back to the default it was created with. The second fix applies to all signal-backed settings, so it is probably out of scope here. The same applies to visitor-invite-form.component.ts:557.
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of Alex Sorafumo
Fixed in a02aae8. timezone now reads both keys with this._settings.get(...) inside the computed, and the two cached signals are removed. get() reads the _overrides and _debug_overrides signals, so the computed recomputes on a building switch and an unset visitors key falls back to bookings again. _updateSignals() is unchanged, as you suggested.
The settings test mock's get was not reactive, so it now reads its version signal (libs/common/src/tests/test-helpers.ts). Unit tests pass for bookings, assets, catering, common, components and workplace, and workplace and concierge build.
| ); | ||
| public readonly timezone = computed(() => | ||
| this._visitor_use_bld_tz() || this._booking_use_bld_tz() | ||
| (this._visitor_use_bld_tz() ?? this._booking_use_bld_tz()) |
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of Alex Sorafumo
This has the same problem as the comment on invite-visitor-form.component.ts:875. settingSignal('visitors.use_building_timezone') keeps its last value after a building switch removes the key, so the Concierge form does not fall back to bookings.use_building_timezone again in that session.
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of Alex Sorafumo
Fixed in a02aae8 with the same this._settings.get(...) change. The new spec test sets settingSignal('visitors.use_building_timezone') to a stale false while the current settings only set bookings.use_building_timezone: true. It fails on the previous code and passes now.
| | visitors.allow_visitor_internal_or_external | visitor-invite.allow-visitor-internal-or-external | 2 | PASS | | | ||
| | visitors.bookable_hours | visitor-invite.bookable-hours | 3 | PASS | Overrides bookings.bookable_hours, falls back to it, and both unset offer the whole day | | ||
| | visitors.use_building_timezone | visitor-invite.building-timezone | 2 | PASS | | | ||
| | visitors.use_building_timezone | visitor-invite.building-timezone, bookings.use-building-timezone | 4 | PASS | True with bookings.use_building_timezone false, and false overriding bookings.use_building_timezone true. The forms read visitors ?? bookings | |
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of Alex Sorafumo
This row went from 2 to 4, but the totals above it did not change. The ### visitor-invite (13 specs, 16 Settings, 29 scenarios) rows (line 310) now add up to 31. The summary row still says 445 scenarios, but bookings.use-building-timezone.spec.ts now has 4 tests instead of 2. The doc says its counts match --list, so the header and summary need +2.
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of Alex Sorafumo
Fixed in a02aae8. I checked the counts with --list (449 tests, including the 2 preflight tests):
- visitor-invite header: 29 → 31 scenarios
- Summary: 445 → 447 scenarios
settingsproject row: 409 → 411 tests. This row was also out of date.
…uilding switch The cached setting signals keep their last value when a building switch unsets a key, so `visitors ?? bookings` never fell back again. Both shared visitor forms now read the keys with `SettingsService.get()` in the `computed`, which tracks the overrides signals. The settings test mock `get` now reads its version signal, so a `computed` over `get()` recomputes when a spec changes the mock. Update the QA doc totals for the 2 new e2e tests (447 scenarios).
…oops The playwright(valid-title) lint rule rejects `test(scenario.label, ...)` and blocks the pre-push hook. Match the other loop specs.
|
Deployment failed for project frontend-templates with the following error: Learn More: https://vercel.com/placeos?upgradeToPro=build-rate-limit |
The visitor invite forms read
bookings.use_building_timezone || visitors.use_building_timezone. Sobookings.use_building_timezone: trueturned the building timezone on even whenvisitors.use_building_timezonewas explicitlyfalse. They now readvisitors ?? bookings, the same as desks, parking and lockers since #655.Alex agreed to this in the "Automated testing standard workflow" chat after Sharmila asked about it on #595.
Based on #595 because it changes that PR's
bookings.use-building-timezonee2e spec. When #595 merges, this retargets todevelop.Changes
visitor-flow-details.component.ts(Workplace visitor flow),invite-visitor-form.component.ts(shared form, Workplace) andvisitor-invite-form.component.ts(shared form, Concierge) read the visitors key first. The two setting signals for the visitors key now default toundefined, so an unset key falls back to bookings.settings.schema.json,docs/settings/workplace.mdanddocs/settings/concierge.mdnow say the visitors key overrides the bookings one.bookings.use-building-timezone.spec.tsnow has 4 cases: bookings true/false with visitors unset, visitors true, and visitors false overriding bookings true. The QA status doc rows are updated to match.Checked locally
nx test bookings: 597 passednx test workplace: 871 passednx build workplaceandnx build concierge: both passe2e:typecheck: cleanbookings.use-building-timezoneandvisitor-invite.building-timezone) against the local stack: 14/14, each spec run twiceBehaviour change
A deployment with
bookings.use_building_timezone: trueandvisitors.use_building_timezone: falsewill now show visitor invite times in local time. This only affects sites that set both keys to different values.