Fix for null last move if the move changed the stage

[JGrzybowski]

During work on my project I've fount that if the move changed the stage undo throws an error.
After some debugging I've found that the last move is calculated using present context instead of previous one.

I'm not so familliar with this codebase (yet!) to be sure this fix is really needed but from logical point of view it would make sense.

I'm glad to hear any critique and questions.

[delucis]

Thanks for catching this! Do you think you could a test to the reducer test suite for undo-ing with stages so we can avoid regressions in the future?

[JGrzybowski]

I'll try my best to recreate this situation in a small sample.
Should I try to insert this case into already present undo/redo test - it might be difficult as it does not take stage or phase into consideration.
Also I'll need some help in testing this with phases as I did not used them. But I'll ask a question here if I would need help

[delucis]

@JGrzybowski Thanks — happy to help with any questions you might have. I think you can add a new but similar test to the current undo/redo, potentially only testing stage changes for now.

[JGrzybowski]

I've got the test butit fails as flow.getMove somehow does not receive playerID in my test. I cannot push the code as the test fails which blocks the git push so I'm pasting it here:

test('undo / redo with state', () => {
  const game = {
    seed: 0,
    setup: ctx => {
      ctx.events.setStage('StartingStage');
      return { A: false, B: false, C: false };
    },
    turn: {
      stages: {
        StartingStage: {
          moves: {
            moveA: {
              move: (G, ctx, moveAisReversible) => {
                ctx.events.setStage('Stage_a');
                return { ...G, moveAisReversible, A: true };
              },
              undoable: G => G.moveAisReversible > 0,
            },
          },
        },
        Stage_a: {
          moves: {
            moveB: {
              move: (G, ctx) => {
                ctx.events.setStage('Stage_b');
                return { ...G, B: true };
              },
              undoable: false,
            },
          },
        },
        Stage_b: {
          moves: {
            moveC: {
              move: (G, ctx) => {
                ctx.events.setStage('Stage_c');
                return { ...G, C: true };
              },
              undoable: true,
            },
          },
        },
        Stage_c: {
          moves: {},
        },
      },
    },
  };

  const reducer = CreateGameReducer({ game, numPlayers: 2 });

  let state = InitializeGame({ game });

  state = reducer(state, makeMove('moveA', true));
  expect(state.G).toMatchObject({
    moveAisReversible: true,
    A: true,
    B: false,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_a');

  state = reducer(state, undo());
  expect(state.G).toMatchObject({
    A: false,
    B: false,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('StartingStage');

  state = reducer(state, redo());
  expect(state.G).toMatchObject({
    moveAisReversible: true,
    A: true,
    B: false,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_a');

  state = reducer(state, undo());
  expect(state.G).toMatchObject({
    A: false,
    B: false,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('StartingStage');

  state = reducer(state, makeMove('moveA', false));
  expect(state.G).toMatchObject({
    moveAisReversible: false,
    A: true,
    B: false,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_a');

  state = reducer(state, makeMove('moveB'));
  expect(state.G).toMatchObject({
    moveAisReversible: false,
    A: true,
    B: true,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_b');

  state = reducer(state, undo());
  expect(state.G).toMatchObject({
    moveAisReversible: false,
    A: true,
    B: true,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_b');

  state = reducer(state, makeMove('moveC'));
  expect(state.G).toMatchObject({
    moveAisReversible: false,
    A: true,
    B: true,
    C: true,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_c');

  state = reducer(state, undo());
  expect(state.G).toMatchObject({
    moveAisReversible: false,
    A: true,
    B: true,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_b');

  state = reducer(state, redo());
  expect(state.G).toMatchObject({
    moveAisReversible: false,
    A: true,
    B: true,
    C: true,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_c');

  state = reducer(state, undo());
  expect(state.G).toMatchObject({
    moveAisReversible: false,
    A: true,
    B: true,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_b');

  state = reducer(state, undo());
  expect(state.G).toMatchObject({
    moveAisReversible: false,
    A: true,
    B: true,
    C: false,
  });
  expect(state.ctx.activePlayers['0']).toBe('Stage_b');
});

EDIT: The test fails at the very begging on the first assertion as it does not make the first move.
It does not make move as processMove function in game.js#80 requires playerId which is not sotred in payload but in context. So I suspect that maybe the player Id should be provided in another way. Or maybe I messed up the test setup/initialization.

[JGrzybowski]

Managed bo by pass the git push limit and to fix playerID issue but now the test looks fine to me but it looks linethe wrong stage is stored in undo redo array. It might be that the issue is not where I did my fix but rather in saving the state.ctx into undo array before resolving the setState event.

[JGrzybowski]

@delucis can you confirm that the test checks expected behaviour of the stages events?

[delucis]

@JGrzybowski Sorry, haven’t had time to go through this yet — will get to it soon!

[delucis]

Thanks again for looking into this. The tests look correct to me, so I think there must be a bug somewhere. I’ve broken up the tests you added to make it clearer where they are failing and it seems that the problem is with the redo state: the activePlayers stored in the redo stack doesn’t seem to be correct. I’ll try and debug where that’s being set and if you spot anything more, let me know!

Edit: If the redo stack has bad state, it comes from the undo stack, so now looking for how the undo is set — this is exactly what you suggested above: “saving the state.ctx into undo array before resolving the setState event”.

[JGrzybowski]

I would wrap those tests in descibre and also I think i know what's the source of the bug.

I cannot verify it now, but I remember that the events like setStage we resolved after the state was added to the undo/redo collection. Which means that if you go back you would go to the previous state instead of the last one. You can see that the events are resolved in block around https://github.com/nicolodavis/boardgame.io/blob/d10ad6510d0ab9d57e879f8d50f9c72d7992c876/src/core/reducer.ts#L208-L212

I think that if we would do the adding to the _undo collection at the very end before returning the state it would fix the problem. Second option would be to move the events resolving before adding the state to the _undo collection

[delucis]

@JGrzybowski More or less got it! But I still need to fix some subtle bugs. The events code also needs to be able to reset the undo stack when a turn ends so you can’t undo a move that ended a turn, which is now not possible because the stack is being updated after events.

[JGrzybowski]

That's awesome! I'll see the solution in the afternoon. So, is it ready to merge? Or do we wait for someone else to review it?

[delucis]

I think it should be ready to merge — just pushed a fix for the bug I’d found before.

There is still some work to be done around undo/redo. For example, #565. So even after merging this, undo/redo with stages probably won’t work. Also, we don’t currently store the ID of the player who made a move in the undo state. That means we can’t check if the player undo-ing is the same player as made the move. This used to be obvious, because only currentPlayer could make moves or undo, but with stages it could be anyone. If you’d be up for contributing to fixing some of this, that would be great!