diff --git a/docs/adr/0057-uprn-confirmation-precedes-finalise.md b/docs/adr/0057-uprn-confirmation-precedes-finalise.md new file mode 100644 index 000000000..81eb1185e --- /dev/null +++ b/docs/adr/0057-uprn-confirmation-precedes-finalise.md @@ -0,0 +1,80 @@ +# UPRN confirmation precedes finalise; the bulk-upload review is one two-tab page and the portfolio "unmatched" tab is retired + +## Status + +proposed + +## Context + +The bulk-upload pipeline is a chain of SQS-driven Lambda stages: +`address2uprn → landlord-overrides → combiner → (review) → finaliser`. +`address2uprn` matches each input row independently against per-postcode EPC +candidates, keeping the best candidate above a `lexiscore >= 0.7` floor. There +is **no cross-row uniqueness guard**: nothing stops one UPRN being the best +match for two *different* addresses. The textbook case is a coarse EPC record +`42 Moreton Road` (no flat token) that `Flat 1, 42 Moreton Road` and +`Flat 2, 42 Moreton Road` both clear — the building-number guard passes and the +flat guard never fires, so both distinct flats collapse onto one UPRN. + +Downstream this is corrosive, not just noisy: + +- The `property` identity insert is `on_conflict_do_nothing` on the partial + unique index `(portfolio_id, uprn) WHERE uprn IS NOT NULL`. Two distinct + addresses carrying the same UPRN therefore **merge into a single property** — + one address silently loses its identity. +- `_build_overrides` (the finaliser) keys `property_overrides` on + `(property_id, override_component, building_part)`. Both merged rows emit the + same tuple, so the single `INSERT ... ON CONFLICT DO UPDATE` raises + `CardinalityViolation` ("cannot affect row a second time"). + +The classifier and finaliser were built assuming a UPRN is always intact and +unique; it is not. Separately, a *post-onboarding* portfolio-level "Unmatched +properties" tab let users fix no-UPRN properties after the fact — too late (the +identity merge has already happened) and in a different place from the rest of +review. + +## Decision + +1. **`address2uprn` emits a UPRN only when confident *and* unambiguous.** Within + a postcode group, a UPRN that is the best match for two or more rows whose + *normalised* input addresses differ is **withheld** (dropped to null) on + every such row; a UPRN shared only by rows with the *same* normalised address + is a genuine re-listing and is kept. `address2uprn` emits an + `address2uprn_status` column (`matched | ambiguous_duplicate | unmatched | + invalid_postcode | error`); withheld rows keep their `lexiscore` for triage. + A `lexiscore` review band `[0.7, X)` that also withholds shaky matches is a + follow-on once `X` is calibrated (see Consequences). +2. **One pre-finalise review page, two tabs**, styled like the retired unmatched + tab: + - **Addresses** — the flagged rows (`unmatched` / `ambiguous_duplicate` / + low-score); the user resolves each via OS Places (reusing the existing + `MatchAddress` / `searchAddresses` / `useAssignUprn` flow + `postcode_search` + cache), producing a valid residential UPRN, or explicitly marks *no-UPRN*. + - **Classification** — the existing description→value verify gate, semantics + unchanged (every `Unknown` must be mapped; the finaliser still fails loud on + an unresolved value). + Finalise is enabled only when **both** tabs are clear. +3. The finaliser runs only after confirmation, so every row carries a confirmed + UPRN **or** an explicit no-UPRN (a first-class terminal state: `property` + identity with `uprn NULL`, no `property_overrides`, building-passport shows + the "not matched yet" card). +4. **Retire the portfolio-level "Unmatched properties" tab** — properties now + arrive pre-matched, so post-onboarding UPRN cleanup is unnecessary. +5. `PropertyOverridePostgresRepository.upsert_all` keeps a **defensive + last-write-wins dedup** as a backstop for the single-statement invariant, but + it is no longer the primary fix. + +## Consequences + +- Distinct addresses can no longer silently merge into one property; the + `CardinalityViolation` can no longer arise from real data. +- Onboarding reaching "complete" now implies confirmed identities, so portfolio + pages need no unmatched tab. +- The ambiguous/low-score subset adds bounded user work at review time; confident + rows pass untouched. +- The `lexiscore` review-band threshold `X` needs calibrating against a real + portfolio's score distribution before it is switched on; until then only the + ambiguous-duplicate rule (which needs no threshold) is active. +- Migration is safe stage-by-stage: withheld UPRNs are caught by the *existing* + unmatched tab until the two-tab page ships, and the tab is removed only after + it does — so an ambiguous property always has somewhere to be resolved. diff --git a/tests/backend/address2UPRN/__init__.py b/tests/backend/address2UPRN/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/backend/address2UPRN/test_resolve_group_ambiguity.py b/tests/backend/address2UPRN/test_resolve_group_ambiguity.py new file mode 100644 index 000000000..1279cd1d6 --- /dev/null +++ b/tests/backend/address2UPRN/test_resolve_group_ambiguity.py @@ -0,0 +1,70 @@ +"""Unit tests for ``resolve_group_ambiguity`` (ADR-0057). + +Each address is matched independently, so one UPRN can be the best match for +two *different* addresses (a coarse EPC record absorbing several real +addresses). Withholding that UPRN is a pure function of a postcode group's +``(uprn, normalised_address)`` pairs — no S3, EPC API, or scoring needed — so +it is unit-tested directly here. +""" + +from __future__ import annotations + +from backend.address2UPRN.scoring import resolve_group_ambiguity + + +def test_distinct_addresses_sharing_one_uprn_are_withheld() -> None: + # Flat 1 and Flat 2 both matched the same coarse EPC UPRN — distinct + # addresses, so both are withheld rather than silently merged. + result = resolve_group_ambiguity( + [ + ("100", "flat 1 42 moreton road"), + ("100", "flat 2 42 moreton road"), + ] + ) + assert result == [(None, "ambiguous_duplicate"), (None, "ambiguous_duplicate")] + + +def test_same_address_listed_twice_keeps_the_uprn() -> None: + # A genuine re-listing of one property (identical normalised address) is a + # real duplicate, not an ambiguous match — the UPRN is kept on both. + result = resolve_group_ambiguity( + [ + ("100", "42 moreton road"), + ("100", "42 moreton road"), + ] + ) + assert result == [("100", "matched"), ("100", "matched")] + + +def test_distinct_uprns_are_each_matched() -> None: + result = resolve_group_ambiguity( + [ + ("100", "42 moreton road"), + ("101", "44 moreton road"), + ] + ) + assert result == [("100", "matched"), ("101", "matched")] + + +def test_none_uprn_is_unmatched() -> None: + result = resolve_group_ambiguity([(None, "99 nowhere lane")]) + assert result == [(None, "unmatched")] + + +def test_order_is_preserved_across_mixed_group() -> None: + # A withheld pair, an unmatched row, and a clean match — all in one group; + # the output aligns positionally with the input. + result = resolve_group_ambiguity( + [ + ("100", "flat 1 42 moreton road"), # ambiguous (with row 3) + (None, "no epc candidate"), # unmatched + ("100", "flat 2 42 moreton road"), # ambiguous (with row 0) + ("200", "sole match road"), # matched + ] + ) + assert result == [ + (None, "ambiguous_duplicate"), + (None, "unmatched"), + (None, "ambiguous_duplicate"), + ("200", "matched"), + ] diff --git a/tests/repositories/property/test_property_override_postgres_repository.py b/tests/repositories/property/test_property_override_postgres_repository.py new file mode 100644 index 000000000..635b32ee5 --- /dev/null +++ b/tests/repositories/property/test_property_override_postgres_repository.py @@ -0,0 +1,102 @@ +"""Integration tests for ``PropertyOverridePostgresRepository.upsert_all``. + +The conflict handling lives entirely in SQL (``INSERT ... ON CONFLICT +(property_id, override_component, building_part) DO UPDATE``), so it can only be +verified against a real Postgres -- the ``db_engine`` fixture in +``tests/conftest.py`` spins one up per test. The batch-dedup below is exactly +the case an in-memory fake cannot catch: only a real ``ON CONFLICT`` statement +raises ``CardinalityViolation`` when a batch would touch one target row twice +(the ADR-0057 backstop). +""" + +from __future__ import annotations + +from collections.abc import Iterator + +import pytest +from sqlalchemy import Engine +from sqlmodel import Session, select + +from infrastructure.postgres.property_override_table import PropertyOverrideRow +from repositories.property.property_override_postgres_repository import ( + PropertyOverridePostgresRepository, +) +from repositories.property.property_override_repository import PropertyOverrideInsert + + +@pytest.fixture +def session(db_engine: Engine) -> Iterator[Session]: + with Session(db_engine) as s: + yield s + + +def _insert( + property_id: int, + override_value: str, + description: str, + *, + building_part: int = 0, + component: str = "property_type", +) -> PropertyOverrideInsert: + return PropertyOverrideInsert( + property_id=property_id, + portfolio_id=820, + building_part=building_part, + override_component=component, + override_value=override_value, + original_spreadsheet_description=description, + ) + + +def _all_rows(session: Session) -> list[PropertyOverrideRow]: + return list(session.exec(select(PropertyOverrideRow)).all()) + + +def test_duplicate_conflict_key_in_one_batch_collapses_to_last( + session: Session, +) -> None: + # arrange: two batch rows sharing the conflict key + # (property_id, override_component, building_part) — before the backstop this + # raised CardinalityViolation ("cannot affect row a second time"). + repo = PropertyOverridePostgresRepository(session) + + # act + repo.upsert_all( + [ + _insert(742961, "House", "House: Semi-Detached"), + _insert(742961, "House", "House: Mid-Terrace"), + ] + ) + session.commit() + + # assert: one row, last occurrence wins (matches ON CONFLICT DO UPDATE). + rows = _all_rows(session) + assert len(rows) == 1 + assert rows[0].override_value == "House" + assert rows[0].original_spreadsheet_description == "House: Mid-Terrace" + + +def test_distinct_conflict_keys_are_all_written(session: Session) -> None: + # arrange: dedup must only collapse exact conflict-key duplicates — a + # different property_id, building_part, or component is a distinct row. + repo = PropertyOverridePostgresRepository(session) + + # act + repo.upsert_all( + [ + _insert(742961, "House", "House: Semi-Detached"), + _insert(742962, "House", "House: Mid-Terrace"), # different property + _insert(742961, "House", "ext", building_part=1), # different part + _insert(742961, "SolidBrick", "solid", component="wall_type"), # component + ] + ) + session.commit() + + # assert + assert len(_all_rows(session)) == 4 + + +def test_empty_batch_is_a_noop(session: Session) -> None: + repo = PropertyOverridePostgresRepository(session) + assert repo.upsert_all([]) == 0 + assert _all_rows(session) == []