@ngrx/data/save/delete-one/error does not remove the change from the changeState

[vbourdeix]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request

What is the current behavior?

As described in this stackoverflow, when dispatching a EntityOp.UNDO_ONE the changeState property should be updated to reflect the cancelling of the original action.

As you can see in this stackblitz, when you look at the console, after trying to delete a hero, a changeState is introduced to the state. As the delete is optimistic, the hero is removed from the entities. When issuing the UNDO_ONE action (after a delete error in this example), the hero is added back to the entities, but the changeState reflecting the removal is kept intact.

Expected behavior:

When issuing an EntityOp.UNDO_ONE action, the changeState related modifications should be removed from the entityCache state too. (This is what is described in the documentation here :

The undo operations replace entities in the collection based on information in the changeState map, reverting them their last known server-side state, and removing them from the changeState map. These entities become "unchanged."

Minimal reproduction of the problem with instructions:

Steps to reproduce :

1. Create an EntityCollectionServiceBase service for the Hero type
2. Try to remove a hero from the list with a failing backend API (or no API for removal) with this.heroesEntityService.delete(hero)
3. Create an effect to catch error cases with deleteError$ = createEffect(() => { return this.actions$.pipe( ofEntityType("Hero"), ofEntityOp([EntityOp.SAVE_DELETE_ONE_ERROR]), map(action => new EntityActionFactory().create( "Hero", EntityOp.UNDO_ONE, action.payload.data.originalAction.payload.data ); ) ); });
   Stackblitz example : https://stackblitz.com/edit/angular-crdbah-ngrx-data-delete

Version of affected browser(s),operating system(s), npm, node and ngrx:

ngrx-data version used : @ngrx/data 8.6.0

[AdditionAddict]

Mimicking the undoAll test located at data/spec/reducers/entity-change-tracker-base.spec.ts below is a failing test.

describe('#undoOne', () => {
    it('should clear one tracked change', () => {
      let { collection, deletedEntity } = createTestTrackedEntities();

      expect(Object.keys(collection.changeState).length).toBe(
        3,
        'tracking 3 entities'
      );

      collection = tracker.undoOne(deletedEntity as Hero, collection);

      expect(Object.keys(collection.changeState).length).toBe(
        2,
        'tracking 2 entities'
      );
    });

    ...

Looking at the code for undoMany which undoOne uses, it seems the line delete chgState[id]; // clear tracking of this entity is not used within the reduce. In fact the variable chgState is unused.

Adding acc.changeState = chgState; after delete chgState[id]; // clear tracking of this entity fixes the reduce but didMutate is true.

I believe the return stmt return didMutate ? collection : { ...collection, changeState }; should be the other way around...return didMutate ? { ...collection, changeState } : collection