Merge pull request #1691 from Hestia-Homes/fix/default-plan-refresh-on-rerun

Refresh a property's default Plan when its owning scenario is re-run

applications/modelling_e2e/handler.py

@@ -241,13 +241,18 @@ 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]
+        # A Plan becomes this property's default when any of:
+        #  - the Scenario is flagged default (w.is_default);
+        #  - the property has no default Plan yet (first run — readers select one
+        #    default per property, so a first run under a non-default Scenario must
+        #    still produce one; also heals default-less properties);
+        #  - this run re-models the SAME Scenario that currently owns the default,
+        #    so a re-run refreshes the shown Plan in place (the save demotes the
+        #    stale one). Without this, and because scenarios are created
+        #    is_default=False, the first-ever Plan stays default forever and a
+        #    re-model never surfaces (property 792168 / portfolio 850).
+        default_scenario_by_pid: dict[int, int] = (
+            uow.plan.default_plan_scenario_by_property([w.property_id for w in writes])
         )
         plan_requests = [
             PlanSaveRequest(
@@ -255,7 +260,11 @@ def _flush_writes(engine: Engine, writes: list[_PropertyWrite]) -> None:
                 property_id=w.property_id,
                 scenario_id=w.scenario_id,
                 portfolio_id=w.portfolio_id,
-                is_default=w.is_default or w.property_id not in pids_with_default,
+                is_default=(
+                    w.is_default
+                    or w.property_id not in default_scenario_by_pid
+                    or default_scenario_by_pid[w.property_id] == w.scenario_id
+                ),
             )
             for w in writes
         ]

repositories/plan/plan_postgres_repository.py

@@ -59,6 +59,26 @@ class PlanPostgresRepository(PlanRepository):
         ).all()
         return {int(row) for row in rows}
 
+    def default_plan_scenario_by_property(
+        self, property_ids: list[int]
+    ) -> dict[int, int]:
+        """Each property's current default Plan's scenario_id (absent when it has
+        no default). One default per property (the demote invariant), so the map
+        is well-defined."""
+        if not property_ids:
+            return {}
+        rows = self._session.exec(
+            select(col(PlanModel.property_id), col(PlanModel.scenario_id)).where(
+                col(PlanModel.property_id).in_(property_ids),
+                col(PlanModel.is_default).is_(True),
+            )
+        ).all()
+        return {
+            int(property_id): int(scenario_id)
+            for property_id, scenario_id in rows
+            if scenario_id is not None
+        }
+
     def save_batch(self, requests: list[PlanSaveRequest]) -> list[int]:
         """Persist all Plans in three statements regardless of batch size.
 

repositories/plan/plan_repository.py

@@ -63,3 +63,19 @@ class PlanRepository(ABC):
         its default whatever the Scenario says (the legacy ``p.is_new`` rule —
         readers need one default Plan per property)."""
         ...
+
+    @abstractmethod
+    def default_plan_scenario_by_property(
+        self, property_ids: list[int]
+    ) -> dict[int, int]:
+        """Each property's current default Plan's ``scenario_id`` (absent when it
+        has no default). One default Plan per property (invariant), so the map is
+        well-defined.
+
+        Lets the orchestrator refresh the default in place: a re-run of the
+        scenario that currently owns the default promotes its fresh Plan (the repo
+        demotes the stale one), while a re-run of a different scenario adds history
+        without hijacking the default. Needed because scenarios are created
+        ``is_default=False``, so the Scenario flag never promotes a re-run and the
+        first-Plan-ever default would otherwise be frozen forever."""
+        ...

tests/applications/modelling_e2e/test_handler.py

@@ -561,10 +561,16 @@ def test_first_plan_for_a_property_becomes_its_default() -> None:
         )
         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
 
