diff --git a/repositories/plan/plan_postgres_repository.py b/repositories/plan/plan_postgres_repository.py index 376cf8b8..7be21bac 100644 --- a/repositories/plan/plan_postgres_repository.py +++ b/repositories/plan/plan_postgres_repository.py @@ -1,6 +1,6 @@ from __future__ import annotations -from sqlmodel import Session, col, delete +from sqlmodel import Session, col, update from domain.modelling.plan import Plan from infrastructure.postgres.modelling import PlanModel, RecommendationModel @@ -10,7 +10,12 @@ from repositories.plan.plan_repository import PlanRepository class PlanPostgresRepository(PlanRepository): """Maps a Plan and its Plan Measures onto the live ``plan`` / ``recommendation`` tables (ADR-0017). Does not commit — the Unit of Work - owns the transaction (ADR-0012).""" + owns the transaction (ADR-0012). + + 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``.""" def __init__(self, session: Session) -> None: self._session = session @@ -24,15 +29,20 @@ class PlanPostgresRepository(PlanRepository): portfolio_id: int, is_default: bool, ) -> int: - # Idempotent replace for (property_id, scenario_id): deleting the Plan - # cascades to its recommendation rows via the plan_id FK (ON DELETE - # CASCADE), so a re-run overwrites rather than duplicating (ADR-0012). - self._session.exec( # type: ignore[call-overload] - delete(PlanModel).where( - col(PlanModel.property_id) == property_id, - col(PlanModel.scenario_id) == scenario_id, + # Soft-replace (ADR-0012): keep prior Plans as history rather than DELETEing + # them — the cascade delete of recommendation rows was the slow part. When + # this Plan is the default, demote every prior Plan for the same + # (property_id, scenario_id) to is_default=False, so exactly one Plan for + # the pair stays default (the one just inserted). + if is_default: + self._session.exec( # type: ignore[call-overload] + update(PlanModel) + .where( + col(PlanModel.property_id) == property_id, + col(PlanModel.scenario_id) == scenario_id, + ) + .values(is_default=False) ) - ) plan_row = PlanModel.from_domain( plan, diff --git a/repositories/plan/plan_repository.py b/repositories/plan/plan_repository.py index 02bafe25..b534e8ea 100644 --- a/repositories/plan/plan_repository.py +++ b/repositories/plan/plan_repository.py @@ -8,10 +8,12 @@ from domain.modelling.plan import Plan class PlanRepository(ABC): """Persists a Plan (and its Plan Measures) for a Property + Scenario. - One Plan per (Property, Scenario). The write is idempotent on re-run: it - replaces the existing Plan for that pair rather than duplicating (ADR-0012 - / ADR-0017). `portfolio_id` and `is_default` are supplied by the - orchestrator (the former from the trigger, the latter from the Scenario). + A re-run INSERTs a fresh Plan and keeps the prior one as history rather than + deleting it. When the new Plan is the default, prior default Plans for the + same (Property, Scenario) are demoted to `is_default=False`, so the current + Plan is the one with `is_default=True` (ADR-0012 / ADR-0017). `portfolio_id` + and `is_default` are supplied by the orchestrator (the former from the + trigger, the latter from the Scenario). """ @abstractmethod @@ -24,6 +26,7 @@ class PlanRepository(ABC): portfolio_id: int, is_default: bool, ) -> int: - """Persist ``plan`` and return its Plan id, replacing any existing Plan - for ``(property_id, scenario_id)``.""" + """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``.""" ... diff --git a/tests/repositories/plan/test_plan_postgres_repository.py b/tests/repositories/plan/test_plan_postgres_repository.py index 3e428bd8..200c38d6 100644 --- a/tests/repositories/plan/test_plan_postgres_repository.py +++ b/tests/repositories/plan/test_plan_postgres_repository.py @@ -2,10 +2,11 @@ Plan Measures to the live ``plan`` / ``recommendation`` tables (ADR-0017). The Plan is the parent; each selected Plan Measure is a ``recommendation`` row -linked by the new ``plan_id`` FK. A re-run replaces (delete the Plan for the -(property, scenario) → cascade its recommendations → insert fresh), so the -batch write is idempotent (ADR-0012). CO₂ is stored in tonnes (calculator kg -÷ 1000) to match the live column contract. +linked by the new ``plan_id`` FK. A re-run INSERTs a fresh Plan and keeps the +prior one as history (no cascade delete); when the new Plan is the default it +demotes prior default Plans for the (property, scenario) to ``is_default=False`` +(ADR-0012). CO₂ is stored in tonnes (calculator kg ÷ 1000) to match the live +column contract. """ from __future__ import annotations @@ -147,31 +148,61 @@ def test_save_persists_null_per_measure_savings_when_unbilled( assert rec_rows[0].energy_cost_savings is None -def test_save_is_idempotent_on_rerun_for_the_same_property_and_scenario( +def test_rerun_keeps_history_and_demotes_the_prior_default_plan( db_engine: Engine, ) -> None: - # Arrange — first run + # Arrange — first (default) run with Session(db_engine) as session: - PlanPostgresRepository(session).save( + first_id = PlanPostgresRepository(session).save( _plan(), property_id=10, scenario_id=7, portfolio_id=1, is_default=True ) session.commit() - # Act — re-run the same (property, scenario) + # Act — re-run the same (property, scenario) as the default with Session(db_engine) as session: - PlanPostgresRepository(session).save( + second_id = PlanPostgresRepository(session).save( _plan(), property_id=10, scenario_id=7, portfolio_id=1, is_default=True ) session.commit() - # Assert — replaced, not duplicated (cascade removed the old measures) + # Assert — the prior Plan is kept as history (no delete), and only the new + # Plan is the default; exactly one Plan for the pair stays is_default=True. with Session(db_engine) as session: plan_rows = session.exec( select(PlanModel).where(col(PlanModel.property_id) == 10) ).all() - rec_rows = session.exec( - select(RecommendationModel).where(col(RecommendationModel.property_id) == 10) - ).all() + by_id = {p.id: p for p in plan_rows} - assert len(plan_rows) == 1 - assert len(rec_rows) == 1 + assert len(plan_rows) == 2 + assert first_id != second_id + assert by_id[first_id].is_default is False # demoted + assert by_id[second_id].is_default is True # the current default + assert sum(1 for p in plan_rows if p.is_default) == 1 + + +def test_rerun_as_non_default_does_not_demote_the_prior_default( + db_engine: Engine, +) -> None: + # Arrange — a default Plan exists + with Session(db_engine) as session: + first_id = PlanPostgresRepository(session).save( + _plan(), property_id=12, scenario_id=7, portfolio_id=1, is_default=True + ) + session.commit() + + # Act — re-run as NON-default (e.g. a non-default scenario); no demotion runs + with Session(db_engine) as session: + PlanPostgresRepository(session).save( + _plan(), property_id=12, scenario_id=7, portfolio_id=1, is_default=False + ) + session.commit() + + # Assert — the prior default is untouched (we only demote when saving a default) + with Session(db_engine) as session: + plan_rows = session.exec( + select(PlanModel).where(col(PlanModel.property_id) == 12) + ).all() + by_id = {p.id: p for p in plan_rows} + + assert len(plan_rows) == 2 + assert by_id[first_id].is_default is True