From 015db4275b0adf8432388d025c10168814a6d6ef Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 7 Jul 2026 16:08:30 +0000 Subject: [PATCH 1/7] =?UTF-8?q?The=20plan=20repository=20reports=20which?= =?UTF-8?q?=20properties=20already=20have=20a=20default=20plan=20?= =?UTF-8?q?=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- repositories/plan/plan_postgres_repository.py | 3 +++ repositories/plan/plan_repository.py | 8 +++++++ tests/orchestration/fakes.py | 5 +++++ .../plan/test_plan_postgres_repository.py | 21 +++++++++++++++++++ 4 files changed, 37 insertions(+) diff --git a/repositories/plan/plan_postgres_repository.py b/repositories/plan/plan_postgres_repository.py index e81e90844..c2685ace4 100644 --- a/repositories/plan/plan_postgres_repository.py +++ b/repositories/plan/plan_postgres_repository.py @@ -45,6 +45,9 @@ 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]: + raise NotImplementedError + 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 6fdc16d63..149a88618 100644 --- a/repositories/plan/plan_repository.py +++ b/repositories/plan/plan_repository.py @@ -54,3 +54,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).""" + ... diff --git a/tests/orchestration/fakes.py b/tests/orchestration/fakes.py index d5fe016a8..52f409693 100644 --- a/tests/orchestration/fakes.py +++ b/tests/orchestration/fakes.py @@ -240,6 +240,11 @@ class FakePlanRepository(PlanRepository): for r in requests ] + def property_ids_with_default_plans(self, property_ids: list[int]) -> set[int]: + # The fake store does not track defaults; every property reads as + # first-time (no default yet). + return set() + class _UnsetProductRepo(ProductRepository): """Default for a `FakeUnitOfWork` built without a catalogue — raises if a diff --git a/tests/repositories/plan/test_plan_postgres_repository.py b/tests/repositories/plan/test_plan_postgres_repository.py index 200c38d6b..2d3fb7473 100644 --- a/tests/repositories/plan/test_plan_postgres_repository.py +++ b/tests/repositories/plan/test_plan_postgres_repository.py @@ -206,3 +206,24 @@ 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} From 371240c8856daf9d31ded655b2f1179839636b34 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 7 Jul 2026 16:11:32 +0000 Subject: [PATCH 2/7] =?UTF-8?q?The=20plan=20repository=20reports=20which?= =?UTF-8?q?=20properties=20already=20have=20a=20default=20plan=20?= =?UTF-8?q?=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- repositories/plan/plan_postgres_repository.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/repositories/plan/plan_postgres_repository.py b/repositories/plan/plan_postgres_repository.py index c2685ace4..aa1e30157 100644 --- a/repositories/plan/plan_postgres_repository.py +++ b/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 @@ -46,7 +46,15 @@ class PlanPostgresRepository(PlanRepository): )[0] def property_ids_with_default_plans(self, property_ids: list[int]) -> set[int]: - raise NotImplementedError + 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. From 101c1f1f2149bb970e0095e007bc45d38b0ebe62 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 7 Jul 2026 16:12:34 +0000 Subject: [PATCH 3/7] =?UTF-8?q?A=20property's=20first=20plan=20becomes=20i?= =?UTF-8?q?ts=20default=20whatever=20the=20scenario=20says=20=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- .../modelling_e2e/test_handler.py | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/applications/modelling_e2e/test_handler.py b/tests/applications/modelling_e2e/test_handler.py index f2e33187b..32319bcd5 100644 --- a/tests/applications/modelling_e2e/test_handler.py +++ b/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 # --------------------------------------------------------------------------- From 0eaf6fa99cb570329dd2b43a3f34d3d276764755 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 7 Jul 2026 16:13:47 +0000 Subject: [PATCH 4/7] =?UTF-8?q?A=20property's=20first=20plan=20becomes=20i?= =?UTF-8?q?ts=20default=20whatever=20the=20scenario=20says=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- applications/modelling_e2e/handler.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/applications/modelling_e2e/handler.py b/applications/modelling_e2e/handler.py index b580a37d4..55a646676 100644 --- a/applications/modelling_e2e/handler.py +++ b/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. + have_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 have_default, ) for w in writes ] From aa94e76d3d27d4ea62e0ef42af72828b7efe90f0 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 7 Jul 2026 16:41:48 +0000 Subject: [PATCH 5/7] =?UTF-8?q?A=20new=20default=20plan=20demotes=20the=20?= =?UTF-8?q?prior=20default=20across=20scenarios=20=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- .../plan/test_plan_postgres_repository.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/repositories/plan/test_plan_postgres_repository.py b/tests/repositories/plan/test_plan_postgres_repository.py index 2d3fb7473..7198a6926 100644 --- a/tests/repositories/plan/test_plan_postgres_repository.py +++ b/tests/repositories/plan/test_plan_postgres_repository.py @@ -227,3 +227,35 @@ def test_reports_which_properties_already_have_a_default_plan( # 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 From 5c982beba403fab9ce430c0742a0095b060fbf82 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 7 Jul 2026 16:42:47 +0000 Subject: [PATCH 6/7] =?UTF-8?q?A=20new=20default=20plan=20demotes=20the=20?= =?UTF-8?q?prior=20default=20across=20scenarios=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- repositories/plan/plan_postgres_repository.py | 19 ++++++++++--------- repositories/plan/plan_repository.py | 3 ++- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/repositories/plan/plan_postgres_repository.py b/repositories/plan/plan_postgres_repository.py index aa1e30157..21540523a 100644 --- a/repositories/plan/plan_postgres_repository.py +++ b/repositories/plan/plan_postgres_repository.py @@ -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 @@ -67,17 +68,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) ) diff --git a/repositories/plan/plan_repository.py b/repositories/plan/plan_repository.py index 149a88618..15cafced1 100644 --- a/repositories/plan/plan_repository.py +++ b/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 From 8dd125f199e363530c1ac99e3e96ab3a06b78d70 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 7 Jul 2026 16:44:23 +0000 Subject: [PATCH 7/7] =?UTF-8?q?Review=20nitpicks:=20pids=5Fwith=5Fdefault?= =?UTF-8?q?=20naming,=20empty-batch=20guard,=20truthful=20fake=20?= =?UTF-8?q?=F0=9F=9F=AA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- applications/modelling_e2e/handler.py | 4 ++-- repositories/plan/plan_postgres_repository.py | 2 ++ tests/orchestration/fakes.py | 7 ++++--- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/applications/modelling_e2e/handler.py b/applications/modelling_e2e/handler.py index 55a646676..51c3e8149 100644 --- a/applications/modelling_e2e/handler.py +++ b/applications/modelling_e2e/handler.py @@ -210,7 +210,7 @@ def _flush_writes(engine: Engine, writes: list[_PropertyWrite]) -> None: # 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. - have_default: set[int] = uow.plan.property_ids_with_default_plans( + pids_with_default: set[int] = uow.plan.property_ids_with_default_plans( [w.property_id for w in writes] ) plan_requests = [ @@ -219,7 +219,7 @@ 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 have_default, + is_default=w.is_default or w.property_id not in pids_with_default, ) for w in writes ] diff --git a/repositories/plan/plan_postgres_repository.py b/repositories/plan/plan_postgres_repository.py index 21540523a..dc3b135a7 100644 --- a/repositories/plan/plan_postgres_repository.py +++ b/repositories/plan/plan_postgres_repository.py @@ -47,6 +47,8 @@ class PlanPostgresRepository(PlanRepository): )[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() diff --git a/tests/orchestration/fakes.py b/tests/orchestration/fakes.py index 52f409693..4c6d323b9 100644 --- a/tests/orchestration/fakes.py +++ b/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 @@ -241,9 +244,7 @@ class FakePlanRepository(PlanRepository): ] def property_ids_with_default_plans(self, property_ids: list[int]) -> set[int]: - # The fake store does not track defaults; every property reads as - # first-time (no default yet). - return set() + return self.default_property_ids.intersection(property_ids) class _UnsetProductRepo(ProductRepository):