From 0ca8ca31a0312e2f5b552e7f6aeabf9cf793f6ce Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Mon, 27 Jul 2026 16:50:41 +0000 Subject: [PATCH] Refresh a property's default Plan when its owning scenario is re-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Plan became default only when its Scenario was flagged default or the property had no Plan yet. Scenarios are created is_default=False, so in practice only the first-ever Plan was ever promoted — re-modelling a property appended a new Plan but left the default frozen on the first run, so a re-model never surfaced on the FE (property 792168 kept its 2026-07-22 scenario-1328 default through three same-scenario re-runs). Also promote when the re-run models the SAME scenario that currently owns the property's default, so a re-run refreshes the shown Plan in place (the save demotes the stale one). A re-run of a different scenario still does not hijack the default. New PlanRepository.default_plan_scenario_by_property surfaces each property's default scenario for the decision. Co-Authored-By: Claude Opus 4.8 (1M context) --- applications/modelling_e2e/handler.py | 25 ++-- repositories/plan/plan_postgres_repository.py | 20 +++ repositories/plan/plan_repository.py | 16 +++ .../modelling_e2e/test_handler.py | 122 +++++++++++++++++- tests/orchestration/fakes.py | 19 ++- .../plan/test_plan_postgres_repository.py | 23 ++++ 6 files changed, 213 insertions(+), 12 deletions(-) diff --git a/applications/modelling_e2e/handler.py b/applications/modelling_e2e/handler.py index 7561b41f4..48e4b8d96 100644 --- a/applications/modelling_e2e/handler.py +++ b/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 ] diff --git a/repositories/plan/plan_postgres_repository.py b/repositories/plan/plan_postgres_repository.py index dc3b135a7..97746aa90 100644 --- a/repositories/plan/plan_postgres_repository.py +++ b/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. diff --git a/repositories/plan/plan_repository.py b/repositories/plan/plan_repository.py index 15cafced1..26bca1d93 100644 --- a/repositories/plan/plan_repository.py +++ b/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.""" + ... diff --git a/tests/applications/modelling_e2e/test_handler.py b/tests/applications/modelling_e2e/test_handler.py index d9114f8b8..f86d84e14 100644 --- a/tests/applications/modelling_e2e/test_handler.py +++ b/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 # --------------------------------------------------------------------------- diff --git a/tests/orchestration/fakes.py b/tests/orchestration/fakes.py index 20391c36c..60acfd9c4 100644 --- a/tests/orchestration/fakes.py +++ b/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): diff --git a/tests/repositories/plan/test_plan_postgres_repository.py b/tests/repositories/plan/test_plan_postgres_repository.py index 7198a6926..61c15e8c3 100644 --- a/tests/repositories/plan/test_plan_postgres_repository.py +++ b/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: