Merge pull request #1490 from Hestia-Homes/fix/first-plan-is-default

A property's first plan becomes its default whatever the scenario says

applications/modelling_e2e/handler.py

@@ -205,13 +205,21 @@ def _flush_writes(engine: Engine, writes: list[_PropertyWrite]) -> None:
             uow.epc.save_batch(lodged_requests)
         if predicted_requests:
             uow.epc.save_batch(predicted_requests)
+        # First plan wins: a property with no default Plan yet gets this Plan
+        # as its default whatever the Scenario's flag says (the legacy
+        # p.is_new rule — readers select one default Plan per property, so a
+        # first run under a non-default Scenario must still produce one).
+        # Also heals properties left default-less by runs before this rule.
+        pids_with_default: set[int] = uow.plan.property_ids_with_default_plans(
+            [w.property_id for w in writes]
+        )
         plan_requests = [
             PlanSaveRequest(
                 w.plan,
                 property_id=w.property_id,
                 scenario_id=w.scenario_id,
                 portfolio_id=w.portfolio_id,
-                is_default=w.is_default,
+                is_default=w.is_default or w.property_id not in pids_with_default,
             )
             for w in writes
         ]

repositories/plan/plan_postgres_repository.py

@@ -3,7 +3,7 @@ from __future__ import annotations
 from typing import Any
 
 from sqlalchemy import insert as _sa_insert
-from sqlmodel import Session, col, update
+from sqlmodel import Session, col, select, update
 
 from domain.modelling.plan import Plan
 from infrastructure.postgres.modelling import PlanModel, RecommendationModel
@@ -26,8 +26,9 @@ class PlanPostgresRepository(PlanRepository):
 
     A re-run INSERTs a fresh Plan rather than deleting the prior one (the cascade
     delete was slow); when the new Plan is the default it demotes any prior
-    default Plan for the same (property_id, scenario_id) to ``is_default=False``,
-    so readers can select the current Plan via ``is_default=True``."""
+    default Plan for the same property — across all Scenarios — to
+    ``is_default=False``, so readers can select the property's one current Plan
+    via ``is_default=True``."""
 
     def __init__(self, session: Session) -> None:
         self._session = session
@@ -45,6 +46,19 @@ class PlanPostgresRepository(PlanRepository):
             [PlanSaveRequest(plan, property_id=property_id, scenario_id=scenario_id, portfolio_id=portfolio_id, is_default=is_default)]
         )[0]
 
+    def property_ids_with_default_plans(self, property_ids: list[int]) -> set[int]:
+        if not property_ids:
+            return set()
+        rows = self._session.exec(
+            select(col(PlanModel.property_id))
+            .distinct()
+            .where(
+                col(PlanModel.property_id).in_(property_ids),
+                col(PlanModel.is_default).is_(True),
+            )
+        ).all()
+        return {int(row) for row in rows}
+
     def save_batch(self, requests: list[PlanSaveRequest]) -> list[int]:
         """Persist all Plans in three statements regardless of batch size.
 
@@ -56,17 +70,17 @@ class PlanPostgresRepository(PlanRepository):
             return []
 
         # Demote prior default Plans for every property in the batch that is
-        # receiving a new default Plan — one UPDATE for the whole batch.
+        # receiving a new default Plan — one UPDATE for the whole batch,
+        # across ALL scenarios: readers select a property's default by
+        # portfolio + is_default alone, so the invariant is one default per
+        # property. Scenario-scoped demotion left a promoted first Plan
+        # (non-default scenario) defaulted forever alongside the default
+        # scenario's later Plan (#1490).
         default_pids = [r.property_id for r in requests if r.is_default]
         if default_pids:
-            # scenario_id is uniform per batch (one scenario per SQS message).
-            scenario_id = requests[0].scenario_id
             self._session.exec(  # type: ignore[call-overload]
                 update(PlanModel)
-                .where(
-                    col(PlanModel.property_id).in_(default_pids),
-                    col(PlanModel.scenario_id) == scenario_id,
-                )
+                .where(col(PlanModel.property_id).in_(default_pids))
                 .values(is_default=False)
             )
 

repositories/plan/plan_repository.py

@@ -43,7 +43,8 @@ class PlanRepository(ABC):
     ) -> int:
         """Persist ``plan`` and return its Plan id. Keeps prior Plans for
         ``(property_id, scenario_id)`` as history; when ``is_default`` is True,
