diff --git a/changelog.d/apiv2-1e-user-policy-repoint.added.md b/changelog.d/apiv2-1e-user-policy-repoint.added.md new file mode 100644 index 000000000..1be1c0689 --- /dev/null +++ b/changelog.d/apiv2-1e-user-policy-repoint.added.md @@ -0,0 +1 @@ +PATCH /v2/user-policies/{id} accepts policy_id, so a saved policy can be pointed at another v2 policy without changing its ID. 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 the Stage 10 mirror made one. Unknown, other-country and conflicting policies are refused as on creation. diff --git a/docs/migration/stage-12a-user-login.md b/docs/migration/stage-12a-user-login.md index c01657bc3..e98c31008 100644 --- a/docs/migration/stage-12a-user-login.md +++ b/docs/migration/stage-12a-user-login.md @@ -183,6 +183,11 @@ 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 + policy ID. 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 d9c360585..5e0b9e286 100644 --- a/policyengine_api/fastapi_routes/v2/user_policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_policies/routes.py @@ -159,8 +159,11 @@ def read() -> UserPolicyPageResponse: 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." + "from the Stage 10 mirror mapping, or clears it. Changing policy_id " + "alone points the row at that v2 policy 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 151ac8499..b7771360c 100644 --- a/policyengine_api/services/v2/user_policies/database_connectors/updates.py +++ b/policyengine_api/services/v2/user_policies/database_connectors/updates.py @@ -7,12 +7,15 @@ from policyengine_api.data.v2.models.base import utc_now 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 UserPolicyUpdateInput + from policyengine_api.services.v2.user_policies.types import ( + UserPolicyReferences, + UserPolicyUpdateInput, + ) + +POLICY_REFERENCE_FIELDS = frozenset({"policy_id", "legacy_policy_id"}) def update_user_policy( @@ -20,17 +23,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(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 508819a83..231605ac6 100644 --- a/policyengine_api/services/v2/user_policies/services.py +++ b/policyengine_api/services/v2/user_policies/services.py @@ -61,6 +61,7 @@ UserPolicyCreationInput, UserPolicyPage, UserPolicyRead, + UserPolicyReferences, UserPolicyUpdateInput, ) from policyengine_api.services.v2.user_policies.validators import ( @@ -533,12 +534,16 @@ def patch_user_policy( """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 + 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: + supplied = association_input.model_fields_set + if "legacy_policy_id" in supplied: if association_input.legacy_policy_id is None: msg = "legacy_policy_id must not be null" raise TypeError(msg) @@ -560,13 +565,17 @@ def patch_user_policy( read_user_policy_mirror_exists(session, association_id=association.id), 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( @@ -574,8 +583,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 5edd99555..3662cecd9 100644 --- a/policyengine_api/services/v2/user_policies/types.py +++ b/policyengine_api/services/v2/user_policies/types.py @@ -85,10 +85,16 @@ def require_policy_reference(self) -> UserPolicyCreationInput: 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. Supplied together, both are stored, and ``policy_id`` + must be that v2 copy when the Stage 10 mirror made one. 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 +103,44 @@ 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"} + { + "policy_id", + "legacy_policy_id", + "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 in ("policy_id", "legacy_policy_id"): + if reference in supplied and getattr(self, reference) is None: + msg = f"{reference} 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.intersection({"policy_id", "legacy_policy_id"}) + ) + + +@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 0130d84dc..fd3cca6c4 100644 --- a/tests/unit/v2/test_user_policy_routes.py +++ b/tests/unit/v2/test_user_policy_routes.py @@ -248,7 +248,7 @@ def test_patch_repoints_legacy_reference_and_rejects_invalid_changes() -> None: json=body, ) for body in ( - {"policy_id": str(POLICY_ID)}, + {"policy_id": None}, {"user_id": str(OWNER_ID)}, {"client_ref": "rf-2"}, {"legacy_policy_id": None}, @@ -265,6 +265,21 @@ def test_patch_repoints_legacy_reference_and_rejects_invalid_changes() -> None: 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 + 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() @@ -381,6 +396,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 be1a98464..d9c7e3d1b 100644 --- a/tests/unit/v2/test_user_policy_service.py +++ b/tests/unit/v2/test_user_policy_service.py @@ -249,6 +249,104 @@ 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_replaces_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.name == "Saved reform" + 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_keeps_both_when_they_agree(store) -> None: + service, selected, _legacy_references = store + created = service.create_user_policy(principal=ALICE, association_input=_fields()) + + repointed = 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 (repointed.policy_id, repointed.legacy_policy_id) == ( + selected.policy_id, + MAPPED_LEGACY_POLICY_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()) + missing_policy_id = UUID("00000000-0000-0000-0000-000000000099") + + with pytest.raises(AssociationPolicyNotFoundError): + service.patch_user_policy( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserPolicyUpdateInput(policy_id=missing_policy_id), + ) + with pytest.raises(AssociationCountryConflictError): + service.patch_user_policy( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserPolicyUpdateInput(policy_id=selected.uk_policy_id), + ) + with selected.sessions.begin() as session: + mapped_policy = session.get(Policy, selected.policy_id) + assert mapped_policy is not None + other_us_policy = Policy( + country_id="us", + tax_benefit_model_id=mapped_policy.tax_benefit_model_id, + tax_benefit_model_version_id=mapped_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 + with pytest.raises(AssociationReferenceConflictError): + service.patch_user_policy( + principal=ALICE, + country_id="us", + association_id=created.id, + association_input=UserPolicyUpdateInput( + policy_id=other_us_policy_id, + legacy_policy_id=MAPPED_LEGACY_POLICY_ID, + ), + ) + + unchanged = service.get_user_policy( + principal=ALICE, country_id="us", association_id=created.id + ) + assert (unchanged.policy_id, unchanged.legacy_policy_id) == (None, 1) + + 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()) @@ -331,7 +429,18 @@ def test_patch_input_requires_a_supported_field() -> None: UserPolicyUpdateInput() with pytest.raises(ValueError): UserPolicyUpdateInput.model_validate( - {"policy_id": "00000000-0000-0000-0000-000000000001"} + {"user_id": "00000000-0000-0000-0000-0000000000a1"} ) with pytest.raises(ValueError): UserPolicyUpdateInput(legacy_policy_id=None) + with pytest.raises(ValueError): + 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"}