From 7b52a6233cbb188428199d22186be2864cd81cb0 Mon Sep 17 00:00:00 2001 From: Adrien Crivelli Date: Tue, 26 Mar 2024 14:15:27 +0800 Subject: [PATCH] BREAKING AbstractDetail never merge mutation results into form #9844 In a typical mutation, we want to fetch `name`, to keep Apollo cache up to date and to show the new `name` as page header and page title. But if we do that, and AbstractDetail auto-merge `name` into the form, then we might override the form field that is currently being edited by the human. And that is the exact bug we were trying to avoid with all the recent changes. So AbstractDetail does not merge mutation results back into the form anymore. And it is up to the consuming component to reproduce it, if needed, possibly on a field-by-field basis, according to its specific needs. The simplest way to re-introduce previous behavior is the following, but it would be more robust to merge only specifics fields known to be mergeable safely: ```ts protected override postUpdate(model): void { this.form.patchValue(model); } ``` --- .../src/lib/classes/abstract-detail.spec.ts | 50 +++++++++++++++---- .../src/lib/classes/abstract-detail.ts | 2 - 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/projects/natural/src/lib/classes/abstract-detail.spec.ts b/projects/natural/src/lib/classes/abstract-detail.spec.ts index 5cc2c2ce..2e17d0e5 100644 --- a/projects/natural/src/lib/classes/abstract-detail.spec.ts +++ b/projects/natural/src/lib/classes/abstract-detail.spec.ts @@ -38,7 +38,7 @@ describe('NaturalAbstractDetail', () => { let service: ItemService; let harness: RouterTestingHarness; let router: Router; - const model = {id: '123', name: 'foo'} as Item; + const model = {id: '123', name: 'my name'} as Item; const serverResponse = {id: '123', name: 'from server'} as Item; async function configure(data: Data | undefined, a: typeof ItemService = ItemService): Promise { @@ -106,8 +106,8 @@ describe('NaturalAbstractDetail', () => { detail.ngOnInit(); await harness.fixture.whenStable(); - expect(detail.data.model.name).toEqual('foo'); - expect(detail.form.controls.name.value).toEqual('foo'); + expect(detail.data.model.name).toEqual('my name'); + expect(detail.form.controls.name.value).toEqual('my name'); const model2 = {id: '123', name: 'updated name'} as Item; model$.next(model2); @@ -115,7 +115,7 @@ describe('NaturalAbstractDetail', () => { expect(detail.data.model.name).toEqual('updated name'); expect(detail.form.controls.name.value) .withContext('form must not be reinitialized whenever model is mutated') - .toEqual('foo'); + .toEqual('my name'); }); it('should populate this.data, also with other keys', async () => { @@ -137,7 +137,7 @@ describe('NaturalAbstractDetail', () => { await harness.fixture.whenStable(); expect(detail.form.value).toEqual({ - name: 'foo', + name: 'my name', description: '', children: [], parent: null, @@ -182,7 +182,7 @@ describe('NaturalAbstractDetail', () => { detail.update(false, true); expect(spy).toHaveBeenCalledOnceWith({ id: '123', - name: 'foo', + name: 'my name', description: '', children: [], parent: null, @@ -200,7 +200,7 @@ describe('NaturalAbstractDetail', () => { detail.update(true, true); expect(spy).toHaveBeenCalledOnceWith({ id: '123', - name: 'foo', + name: 'my name', description: '', children: [], parent: null, @@ -231,7 +231,7 @@ describe('NaturalAbstractDetail', () => { await harness.fixture.whenStable(); expect(spy).toHaveBeenCalledOnceWith({ id: '123', // This is an extra property that shouldn't really be there, but it's ok to have it because it has no effect - name: 'foo', + name: 'my name', description: '', children: [], parent: null, @@ -312,7 +312,7 @@ describe('NaturalAbstractDetail', () => { await harness.fixture.whenStable(); expect(detail.form.value).toEqual({ - name: 'foo', + name: 'my name', description: '', children: [], parent: null, @@ -324,4 +324,36 @@ describe('NaturalAbstractDetail', () => { detail.update(); expect(spy).toHaveBeenCalledOnceWith({id: '123', extraField: 'new value'} as any); }); + + it('should not automatically merge server response back into this.form after update', async () => { + await configure({ + model: of(model), + }); + detail.ngOnInit(); + await harness.fixture.whenStable(); + + const spy = spyOn(service, 'update').and.callFake(() => of(serverResponse)); + detail.update(); + expect(spy).toHaveBeenCalledOnceWith({id: '123'}); + expect(detail.form.controls.name.value).toEqual('my name'); + }); + + it('should not automatically merge server response back into this.form after creation', async () => { + await configure({ + model: of(model), + }); + detail.ngOnInit(); + await harness.fixture.whenStable(); + + const spy = spyOn(service, 'create').and.callFake(() => of(serverResponse)); + detail.create(); + expect(spy).toHaveBeenCalledOnceWith({ + id: '123', // This is an extra property that shouldn't really be there, but it's ok to have it because it has no effect + name: 'my name', + description: '', + children: [], + parent: null, + } as ItemInput); + expect(detail.form.controls.name.value).toEqual('my name'); + }); }); diff --git a/projects/natural/src/lib/classes/abstract-detail.ts b/projects/natural/src/lib/classes/abstract-detail.ts index 50d1c40f..d511610a 100644 --- a/projects/natural/src/lib/classes/abstract-detail.ts +++ b/projects/natural/src/lib/classes/abstract-detail.ts @@ -191,7 +191,6 @@ export class NaturalAbstractDetail< update.subscribe(model => { this.#changes.commit(newValues); this.alertService.info($localize`Mis à jour`); - this.form.patchValue(model); this.postUpdate(model); }); }); @@ -212,7 +211,6 @@ export class NaturalAbstractDetail< .pipe( switchMap(model => { this.alertService.info($localize`Créé`); - this.form.patchValue(model); return this.postCreate(model).pipe(endWith(model), last()); }),