From e86d9ce22991183343eefb1bca4bbd00e99a6d5d Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 02:10:44 +0400 Subject: [PATCH 1/3] Let a saved policy be repointed to a v2 policy PATCH /v2/user-policies/{id} now accepts policy_id. Supplied alone, it points the saved policy at that v2 policy and clears legacy_policy_id, which no longer describes the saved content. Supplied with legacy_policy_id, both are stored, and policy_id must be the v2 copy of that v1 policy when one exists, exactly as creation decides. The policy must exist in the row's country. The row keeps its ID, owner, name and creation time, so an edited reform keeps the ID that reports link to. Co-Authored-By: Claude Opus 5.5 --- ...v2plan-1f-121-repoint-saved-items.added.md | 1 + docs/migration/stage-12a-user-login.md | 6 + .../fastapi_routes/v2/user_policies/routes.py | 10 +- .../database_connectors/updates.py | 23 ++- .../services/v2/user_policies/services.py | 39 ++-- .../services/v2/user_policies/types.py | 47 ++++- tests/unit/v2/test_user_policy_routes.py | 22 ++- tests/unit/v2/test_user_policy_service.py | 173 +++++++++++++++++- 8 files changed, 277 insertions(+), 44 deletions(-) create mode 100644 changelog.d/v2plan-1f-121-repoint-saved-items.added.md diff --git a/changelog.d/v2plan-1f-121-repoint-saved-items.added.md b/changelog.d/v2plan-1f-121-repoint-saved-items.added.md new file mode 100644 index 000000000..fae825693 --- /dev/null +++ b/changelog.d/v2plan-1f-121-repoint-saved-items.added.md @@ -0,0 +1 @@ +PATCH /v2/user-policies/{id} accepts policy_id, so a saved policy can point at another v2 policy in its country while keeping its ID, name and creation time. Supplied alone, policy_id clears the v1 reference (legacy_policy_id); supplied with legacy_policy_id, both are stored, and policy_id must be the v2 copy of that v1 policy when one exists. Unknown, other-country and conflicting policies are refused as on creation, and rows maintained by the v1 saved-policy mirror still return 409. diff --git a/docs/migration/stage-12a-user-login.md b/docs/migration/stage-12a-user-login.md index c01657bc3..62e915479 100644 --- a/docs/migration/stage-12a-user-login.md +++ b/docs/migration/stage-12a-user-login.md @@ -183,6 +183,12 @@ Concurrency on Postgres: `legacy_policy_mappings` / `legacy_household_mappings` when a mapping exists. - Changing the legacy ID re-resolves the v2 reference, or clears it. + - `PATCH /v2/user-policies` also accepts `policy_id`, which must name a v2 + policy in the row's country. Supplied alone, it points the row at that + policy and clears `legacy_policy_id`, so an edited reform keeps its saved + ID, name and creation time. Supplied with `legacy_policy_id`, both are + stored, and `policy_id` must be the mirror copy of that v1 policy when + one exists. - `user-simulations` and `user-reports` accept only legacy IDs in 12A. - **Stage 10 mirror rows.** A `user_policies` row with a `legacy_user_policy_mappings` entry returns 409 on PATCH and DELETE. The diff --git a/policyengine_api/fastapi_routes/v2/user_policies/routes.py b/policyengine_api/fastapi_routes/v2/user_policies/routes.py index bb5a06acc..6ff192fee 100644 --- a/policyengine_api/fastapi_routes/v2/user_policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_policies/routes.py @@ -168,9 +168,13 @@ def read() -> UserPolicyPageResponse: response_model=UserPolicyDetailResponse, summary="Update or repoint one of the current user's saved policies", description=( - "Changing legacy_policy_id repoints the row and re-resolves policy_id " - "from the Stage 10 mirror mapping, or clears it. Rows maintained by " - "the v1 saved-policy mirror return 409." + "The row keeps its id, owner and creation time. Changing " + "legacy_policy_id repoints the row and re-resolves policy_id from the " + "Stage 10 mirror mapping, or clears it. Changing policy_id alone points " + "the row at that v2 policy, which must exist in the same country, and " + "clears legacy_policy_id. Supplied together, both are stored, and " + "policy_id must be the mirror copy of legacy_policy_id when one exists. " + "Rows maintained by the v1 saved-policy mirror return 409." ), ) def patch_user_policy_route( diff --git a/policyengine_api/services/v2/user_policies/database_connectors/updates.py b/policyengine_api/services/v2/user_policies/database_connectors/updates.py index 328c3d32d..2208e5636 100644 --- a/policyengine_api/services/v2/user_policies/database_connectors/updates.py +++ b/policyengine_api/services/v2/user_policies/database_connectors/updates.py @@ -5,15 +5,17 @@ from typing import TYPE_CHECKING from policyengine_api.data.v2.models.base import utc_now +from policyengine_api.services.v2.user_policies.types import ( + USER_POLICY_REFERENCE_FIELDS, +) if TYPE_CHECKING: - from uuid import UUID - from sqlmodel import Session from policyengine_api.data.v2.models import LegacyUserPolicyMapping, UserPolicy from policyengine_api.services.v2.user_policies.types import ( LegacyUserPolicyRevision, + UserPolicyReferences, UserPolicyUpdateInput, ) @@ -23,17 +25,20 @@ def update_user_policy( association: UserPolicy, association_input: UserPolicyUpdateInput, *, - repointed_policy_id: UUID | None = None, - repoint: bool = False, + references: UserPolicyReferences | None = None, ) -> UserPolicy: - """Apply the supplied fields, bump ``updated_at`` and return the row. + """Apply the supplied presentation fields, bump ``updated_at`` and return the row. - With ``repoint``, also set ``policy_id`` to ``repointed_policy_id``. + With ``references``, also store both of its policy references; the service + resolves them, so the request's own reference fields are never copied. """ - for field_name, value in association_input.model_dump(exclude_unset=True).items(): + for field_name, value in association_input.model_dump( + exclude_unset=True, exclude=set(USER_POLICY_REFERENCE_FIELDS) + ).items(): setattr(association, field_name, value) - if repoint: - association.policy_id = repointed_policy_id + if references is not None: + association.policy_id = references.policy_id + association.legacy_policy_id = references.legacy_policy_id association.updated_at = utc_now() session.add(association) session.flush() diff --git a/policyengine_api/services/v2/user_policies/services.py b/policyengine_api/services/v2/user_policies/services.py index 4ccbad927..8bf3dbe4a 100644 --- a/policyengine_api/services/v2/user_policies/services.py +++ b/policyengine_api/services/v2/user_policies/services.py @@ -61,6 +61,7 @@ UserPolicyListing, UserPolicyPage, UserPolicyRead, + UserPolicyReferences, UserPolicyUpdateInput, ) from policyengine_api.services.v2.user_policies.validators import ( @@ -500,13 +501,18 @@ def patch_user_policy( ) -> UserPolicyRead: """Update the supplied fields of the principal's association in this country. - A new ``legacy_policy_id`` must exist in v1 and repoints ``policy_id`` to the - v2 copy of that policy, or to None when there is no copy. Raise - UserPolicyNotFoundError when the association is not the principal's, and - MirrorOwnedAssociationError when the v1 saved-policy mirror maintains it. + The association keeps its id, owner, country and creation time. A new + ``legacy_policy_id`` must exist in v1 (LegacyReferenceNotFoundError) and + repoints ``policy_id`` to the v2 copy of that policy, or to None when there + is no copy. A new ``policy_id`` must exist in this country + (AssociationPolicyNotFoundError or AssociationCountryConflictError). + Without ``legacy_policy_id`` it clears the v1 reference; with it, both are + stored, and it must be the v2 copy of that v1 policy when one exists + (AssociationReferenceConflictError). Raise UserPolicyNotFoundError when the + association is not the principal's, and MirrorOwnedAssociationError when + the v1 saved-policy mirror maintains it. """ - repoint = "legacy_policy_id" in association_input.model_fields_set - if repoint: + if "legacy_policy_id" in association_input.model_fields_set: if association_input.legacy_policy_id is None: msg = "legacy_policy_id must not be null" raise TypeError(msg) @@ -530,13 +536,17 @@ def patch_user_policy( ), association_id=association.id, ) - repointed_policy_id = ( - read_mirrored_policy_ids( - session, - country_id=country_id, - legacy_policy_ids=(association_input.legacy_policy_id,), - ).get(association_input.legacy_policy_id) - if repoint and association_input.legacy_policy_id is not None + references = ( + UserPolicyReferences( + policy_id=resolve_user_policy_reference( + session, + country_id=country_id, + policy_id=association_input.policy_id, + legacy_policy_id=association_input.legacy_policy_id, + ), + legacy_policy_id=association_input.legacy_policy_id, + ) + if association_input.repoints else None ) return user_policy_read( @@ -544,8 +554,7 @@ def patch_user_policy( session, association, association_input, - repointed_policy_id=repointed_policy_id, - repoint=repoint, + references=references, ) ) diff --git a/policyengine_api/services/v2/user_policies/types.py b/policyengine_api/services/v2/user_policies/types.py index c24b2d49c..296fc821c 100644 --- a/policyengine_api/services/v2/user_policies/types.py +++ b/policyengine_api/services/v2/user_policies/types.py @@ -82,13 +82,22 @@ def require_policy_reference(self) -> UserPolicyCreationInput: return self +USER_POLICY_REFERENCE_FIELDS = frozenset({"policy_id", "legacy_policy_id"}) + + class UserPolicyUpdateInput(StrictAssociationInput): """A partial update of a saved policy; only supplied fields are changed. - At least one field must be supplied. ``legacy_policy_id`` may point the - association at another v1 policy but cannot be cleared. + At least one field must be supplied. ``policy_id`` points the association at + another v2 policy; supplied without ``legacy_policy_id``, it also clears the + v1 reference, which no longer describes the saved policy. ``legacy_policy_id`` + alone points the association at another v1 policy and takes ``policy_id`` from + that policy's v2 copy, or clears it when there is none. Supplied together, + both are stored, and ``policy_id`` must be that v2 copy when one exists. + Neither reference can be set to null. """ + policy_id: ResourceId | None = None legacy_policy_id: LegacyRecordId | None = None name: AssociationName | None = None description: AssociationDescription | None = None @@ -97,21 +106,41 @@ class UserPolicyUpdateInput(StrictAssociationInput): @model_validator(mode="after") def require_supplied_field(self) -> UserPolicyUpdateInput: - """Require a supplied field, and reject an explicit null legacy_policy_id.""" + """Require a supplied field, and reject an explicit null policy reference.""" supplied = self.model_fields_set.intersection( - {"legacy_policy_id", "name", "description", "baseline", "provenance"} + { + *USER_POLICY_REFERENCE_FIELDS, + "name", + "description", + "baseline", + "provenance", + } ) if not supplied: msg = ( - "At least one of legacy_policy_id, name, description, baseline, " - "or provenance must be supplied" + "At least one of policy_id, legacy_policy_id, name, description, " + "baseline, or provenance must be supplied" ) raise ValueError(msg) - if "legacy_policy_id" in supplied and self.legacy_policy_id is None: - msg = "legacy_policy_id must not be null" - raise ValueError(msg) + for reference_field in sorted(USER_POLICY_REFERENCE_FIELDS): + if reference_field in supplied and getattr(self, reference_field) is None: + msg = f"{reference_field} must not be null" + raise ValueError(msg) return self + @property + def repoints(self) -> bool: + """Whether the update changes which policy the association references.""" + return bool(self.model_fields_set & USER_POLICY_REFERENCE_FIELDS) + + +@dataclass(frozen=True) +class UserPolicyReferences: + """The v2 and v1 policy references a repointed association stores.""" + + policy_id: UUID | None + legacy_policy_id: int | None + class LegacyUserPolicySnapshot(StrictAssociationInput): """Detached complete committed v1 saved-policy row.""" diff --git a/tests/unit/v2/test_user_policy_routes.py b/tests/unit/v2/test_user_policy_routes.py index 10d837038..37c969add 100644 --- a/tests/unit/v2/test_user_policy_routes.py +++ b/tests/unit/v2/test_user_policy_routes.py @@ -250,7 +250,8 @@ def test_patch_repoints_legacy_reference_and_rejects_invalid_changes() -> None: json=body, ) for body in ( - {"policy_id": str(POLICY_ID)}, + {"policy_id": None}, + {"policy_id": "not-a-uuid"}, {"user_id": str(OWNER_ID)}, {"client_ref": "rf-2"}, {"legacy_policy_id": None}, @@ -262,11 +263,27 @@ def test_patch_repoints_legacy_reference_and_rejects_invalid_changes() -> None: association_input = service.calls[0][1]["association_input"] assert association_input.legacy_policy_id == 101 assert association_input.model_fields_set == {"legacy_policy_id", "name"} - assert [response.status_code for response in invalid] == [422] * 5 + assert [response.status_code for response in invalid] == [422] * 6 assert [call[0] for call in service.calls] == ["patch_user_policy"] assert flask_calls["count"] == 0 +def test_patch_repoints_to_a_v2_policy_reference() -> None: + client, service, flask_calls = _client() + + repointed = client.patch( + f"/v2/user-policies/{ASSOCIATION_ID}?country_id=us", + json={"policy_id": str(POLICY_ID)}, + ) + + assert repointed.status_code == 200 + assert service.calls[0][0] == "patch_user_policy" + association_input = service.calls[0][1]["association_input"] + assert association_input.policy_id == POLICY_ID + assert association_input.model_fields_set == {"policy_id"} + assert flask_calls["count"] == 0 + + def test_delete_returns_no_content_for_the_session_user() -> None: client, service, _flask_calls = _client() @@ -383,6 +400,7 @@ def test_openapi_publishes_signed_in_association_contracts() -> None: patch_schema = schema["components"]["schemas"][patch_ref.rsplit("/", 1)[-1]] assert patch_schema["additionalProperties"] is False assert set(patch_schema["properties"]) == { + "policy_id", "legacy_policy_id", "name", "description", diff --git a/tests/unit/v2/test_user_policy_service.py b/tests/unit/v2/test_user_policy_service.py index 70352bb34..f1a69c048 100644 --- a/tests/unit/v2/test_user_policy_service.py +++ b/tests/unit/v2/test_user_policy_service.py @@ -164,7 +164,7 @@ def test_create_rejects_bad_references_before_writing(store) -> None: def test_other_users_rows_are_not_found_and_lists_are_owner_scoped(store) -> None: - service, _selected, _legacy_references = store + service, selected, _legacy_references = store alice_row = service.create_user_policy( principal=ALICE, association_input=_fields(name="Alice") ) @@ -183,6 +183,12 @@ def test_other_users_rows_are_not_found_and_lists_are_owner_scoped(store) -> Non association_id=alice_row.id, association_input=UserPolicyUpdateInput(name="stolen"), ), + lambda: service.patch_user_policy( + principal=BOB, + country_id="us", + association_id=alice_row.id, + association_input=UserPolicyUpdateInput(policy_id=selected.policy_id), + ), lambda: service.delete_user_policy( principal=BOB, country_id="us", association_id=alice_row.id ), @@ -253,6 +259,145 @@ def test_repointing_re_resolves_or_clears_the_v2_reference(store) -> None: assert legacy_references.calls[-1] == ("us", {"policy": [404]}) +def test_repointing_to_a_v2_policy_keeps_the_saved_row_and_clears_the_v1_reference( + store, +) -> None: + service, selected, legacy_references = store + created = service.create_user_policy(principal=ALICE, association_input=_fields()) + v1_checks_before_repoint = list(legacy_references.calls) + + repointed = service.patch_user_policy( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserPolicyUpdateInput(policy_id=selected.policy_id), + ) + + assert (repointed.policy_id, repointed.legacy_policy_id) == ( + selected.policy_id, + None, + ) + assert (repointed.id, repointed.name, repointed.description) == ( + created.id, + "Saved reform", + "Personal note", + ) + assert repointed.created_at == created.created_at + assert repointed.updated_at >= created.updated_at + assert legacy_references.calls == v1_checks_before_repoint + with selected.sessions() as session: + stored = session.get(UserPolicy, created.id) + assert stored is not None + assert (stored.policy_id, stored.legacy_policy_id) == ( + selected.policy_id, + None, + ) + + +def test_repointing_to_both_references_stores_both(store) -> None: + service, selected, legacy_references = store + created = service.create_user_policy(principal=ALICE, association_input=_fields()) + + mirrored = service.patch_user_policy( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserPolicyUpdateInput( + policy_id=selected.policy_id, + legacy_policy_id=MAPPED_LEGACY_POLICY_ID, + ), + ) + assert (mirrored.policy_id, mirrored.legacy_policy_id) == ( + selected.policy_id, + MAPPED_LEGACY_POLICY_ID, + ) + assert legacy_references.calls[-1] == ( + "us", + {"policy": [MAPPED_LEGACY_POLICY_ID]}, + ) + + # v1 policy 3 has no v2 copy, so nothing can contradict the v2 reference: + # both are stored, as creation stores them. + unmirrored = service.patch_user_policy( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserPolicyUpdateInput( + policy_id=selected.policy_id, legacy_policy_id=3 + ), + ) + assert (unmirrored.policy_id, unmirrored.legacy_policy_id) == ( + selected.policy_id, + 3, + ) + assert unmirrored.id == created.id + + +def test_repointing_to_a_missing_foreign_or_conflicting_policy_changes_nothing( + store, +) -> None: + service, selected, _legacy_references = store + created = service.create_user_policy(principal=ALICE, association_input=_fields()) + with selected.sessions.begin() as session: + mirrored_policy = session.get(Policy, selected.policy_id) + assert mirrored_policy is not None + other_us_policy = Policy( + country_id="us", + tax_benefit_model_id=mirrored_policy.tax_benefit_model_id, + tax_benefit_model_version_id=mirrored_policy.tax_benefit_model_version_id, + canonicalization_version=1, + content_hash="e" * 64, + ) + session.add(other_us_policy) + session.flush() + other_us_policy_id = other_us_policy.id + + rejected_updates = ( + ( + AssociationPolicyNotFoundError, + UserPolicyUpdateInput( + policy_id=UUID("00000000-0000-0000-0000-000000000099"), name="Lost" + ), + ), + ( + AssociationCountryConflictError, + UserPolicyUpdateInput(policy_id=selected.uk_policy_id, name="Lost"), + ), + ( + AssociationReferenceConflictError, + UserPolicyUpdateInput( + policy_id=other_us_policy_id, + legacy_policy_id=MAPPED_LEGACY_POLICY_ID, + name="Lost", + ), + ), + ( + LegacyReferenceNotFoundError, + UserPolicyUpdateInput( + policy_id=selected.policy_id, legacy_policy_id=404, name="Lost" + ), + ), + ) + for expected_error, rejected_update in rejected_updates: + with pytest.raises(expected_error): + service.patch_user_policy( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=rejected_update, + ) + + unchanged = service.get_user_policy( + principal=ALICE, country_id="us", association_id=created.id + ) + assert (unchanged.policy_id, unchanged.legacy_policy_id, unchanged.name) == ( + None, + 1, + "Saved reform", + ) + assert unchanged.updated_at == created.updated_at + + def test_rows_owned_by_the_stage_10_mirror_conflict(store) -> None: service, selected, _legacy_references = store created = service.create_user_policy(principal=ALICE, association_input=_fields()) @@ -274,6 +419,13 @@ def test_rows_owned_by_the_stage_10_mirror_conflict(store) -> None: association_id=created.id, association_input=UserPolicyUpdateInput(name="Renamed"), ) + with pytest.raises(MirrorOwnedAssociationError): + service.patch_user_policy( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserPolicyUpdateInput(policy_id=selected.policy_id), + ) with pytest.raises(MirrorOwnedAssociationError): service.delete_user_policy( principal=ALICE, country_id="us", association_id=created.id @@ -331,13 +483,22 @@ def test_stage_10_projection_records_the_v1_reform_id() -> None: def test_patch_input_requires_a_supported_field() -> None: - with pytest.raises(ValueError, match="At least one of legacy_policy_id"): + with pytest.raises(ValueError, match="At least one of policy_id, legacy_policy_id"): UserPolicyUpdateInput() - with pytest.raises( - ValueError, match=r"policy_id\n Extra inputs are not permitted" - ): + with pytest.raises(ValueError, match=r"user_id\n Extra inputs are not permitted"): UserPolicyUpdateInput.model_validate( - {"policy_id": "00000000-0000-0000-0000-000000000001"} + {"user_id": "00000000-0000-0000-0000-0000000000a1"} ) with pytest.raises(ValueError, match="legacy_policy_id must not be null"): UserPolicyUpdateInput(legacy_policy_id=None) + with pytest.raises(ValueError, match="policy_id must not be null"): + UserPolicyUpdateInput(policy_id=None) + + +def test_patch_input_accepts_a_v2_policy_reference() -> None: + repoint = UserPolicyUpdateInput.model_validate( + {"policy_id": "00000000-0000-0000-0000-000000000001"} + ) + + assert repoint.policy_id == UUID("00000000-0000-0000-0000-000000000001") + assert repoint.model_fields_set == {"policy_id"} From 8be3803281a32549374aa50ac564de245d23e849 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 02:21:42 +0400 Subject: [PATCH 2/3] Let a saved household be repointed to a v2 household PATCH /v2/user-households/{id} now accepts household_id, with the same rules as a saved policy's policy_id: supplied alone, it points the saved household at that v2 household and clears legacy_household_id; supplied with legacy_household_id, both are stored, and household_id must be the v2 copy of that v1 household when one exists. The household must exist in the row's country, and the row keeps its ID, owner, name and creation time. Without this, editing a saved household changed its ID. Co-Authored-By: Claude Opus 5.5 --- ...v2plan-1f-121-repoint-saved-items.added.md | 2 +- docs/migration/stage-12a-user-login.md | 4 + .../v2/user_households/routes.py | 9 +- .../database_connectors/updates.py | 23 ++- .../services/v2/user_households/services.py | 37 ++-- .../services/v2/user_households/types.py | 41 +++- tests/unit/v2/test_user_household_routes.py | 37 +++- tests/unit/v2/test_user_household_service.py | 176 +++++++++++++++++- tests/unit/v2/user_service_support.py | 11 +- 9 files changed, 295 insertions(+), 45 deletions(-) diff --git a/changelog.d/v2plan-1f-121-repoint-saved-items.added.md b/changelog.d/v2plan-1f-121-repoint-saved-items.added.md index fae825693..14b849f5b 100644 --- a/changelog.d/v2plan-1f-121-repoint-saved-items.added.md +++ b/changelog.d/v2plan-1f-121-repoint-saved-items.added.md @@ -1 +1 @@ -PATCH /v2/user-policies/{id} accepts policy_id, so a saved policy can point at another v2 policy in its country while keeping its ID, name and creation time. Supplied alone, policy_id clears the v1 reference (legacy_policy_id); supplied with legacy_policy_id, both are stored, and policy_id must be the v2 copy of that v1 policy when one exists. Unknown, other-country and conflicting policies are refused as on creation, and rows maintained by the v1 saved-policy mirror still return 409. +PATCH /v2/user-policies/{id} accepts policy_id and PATCH /v2/user-households/{id} accepts household_id, so a saved policy or household can point at another v2 record in its country while keeping its ID, name and creation time. Supplied alone, the v2 reference clears the v1 reference (legacy_policy_id or legacy_household_id); supplied with it, both are stored, and the v2 reference must be the v2 copy of that v1 record when one exists. Unknown, other-country and conflicting records are refused as on creation, and saved policies maintained by the v1 saved-policy mirror still return 409. diff --git a/docs/migration/stage-12a-user-login.md b/docs/migration/stage-12a-user-login.md index 62e915479..3fc391392 100644 --- a/docs/migration/stage-12a-user-login.md +++ b/docs/migration/stage-12a-user-login.md @@ -189,6 +189,10 @@ Concurrency on Postgres: ID, name and creation time. Supplied with `legacy_policy_id`, both are stored, and `policy_id` must be the mirror copy of that v1 policy when one exists. + - `PATCH /v2/user-households` accepts `household_id` the same way: it must + name a v2 household in the row's country; alone, it clears + `legacy_household_id`; with `legacy_household_id`, both are stored, and + it must be the copy of that v1 household when one exists. - `user-simulations` and `user-reports` accept only legacy IDs in 12A. - **Stage 10 mirror rows.** A `user_policies` row with a `legacy_user_policy_mappings` entry returns 409 on PATCH and DELETE. The diff --git a/policyengine_api/fastapi_routes/v2/user_households/routes.py b/policyengine_api/fastapi_routes/v2/user_households/routes.py index c642a79d8..4e86c9b86 100644 --- a/policyengine_api/fastapi_routes/v2/user_households/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_households/routes.py @@ -179,8 +179,13 @@ def read() -> UserHouseholdPageResponse: response_model=UserHouseholdDetailResponse, summary="Update or repoint one of the current user's saved households", description=( - "Changing legacy_household_id repoints the row and re-resolves " - "household_id from the Stage 11 copy mapping, or clears it." + "The row keeps its id, owner and creation time. Changing " + "legacy_household_id repoints the row and re-resolves household_id " + "from the Stage 11 copy mapping, or clears it. Changing household_id " + "alone points the row at that v2 household, which must exist in the " + "same country, and clears legacy_household_id. Supplied together, both " + "are stored, and household_id must be the copy of legacy_household_id " + "when one exists." ), ) def patch_user_household_route( diff --git a/policyengine_api/services/v2/user_households/database_connectors/updates.py b/policyengine_api/services/v2/user_households/database_connectors/updates.py index ed6cfb989..e0de95d8e 100644 --- a/policyengine_api/services/v2/user_households/database_connectors/updates.py +++ b/policyengine_api/services/v2/user_households/database_connectors/updates.py @@ -5,14 +5,16 @@ from typing import TYPE_CHECKING from policyengine_api.data.v2.models.base import utc_now +from policyengine_api.services.v2.user_households.types import ( + USER_HOUSEHOLD_REFERENCE_FIELDS, +) if TYPE_CHECKING: - from uuid import UUID - from sqlmodel import Session from policyengine_api.data.v2.models import UserHouseholdAssociation from policyengine_api.services.v2.user_households.types import ( + UserHouseholdReferences, UserHouseholdUpdateInput, ) @@ -22,17 +24,20 @@ def update_user_household( association: UserHouseholdAssociation, association_input: UserHouseholdUpdateInput, *, - repointed_household_id: UUID | None = None, - repoint: bool = False, + references: UserHouseholdReferences | None = None, ) -> UserHouseholdAssociation: - """Apply the supplied fields, bump ``updated_at`` and return the row. + """Apply the supplied presentation fields, bump ``updated_at`` and return the row. - With ``repoint``, also set ``household_id`` to ``repointed_household_id``. + With ``references``, also store both of its household references; the + service resolves them, so the request's own reference fields are never copied. """ - for field_name, value in association_input.model_dump(exclude_unset=True).items(): + for field_name, value in association_input.model_dump( + exclude_unset=True, exclude=set(USER_HOUSEHOLD_REFERENCE_FIELDS) + ).items(): setattr(association, field_name, value) - if repoint: - association.household_id = repointed_household_id + if references is not None: + association.household_id = references.household_id + association.legacy_household_id = references.legacy_household_id association.updated_at = utc_now() session.add(association) session.flush() diff --git a/policyengine_api/services/v2/user_households/services.py b/policyengine_api/services/v2/user_households/services.py index a2630e897..e2c791b76 100644 --- a/policyengine_api/services/v2/user_households/services.py +++ b/policyengine_api/services/v2/user_households/services.py @@ -42,6 +42,7 @@ from policyengine_api.services.v2.user_households.types import ( UserHouseholdFilter, UserHouseholdListing, + UserHouseholdReferences, ) from policyengine_api.services.v2.user_households.validators import ( require_user_household, @@ -267,12 +268,17 @@ def patch_user_household( ) -> UserHouseholdRead: """Update the supplied fields of the principal's association in this country. - A new ``legacy_household_id`` must exist in v1 and repoints ``household_id`` - to the v2 copy of that household, or to None when there is no copy. Raise - UserHouseholdNotFoundError when the association is not the principal's. + The association keeps its id, owner, country and creation time. A new + ``legacy_household_id`` must exist in v1 (LegacyReferenceNotFoundError) and + repoints ``household_id`` to the v2 copy of that household, or to None when + there is no copy. A new ``household_id`` must exist in this country + (AssociationHouseholdNotFoundError or AssociationCountryConflictError). + Without ``legacy_household_id`` it clears the v1 reference; with it, both + are stored, and it must be the v2 copy of that v1 household when one exists + (AssociationReferenceConflictError). Raise UserHouseholdNotFoundError when + the association is not the principal's. """ - repoint = "legacy_household_id" in association_input.model_fields_set - if repoint: + if "legacy_household_id" in association_input.model_fields_set: if association_input.legacy_household_id is None: msg = "legacy_household_id must not be null" raise TypeError(msg) @@ -290,13 +296,17 @@ def patch_user_household( country_id=country_id, association_id=association_id, ) - repointed_household_id = ( - read_copied_household_ids( - session, - country_id=country_id, - legacy_household_ids=(association_input.legacy_household_id,), - ).get(association_input.legacy_household_id) - if repoint and association_input.legacy_household_id is not None + references = ( + UserHouseholdReferences( + household_id=resolve_user_household_reference( + session, + country_id=country_id, + household_id=association_input.household_id, + legacy_household_id=association_input.legacy_household_id, + ), + legacy_household_id=association_input.legacy_household_id, + ) + if association_input.repoints else None ) return user_household_read( @@ -304,8 +314,7 @@ def patch_user_household( session, association, association_input, - repointed_household_id=repointed_household_id, - repoint=repoint, + references=references, ) ) diff --git a/policyengine_api/services/v2/user_households/types.py b/policyengine_api/services/v2/user_households/types.py index 599c1e35d..40309e868 100644 --- a/policyengine_api/services/v2/user_households/types.py +++ b/policyengine_api/services/v2/user_households/types.py @@ -61,33 +61,56 @@ def require_household_reference(self) -> UserHouseholdCreationInput: return self +USER_HOUSEHOLD_REFERENCE_FIELDS = frozenset({"household_id", "legacy_household_id"}) + + class UserHouseholdUpdateInput(StrictUserHouseholdInput): """A partial update of a saved household; only supplied fields are changed. - At least one field must be supplied. ``legacy_household_id`` may point the - association at another v1 household but cannot be cleared. + At least one field must be supplied. ``household_id`` points the association + at another v2 household; supplied without ``legacy_household_id``, it also + clears the v1 reference, which no longer describes the saved household. + ``legacy_household_id`` alone points the association at another v1 household + and takes ``household_id`` from that household's v2 copy, or clears it when + there is none. Supplied together, both are stored, and ``household_id`` must + be that v2 copy when one exists. Neither reference can be set to null. """ + household_id: ResourceId | None = None legacy_household_id: LegacyRecordId | None = None name: AssociationName | None = None description: AssociationDescription | None = None @model_validator(mode="after") def require_valid_supplied_field(self) -> UserHouseholdUpdateInput: - """Require a supplied field, and reject an explicit null legacy_household_id.""" - mutable_fields = {"legacy_household_id", "name", "description"} + """Require a supplied field, and reject an explicit null household reference.""" + mutable_fields = {*USER_HOUSEHOLD_REFERENCE_FIELDS, "name", "description"} supplied = self.model_fields_set.intersection(mutable_fields) if not supplied: msg = ( - "At least one of legacy_household_id, name, or description must be " - "supplied" + "At least one of household_id, legacy_household_id, name, or " + "description must be supplied" ) raise ValueError(msg) - if "legacy_household_id" in supplied and self.legacy_household_id is None: - msg = "legacy_household_id must not be null" - raise ValueError(msg) + for reference_field in sorted(USER_HOUSEHOLD_REFERENCE_FIELDS): + if reference_field in supplied and getattr(self, reference_field) is None: + msg = f"{reference_field} must not be null" + raise ValueError(msg) return self + @property + def repoints(self) -> bool: + """Whether the update changes which household the association references.""" + return bool(self.model_fields_set & USER_HOUSEHOLD_REFERENCE_FIELDS) + + +@dataclass(frozen=True) +class UserHouseholdReferences: + """The v2 and v1 household references a repointed association stores.""" + + household_id: UUID | None + legacy_household_id: int | None + @dataclass(frozen=True) class UserHouseholdRead: diff --git a/tests/unit/v2/test_user_household_routes.py b/tests/unit/v2/test_user_household_routes.py index b9bf0df75..2e1553bcb 100644 --- a/tests/unit/v2/test_user_household_routes.py +++ b/tests/unit/v2/test_user_household_routes.py @@ -141,17 +141,26 @@ def test_list_and_patch_use_the_session_user_and_legacy_fields() -> None: f"/v2/user-households/{ASSOCIATION_ID}?country_id=us", json={"legacy_household_id": 124}, ) - reassigned = client.patch( - f"/v2/user-households/{ASSOCIATION_ID}?country_id=us", - json={"household_id": str(HOUSEHOLD_ID)}, - ) user_filter = client.get(f"/v2/user-households?country_id=us&user_id={OWNER_ID}") + invalid_patches = [ + client.patch( + f"/v2/user-households/{ASSOCIATION_ID}?country_id=us", + json=body, + ) + for body in ( + {"household_id": None}, + {"household_id": "not-a-uuid"}, + {"user_id": str(OWNER_ID)}, + {"legacy_household_id": None}, + {}, + ) + ] assert page.status_code == 200 assert page.json()["result"]["has_more"] is True assert patched.status_code == 200 - assert reassigned.status_code == 422 assert user_filter.status_code == 422 + assert [response.status_code for response in invalid_patches] == [422] * 5 assert service.calls[0] == ( "list_user_households", { @@ -168,6 +177,22 @@ def test_list_and_patch_use_the_session_user_and_legacy_fields() -> None: assert len(service.calls) == 2 +def test_patch_repoints_to_a_v2_household_reference() -> None: + client, service, flask_calls = _client() + + repointed = client.patch( + f"/v2/user-households/{ASSOCIATION_ID}?country_id=us", + json={"household_id": str(HOUSEHOLD_ID)}, + ) + + assert repointed.status_code == 200 + assert service.calls[0][0] == "patch_user_household" + association_input = service.calls[0][1]["association_input"] + assert association_input.household_id == HOUSEHOLD_ID + assert association_input.model_fields_set == {"household_id"} + assert flask_calls["count"] == 0 + + @pytest.mark.parametrize( ("error", "status"), [ @@ -241,7 +266,9 @@ def test_openapi_publishes_signed_in_household_contract() -> None: patch = schema["paths"]["/v2/user-households/{association_id}"]["patch"] patch_ref = patch["requestBody"]["content"]["application/json"]["schema"]["$ref"] patch_schema = schema["components"]["schemas"][patch_ref.rsplit("/", 1)[-1]] + assert patch_schema["additionalProperties"] is False assert set(patch_schema["properties"]) == { + "household_id", "legacy_household_id", "name", "description", diff --git a/tests/unit/v2/test_user_household_service.py b/tests/unit/v2/test_user_household_service.py index 944018c53..e084c4df6 100644 --- a/tests/unit/v2/test_user_household_service.py +++ b/tests/unit/v2/test_user_household_service.py @@ -2,10 +2,15 @@ from __future__ import annotations +from uuid import UUID + import pytest from sqlmodel import select from policyengine_api.data.v2.models import Household, UserHouseholdAssociation +from policyengine_api.services.v2.associations.validators import ( + AssociationReferenceConflictError, +) from policyengine_api.services.v2.legacy_refs.validators import ( LegacyReferenceNotFoundError, ) @@ -20,6 +25,7 @@ UserHouseholdUpdateInput, ) from policyengine_api.services.v2.user_households.validators import ( + AssociationCountryConflictError, AssociationHouseholdNotFoundError, UserHouseholdNotFoundError, ) @@ -101,6 +107,15 @@ def test_ownership_repointing_and_deletion(store) -> None: service.delete_user_household( principal=BOB, country_id="us", association_id=created.id ) + with pytest.raises(UserHouseholdNotFoundError): + service.patch_user_household( + principal=BOB, + country_id="us", + association_id=created.id, + association_input=UserHouseholdUpdateInput( + household_id=selected.household_id + ), + ) assert service.list_user_households(principal=BOB, country_id="us").items == () repointed = service.patch_user_household( @@ -132,14 +147,167 @@ def test_ownership_repointing_and_deletion(store) -> None: assert session.get(Household, selected.household_id) is not None +def test_repointing_to_a_v2_household_keeps_the_saved_row_and_clears_the_v1_reference( + store, +) -> None: + service, selected = store + created = service.create_user_household( + principal=ALICE, association_input=_fields(name="Family", description="Note") + ) + + repointed = service.patch_user_household( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserHouseholdUpdateInput(household_id=selected.household_id), + ) + + assert (repointed.household_id, repointed.legacy_household_id) == ( + selected.household_id, + None, + ) + assert (repointed.id, repointed.name, repointed.description) == ( + created.id, + "Family", + "Note", + ) + assert repointed.created_at == created.created_at + assert repointed.updated_at >= created.updated_at + with selected.sessions() as session: + stored = session.get(UserHouseholdAssociation, created.id) + assert stored is not None + assert (stored.household_id, stored.legacy_household_id) == ( + selected.household_id, + None, + ) + + +def test_repointing_to_both_household_references_stores_both(store) -> None: + service, selected = store + created = service.create_user_household( + principal=ALICE, association_input=_fields() + ) + + copied = service.patch_user_household( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserHouseholdUpdateInput( + household_id=selected.household_id, + legacy_household_id=MAPPED_LEGACY_HOUSEHOLD_ID, + ), + ) + assert (copied.household_id, copied.legacy_household_id) == ( + selected.household_id, + MAPPED_LEGACY_HOUSEHOLD_ID, + ) + + # v1 household 11 has no v2 copy, so nothing can contradict the v2 + # reference: both are stored, as creation stores them. + uncopied = service.patch_user_household( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserHouseholdUpdateInput( + household_id=selected.household_id, legacy_household_id=11 + ), + ) + assert (uncopied.household_id, uncopied.legacy_household_id) == ( + selected.household_id, + 11, + ) + assert uncopied.id == created.id + + +def test_repointing_to_a_missing_foreign_or_conflicting_household_changes_nothing( + store, +) -> None: + service, selected = store + created = service.create_user_household( + principal=ALICE, association_input=_fields(name="Family") + ) + with selected.sessions.begin() as session: + other_us_household = Household( + country_id="us", + default_year=2026, + household_data={"people": [], "household": []}, + canonicalization_version=1, + content_hash="e" * 64, + ) + session.add(other_us_household) + session.flush() + other_us_household_id = other_us_household.id + + rejected_updates = ( + ( + AssociationHouseholdNotFoundError, + UserHouseholdUpdateInput( + household_id=UUID("00000000-0000-0000-0000-000000000099"), + name="Lost", + ), + ), + ( + AssociationCountryConflictError, + UserHouseholdUpdateInput( + household_id=selected.uk_household_id, name="Lost" + ), + ), + ( + AssociationReferenceConflictError, + UserHouseholdUpdateInput( + household_id=other_us_household_id, + legacy_household_id=MAPPED_LEGACY_HOUSEHOLD_ID, + name="Lost", + ), + ), + ( + LegacyReferenceNotFoundError, + UserHouseholdUpdateInput( + household_id=selected.household_id, + legacy_household_id=404, + name="Lost", + ), + ), + ) + for expected_error, rejected_update in rejected_updates: + with pytest.raises(expected_error): + service.patch_user_household( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=rejected_update, + ) + + unchanged = service.get_user_household( + principal=ALICE, country_id="us", association_id=created.id + ) + assert ( + unchanged.household_id, + unchanged.legacy_household_id, + unchanged.name, + ) == (None, 10, "Family") + assert unchanged.updated_at == created.updated_at + + def test_patch_input_rejects_reassignment_and_empty_changes() -> None: - with pytest.raises(ValueError, match="At least one of legacy_household_id"): - UserHouseholdUpdateInput() with pytest.raises( - ValueError, match=r"household_id\n Extra inputs are not permitted" + ValueError, match="At least one of household_id, legacy_household_id" ): + UserHouseholdUpdateInput() + with pytest.raises(ValueError, match=r"user_id\n Extra inputs are not permitted"): UserHouseholdUpdateInput.model_validate( - {"household_id": "00000000-0000-0000-0000-000000000001"} + {"user_id": "00000000-0000-0000-0000-0000000000a1"} ) with pytest.raises(ValueError, match="legacy_household_id must not be null"): UserHouseholdUpdateInput(legacy_household_id=None) + with pytest.raises(ValueError, match="household_id must not be null"): + UserHouseholdUpdateInput(household_id=None) + + +def test_patch_input_accepts_a_v2_household_reference() -> None: + repoint = UserHouseholdUpdateInput.model_validate( + {"household_id": "00000000-0000-0000-0000-000000000001"} + ) + + assert repoint.household_id == UUID("00000000-0000-0000-0000-000000000001") + assert repoint.model_fields_set == {"household_id"} diff --git a/tests/unit/v2/user_service_support.py b/tests/unit/v2/user_service_support.py index ce54237e3..61f0f3f4b 100644 --- a/tests/unit/v2/user_service_support.py +++ b/tests/unit/v2/user_service_support.py @@ -106,6 +106,7 @@ class V2Store: policy_id: UUID uk_policy_id: UUID household_id: UUID + uk_household_id: UUID @contextmanager @@ -166,7 +167,14 @@ def begin_transaction(connection) -> None: canonicalization_version=1, content_hash="c" * 64, ) - session.add_all([value, uk_policy, household]) + uk_household = Household( + country_id="uk", + default_year=2026, + household_data={"people": [], "household": []}, + canonicalization_version=1, + content_hash="f" * 64, + ) + session.add_all([value, uk_policy, household, uk_household]) session.flush() session.add_all( [ @@ -192,6 +200,7 @@ def begin_transaction(connection) -> None: policy_id=policy.id, uk_policy_id=uk_policy.id, household_id=household.id, + uk_household_id=uk_household.id, ) try: yield store From 15f15aaa961401da2cba8948bf3f50dc3c4c3d5c Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 02:24:27 +0400 Subject: [PATCH 3/3] Pin the status codes of a refused household repoint A household repoint can now fail with a reference conflict, and the household route tests sent only DELETE requests through the failure mapping. Send a repointing PATCH through it and pin each refusal's status: 400 for a country or reference conflict, 404 for a missing household or saved household, and 422 for a missing v1 household. Co-Authored-By: Claude Opus 5.5 --- tests/unit/v2/test_user_household_routes.py | 34 +++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/unit/v2/test_user_household_routes.py b/tests/unit/v2/test_user_household_routes.py index 2e1553bcb..3f92084e4 100644 --- a/tests/unit/v2/test_user_household_routes.py +++ b/tests/unit/v2/test_user_household_routes.py @@ -9,6 +9,9 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from policyengine_api.data.v2.settings import V2ConfigurationError +from policyengine_api.services.v2.associations.validators import ( + AssociationReferenceConflictError, +) from policyengine_api.services.v2.legacy_refs.validators import ( LegacyReferenceNotFoundError, ) @@ -221,6 +224,37 @@ def test_failures_map_to_secret_safe_typed_errors( assert "credential" not in response.text +@pytest.mark.parametrize( + ("error", "status", "message"), + [ + (AssociationCountryConflictError("different country"), 400, "country"), + ( + AssociationReferenceConflictError("household_id mismatch"), + 400, + "mismatch", + ), + (AssociationHouseholdNotFoundError("household was not found"), 404, "not"), + (UserHouseholdNotFoundError("association was not found"), 404, "not"), + (LegacyReferenceNotFoundError("v1 household 9 was not found"), 422, "v1"), + ], +) +def test_repoint_failures_map_to_typed_errors( + error: Exception, status: int, message: str +) -> None: + service = RecordingService(_result) + service.error = error + client, _service, _flask_calls = _client(service) + + response = client.patch( + f"/v2/user-households/{ASSOCIATION_ID}?country_id=us", + json={"household_id": str(HOUSEHOLD_ID), "legacy_household_id": 9}, + ) + + assert response.status_code == status + assert response.json()["status"] == "error" + assert message in response.json()["message"].lower() + + def test_patch_preflight_and_native_routes_avoid_cloud_sql_and_flask( monkeypatch, ) -> None: