Filed at origin/19.0 (23f43963). Noticed during the expert review of #477.
Summary
spp.change.request.type.target_type — "Individual" / "Group/Household" / "Both" (spp_change_request_v2/models/change_request_type.py:70-79) — is enforced in two UI-only places:
_onchange_request_type_id clears a mismatched registrant when the type changes (models/change_request.py:613-639)_compute_registrant_domain narrows the registrant picker (models/change_request.py:486-502)
There is no @api.constrains and no create/write validation, so every non-form path accepts an individual-only type with a group registrant, or vice versa:
- ORM / RPC
create()
- data import
- the API —
ChangeRequestService.create_change_request resolves the type and the registrant independently and creates the CR with no target-type check (spp_api_v2_change_request/services/change_request_service.py:260-302)
Impact
The mismatch is not caught anywhere downstream either, so such a CR proceeds through routing and apply with a registrant its strategy was not written for. Severity depends on the strategy — some branch on is_group, some do not — but the invariant the configuration is meant to express ("this request type applies to individuals only") is not enforced as one today.
It also shows up in the test suite: test_group_scope_group_registrant (tests/test_conflict_detection_extended.py:555) pairs the shared target_type = "individual" fixture type with a group registrant, and create() accepts it. That is fine as a unit test of _get_group_member_ids, but it exercises a configuration the UI does not allow a user to reach.
Suggested fix
An @api.constrains("request_type_id", "registrant_id") on spp.change.request mirroring the onchange:
target_type == "individual" and registrant_id.is_group → ValidationErrortarget_type == "group" and not registrant_id.is_group → ValidationError
with tests for the ORM path and the API path, plus a fixture fix in test_group_scope_group_registrant (use a target_type = "group" or "both" type) so the suite stops depending on the gap.
Worth checking demo/existing data before adding the constraint — any already-stored mismatch would make those records unwritable.
Filed at
origin/19.0(23f43963). Noticed during the expert review of #477.Summary
spp.change.request.type.target_type— "Individual" / "Group/Household" / "Both" (spp_change_request_v2/models/change_request_type.py:70-79) — is enforced in two UI-only places:_onchange_request_type_idclears a mismatched registrant when the type changes (models/change_request.py:613-639)_compute_registrant_domainnarrows the registrant picker (models/change_request.py:486-502)There is no
@api.constrainsand no create/write validation, so every non-form path accepts an individual-only type with a group registrant, or vice versa:create()ChangeRequestService.create_change_requestresolves the type and the registrant independently and creates the CR with no target-type check (spp_api_v2_change_request/services/change_request_service.py:260-302)Impact
The mismatch is not caught anywhere downstream either, so such a CR proceeds through routing and apply with a registrant its strategy was not written for. Severity depends on the strategy — some branch on
is_group, some do not — but the invariant the configuration is meant to express ("this request type applies to individuals only") is not enforced as one today.It also shows up in the test suite:
test_group_scope_group_registrant(tests/test_conflict_detection_extended.py:555) pairs the sharedtarget_type = "individual"fixture type with a group registrant, andcreate()accepts it. That is fine as a unit test of_get_group_member_ids, but it exercises a configuration the UI does not allow a user to reach.Suggested fix
An
@api.constrains("request_type_id", "registrant_id")onspp.change.requestmirroring the onchange:target_type == "individual"andregistrant_id.is_group→ValidationErrortarget_type == "group"and notregistrant_id.is_group→ValidationErrorwith tests for the ORM path and the API path, plus a fixture fix in
test_group_scope_group_registrant(use atarget_type = "group"or"both"type) so the suite stops depending on the gap.Worth checking demo/existing data before adding the constraint — any already-stored mismatch would make those records unwritable.