diff --git a/applications/modelling_e2e/handler.py b/applications/modelling_e2e/handler.py index b580a37d4..51c3e8149 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. + 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 ] diff --git a/repositories/plan/plan_postgres_repository.py b/repositories/plan/plan_postgres_repository.py index e81e90844..dc3b135a7 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 @@ -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) ) diff --git a/repositories/plan/plan_repository.py b/repositories/plan/plan_repository.py index 6fdc16d63..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 @@ -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).""" + ... 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 # --------------------------------------------------------------------------- diff --git a/tests/orchestration/fakes.py b/tests/orchestration/fakes.py index d5fe016a8..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 @@ -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 diff --git a/tests/repositories/plan/test_plan_postgres_repository.py b/tests/repositories/plan/test_plan_postgres_repository.py index 200c38d6b..7198a6926 100644 --- a/tests/repositories/plan/test_plan_postgres_repository.py +++ b/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