Add notion of actionPlayers (#145)

* move player check from server to flow

* add activePlayers() fn to turn-order

* rename activePlayers to actionPlayers

* use actionPlayers in canMakeMove

* add expectations for actionPlayers

* pass object instead of playerID to canMakeMove

* make validator and canMakeMove

* disallow currentPlayer to make moves by default

* call canMakeMove in isActive computation

* minor style tweak

* pass correct value of G in server/index.js

* pass playerID = undefined in singleplayer

* arg consistency in canMakeMOve

* remove ability to change actionPlayers from TurnOrder. Let's wait for the Events API and figure it out then.

* pass entire payload object in server/index.js

* fix some comments

src/client/client.js

@@ -83,6 +83,11 @@ class _ClientImpl {
       this.store = this.multiplayerClient.createStore(GameReducer, enhancer);
     } else {
       this.store = createStore(GameReducer, enhancer);
+
+      // If no playerID was provided, set it to undefined.
+      if (this.playerID === null) {
+        this.playerID = undefined;
+      }
     }
 
     this.createDispatchers();
@@ -107,8 +112,9 @@ class _ClientImpl {
         isActive = false;
       }
       if (
-        state.ctx.currentPlayer != 'any' &&
-        this.playerID != state.ctx.currentPlayer
+        !this.game.flow.canMakeMove(state.G, state.ctx, {
+          playerID: this.playerID,
+        })
       ) {
         isActive = false;
       }

src/core/flow.js

@@ -9,6 +9,36 @@
 import { TurnOrder } from './turn-order';
 import { Random } from './random';
 
+/**
+ * This function checks whether a player is allowed to make a move.
+ *
+ * @param {object} G     - The Game instance
+ * @param {object} ctx   - The ctx instance
+ * @param {object} opts  - Options - used here to transport the playerID.
+ */
+function canMakeMoveDefault(G, ctx, opts) {
+  const { playerID } = opts || {};
+
+  // In multiplayer mode, the default playerID is null, which corresponds
+  // to a spectator that can't make moves.
+  if (playerID === null) {
+    return true;
+  }
+
+  // In singleplayer mode (and most unit tests), the default playerID
+  // is undefined, and can always make moves.
+  if (playerID === undefined) {
+    return true;
+  }
+
+  const actionPlayers = ctx.actionPlayers || [];
+
+  // Explicitly do not allow the current player
+  // When he is allowed to make a move, his playerID
+  // must be included in actionPlayers.
+  return actionPlayers.includes(playerID) || actionPlayers.includes('any');
+}
+
 /**
  * Helper to create a reducer that manages ctx (with the
  * ability to also update G).
@@ -25,10 +55,6 @@ import { Random } from './random';
  *                             reducer will handle. Each function
  *                             has the following signature:
  *                             ({G, ctx}) => {G, ctx}
- * @param {...object} validator - A move validator that returns false
- *                                if a particular type of move is invalid
- *                                at this point in the game.
- *                                (G, ctx, moveName) => boolean
  * @param {...object} processMove - A function that's called whenever a move is made.
  *                                  (state, action, dispatch) => state.
  * @param {...object} optimisticUpdate - (G, ctx, move) => boolean
@@ -37,20 +63,25 @@ import { Random } from './random';
  *                                       the client while waiting for
  *                                       the result of execution from
  *                                       the server.
+ * @param {...object} canMakeMove - (G, ctx, opts) => boolean
+ *                                  Predicate to determine whether the player
+ *                                  identified by playerID is allowed to make a move.
+ *                                  opts contains an object { type, args, playerID },
+ *                                  which is the payload of makeMove().
  */
 export function Flow({
   ctx,
   events,
   init,
-  validator,
   processMove,
   optimisticUpdate,
+  canMakeMove,
 }) {
   if (!ctx) ctx = () => ({});
   if (!events) events = {};
   if (!init) init = state => state;
-  if (!validator) validator = () => true;
   if (!processMove) processMove = state => state;
+  if (!canMakeMove) canMakeMove = canMakeMoveDefault;
 
   if (optimisticUpdate === undefined) {
     optimisticUpdate = () => true;
@@ -72,13 +103,6 @@ export function Flow({
     ctx,
     init,
 
-    // Disallow moves once the game is over.
-    // Also call any provided additional validation.
-    validator: (G, ctx, move) => {
-      if (ctx.gameover !== undefined) return false;
-      return validator(G, ctx, move);
-    },
-
     eventNames: Object.getOwnPropertyNames(events),
 
     processMove: (state, action) => {
@@ -90,6 +114,13 @@ export function Flow({
     },
 
     optimisticUpdate,
+
+    // Disallow moves once the game is over.
+    // Also call any provided additional validation.
+    canMakeMove: (G, ctx, opts) => {
+      if (ctx.gameover !== undefined) return false;
+      return canMakeMove(G, ctx, opts);
+    },
   };
 }
 
@@ -199,6 +230,7 @@ export function FlowWithPhases({
   endPhase,
   undo,
   optimisticUpdate,
+  canMakeMove,
 }) {
   // Attach defaults.
   if (endPhase === undefined && phases) {
@@ -273,6 +305,7 @@ export function FlowWithPhases({
     const ctx = { ...state.ctx };
     const G = phaseConfig.onPhaseBegin(state.G, ctx);
     ctx.currentPlayer = phaseConfig.turnOrder.first(G, ctx);
+    ctx.actionPlayers = [ctx.currentPlayer];
     return { ...state, G, ctx };
   };
 
@@ -368,10 +401,11 @@ export function FlowWithPhases({
 
     // Update current player.
     const currentPlayer = conf.turnOrder.next(G, ctx);
+    const actionPlayers = [currentPlayer];
     // Update turn.
     const turn = ctx.turn + 1;
     // Update state.
-    ctx = { ...ctx, currentPlayer, turn, currentPlayerMoves: 0 };
+    ctx = { ...ctx, currentPlayer, actionPlayers, turn, currentPlayerMoves: 0 };
 
     // End phase if condition is met.
     const end = conf.endPhaseIf(G, ctx);
@@ -420,12 +454,14 @@ export function FlowWithPhases({
   }
 
   function processMove(state, action, dispatch) {
-    // Update currentPlayerMoves.
-    const currentPlayerMoves = state.ctx.currentPlayerMoves + 1;
-    state = { ...state, ctx: { ...state.ctx, currentPlayerMoves } };
-
     const conf = phaseMap[state.ctx.phase];
 
+    const currentPlayerMoves = state.ctx.currentPlayerMoves + 1;
+    state = {
+      ...state,
+      ctx: { ...state.ctx, currentPlayerMoves },
+    };
+
     const G = conf.onMove(state.G, state.ctx, action);
     state = { ...state, G };
 
@@ -465,13 +501,21 @@ export function FlowWithPhases({
     return state;
   }
 
-  const validator = (G, ctx, move) => {
-    const conf = phaseMap[ctx.phase];
+  const canMakeMoveWrap = (G, ctx, opts) => {
+    const conf = phaseMap[ctx.phase] || {};
     if (conf.allowedMoves) {
       const set = new Set(conf.allowedMoves);
-      return set.has(move.type);
+      if (!set.has(opts.type)) {
+        return false;
+      }
     }
-    return true;
+
+    // run user-provided validation
+    if (canMakeMove !== undefined && !canMakeMove(G, ctx, opts)) {
+      return false;
+    }
+
+    return canMakeMoveDefault(G, ctx, opts);
   };
 
   let enabledEvents = {};
@@ -501,7 +545,7 @@ export function FlowWithPhases({
       return optimisticUpdate(G, ctx, action);
     },
     events: enabledEvents,
-    validator,
     processMove,
+    canMakeMove: canMakeMoveWrap,
   });
 }

src/core/flow.test.js

@@ -20,7 +20,7 @@ test('Flow', () => {
   expect(flow.ctx()).toMatchObject({});
   expect(flow.eventNames.length).toBe(0);
   expect(flow.init({ a: 5 })).toMatchObject({ a: 5 });
-  expect(flow.validator({}, {}, undefined)).toBe(true);
+  expect(flow.canMakeMove({}, {}, undefined)).toBe(true);
   expect(flow.processMove({ b: 6 })).toMatchObject({ b: 6 });
   expect(flow.optimisticUpdate()).toBe(true);
 });
@@ -410,7 +410,7 @@ test('endTurnIf', () => {
   }
 });
 
-test('validator', () => {
+test('canMakeMove', () => {
   let game = Game({
     moves: {
       A: () => ({ A: true }),
@@ -433,9 +433,9 @@ test('validator', () => {
   // Basic.
   let flow;
   flow = Flow({});
-  expect(flow.validator(state.G, state.ctx)).toBe(true);
-  flow = Flow({ validator: () => false });
-  expect(flow.validator(state.G, state.ctx)).toBe(false);
+  expect(flow.canMakeMove(state.G, state.ctx)).toBe(true);
+  flow = Flow({ canMakeMove: () => false });
+  expect(flow.canMakeMove(state.G, state.ctx)).toBe(false);
 
   // B is disallowed in phase A.
   state = reducer(state, makeMove('B'));
@@ -470,6 +470,10 @@ test('validator', () => {
   expect(state.G).not.toMatchObject({ A: true });
   state = reducer(state, makeMove('B'));
   expect(state.G).not.toMatchObject({ B: true });
+
+  // the flow runs a user-provided validation
+  flow = FlowWithPhases({ canMakeMove: () => true });
+  expect(flow.canMakeMove(state.G, state.ctx)).toBe(false);
 });
 
 test('undo / redo', () => {
@@ -529,3 +533,29 @@ test('undo / redo', () => {
   state = reducer(state, gameEvent('undo'));
   expect(state.G).toEqual({ A: true });
 });
+
+test('canMakeMove', () => {
+  // default behaviour
+  const pid = { playerID: 0 };
+
+  let flow = Flow({});
+  expect(flow.canMakeMove({}, {}, pid)).toBe(false);
+  // NOTE: currentPlayer is not allowed to make a move by default.
+  // his playerID must be included in the actionPlayers array.
+  expect(flow.canMakeMove({}, { currentPlayer: 0 }, pid)).toBe(false);
+  expect(flow.canMakeMove({}, { actionPlayers: ['any'] }, pid)).toBe(true);
+  expect(flow.canMakeMove({}, { actionPlayers: [0] }, pid)).toBe(true);
+  expect(flow.canMakeMove({}, { actionPlayers: [1, 2, 3] }, pid)).toBe(false);
+
+  // no one can make a move
+  flow = Flow({ canMakeMove: () => false });
+  expect(flow.canMakeMove({}, {}, pid)).toBe(false);
+  expect(flow.canMakeMove({}, { currentPlayer: 0 }, pid)).toBe(false);
+  expect(flow.canMakeMove({}, {}, 'any')).toBe(false);
+
+  // flow with phases passes canMakeMove
+  flow = FlowWithPhases({ canMakeMove: () => false });
+  expect(flow.canMakeMove({}, {}, pid)).toBe(false);
+  expect(flow.canMakeMove({}, { currentPlayer: 0 }, pid)).toBe(false);
+  expect(flow.canMakeMove({}, {}, 'any')).toBe(false);
+});

src/core/reducer.js

@@ -99,7 +99,7 @@ export function createGameReducer({ game, numPlayers, multiplayer }) {
 
       case Actions.MAKE_MOVE: {
         // Ignore the move if it isn't valid at this point.
-        if (!game.flow.validator(state.G, state.ctx, action.payload)) {
+        if (!game.flow.canMakeMove(state.G, state.ctx, action.payload)) {
           return state;
         }
 

src/server/index.js

@@ -58,11 +58,12 @@ export function Server({ games, db, _clientInfo, _roomInfo }) {
           return;
         }
 
-        // Bail out if the player making the move is not
-        // the current player.
+        // Check whether the player is allowed to make the move
         if (
-          state.ctx.currentPlayer != 'any' &&
-          playerID != state.ctx.currentPlayer
+          !game.flow.canMakeMove(state.G, state.ctx, {
+            ...action.payload,
+            playerID,
+          })
         ) {
           return;
         }

src/server/index.test.js

@@ -176,6 +176,7 @@ test('action', async () => {
           ctx: {
             _random: { seed: 0 },
             currentPlayer: '0',
+            actionPlayers: ['0'],
             currentPlayerMoves: 0,
             numPlayers: 2,
             phase: 'default',
@@ -186,6 +187,7 @@ test('action', async () => {
       ctx: {
         _random: { seed: 0 },
         currentPlayer: '0',
+        actionPlayers: ['0'],
         currentPlayerMoves: 0,
         numPlayers: 2,
         phase: 'default',
@@ -201,6 +203,7 @@ test('action', async () => {
         ctx: {
           _random: { seed: 0 },
           currentPlayer: '1',
+          actionPlayers: ['1'],
           currentPlayerMoves: 0,
           numPlayers: 2,
           phase: 'default',
@@ -211,6 +214,7 @@ test('action', async () => {
     ctx: {
       _random: undefined,
       currentPlayer: '1',
+      actionPlayers: ['1'],
       currentPlayerMoves: 0,
       numPlayers: 2,
       phase: 'default',