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

service/game/models.py

@@ -107,10 +107,20 @@ def with_list_data(self):
             .distinct("game_id")
             .values("id")
         )
+        latest_orderable_phase_state_ids = (
+            PhaseState.objects.filter(
+                phase__status=PhaseStatus.COMPLETED, has_possible_orders=True
+            )
+            .order_by("member_id", "-phase__ordinal", "-phase_id")
+            .distinct("member_id")
+            .values("id")
+        )
         phase_states_prefetch = Prefetch(
             "phase_states",
             queryset=PhaseState.objects.filter(
-                Q(phase__in=current_phase_ids) | Q(phase__in=latest_completed_phase_ids)
+                Q(phase__in=current_phase_ids)
+                | Q(phase__in=latest_completed_phase_ids)
+                | Q(id__in=latest_orderable_phase_state_ids)
             )
             .select_related("member")
             .annotate(order_count=Count("orders")),
@@ -175,10 +185,20 @@ def with_retrieve_data(self):
             .distinct("game_id")
             .values("id")
         )
+        latest_orderable_phase_state_ids = (
+            PhaseState.objects.filter(
+                phase__status=PhaseStatus.COMPLETED, has_possible_orders=True
+            )
+            .order_by("member_id", "-phase__ordinal", "-phase_id")
+            .distinct("member_id")
+            .values("id")
+        )
         phase_states_prefetch = Prefetch(
             "phase_states",
             queryset=PhaseState.objects.filter(
-                Q(phase__in=current_phase_ids) | Q(phase__in=latest_completed_phase_ids)
+                Q(phase__in=current_phase_ids)
+                | Q(phase__in=latest_completed_phase_ids)
+                | Q(id__in=latest_orderable_phase_state_ids)
             ).select_related("member__user").annotate(
                 order_count=Count("orders")
             )
@@ -558,17 +578,39 @@ def anonymity_active(self):
     def bot_members(self):
         return self.members.filter(user__profile__kind__in=UserKind.BOT_KINDS).select_related("user")
 
+    def _latest_orderable_outcomes(self):
+        if "phases" in getattr(self, "_prefetched_objects_cache", {}):
+            completed = sorted(
+                (p for p in self.phases.all() if p.status == PhaseStatus.COMPLETED),
+                key=lambda p: p.ordinal,
+                reverse=True,
+            )
+            outcomes = {}
+            for phase in completed:
+                for phase_state in phase.phase_states.all():
+                    if not phase_state.has_possible_orders:
+                        continue
+                    outcomes.setdefault(phase_state.member_id, phase_state.orders_outcome)
+            return outcomes
+
+        return dict(
+            PhaseState.objects.filter(
+                phase__game=self,
+                phase__status=PhaseStatus.COMPLETED,
+                has_possible_orders=True,
+            )
+            .order_by("member_id", "-phase__ordinal", "-phase_id")
+            .distinct("member_id")
+            .values_list("member_id", "orders_outcome")
+        )
+
     @cached_property
     def nmrd_member_ids(self):
         with tracer.start_as_current_span("game.models.nmrd_member_ids"):
-            completed = [p for p in self.phases.all() if p.status == PhaseStatus.COMPLETED]
-            if not completed:
-                return set()
-            latest = max(completed, key=lambda p: p.ordinal)
             return {
-                phase_state.member_id
-                for phase_state in latest.phase_states.all()
-                if phase_state.orders_outcome == PhaseState.OrdersOutcome.NMR
+                member_id
+                for member_id, outcome in self._latest_orderable_outcomes().items()
+                if outcome == PhaseState.OrdersOutcome.NMR
             }
 
     def get_phase_duration_seconds(self, phase_type):
@@ -663,7 +705,12 @@ def can_manage(self, user):
 
     def can_remove_member(self, member):
         with tracer.start_as_current_span("game.models.can_remove_member"):
-            if member.is_game_master or member.kicked or member.replaced_by_id is not None:
+            if (
+                member.is_game_master
+                or member.kicked
+                or member.eliminated
+                or member.replaced_by_id is not None
+            ):
                 return False
             if self.status in (GameStatus.PENDING, GameStatus.MUSTERING):
                 return True

service/game/tests.py

@@ -10,7 +10,7 @@
 from rest_framework import status
 from common.constants import PhaseStatus, PhaseType, GameStatus, MovementPhaseDuration, DeadlineMode, OrderType, PhaseFrequency, UnitType
 
-from phase.models import Phase
+from phase.models import Phase, PhaseState
 from nation.models import Nation
 from province.models import Province
 from notification.models import Notification, NotificationDelivery
@@ -1214,6 +1214,59 @@ def test_list_games_query_count_with_phase_states(
 
         assert query_count == 8
 
+    @pytest.mark.django_db
+    def test_list_games_query_count_with_deep_phase_history(
+        self,
+        authenticated_client,
+        db,
+        classical_variant,
+        primary_user,
+        secondary_user,
+        classical_england_nation,
+        classical_france_nation,
+    ):
+        for i in range(2):
+            game = Game.objects.create(
+                name=f"Deep game {i}",
+                variant=classical_variant,
+                status=GameStatus.ACTIVE,
+            )
+            member1 = game.members.create(user=primary_user, nation=classical_england_nation)
+            member2 = game.members.create(user=secondary_user, nation=classical_france_nation)
+
+            for ordinal in range(1, 9):
+                phase = game.phases.create(
+                    game=game,
+                    variant=game.variant,
+                    season="Spring",
+                    year=1900 + ordinal,
+                    type=PhaseType.MOVEMENT,
+                    status=PhaseStatus.ACTIVE if ordinal == 8 else PhaseStatus.COMPLETED,
+                    ordinal=ordinal,
+                )
+                for member in (member1, member2):
+                    phase.phase_states.create(
+                        member=member,
+                        has_possible_orders=ordinal < 3,
+                        orders_outcome=(
+                            PhaseState.OrdersOutcome.NMR if ordinal == 2 else None
+                        ),
+                    )
+
+        url = reverse(list_viewname)
+        connection.queries_log.clear()
+
+        with override_settings(DEBUG=True):
+            response = authenticated_client.get(url)
+
+        assert response.status_code == status.HTTP_200_OK
+        assert len(connection.queries) == 8
+        assert all(
+            member["removable"]
+            for game_data in response.data["results"]
+            for member in game_data["members"]
+        )
+
     @pytest.mark.django_db
     def test_list_games_hydrates_units_for_current_phase_only(
         self,

service/member/tests.py

@@ -523,6 +523,26 @@ def _record_nmr(game, member):
     return phase
 
 
+def _record_history(game, member, entries):
+    active_phase = game.current_phase
+    active_phase.ordinal = len(entries) + 1
+    active_phase.save(update_fields=["ordinal"])
+    for ordinal, (phase_type, has_possible_orders, orders_outcome) in enumerate(entries, start=1):
+        phase = game.phases.create(
+            variant=game.variant,
+            season="Spring",
+            year=1900 + ordinal,
+            type=phase_type,
+            status=PhaseStatus.COMPLETED,
+            ordinal=ordinal,
+        )
+        phase.phase_states.create(
+            member=member,
+            has_possible_orders=has_possible_orders,
+            orders_outcome=orders_outcome,
+        )
+
+
 class TestRemoveMemberFromActiveGame:
 
     @pytest.mark.django_db
@@ -704,6 +724,124 @@ def test_bot_with_a_clean_record_can_be_removed(
 
         assert response.status_code == status.HTTP_204_NO_CONTENT
 
+    @pytest.mark.django_db
+    def test_member_who_missed_orders_stays_removable_across_a_phase_they_could_not_order_in(
+        self, authenticated_client, active_game_factory
+    ):
+        game = active_game_factory()
+        member = game.members.exclude(user=game.admin).first()
+        _record_history(
+            game,
+            member,
+            [
+                ("Movement", True, PhaseState.OrdersOutcome.NMR),
+                ("Retreat", False, None),
+            ],
+        )
+
+        url = reverse(kick_viewname, args=[game.id, member.id])
+        response = authenticated_client.delete(url)
+
+        assert response.status_code == status.HTTP_204_NO_CONTENT
+
+    @pytest.mark.django_db
+    def test_member_who_missed_orders_stays_removable_across_several_such_phases(
+        self, authenticated_client, active_game_factory
+    ):
+        game = active_game_factory()
+        member = game.members.exclude(user=game.admin).first()
+        _record_history(
+            game,
+            member,
+            [
+                ("Movement", True, PhaseState.OrdersOutcome.NMR),
+                ("Retreat", False, None),
+                ("Adjustment", False, None),
+            ],
+        )
+
+        url = reverse(kick_viewname, args=[game.id, member.id])
+        response = authenticated_client.delete(url)
+
+        assert response.status_code == status.HTTP_204_NO_CONTENT
+
+    @pytest.mark.django_db
+    def test_member_who_ordered_after_missing_orders_cannot_be_removed(
+        self, authenticated_client, active_game_factory
+    ):
+        game = active_game_factory()
+        member = game.members.exclude(user=game.admin).first()
+        _record_history(
+            game,
+            member,
+            [
+                ("Movement", True, PhaseState.OrdersOutcome.NMR),
+                ("Retreat", True, PhaseState.OrdersOutcome.RECEIVED),
+                ("Adjustment", False, None),
+            ],
+        )
+
+        url = reverse(kick_viewname, args=[game.id, member.id])
+        response = authenticated_client.delete(url)
+
+        assert response.status_code == status.HTTP_403_FORBIDDEN
+
+    @pytest.mark.django_db
+    def test_member_who_never_had_orders_to_give_cannot_be_removed(
+        self, authenticated_client, active_game_factory
+    ):
+        game = active_game_factory()
+        member = game.members.exclude(user=game.admin).first()
+        _record_history(game, member, [("Movement", False, None)])
+
+        url = reverse(kick_viewname, args=[game.id, member.id])
+        response = authenticated_client.delete(url)
+
+        assert response.status_code == status.HTTP_403_FORBIDDEN
+
+    @pytest.mark.django_db
+    def test_eliminated_member_who_missed_orders_cannot_be_removed(
+        self, authenticated_client, active_game_factory
+    ):
+        game = active_game_factory()
+        member = game.members.exclude(user=game.admin).first()
+        _record_history(
+            game,
+            member,
+            [
+                ("Movement", True, PhaseState.OrdersOutcome.NMR),
+                ("Movement", False, None),
+            ],
+        )
+        member.eliminated = True
+        member.save()
+
+        url = reverse(kick_viewname, args=[game.id, member.id])
+        response = authenticated_client.delete(url)
+
+        assert response.status_code == status.HTTP_403_FORBIDDEN
+
+    @pytest.mark.django_db
+    def test_removability_does_not_scale_queries_with_phase_history(
+        self, active_game_factory
+    ):
+        game = active_game_factory()
+        member = game.members.exclude(user=game.admin).first()
+        _record_history(
+            game,
+            member,
+            [("Movement", True, PhaseState.OrdersOutcome.NMR)]
+            + [("Movement", False, None)] * 20,
+        )
+        fresh = Game.objects.get(id=game.id)
+        fresh_member = fresh.members.get(id=member.id)
+
+        connection.queries_log.clear()
+        with override_settings(DEBUG=True):
+            assert fresh.can_remove_member(fresh_member) is True
+
+        assert len(connection.queries) == 3
+
     @pytest.mark.django_db
     def test_already_removed_member_cannot_be_removed_again(
         self, authenticated_client, active_game_factory
@@ -742,6 +880,26 @@ def test_removable_is_true_after_a_missed_phase(self, authenticated_client, acti
         members_by_id = {m["id"]: m for m in response.data["members"]}
         assert members_by_id[member.id]["removable"] is True
 
+    @pytest.mark.django_db
+    def test_removable_survives_a_phase_the_member_could_not_order_in(
+        self, authenticated_client, active_game_factory
+    ):
+        game = active_game_factory()
+        member = game.members.exclude(user=game.admin).first()
+        _record_history(
+            game,
+            member,
+            [
+                ("Movement", True, PhaseState.OrdersOutcome.NMR),
+                ("Retreat", False, None),
+            ],
+        )
+
+        response = authenticated_client.get(reverse(retrieve_viewname, args=[game.id]))
+
+        members_by_id = {m["id"]: m for m in response.data["members"]}
+        assert members_by_id[member.id]["removable"] is True
+
     @pytest.mark.django_db
     def test_removable_is_false_once_removed(self, authenticated_client, active_game_factory):
         game = active_game_factory()