+        # pid_has_default's current default is for a DIFFERENT scenario than the
+        # one being modelled (SCENARIO_ID), so it is not refreshed and keeps the
+        # non-default flag; pid_first has no default at all.
+        mock_uow.plan.default_plan_scenario_by_property.return_value = {
+            pid_has_default: SCENARIO_ID + 1
+        }
+
         # Act
         from applications.modelling_e2e.handler import handler
         handler.__wrapped__(  # type: ignore[attr-defined]
@@ -581,6 +587,120 @@ def test_first_plan_for_a_property_becomes_its_default() -> None:
     assert by_pid[pid_has_default].is_default is False
 
 
+def test_re_running_the_scenario_that_holds_the_default_refreshes_it() -> None:
+    """A re-run of the scenario that currently owns the property's default Plan
+    promotes the fresh Plan to default (the repo demotes the stale one), so a
+    re-model surfaces on the FE. Provenance: property 792168 kept its 2026-07-22
+    scenario-1328 plan default through three same-scenario re-runs."""
+    # Arrange — the property's default is for the SAME scenario being modelled.
+    pid = 111
+    mock_engine = _engine_mock([pid], [UPRN], [POSTCODE])
+    scenario = MagicMock()
+    scenario.is_default = False
+    mock_uow = MagicMock()
+    mock_uow.plan.default_plan_scenario_by_property.return_value = {pid: SCENARIO_ID}
+
+    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"))
+        MockUoW.return_value.__enter__.return_value = mock_uow
+        MockUoW.return_value.__exit__.return_value = False
+
+        # Act
+        _call_handler(
+            {"property_ids": [pid], "portfolio_id": PORTFOLIO_ID,
+             "scenario_id": SCENARIO_ID, "refetch_solar": False, "dry_run": False}
+        )
+
+    # Assert — the fresh plan for the default-holding scenario is promoted.
+    plan_requests = mock_uow.plan.save_batch.call_args.args[0]
+    assert plan_requests[0].is_default is True
+
+
+def test_re_running_a_different_scenario_does_not_steal_the_default() -> None:
+    """A re-run of a scenario OTHER than the one holding the default does not
+    hijack it — the shown scenario only changes via scenario.is_default."""
+    # Arrange — the property's default is for a different scenario.
+    pid = 111
+    mock_engine = _engine_mock([pid], [UPRN], [POSTCODE])
+    scenario = MagicMock()
+    scenario.is_default = False
+    mock_uow = MagicMock()
+    mock_uow.plan.default_plan_scenario_by_property.return_value = {pid: SCENARIO_ID + 1}
+
+    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"))
+        MockUoW.return_value.__enter__.return_value = mock_uow
+        MockUoW.return_value.__exit__.return_value = False
+
+        # Act
+        _call_handler(
+            {"property_ids": [pid], "portfolio_id": PORTFOLIO_ID,
+             "scenario_id": SCENARIO_ID, "refetch_solar": False, "dry_run": False}
+        )
+
+    # Assert — the other scenario's re-run does not become the default.
+    plan_requests = mock_uow.plan.save_batch.call_args.args[0]
+    assert plan_requests[0].is_default is False
+
+
 # ---------------------------------------------------------------------------
 # Lodged EPC path
 # ---------------------------------------------------------------------------

tests/orchestration/fakes.py

@@ -233,7 +233,9 @@ class FakePlanRepository(PlanRepository):
 
     def __init__(self) -> None:
         self.saved: dict[tuple[int, int], Plan] = {}
-        self.default_property_ids: set[int] = set()
+        # One default Plan per property: the scenario that currently owns it (a
+        # new default demotes any prior one, mirroring the postgres invariant).
+        self.default_scenario_by_property: dict[int, int] = {}
         self._next_id = 1
 
     def save(
@@ -247,7 +249,7 @@ class FakePlanRepository(PlanRepository):
     ) -> int:
         self.saved[(property_id, scenario_id)] = plan
         if is_default:
-            self.default_property_ids.add(property_id)
+            self.default_scenario_by_property[property_id] = scenario_id
         plan_id = self._next_id
         self._next_id += 1
         return plan_id
@@ -265,7 +267,18 @@ class FakePlanRepository(PlanRepository):
         ]
 
     def property_ids_with_default_plans(self, property_ids: list[int]) -> set[int]:
-        return self.default_property_ids.intersection(property_ids)
+        return {
+            pid for pid in property_ids if pid in self.default_scenario_by_property
+        }
+
+    def default_plan_scenario_by_property(
+        self, property_ids: list[int]
+    ) -> dict[int, int]:
+        return {
+            pid: self.default_scenario_by_property[pid]
+            for pid in property_ids
+            if pid in self.default_scenario_by_property
+        }
 
 
 class _UnsetProductRepo(ProductRepository):

tests/repositories/plan/test_plan_postgres_repository.py

@@ -229,6 +229,29 @@ def test_reports_which_properties_already_have_a_default_plan(
     assert have_default == {20}
 
 
+def test_reports_the_scenario_holding_each_propertys_default_plan(
+    db_engine: Engine,
+) -> None:
+    # Arrange — property 40's default is scenario 7 (it also has a non-default
+    # plan under scenario 8); property 41 has only a non-default plan; 42 none.
+    with Session(db_engine) as session:
+        repo = PlanPostgresRepository(session)
+        repo.save(_plan(), property_id=40, scenario_id=7, portfolio_id=1, is_default=True)
+        repo.save(_plan(), property_id=40, scenario_id=8, portfolio_id=1, is_default=False)
+        repo.save(_plan(), property_id=41, scenario_id=7, portfolio_id=1, is_default=False)
+        session.commit()
+
+    # Act
+    with Session(db_engine) as session:
+        by_property = PlanPostgresRepository(session).default_plan_scenario_by_property(
+            [40, 41, 42]
+        )
+
+    # Assert — only the property with a default Plan is present, mapped to the
+    # scenario that owns that default (used to refresh the default in place).
+    assert by_property == {40: 7}
+
+
 def test_a_new_default_demotes_the_prior_default_across_scenarios(
     db_engine: Engine,
 ) -> None: