Skip to content

Commit

Permalink
BREAKING AbstractDetail never merge mutation results into form #9844
Browse files Browse the repository at this point in the history
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);
}
```
  • Loading branch information
PowerKiKi committed Mar 26, 2024
1 parent 2c9befd commit 7b52a62
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 11 deletions.
50 changes: 41 additions & 9 deletions projects/natural/src/lib/classes/abstract-detail.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
Expand Down Expand Up @@ -106,16 +106,16 @@ 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);

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 () => {
Expand All @@ -137,7 +137,7 @@ describe('NaturalAbstractDetail', () => {
await harness.fixture.whenStable();

expect(detail.form.value).toEqual({
name: 'foo',
name: 'my name',
description: '',
children: [],
parent: null,
Expand Down Expand Up @@ -182,7 +182,7 @@ describe('NaturalAbstractDetail', () => {
detail.update(false, true);
expect(spy).toHaveBeenCalledOnceWith({
id: '123',
name: 'foo',
name: 'my name',
description: '',
children: [],
parent: null,
Expand All @@ -200,7 +200,7 @@ describe('NaturalAbstractDetail', () => {
detail.update(true, true);
expect(spy).toHaveBeenCalledOnceWith({
id: '123',
name: 'foo',
name: 'my name',
description: '',
children: [],
parent: null,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -312,7 +312,7 @@ describe('NaturalAbstractDetail', () => {
await harness.fixture.whenStable();

expect(detail.form.value).toEqual({
name: 'foo',
name: 'my name',
description: '',
children: [],
parent: null,
Expand All @@ -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');
});
});
2 changes: 0 additions & 2 deletions projects/natural/src/lib/classes/abstract-detail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
Expand All @@ -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());
}),
Expand Down

0 comments on commit 7b52a62

Please sign in to comment.