Keep NMR'd players removable across phases they could not order in

[johnpooch]

What this PR does

Fixes a Discord bug report: a GM could not remove a player who had NMR'd, while removing two other players from the same game worked fine.

Game.can_remove_member gates on nmrd_member_ids (service/game/models.py), which only ever looked at the single most recent completed phase. A player who missed orders in a movement phase stopped being removable as soon as any later phase resolved in which they had nothing to order — a retreat phase with no dislodged units, or an adjustment phase with no builds or disbands.

Those phase states carry has_possible_orders=False, and _set_orders_outcome (service/phase/models.py:298) only writes an outcome for states with has_possible_orders=True, so their orders_outcome stays NULL. The NMR simply dropped out of view. The frontend hides the control entirely when removable is false (packages/web/src/components/PlayerInfoContent.tsx:103), so the GM got no error — the button was just gone.

This also explains why the other two removals succeeded: they were players whose miss landed in the latest completed phase, or who were already in civil disorder.

The change

nmrd_member_ids now resolves each member's outcome from the most recent completed phase in which they actually had orders to give, walking back past phases they could not order in. A member who submitted orders after missing them is still not removable.

It resolves those outcomes two ways, mirroring how current_phase already chooses between prefetched and live data:

• Prefetched (game list and retrieve): walks the prefetched phases. with_list_data and with_retrieve_data narrowed phase_states to the current and latest completed phase, so the walk would have found nothing beyond those; both now also load each member's latest completed orderable phase state via a set-based subquery, matching the existing distinct(...) idiom beside it. Endpoints stay at a constant query count.
• Not prefetched (IsRemovableMember via resolve_game, MemberSerializer.get_removable via obj.game): a single DISTINCT ON query. Flat at 3 queries regardless of history depth.

Also guards removal on member.eliminated, matching Member.replaceable which already excludes eliminated members. Without it, the new backwards walk never expires, so an eliminated player who missed orders before elimination would stay removable for the rest of the game.

No migration, no serializer or schema change, so no codegen. The fix is computed at read time: affected games become removable as soon as this deploys.

Checklist

• This PR does one thing — no unrelated fixes, refactors, or drive-by cleanups bundled in
• For PRs of any significant complexity: I ran /review-pr against this PR in Claude Code and addressed (or responded to) its findings
• Tests cover the change
• Screenshots embedded in the PR description for any visual changes (see CLAUDE.md)

Review findings

/review-pr raised four findings on the first commit. Three are fixed in 99d1ac3:

1. N+1 on un-prefetched paths — the walk called phase_states.all() per phase. Measured 24 queries on a 20-phase history; now 3, flat. Fixed by the dual path above, with a test that pins the count.
2. Eliminated players stayed removable forever — fixed by the eliminated guard, with a test.
3. Colliding test ordinals — my new tests put completed phases at ordinal 1, the same ordinal the active game factory gives its live phase. Since current_phase is the highest ordinal broken by id, the completed phase shadowed the live one and the removal never exercised _vacate_phase. The history now sits below the live phase.

One finding I did not fix, flagging for your call:

4. latest_orderable_phase_state_ids is not scoped to the page's games, so Postgres computes the DISTINCT ON over the whole PhaseState ⋈ Phase set. The query count stays at 8, so no test catches it. I left it because the two subqueries directly above it (current_phase_ids, latest_completed_phase_ids) are already global in exactly the same way, and diverging from them unilaterally felt like the wrong call — but PhaseState is a much bigger table than Phase, so the argument is weaker here.

   The clean fix is to stop deriving this from phase history at all and denormalise onto Member (a missed_orders boolean maintained at resolution). That makes it O(1) everywhere, removes both prefetch changes, and lets nmrd_member_ids go away entirely. It needs a migration with a backfill, which is an architectural decision I did not want to make silently. Happy to do it in a follow-up if you want it.

On tests

Six new tests in service/member/tests.py and one in service/game/tests.py. I verified each fails without the specific change it covers:

• Against origin/main, the three "stays removable" tests and the deep-history query-count test fail.
• Against the first commit, the eliminated test and the query-count test fail.
• Reverting only the prefetch widening → the deep-history test fails with removable is False, because the walk silently sees no orderable states.

Full backend suite: 2365 passed, 8 skipped.

On screenshots

No frontend code changed. The visible effect is that the pre-existing "Remove" control appears for a player it was wrongly hidden for; there is no new or altered component, layout, copy, or state to show. Flagging this rather than silently skipping the checklist item.

Note on scope

I could not confirm the diagnosis against the reporter's actual game — production queries were blocked by the permission classifier in this session. The diagnosis is instead confirmed by local reproduction: the failing scenario reproduces exactly as described (403 on kick) against a game with an NMR followed by a non-orderable phase.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YD3HRmfKzkav1N9x4LS5jV

[github-actions]

Warning

WIP limit exceeded. There are now 8 open pull requests — the project target is 5 or fewer.

Please close or merge an existing PR before continuing with this one.