-        demotes those prior Plans to ``is_default=False``."""
+        demotes the property's prior default Plan — whatever its Scenario — to
+        ``is_default=False`` (one default Plan per property)."""
         ...
 
     @abstractmethod
@@ -54,3 +55,11 @@ class PlanRepository(ABC):
         UPDATE only when at least one request has ``is_default=True``. Keeps
         prior Plans as history (ADR-0017)."""
         ...
+
+    @abstractmethod
+    def property_ids_with_default_plans(self, property_ids: list[int]) -> set[int]:
+        """The subset of ``property_ids`` that already have a default Plan.
+        A property outside this set is receiving its first Plan, which becomes
+        its default whatever the Scenario says (the legacy ``p.is_new`` rule —
+        readers need one default Plan per property)."""
+        ...

tests/applications/modelling_e2e/test_handler.py

@@ -511,6 +511,70 @@ def test_attach_mode_partial_failure_persists_successes_then_records_failure() -
     ]
 
 
+def test_first_plan_for_a_property_becomes_its_default() -> None:
+    """A property's first Plan is its default whatever the Scenario says (the
+    legacy p.is_new rule — readers select one default Plan per property); a
+    property that already has a default Plan keeps the Scenario's flag."""
+    # Arrange — a non-default scenario; 111 has no default plan yet, 222 does
+    pid_first, pid_has_default = 111, 222
+    mock_engine = _engine_mock(
+        [pid_first, pid_has_default], [1001, 1002], [POSTCODE, POSTCODE]
+    )
+    scenario = MagicMock()
+    scenario.is_default = False
+
+    with ExitStack() as stack:
+        stack.enter_context(patch("applications.modelling_e2e.handler.os.environ", _ENV))
+        stack.enter_context(
+            patch("applications.modelling_e2e.handler._get_engine", return_value=mock_engine)
+        )
+        stack.enter_context(
+            patch("applications.modelling_e2e.handler.EpcClientService")
+        ).return_value.get_by_uprn.return_value = MagicMock()
+        stack.enter_context(patch("applications.modelling_e2e.handler.GeospatialS3Repository"))
+        stack.enter_context(patch("applications.modelling_e2e.handler.GoogleSolarApiClient"))
+        stack.enter_context(
+            patch("applications.modelling_e2e.handler._spatial_for", return_value=None)
+        )
+        stack.enter_context(
+            patch("applications.modelling_e2e.handler._solar_insights_for", return_value=None)
+        )
+        stack.enter_context(
+            patch("applications.modelling_e2e.handler.overlays_from", return_value=[])
+        )
+        stack.enter_context(
+            patch("applications.modelling_e2e.handler.PropertyOverridesPostgresReader")
+        ).return_value.overrides_for_many.return_value = {}
+        stack.enter_context(
+            patch("applications.modelling_e2e.handler.ScenarioPostgresRepository")
+        ).return_value.get_many.return_value = [scenario]
+        stack.enter_context(patch("applications.modelling_e2e.handler.catalogue_snapshot_with_off_catalogue_overrides"))
+        stack.enter_context(patch("applications.modelling_e2e.handler.Session"))
+        stack.enter_context(
+            patch("applications.modelling_e2e.handler.run_modelling", return_value=_plan_mock())
+        )
+        MockUoW = stack.enter_context(patch("applications.modelling_e2e.handler.PostgresUnitOfWork"))
+        mock_uow = MagicMock()
+        mock_uow.plan.property_ids_with_default_plans.return_value = {pid_has_default}
+        MockUoW.return_value.__enter__.return_value = mock_uow
+        MockUoW.return_value.__exit__.return_value = False
+
+        # Act
+        from applications.modelling_e2e.handler import handler
+        handler.__wrapped__(  # type: ignore[attr-defined]
+            {"property_ids": [pid_first, pid_has_default],
+             "portfolio_id": PORTFOLIO_ID, "scenario_id": SCENARIO_ID,
+             "refetch_solar": False, "dry_run": False},
+            None, _mock_orchestrator(), uuid4(),
+        )
+
+    # Assert — the first-timer's plan is promoted; the other keeps the flag
+    plan_requests = mock_uow.plan.save_batch.call_args.args[0]
+    by_pid = {r.property_id: r for r in plan_requests}
+    assert by_pid[pid_first].is_default is True
+    assert by_pid[pid_has_default].is_default is False
+
+
 # ---------------------------------------------------------------------------
 # Lodged EPC path
 # ---------------------------------------------------------------------------

