From eb8c3b3a0644bbc22c5e55b720bf997c3a5b2e34 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Tue, 4 Aug 2026 13:54:18 +1000 Subject: [PATCH] fix(bookings): keep user input entered while the booking form initialises `newForm` defers itself until the current user has loaded, then re-enters and resets the form. The form is already rendered and interactive by then, so anything typed or toggled in that window was silently destroyed: the title reverted to the default, All Day switched back off, Require locker switched back on. It only bites on a slow load, which is why it reads as the app being unreliable rather than as a reproducible bug. Carry the user's edits across the deferred re-entry and replay them over the incoming booking. Only fields the user actually touched are carried, read from the signal-forms dirty flags, so programmatic writes are ignored and a field left alone still takes its value from the booking being opened. Replayed before `applyDurationSettings`, so a restored `all_day` still drives the time-sync window, and before `_syncWindowIfUnchanged`, which compares against the initial window and so leaves a user-changed one alone of its own accord. Fixes PPT-2643 Co-Authored-By: Claude Opus 5 (1M context) --- libs/bookings/src/lib/booking-form.service.ts | 52 +++- .../src/test/booking-form.service.spec.ts | 250 +++++++++++++----- 2 files changed, 231 insertions(+), 71 deletions(-) diff --git a/libs/bookings/src/lib/booking-form.service.ts b/libs/bookings/src/lib/booking-form.service.ts index 2b3d19a7c2..7861525673 100644 --- a/libs/bookings/src/lib/booking-form.service.ts +++ b/libs/bookings/src/lib/booking-form.service.ts @@ -314,11 +314,44 @@ export class BookingFormService extends AsyncHandler { private _asset_window = ''; public readonly view = signal('form'); + /** + * Edits the user made while `newForm` was waiting on the current user, + * carried across the deferred re-entry so the reset does not discard them. + */ + private _pending_user_edits: Partial | null = null; + /** Apply a partial patch to the booking form model. */ private _patch(value: Partial, _opts?: unknown) { this.model.update((m) => ({ ...m, ...value })); } + /** + * The fields the user has actually edited, with their current values. + * + * Reads the signal-forms dirty flags rather than diffing against a + * default. Programmatic writes (`_patch`, `model.set`) do not mark a field + * dirty, so this returns genuine user input and nothing else — which is + * what makes it safe to replay over a freshly loaded booking. + */ + private _userEditedValues(): Partial { + const form = this.form as any; + if (!form) return {}; + const model = untracked(this.model) as Record; + const edits: Record = {}; + for (const key of Object.keys(model || {})) { + const field = form[key]; + if (typeof field !== 'function') continue; + // A field can throw if the tree has no matching sub-field, which + // is not worth losing the rest of the user's input over. + try { + if (field()?.dirty?.()) edits[key] = model[key]; + } catch { + continue; + } + } + return edits; + } + private _syncAssetOptions() { const { date, duration } = untracked(this.model); const next_asset_window = assetWindowKey(date, duration); @@ -783,9 +816,18 @@ export class BookingFormService extends AsyncHandler { public newForm(type: BookingType, booking: Booking = new Booking({})) { if (!currentUserIsLoaded()) { - currentUserLoaded().then(() => this.newForm(type, booking)); + // The form is already rendered and interactive at this point, so + // anything typed or toggled before the user resolves would be + // destroyed by the reset below. Capture it on the way back in — + // as late as possible, so we take the user's final state. + currentUserLoaded().then(() => { + this._pending_user_edits = this._userEditedValues(); + this.newForm(type, booking); + }); return; } + const user_edits = this._pending_user_edits; + this._pending_user_edits = null; // Never apply an existing booking's edit state to a different type // (e.g. editing parking then opening the desk form). if (isCrossTypeEdit(booking, type)) booking = new Booking({}); @@ -822,6 +864,14 @@ export class BookingFormService extends AsyncHandler { ), { emitEvent: false }, ); + // Re-apply the user's own edits over the incoming booking. Done here, + // before `applyDurationSettings`, so a restored `all_day` still drives + // the time-sync window; and before `_syncWindowIfUnchanged`, which + // compares against `initial_date`/`initial_duration` and so leaves a + // user-changed window alone of its own accord. + if (user_edits && Object.keys(user_edits).length) { + this._patch(user_edits, { emitEvent: false }); + } this.applyDurationSettings(); this._syncAssetOptions(); const form_change = effect( diff --git a/libs/bookings/src/test/booking-form.service.spec.ts b/libs/bookings/src/test/booking-form.service.spec.ts index e4d6dc271d..d24a803df8 100644 --- a/libs/bookings/src/test/booking-form.service.spec.ts +++ b/libs/bookings/src/test/booking-form.service.spec.ts @@ -1717,13 +1717,13 @@ describe('BookingFormService', () => { ).mockResolvedValue(true); const saved_desks: string[] = []; vi.spyOn(spectator.service, 'postForm').mockImplementation(async () => { - const value = spectator.service.model(); - saved_desks.push(value.asset_id); - return new Booking({ - id: `booking-${saved_desks.length}`, - user_email: value.user_email, - asset_id: value.asset_id, - }); + const value = spectator.service.model(); + saved_desks.push(value.asset_id); + return new Booking({ + id: `booking-${saved_desks.length}`, + user_email: value.user_email, + asset_id: value.asset_id, + }); }); spectator.service.newForm( 'desk', @@ -1803,17 +1803,17 @@ describe('BookingFormService', () => { extension_name: string; }[] = []; vi.spyOn(spectator.service, 'postForm').mockImplementation(async () => { - const value = spectator.service.model(); - saved_forms.push({ - asset_id: value.asset_id, - resource_id: value.resources?.[0]?.id, - extension_name: value.name, - }); - return new Booking({ - id: `booking-${saved_forms.length}`, - user_email: value.user_email, - asset_id: value.asset_id, - }); + const value = spectator.service.model(); + saved_forms.push({ + asset_id: value.asset_id, + resource_id: value.resources?.[0]?.id, + extension_name: value.name, + }); + return new Booking({ + id: `booking-${saved_forms.length}`, + user_email: value.user_email, + asset_id: value.asset_id, + }); }); spectator.service.newForm( 'desk', @@ -1897,14 +1897,14 @@ describe('BookingFormService', () => { ).mockResolvedValue(true); const child_parent_ids: string[] = []; vi.spyOn(spectator.service, 'postForm').mockImplementation(async () => { - const value = spectator.service.model(); - child_parent_ids.push(value.parent_id); - return new Booking({ - id: `booking-child-${child_parent_ids.length}`, - parent_id: value.parent_id, - user_email: value.user_email, - asset_id: value.asset_id, - }); + const value = spectator.service.model(); + child_parent_ids.push(value.parent_id); + return new Booking({ + id: `booking-child-${child_parent_ids.length}`, + parent_id: value.parent_id, + user_email: value.user_email, + asset_id: value.asset_id, + }); }); spectator.service.newForm( 'desk', @@ -1998,17 +1998,17 @@ describe('BookingFormService', () => { ).mockResolvedValue(true); const saved_users: string[] = []; vi.spyOn(spectator.service, 'postForm').mockImplementation(async () => { - const value = spectator.service.model(); - saved_users.push(value.user_email); - if (value.user_email === 'member.one@example.com') { - throw new Error('Save failed'); - } - return new Booking({ - id: `booking-child-${saved_users.length}`, - parent_id: value.parent_id, - user_email: value.user_email, - asset_id: value.asset_id, - }); + const value = spectator.service.model(); + saved_users.push(value.user_email); + if (value.user_email === 'member.one@example.com') { + throw new Error('Save failed'); + } + return new Booking({ + id: `booking-child-${saved_users.length}`, + parent_id: value.parent_id, + user_email: value.user_email, + asset_id: value.asset_id, + }); }); spectator.service.newForm( 'desk', @@ -2114,13 +2114,13 @@ describe('BookingFormService', () => { ).mockResolvedValue(true); const saved_names: string[] = []; vi.spyOn(spectator.service, 'postForm').mockImplementation(async () => { - const value = spectator.service.model(); - saved_names.push(value.asset_name); - return new Booking({ - id: `booking-${saved_names.length}`, - user_email: value.user_email, - asset_id: value.asset_id, - }); + const value = spectator.service.model(); + saved_names.push(value.asset_name); + return new Booking({ + id: `booking-${saved_names.length}`, + user_email: value.user_email, + asset_id: value.asset_id, + }); }); spectator.service.newForm( 'desk', @@ -2199,16 +2199,16 @@ describe('BookingFormService', () => { ).mockResolvedValue([all_desks[2]]); const saved_forms: { user_email: string; asset_id: string }[] = []; vi.spyOn(spectator.service, 'postForm').mockImplementation(async () => { - const value = spectator.service.model(); - saved_forms.push({ - user_email: value.user_email, - asset_id: value.asset_id, - }); - return new Booking({ - id: `booking-${saved_forms.length}`, - user_email: value.user_email, - asset_id: value.asset_id, - }); + const value = spectator.service.model(); + saved_forms.push({ + user_email: value.user_email, + asset_id: value.asset_id, + }); + return new Booking({ + id: `booking-${saved_forms.length}`, + user_email: value.user_email, + asset_id: value.asset_id, + }); }); spectator.service.newForm( 'desk', @@ -2306,14 +2306,14 @@ describe('BookingFormService', () => { ).mockResolvedValue([all_desks[1]]); const saved_forms: { id: string; parent_id: string }[] = []; vi.spyOn(spectator.service, 'postForm').mockImplementation(async () => { - const value = spectator.service.model(); - saved_forms.push({ id: value.id, parent_id: value.parent_id }); - return new Booking({ - id: value.id || `booking-child-${saved_forms.length}`, - parent_id: value.parent_id, - user_email: value.user_email, - asset_id: value.asset_id, - }); + const value = spectator.service.model(); + saved_forms.push({ id: value.id, parent_id: value.parent_id }); + return new Booking({ + id: value.id || `booking-child-${saved_forms.length}`, + parent_id: value.parent_id, + user_email: value.user_email, + asset_id: value.asset_id, + }); }); spectator.service.newForm( 'desk', @@ -2452,14 +2452,14 @@ describe('BookingFormService', () => { ]; let booking_count = 0; vi.spyOn(spectator.service, 'postForm').mockImplementation(async () => { - const value = spectator.service.model(); - booking_count++; - return new Booking({ - id: `booking-${booking_count}`, - user_email: value.user_email, - asset_id: value.asset_id, - extension_data: { group_members: group_members_payload }, - }); + const value = spectator.service.model(); + booking_count++; + return new Booking({ + id: `booking-${booking_count}`, + user_email: value.user_email, + asset_id: value.asset_id, + extension_data: { group_members: group_members_payload }, + }); }); spectator.service.newForm( 'desk', @@ -2883,4 +2883,114 @@ describe('BookingFormService', () => { expect(savedBookings().length).toBe(1); expect((savedBookings()[0] as Booking).asset_ids).toEqual(['desk-2']); }); + + describe('initialisation while the user is still loading', () => { + /** + * Put the service into the state `newForm` sees on a slow load: no + * current user yet, so it defers and returns while the form is already + * rendered and interactive. + * + * Nothing is mocked. `currentUserIsLoaded()` is real, and it reports + * "loaded" whenever it detects a test runtime, so the runtime probe + * (`typeof vi`) is what has to be neutralised to reach the branch. + * That keeps the real promise plumbing in `currentUserLoaded()` under + * test rather than a stub of it. + */ + function deferCurrentUser() { + const runtime_vi = (globalThis as any).vi; + setCurrentUser(new StaffUser({})); + (globalThis as any).vi = undefined; + const restore = () => ((globalThis as any).vi = runtime_vi); + return { + restore, + release: async () => { + restore(); + setCurrentUser( + new StaffUser({ + id: 'current-user', + email: 'current.user@example.com', + name: 'Current User', + }), + ); + // let the `.then` re-entry and its patches settle + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }, + }; + } + + /** Type into a field the way the Field directive does. */ + function userEdits(field: string, value: any) { + const node = (spectator.service.form as any)[field](); + node.value.set(value); + node.markAsDirty(); + } + + it('keeps input the user entered before initialisation finished', async () => { + const deferred = deferCurrentUser(); + try { + spectator.service.newForm('desk'); + + userEdits('title', 'Quiet corner desk'); + userEdits('all_day', true); + + await deferred.release(); + + expect(spectator.service.model().title).toBe( + 'Quiet corner desk', + ); + expect(spectator.service.model().all_day).toBe(true); + } finally { + deferred.restore(); + } + }); + + it('does not resurrect that input on the next new form', async () => { + // The preserved edits are one-shot. If they leaked, opening a + // second form would silently inherit the previous booking's title. + const deferred = deferCurrentUser(); + try { + spectator.service.newForm('desk'); + userEdits('title', 'Quiet corner desk'); + await deferred.release(); + } finally { + deferred.restore(); + } + + spectator.service.newForm('desk'); + + expect(spectator.service.model().title).not.toBe( + 'Quiet corner desk', + ); + }); + + it('leaves untouched fields to the incoming booking', async () => { + // Only dirty fields are carried across. A field the user never + // touched must still take its value from the booking being opened. + const deferred = deferCurrentUser(); + try { + spectator.service.newForm( + 'desk', + new Booking({ + id: 'bkn-1', + booking_type: 'desk', + title: 'From the booking', + asset_id: 'desk-1', + }), + ); + + userEdits('all_day', true); + + await deferred.release(); + + expect(spectator.service.model().all_day).toBe(true); + expect(spectator.service.model().title).toBe( + 'From the booking', + ); + } finally { + deferred.restore(); + } + }); + }); });