tests/orchestration/fakes.py

@@ -212,6 +212,7 @@ class FakePlanRepository(PlanRepository):
 
     def __init__(self) -> None:
         self.saved: dict[tuple[int, int], Plan] = {}
+        self.default_property_ids: set[int] = set()
         self._next_id = 1
 
     def save(
@@ -224,6 +225,8 @@ class FakePlanRepository(PlanRepository):
         is_default: bool,
     ) -> int:
         self.saved[(property_id, scenario_id)] = plan
+        if is_default:
+            self.default_property_ids.add(property_id)
         plan_id = self._next_id
         self._next_id += 1
         return plan_id
@@ -240,6 +243,9 @@ class FakePlanRepository(PlanRepository):
             for r in requests
         ]
 
+    def property_ids_with_default_plans(self, property_ids: list[int]) -> set[int]:
+        return self.default_property_ids.intersection(property_ids)
+
 
 class _UnsetProductRepo(ProductRepository):
     """Default for a `FakeUnitOfWork` built without a catalogue — raises if a

tests/repositories/plan/test_plan_postgres_repository.py

@@ -206,3 +206,56 @@ def test_rerun_as_non_default_does_not_demote_the_prior_default(
 
     assert len(plan_rows) == 2
     assert by_id[first_id].is_default is True
+
+
+def test_reports_which_properties_already_have_a_default_plan(
+    db_engine: Engine,
+) -> None:
+    # Arrange — property 20 has a default Plan, 21 only a non-default one,
+    # 22 has no Plans at all
+    with Session(db_engine) as session:
+        repo = PlanPostgresRepository(session)
+        repo.save(_plan(), property_id=20, scenario_id=7, portfolio_id=1, is_default=True)
+        repo.save(_plan(), property_id=21, scenario_id=7, portfolio_id=1, is_default=False)
+        session.commit()
+
+    # Act
+    with Session(db_engine) as session:
+        have_default = PlanPostgresRepository(session).property_ids_with_default_plans(
+            [20, 21, 22]
+        )
+
+    # Assert — only the property with a default Plan is reported
+    assert have_default == {20}
+
+
+def test_a_new_default_demotes_the_prior_default_across_scenarios(
+    db_engine: Engine,
+) -> None:
+    """One default Plan per property: a promoted first Plan (non-default
+    scenario) must lose the default when the default scenario's Plan lands —
+    readers filter portfolio + is_default only, never scenario (#1490)."""
+    # Arrange — the property's first Plan, promoted under non-default scenario 7
+    with Session(db_engine) as session:
+        first_id = PlanPostgresRepository(session).save(
+            _plan(), property_id=30, scenario_id=7, portfolio_id=1, is_default=True
+        )
+        session.commit()
+
+    # Act — the default scenario (8) models the same property
+    with Session(db_engine) as session:
+        second_id = PlanPostgresRepository(session).save(
+            _plan(), property_id=30, scenario_id=8, portfolio_id=1, is_default=True
+        )
+        session.commit()
+
+    # Assert — exactly one default for the property, the newest plan
+    with Session(db_engine) as session:
+        plan_rows = session.exec(
+            select(PlanModel).where(col(PlanModel.property_id) == 30)
+        ).all()
+        by_id = {p.id: p for p in plan_rows}
+
+    assert by_id[first_id].is_default is False  # demoted across scenarios
+    assert by_id[second_id].is_default is True
+    assert sum(1 for p in plan_rows if p.is_default) == 1