feat: DCI module hardening and DCI-to-CEL integration#221
Merged
Conversation
Brings the previously-uncommitted spp_dci_client_sr, spp_dci_client_compliance,
spp_dci_compliance, spp_dci_indicators, and spp_dci_server_social modules
into the branch alongside a fix for sr_sender's signature verifier path.
sr_sender previously:
- exposed algorithm selection keys 'Ed25519' / 'RSA-SHA256' / 'ES256' that did
not match the lowercase identifiers DCIVerifier and the sibling sender
models expect, so signature verification always failed;
- imported DCIVerifier from a non-existent 'odoo.addons.spp_dci.crypto' path,
so get_verifier() raised ImportError;
- passed the PEM public key as str rather than bytes, so even with the right
import DCIVerifier rejected it.
Aligns the algorithm enum and the verifier call site with the crvs/dr/ibr/
server-side senders. Adds a pre-migration that normalises existing rows
('Ed25519' -> 'ed25519', 'RSA-SHA256' -> 'rs256', 'ES256' -> 'ed25519' with a
warning since the verifier does not implement ECDSA). Adds tests exercising
the real DCIVerifier (the previous test patched the broken path and never
reached the production code) and a regression test pinning the enum to match
siblings.
Also adds jsonschema to requirements for upcoming SPDCI conformance work.
…arer token
The trigger controller previously fell back to a hardcoded shared secret
('compliance-test-api-key-12345') for its bearer token and exposed five
auth='none' routes the moment the module was installed. An accidental
install in production would therefore publish an unauthenticated DCI
client surface backed by a well-known credential.
Two safeguards:
- _compliance_enabled(): every route returns 404 unless test_enable is on
or dci.client_compliance.enabled is explicitly 'true'.
- _get_compliance_bearer_token(): raises UserError if
dci.client_compliance.bearer_token is unset; no built-in default.
The data file no longer pre-creates the test data source (it would have
required the missing secret to install) and no longer presets the bearer
token. A post-migration deletes the old well-known token from existing
installs so an upgrade does not silently keep the shared secret.
Replaces the two-method smoke test suite with seven tests: HTTP-level
checks under the gate-on path plus unit tests for the gate and bearer
helpers, including the fail-closed default and missing-token path.
The five DCI cache and log views (sr_record, disability_status, crvs_event, duplication_check, transaction) previously displayed raw envelopes, full identifiers, and disability data to any user with model read access. The underlying model ACLs are correct but view-level gating provides defence-in-depth: if an admin later grants broader read on the model the PII does not follow. Introduces spp_dci.group_dci_admin (implies base.group_system) and applies groups="spp_dci.group_dci_admin" to: - sr_record: sr_name (list), identifier_value, demographics group, address group, enrolled_programs, raw_data page - disability_status: has_disability, disability_types (list + form), disability_info page, raw_data page - crvs_event: person_id, identifier_value (list + form), raw_data page - duplication_check: identifier_value (list + form), matched_programs page, raw_response page - transaction: request_payload page, response_payload page Per-module regression tests parse view arch_db and assert the gating attribute is present on the expected fields and pages, so a future view edit cannot silently drop the protection. Also adds an integrity test for the new group itself.
Previously verify_bearer_token treated an empty 'dci.api_tokens' system parameter as "accept any non-empty token", so every bearer-authenticated DCI route was effectively unauthenticated until an operator remembered to populate the parameter. The four bypass flags already defaulted to 'false', but this one fail-open default did the same damage on its own. Behaviour change: an empty token list now rejects with err.auth.no_tokens_configured. Operators who need the old behaviour for development must set 'dci.api_tokens_required=false' explicitly, which triggers a CRITICAL log warning identical to the existing 'dci.bypass_bearer_auth' code path. A post-migration logs a CRITICAL warning when an upgraded deployment has neither configured tokens nor opted out, so the silent change is visible at upgrade time. Centralises every dev-mode flag in _SECURITY_FLAG_DEFAULTS via a single _read_security_flag helper, so a regression test in TestSecurityDefaults can pin every safe default. Adds eleven tests covering the new fail-closed path, the explicit opt-out, configured tokens, the bypass sentinel, and the existing header-parsing error paths (which had no test coverage at all).
…None mismatches The two test files lived on disk but were never imported into the package, so 12 tests had been silently dormant since the server module was written. Wiring them surfaced three real bugs in the test code: - test_notification_log_has_receipt_fields used assertIsNone on Odoo Datetime/Char fields; the ORM returns False for unset values, never None. - test_notification_log_receipt_update and test_receipt_update_marks_received passed datetime.now(UTC) (tz-aware) to a Datetime write, which Odoo rejects. Replaced with fields.Datetime.now(). After the fixes, spp_dci_server runs 131 post_install tests (up from 119) with 0 failures.
verify_bearer_token used 'token not in accepted_tokens', which uses Python's '==' under the hood and short-circuits on the first mismatched byte. Response latency would therefore leak how many leading bytes of the supplied token matched a configured one, letting an attacker recover the accepted token byte by byte. Replaced with an explicit loop calling hmac.compare_digest() once per configured candidate, accumulating the result into a 'matched' flag so the loop runs in constant time regardless of where (if anywhere) the match occurs. A regression test patches hmac.compare_digest with wraps= to assert that the loop ran once per configured token, pinning the constant-time invariant. Also annotates the legitimate config-flag string compare on 'dci.api_tokens_required' with nosemgrep so the same semgrep rule does not keep flagging it.
The test suite for spp_dci_client_sr had 11 erroring tests because
test_sr_service.py patched odoo.addons.spp_dci_client.services.dci_client,
a non-existent module path. Following the trail uncovered two real
production bugs in SRService that the broken tests had been hiding:
- SRService.__init__ called DCIClient(env, data_source_code) - both args
reversed and the second arg passed as a string code instead of the
resolved data.source record. DCIClient signature is (data_source, env);
crvs/dr/ibr services do it correctly.
- Every search/subscribe call passed reg_type=... but DCIClient.search
and search_async accept registry_type=. The kwarg has been wrong since
the file was written.
After these fixes, eight tests still fail because their mock responses
return bare {"data": {"reg_records": [...]}} instead of the DCI envelope
shape DCIClient actually returns. That is a separate test-fixture
rewrite tracked outside this commit. The error count drops from 11 to
8 and the three previously-uncovered SRSender tests run cleanly.
…usage Annotate each .sudo() call in the trigger controller with a nosemgrep directive and an inline rationale so the semgrep odoo-sudo-without-context rule stops flagging them on every change. The controller serves auth='none' HTTP routes (because the test framework calling them is anonymous), so request.env is the public user, which has no read on ir.config_parameter or spp.dci.data.source. The audit boundary is _compliance_enabled(), which gates every route on test_enable or the explicit dci.client_compliance.enabled flag. No behaviour change.
…cation tests
Two related issues uncovered while triaging the 24 failures/errors in
this module's test suite.
Production bug in _execute_dci_notification:
Subscription = self.env.get("spp.dci.subscription")
if not Subscription: return # always trips!
env.get(name) returns an empty recordset when the model IS registered
(Mapping.get falls back to __getitem__, which returns an empty
recordset of the model). Empty recordsets are *falsy*, so the truthy
check silently aborted even when the model was available. Replaced
with explicit membership: ``if "spp.dci.subscription" not in self.env``.
Test infrastructure:
_schedule_dci_notification queues its callback via env.cr.postcommit.
TransactionCase never commits (it rolls back via savepoint), so the
callback never fired and assert_called_once raised. Tests now drain
the queue with env.cr.postcommit.run() at the assertion site and
with .clear() between setUpClass-time fixtures and per-test mocks
to avoid double counting.
Notification suite: 7 -> 1 failures.
Still failing:
- test_unlink_triggers_delete_notification: cascading unlink through
spp_service_points + spp_cel_domain triggers a second
_queue_dci_notification_job call. Needs scoped hook isolation.
- test_search_service (17 cases): independent root cause around
search-domain construction / test ACL setup, untouched here.
… envelope shape
The mocks returned bare {"data": {"reg_records": [...]}}, but
DCIClient.search/.search_async return the full DCI envelope
({header, message: {search_response: [{status, data: {reg_records: []}}],
correlation_id}}). SRService unwraps that envelope, so every mock
silently fell through to "no results" and the tests either failed or
errored.
Extracted three small builders (_sync_search_envelope,
_async_search_envelope, _subscribe_envelope) so the contract is
obvious in one place and future cases reuse it. Patched the right
DCIClient method for each test: async tests previously patched .search
even though search_person(async_mode=True) calls .search_async.
Also corrected two assertions that contradicted the service contract:
search_person and sync_person_to_local raise UserError on missing data
and on API failures (as their docstrings document); they don't return
None. Renamed the affected tests to assertRaises.
Production bug fixed along the way: sr_record._update_from_sr_response
wrote datetime.now(UTC) (tz-aware) into a Datetime field, which Odoo
rejects. Switched to fields.Datetime.now() (naive UTC, the Odoo idiom).
spp_dci_client_sr: 0 failed, 0 errored of 42 tests (was 8 errors).
The 17 failures in test_search_service.py traced to six distinct
issues. Most were test-side, two were real production bugs.
Test fixture bugs (common.py):
- test_id_type was created on spp.id.type, but spp.registry.id.id_type_id
is Many2one("spp.vocabulary.code"). The wrong-model record's integer id
collided with a vocabulary code in urn:openspp:vocab:relationship, so
every search-by-identifier returned 0 records. Switched to env.ref on
the real spp_vocabulary.code_id_type_national_id record.
- test sender required individual consent by default and no consent
records existed, so every search came back empty. Defaulted to
legal_basis='legal_obligation' (NON_CONSENT bypass) for the default
fixture; consent-flow tests can override.
Test-side fixes (test_search_service.py):
- Pagination was used for SearchCriteria but it's the response schema
(requires total_count). Switched all request-side calls to
PaginationRequest.
- query_type='namedQuery' is not implemented; replaced with
query_type='expression' + query={'seq': []} (match-all).
- test_search_by_expression_city expected 2 partners in "Test City";
the fixture creates 3 (two individuals + the household). Updated.
- test_search_error_handling_sanitized patched .search on a recordset;
Odoo 19 recordsets are immutable. Patched the model class via
type(...).
- test_search_without_permission relied on the test admin lacking
registry_viewer, but has_group returns True for superuser. Now
spawns a plain internal user and runs the service in that user's env.
Production bugs:
- _parse_expression dropped the OR operator: with seq=[A] or=[B] it
produced [A, B] (AND), not ['|', A, B] (OR). Rewrote to build per-
alternative AND groups and join with '|' so multi-condition OR
branches now generate correct prefix notation.
- execute_search caught every per-item Exception including AccessError
and reported "rjct", hiding permission failures. AccessError now
propagates to the FastAPI layer so callers see a real 403.
Notification suite is untouched; the only remaining failure is
test_unlink_triggers_delete_notification (cascading hooks in
spp_service_points + spp_cel_domain) which is tracked separately.
…m ondelete Odoo's _ondelete_methods scans class attributes via hasattr(func, '_ondelete') to find callbacks that should run when records are deleted. A bare MagicMock auto-creates a truthy child mock for any attribute access, so the patched _queue_dci_notification_job ended up registered as an ondelete callback and was invoked once by Odoo during unlink (with the recordset) and once by the production code from postcommit (with event_type, partner_ids). The test saw two calls and failed assert_called_once. Switching to MagicMock(spec=real_method) restricts attribute access to the method's actual surface, so hasattr(mock, '_ondelete') returns False and Odoo no longer mistakes the mock for an ondelete hook. spp_dci_server_social: 0 failed, 0 errors of 46 tests (was 18+6).
The SR service's sync_person_to_local and the SR/DR callback routers all looked up identifier records via self.env["spp.id"], but no such model exists - the model is spp.registry.id. Every code path that relied on resolving a partner from an identifier raised KeyError on the first call. Production callers (the DCI callback flow and any sync without an explicit partner_id) were therefore dead on arrival. The earlier SRService test suite missed this because every test passed partner_id explicitly, skipping the lookup branch. Adding test_sync_person_to_local_looks_up_partner_by_identifier exercises the fixed path and locks in the correct model name. Note: spp_dci_indicators/tests/test_cel_integration_live.py also references spp.id (3 sites) but the whole file is gated by the 'dci_integration' tag (skipped without a live mock registry) and the same setUp also creates spp.id.type records with a non-existent 'code' field. Re-enabling those integration tests will require a fixture rewrite of its own; out of scope for this commit.
…='part'
Adds a local coverage runner (scripts/coverage_single_module.sh)
mirroring the CI recipe, and two router test modules that take
spp_dci_server from 37% to 42% line+branch:
- test_jwks_router (5 tests, 100% of routers/jwks.py): empty-key,
active-key, draft/revoked filtering, per-key serialisation failure
isolation, catastrophic-failure empty-set fallback.
- test_search_router (9 tests, 90% of routers/search.py): all-succ,
partial, all-rjct, invalid request -> 400, service exception ->
per-item rjct, ImportError fallback, no-signing-key path, signing
failure tolerance, unexpected exception -> 500.
Production bug surfaced by the partial-success test: the route set
overall_status='part' on mixed outcomes, but RequestStatus (per SPDCI
v1.0.0) only allows rcvd/pdng/succ/rjct, so DCICallbackHeader
validation raised and the whole route returned 500 for every partial
result. Maps to 'succ' with a "{n}/{total}" status_reason_message;
per-item statuses already carry the detail.
…uters Three more router test modules taking spp_dci_server from 42% to 52% line+branch: - test_callback_routers (16 tests): _validate_callback_payload helper (6 cases), all four on-* endpoints (12 sub-tests covering ACK / ERR / invalid-JSON for each), async_txn_status (3 cases), and the registry alias 501-stub endpoints for disability/crvs/farmer (4 sub-tests covering the rjct message format). - test_receipt_router (6 tests): receipt with known notification_id, subscription_code fallback, unknown notification -> warn, malformed request -> 400, unexpected exception -> 500, /sync/receipt delegation. Production bug fixed along the way: routers/receipt.py wrote datetime.now(UTC) (tz-aware) into a Datetime field. Same class of bug as the earlier sr_record.py fix; replaced with fields.Datetime.now(). Per-file coverage now: routers/jwks.py 100% routers/registry_aliases 100% routers/receipt.py 93% routers/search.py 90% routers/callbacks.py 86%
…e/txn_status) Adds 19 tests across four router endpoints, lifting spp_dci_server from 52% to 60% line+branch. routers/async_router.py jumps from 0% to 91%. Patches transaction.with_delay so the job-worker dispatch is exercised as a unit test without actually enqueueing background jobs - the returned mock provides a uuid attribute for the production code to store on the transaction record. Covers: - async_search: valid request -> 202 ACK with transaction + queued job, invalid body -> 400, unexpected exception -> 500. - subscribe: full happy path with auto_approve=True activating the subscription via action_confirm, invalid body -> 400, unknown sender -> 403, optional sender_uri branch. - unsubscribe: cancels subscription owned by sender, ACKs even when subscription_code is unknown (warning logged), invalid body -> 400. - txn_status: lookup by transaction_id / correlation_id / reference_id_list, missing transaction -> rjct, stored response_payload returned verbatim, malformed payload -> minimal fallback, invalid body -> 400.
…er.spp_reg_id Adds 17 tests for the two bulk_upload endpoints, taking spp_dci_server from 60% to 68% line+branch. bulk_search_upload (async, 9 tests): JSON + CSV happy paths, empty file -> 400, unknown format -> 400, malformed JSON -> 400 with structured error envelope, pending-job quota -> 429, oversize file -> 413, unknown sender still queues, unexpected exception -> 500. bulk_verify_identifiers (sync, 8 tests): permission required -> 403, found+not_found split, empty identifier list returns zero totals, too-many items -> 400, unknown format -> 400, invalid JSON -> 400, unexpected exception -> 500. Production bug surfaced by the found+not_found test: bulk_verify_identifiers gated every "found" entry on ``reg_id.partner_id.spp_reg_id`` - but ``spp_reg_id`` is not a field on res.partner anywhere in the codebase. Every verify request therefore returned 0 found and N not_found regardless of registry contents. Replaced with the ADR-016 namespace#value form built from the matched spp.registry.id (whose value *is* the external identifier per the "never expose DB IDs" principle). Per-file coverage now: routers/jwks.py 100% routers/registry_aliases 100% routers/receipt.py 93% routers/async_router.py 91% routers/search.py 90% routers/callbacks.py 86% routers/bulk_upload.py 84%
Adds 39 tests across two service-layer adapters, taking spp_dci_server from 68% to 75% line+branch. DCIConsentAdapter (18 tests, 14% -> 83%): - legal-basis bypass for each NON_CONSENT_LEGAL_BASES value - can_access_registrant: no-sender, bypass, consent-not-required, consent-lookup-positive, consent-lookup-negative - filter_dci_response: bypass metadata, ConsentService delegate path, fallback access path, fallback no-access path - build_consented_domain: no-sender, bypass, required filter applied, required=False no filter - log_dci_access: noop, log-call, log-tolerates-failure DCIVocabularyAdapter (21 tests, 15% -> 92%): - mapping_model lazy load (installed and not-installed paths) - map_gender_to_dci, _from_string with vocabulary-mapping-success, -returns-None, -raises, None input - marital_status / relationship / disability mappings with the same three branches each - _fallback_gender_map and _fallback_marital_status_map via code path, via name path, and both-missing.
…rror middleware Adds 19 tests across three smaller modules. spp_dci_server: 75% -> 80%. DCIRateLimitMiddleware (10 tests, 20% -> ~75%): - no-sender skip path - first-request initialisation - minute-window reset after 60s - per-minute and per-day 429 paths with the correct error_code - happy path under limits - default-limit fallback when sender doesn't set them - async check_dci_rate_limit dependency wrapper Note: counter values aren't asserted after the limit check because the middleware uses raw SQL UPDATE + invalidate_recordset which doesn't always surface the new value through the ORM cache within the same Python frame. Behaviour is verified via raise / no-raise instead. DCIErrorResponseMiddleware + _format_dci_error_response (6 tests, 23% -> 90%): - format helper produces the expected DCI envelope shape - pass-through for 2xx - transforms plain-detail 4xx -> DCI envelope - transforms dict detail via JSON serialisation - already-DCI-formatted bodies pass through unchanged - non-JSON bodies returned verbatim SppDciServerEndpoint (3 tests): - 'dci_api' value registered in the app selection - DCI router set carries JWKS + registry-alias paths - expected URL prefixes (/social/registry, /disability/registry, /crvs/registry, /farmer/registry) all present
…essing helpers Adds 26 tests across two files. spp_dci_server: 80% -> 84% line+branch. test_server_key (17 tests, 69% -> 97%): - key_id format constraint (rejects "not allowed!" style) - activate-without-keys constraint - expires_at must be future - single-active-key constraint - action_generate_key state guard, double-generation guard, crypto-failure propagation - action_activate happy path, already-active, already-revoked, missing-keys, rotation deactivates the previous active key - action_revoke happy path, already-revoked guard - get_active_key returns active or raises when none - get_signer for active key, blocks when not active, blocks when private_key is missing (cleared via raw SQL to bypass the _check_keys_present constraint) test_transaction_processing (9 tests, 35% -> 49%): - _get_callback_action mapping for search/subscribe/unsubscribe/ txn_status/notify - _build_minimal_txn_status for search, subscribe, with reason codes - process_async_search: success without callback, success with callback (_send_callback invoked), service-error rejection, unsupported reg_type rejection - action_retry_callback can be invoked on a callback_failed txn - action_view_job raises UserError when the queue.job is missing
Post-edit lint hook reformatting of the test files added during the Phase B coverage work: collapses multi-line calls that fit on one line and normalises import ordering. No logic changes; all 290 tests still pass.
…ocessors Adds TestTransactionAsyncProcessors covering the three job_worker hand-off methods that were previously untested, plus the dict-based callback sender they share: - process_async_subscribe: response built from existing subscriptions, placeholder when none found, callback dispatched when callback_uri set, rejection on malformed payload - process_async_unsubscribe: per-code status, callback path, rejection - process_async_txn_status: referenced-payload passthrough, minimal fallback when no payload, not-found -> rjct, correlation_id lookup, rejection - _send_callback_dict: success (state -> callback_sent), SSRF block (state -> callback_failed), no-op without callback_uri transaction.py 49% -> 79%; spp_dci_server overall 84% -> 89% line+branch. All 305 tests pass.
…istry; fix DR state filter Adds populated-cache tests for the DR/CRVS/IBR symbol providers and the SR not-installed branch, plus tests for the working CEL registry extension (load_profile DCI symbol docs). spp_dci_indicators 53% -> 68%. Production bug fixed (DRSymbolProvider._ensure_loaded): the disability cache query filtered state in ['active','draft'], but spp.dci.disability.status only has synced/stale/error states - so a synced disability record never loaded and dr.has_disability always returned False. Now filters synced/stale (error excluded). A regression test creates a synced record and asserts the data surfaces through dr.has_disability / types / severity / has_type. Scope note: the query_live() live-fetch paths and the CELExecutorDCIExtension._build_symbol_context hook are intentionally left untested. They are non-functional in their current form (see the follow-up report) and covering them would only codify broken behaviour; they need a design fix before they are worth testing.
Adds TestTriggerControllerSuccessPaths (8 HttpCase tests) exercising the success bodies of the search/subscribe/unsubscribe/txn_status endpoints with DCIClient mocked, plus the query-value-building branches (dict query -> 'type:value', string query, non-idtype query_type) and the string->list coercion for unsubscribe codes. Also asserts a client error surfaces as a 500 with error_type. spp_dci_client_compliance 77% -> 86% line+branch; all 15 tests pass.
Two new test modules take spp_dci_server_social 76% -> 85% line+branch. test_social_routers (4 tests): social_search.sync_search success + 500-on-error, sync_notify 501 stub payload, and the sr_alias delegation - bringing social_search.py (57%) and sr_alias.py to full coverage. test_search_service_internals (13 tests): the standalone helpers the end-to-end suite doesn't reach - _condition_to_domain attribute/operator mapping (incl. passthrough), _parse_predicate (empty, invalid type, the four expression shapes via mocked CEL service, compile-failure ValueError), _to_dci_member (with identifiers + demographics, and tolerating a _to_dci_person failure), and the _map_gender hardcoded fallback branches (string values, gender_id code, none). All 63 tests pass.
…nt internals Four new test modules take spp_dci_client 74% -> 89% line+branch (data_source.py 77% -> 89%, client.py 73% -> 87%, errors.py -> 100%). - test_errors: format_http_error (known/unknown codes, 401 + 5xx hints), format_connection_error (all types + unknown), raise_user_friendly_error. - test_data_source_validators: the @api.constrains validators (code format, base_url scheme/trailing-slash, oauth2 required fields, bearer token, timeout > 0, sender_id for authenticated). - test_data_source_http: get_oauth2_token (non-oauth2 reject, cache hit, body + query credential fetch, missing-token, HTTPStatusError, RequestError) with httpx mocked; get_headers (oauth2/bearer/basic); test_connection (success activates, http-error + request-error set error state). - test_client_convenience + signing/helpers + _make_request: _parse_query branches, search_by_id/_opencrvs/_predicate/_date_range (sync+async, DCI + OpenCRVS), _sign_request (unsigned/active/inactive key), _copy_envelope_for_log, and _make_request happy/HTTP-error/connection- error/401-retry paths. All 144 tests pass.
… add sender/signature/callback tests Production bug: verify_dr_signature / verify_ibr_signature verified the header via envelope.header.model_dump() (datetime objects) while signers serialize with mode='json' (ISO strings). The mismatch made every validly-signed inbound callback fail with 401 'Invalid signature' - the DR and IBR callback endpoints were effectively broken for signed traffic. Fixed to model_dump(mode='json') to match the crvs sibling (and sr, fixed in the next commit). Coverage, by porting the proven crvs test scaffolding (common.py + sender + signature-middleware tests, which exposed the bug) and adding an IBR callback router suite: - spp_dci_client_dr 61% -> 85% (dr_sender 19->96%, signature 16->88%) - spp_dci_client_ibr 57% -> 91% (ibr_sender 19->96%, signature 16->88%, callback 20->84%) All tests pass (dr 144, ibr 103).
… internals Add test_crvs_event (model lifecycle + process_event birth/death/marriage/ divorce), test_callback (notification router via-service + fallback paths), and test_crvs_service_internals (process_notification, error/response-shape branches, _extract_birth_data variants). crvs 53% -> 90%. Fix callback._create_event_directly 500: header was serialized with model_dump() leaving raw datetime objects that json.dumps rejected, so the no-data-source fallback path crashed instead of recording the event. Use model_dump(mode="json").
… service Port signature-middleware suite + shared common.py from the crvs sibling, add test_callback (on-search / on-subscribe / on-notify endpoints plus _process_sr_search_result / _find_partner_by_identifier / _update_sr_record helpers), and test_sr_service_internals (check_connection, async search, search_household, check_eligibility, get_program_enrollment, unsubscribe and subscribe failure branches). sr 52% -> 87%. Fix two real source bugs surfaced by the tests: - signature.verify_sr_signature verified the header with model_dump() while signing uses model_dump(mode="json"); datetime fields serialized differently, so every validly-signed SR callback was rejected with 401. - callback._update_sr_record wrote last_sync_date=datetime.now(UTC), a tz-aware value the Odoo Datetime field rejects, so SR search/notification callbacks silently failed to persist (or returned 500). Use fields.Datetime.now().
…backs The on-search callback handlers in dr/ibr/sr computed envelope.header.model_dump() and discarded the result (sr assigned it to an unused local). The header is not signed, json.dumps'd, or stored in these handlers, so the call was pure dead code. Removing it also eliminates the last bare model_dump() (without mode="json") occurrences in DCI source, which a close-out audit flagged as a misleading echo of the recently-fixed serialization bug.
…field
The data source form only exposes oauth2_client_secret_display (a masked,
write-only field) and never oauth2_client_secret. That display field was
computed with @api.depends('oauth2_client_secret') and an inverse that wrote
the same field back - a self-referential cycle. On create/write the display
recomputed to the mask before the inverse read it, so the user-entered secret
was dropped and the OAuth2 constraint failed with 'OAuth2 Client Secret is
required', blocking OAuth2 data source configuration in the UI.
Translate the display value into the real secret field in create/write before
calling super(), avoiding the recompute cycle. Add tests covering the UI save
path (create and write via the display field), which previously errored.
Bridge spp.data.provider to a DCI Data Source. Adds dci_data_source_id (Many2one to spp.dci.data.source) and a stored is_dci_backed flag, plus a 'DCI Integration' tab (first page) with the linked-source field and an info banner explaining that a DCI-backed provider's values are fetched via the DCI protocol using the linked Data Source - the provider's own Base URL and Authentication are ignored at runtime (those fields go read-only when a source is linked). This is the configuration half (Part 1) of wiring CEL external variables to DCI registries; the runtime fetch into spp.data.value follows separately.
…riables
Implement the runtime fetch (Part 2 core). spp.dci.cel.fetcher resolves each
subject's identifier (UIN/DRN/NATIONAL_ID priority, else first registry id)
and calls the CRVS service against the provider's linked DCI Data Source,
returning {subject_id: value}. The cache manager override routes external
variables whose provider is DCI-backed to the fetcher instead of the base
'returns empty' branch, so the existing precompute/refresh/TTL machinery
caches DCI values into spp.data.value for all CEL consumers.
CRVS metrics: crvs.dci.is_alive (not check_death), crvs.dci.birth_verified
(verify_birth). Handlers keyed by cel_accessor. A single subject's fetch
failure is logged and skipped, never aborting the batch.
…format
Update every predefined DCI variable's cel_accessor to the new convention
where 'dci' follows the registry segment (e.g. crvs.is_alive ->
crvs.dci.is_alive). Malformed expression-accessors (e.g. "dr.severity('Vision')
>= 3") become clean metric tokens derived from the variable name
(dr.dci.vision_severe). Variable names (dci.<registry>.<metric>) are unchanged.
This is the token the CEL author types and the key the DCI fetcher matches on,
so the seeded variables now align with the fetcher's handler keys.
Users can now write the dotted form (crvs.dci.is_alive == true) in any CEL
expression and have it resolve against DCI data, completing the Option-A
vertical (fetch -> cache -> query).
- spp_dci_indicators: resolver pre-pass (CelVariableResolverDCI) rewrites any
registered cached-variable dotted accessor into its metric('<accessor>', me)
call before normal resolution. The base resolver only matches single-identifier
tokens, so a dotted accessor was mis-read as field navigation; this leaves
ordinary navigation (me.gender) untouched since it matches no accessor.
- spp_cel_domain: metric comparison coerces a boolean RHS to 1/0 (Postgres
rejects numeric = boolean); booleans are cached as 1/0 accordingly.
- End-to-end test proves crvs.dci.is_alive == true filters the alive partner
and excludes the dead one off the cached value.
spp_dci_indicators 73 tests green; spp_cel_domain 573 tests green.
DR handlers (partner-based, via DRService.get_disability_status): dr.dci.has_disability, dr.dci.assessed, dr.dci.vision_severe/hearing_severe/mobility_severe (severity >= 3). Handler signature widened to (data_source, partner, id_type, id_value) so CRVS (identifier-based) and DR (partner-based) share one dispatch. Sync trigger: spp.dci.cel.fetcher.sync_for_partners() precomputes all DCI-backed variables for given partners (caches into spp.data.value via the cache manager); a 'Sync DCI Values' server action (bound to res.partner list/form) and an inactive daily cron (cron_sync_all_registrants) drive it. Prereq fix: DRService.__init__ validated registry_type against the short code 'DR'; corrected to the canonical RegistryType.DISABILITY_REGISTRY.value so a UI-configured DR data source is accepted. Tests updated to seed canonical values. spp_dci_client_dr 144 tests, spp_dci_indicators 82 tests, all green.
…/check_death OpenCRVS does not support the standard DCI idtype-value query; its DCI bridge returned HTTP 500 for those requests. Switch verify_birth (event_type='birth') and check_death (event_type='death') to client.search_by_id_opencrvs, which builds the OpenCRVS-specific expression envelope. Response parsing is unchanged. Tests updated to mock search_by_id_opencrvs.
…ults OpenCRVS returns status='succ' with a populated data envelope whose reg_records list is empty for a no-match. check_death only tested for data presence, so it reported every queried person as deceased (false positive), and verify_birth parsed the envelope as a record. Add _records_from_search_item to extract the actual records (DCI v1.0.0 data.reg_records, a bare list, or a single record dict); check_death/verify_birth now use it. Regression tests added.
Delete the original (non-functional) DCI->CEL design that tried to inject runtime Python symbol objects into CEL evaluation - the engine is a SQL compiler and never calls _build_symbol_context, so this path was dead. It is fully superseded by the fetcher -> spp.data.value -> metric() mechanism. Removed: - models/cel_extension.py (CELExecutorDCIExtension._build_symbol_context + CELRegistryDCIExtension doc stubs) - services/ (spp.dci.cel.integration: get_dci_symbols / register_dci_symbols / get_symbol_documentation) - symbols/ (DR/CRVS/IBR/SR SymbolProvider classes + query_live) - their tests (test_symbol_providers, test_symbol_providers_data, test_cel_extension, and TestCELSymbolProvidersIntegration) No references remained outside the deleted code; module installs and the remaining 46 tests pass.
Wire the existing-but-unconnected params/params_hash path so a metric() call
with named args is keyed by them in the cache:
- translator reads Call.kwargs into MetricCompare.params
(e.g. metric('dr.dci.severity', me, arg='Vision'))
- executor hashes p.params via spp.data.value._hash_params instead of the
hardcoded params_hash='' default; it threads through cache-status and the
in-select SQL automatically.
Enables parameterized DCI methods (severity('type'), has_event('event')).
Test proves two cached rows under one metric name are discriminated by params
(Vision=4>=3 matches, Hearing=1>=3 excluded). spp_cel_domain 573 tests green.
…& has_event('event')
Users can write dr.dci.severity('Vision') >= 3 and crvs.dci.has_event('death').
The resolver rewrites accessor('arg') -> metric('accessor', me, arg='arg'); the
fetcher materializes one params-keyed (params_hash) cache row per enumerated
argument:
- dr.dci.severity -> functional score per type (Vision/Hearing/Mobility)
- crvs.dci.has_event -> birth (verify_birth) / death (check_death)
Adds the dr.dci.severity and crvs.dci.has_event variables. Constraint: arguments
are a finite, pre-synced set. Tests cover resolver rewrite, per-arg
materialization, and end-to-end discrimination by argument.
Quality-gate cleanup ahead of PR: - ruff-format + prettier across the changed files - fix the few ruff errors that aren't auto-fixable: - cel_variable_resolver_dci: % -> f-string (UP031) - sr_sender: drop unused 'result' assignment (F841) - sr_service: drop the F821 'models.Model' string return annotation - test_sr_sender: noqa the intentional blind-Exception assert (B017) - test_transaction_processing / test_dci_notifications: drop unused locals (F841) - .ruff.toml: per-file E501 ignore for spp_dci_compliance/schemas/spdci_schemas.py (SPDCI-spec-verbatim description strings; reflowing hurts fidelity) No behavior change. Affected suites all green (crvs 98, sr 98, compliance 27, indicators 54, server 305, server_social 63).
The docs described the deleted symbol-injection design (_build_symbol_context,
SymbolProvider classes, lazy per-evaluation loading) and the old accessor
syntax that never resolved. Rewrite them around what actually ships:
- the flow: DCI Data Source -> DCI-backed Data Provider (DCI Integration tab)
-> variable -> Sync DCI Values -> spp.data.value TTL cache -> CEL (SQL);
explicitly no live per-record registry calls during evaluation
- working variables with the <registry>.dci.<metric> accessors, incl. the
parameterized severity('type') / has_event('event') and their
enumerated-argument constraint
- setup walkthrough matching the real menus, sync triggers, identifier
requirement; troubleshooting for unlinked providers / unsynced or expired
values / missing identifiers
- IBR/SR and is_married documented as planned-not-wired
- readme/DESCRIPTION.md rewritten; README.rst + index.html regenerated via
oca-gen-addon-readme (no stale references remain)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new modules and enhancements to the OpenSPP DCI integration, including the spp_dci_client_compliance, spp_dci_client_sr, spp_dci_compliance, and spp_dci_indicators modules, alongside security improvements such as PII gating via the new group_dci_admin security group. The review feedback highlights critical improvements: refactoring a post-migration script to use Odoo's ORM instead of raw SQL to prevent cache desynchronization, updating partner lookup logic in the Social Registry client to support namespace URIs, correcting a batch deletion offset bug in the compliance callback log to prevent skipping records, and resolving an N+1 query bottleneck in the DCI indicators fetcher by pre-fetching registry IDs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+24
to
+50
| cr.execute( | ||
| """ | ||
| SELECT value FROM ir_config_parameter | ||
| WHERE key = 'dci.client_compliance.bearer_token' | ||
| """ | ||
| ) | ||
| row = cr.fetchone() | ||
| if row and row[0] == "compliance-test-api-key-12345": | ||
| _logger.warning( | ||
| "Resetting the default DCI compliance bearer token. " | ||
| "Set 'dci.client_compliance.bearer_token' before re-enabling " | ||
| "the trigger endpoints." | ||
| ) | ||
| cr.execute( | ||
| """ | ||
| DELETE FROM ir_config_parameter | ||
| WHERE key = 'dci.client_compliance.bearer_token' | ||
| """ | ||
| ) | ||
|
|
||
| cr.execute( | ||
| """ | ||
| INSERT INTO ir_config_parameter (key, value, create_uid, write_uid, create_date, write_date) | ||
| VALUES ('dci.client_compliance.enabled', 'false', 1, 1, NOW(), NOW()) | ||
| ON CONFLICT (key) DO NOTHING | ||
| """ | ||
| ) |
There was a problem hiding this comment.
Using raw SQL to modify ir.config_parameter bypasses Odoo's ORM cache, which can lead to cache desynchronization issues where Odoo continues to use the cached old values. Since this is a post-migration script, the registry is fully loaded. Please use the Odoo ORM (api.Environment) to safely delete and set system parameters.
from odoo import api, SUPERUSER_ID
env = api.Environment(cr, SUPERUSER_ID, {})
ICP = env["ir.config_parameter"]
token_param = ICP.search([("key", "=", "dci.client_compliance.bearer_token")], limit=1)
if token_param and token_param.value == "compliance-test-api-key-12345":
_logger.warning(
"Resetting the default DCI compliance bearer token. "
"Set 'dci.client_compliance.bearer_token' before re-enabling "
"the trigger endpoints."
)
token_param.unlink()
ICP.set_param("dci.client_compliance.enabled", "false")References
- A non-deterministic database search (using
searchwithlimit=1but noorder) can be acceptable if the practical context (e.g., single-provider deployments) makes it functionally deterministic. However, this should be acknowledged as technical debt to be addressed if the context changes.
Member
Author
There was a problem hiding this comment.
Addressed in c17bbac — the migration now goes through the ORM (api.Environment + ir.config_parameter). One deviation from the suggested snippet: set_param is only called when the parameter is absent, preserving the original ON CONFLICT DO NOTHING semantics (an unconditional set_param would overwrite an operator's explicit choice).
Comment on lines
+380
to
+390
| id_record = ( | ||
| self.env["spp.registry.id"] | ||
| .sudo() | ||
| .search( | ||
| [ | ||
| ("id_type_id.code", "=", identifier_type), | ||
| ("value", "=", identifier_value), | ||
| ], | ||
| limit=1, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
If identifier_type is a namespace URI (e.g., starting with urn:), searching against id_type_id.code will fail to find the local partner, raising an unhandled UserError. Please update the lookup logic to search against id_type_id.namespace_uri when the identifier type is a URI, matching the robust lookup pattern used in callback.py.
domain = [("value", "=", identifier_value)]
if identifier_type.startswith("urn:"):
domain.append(("id_type_id.namespace_uri", "ilike", identifier_type))
else:
domain.append(("id_type_id.code", "=", identifier_type))
id_record = (
self.env["spp.registry.id"]
.sudo()
.search(domain, limit=1)
)References
- A non-deterministic database search (using
searchwithlimit=1but noorder) can be acceptable if the practical context (e.g., single-provider deployments) makes it functionally deterministic. However, this should be acknowledged as technical debt to be addressed if the context changes.
Member
Author
There was a problem hiding this comment.
Addressed in c17bbac — sync_person_to_local now falls back to an id_type_id.namespace_uri match when the code-exact search finds nothing, mirroring the callback router's lookup. Covered by a new test (test_sync_person_to_local_looks_up_partner_by_namespace_uri) that reproduced the URN-form failure first.
Comment on lines
+396
to
+403
| _logger.info("Queueing %d DCI callback logs for cleanup in batches of %d", total, batch_size) | ||
| for offset in range(0, total, batch_size): | ||
| self.with_delay(description=f"Cleanup DCI logs batch {offset // batch_size + 1}")._cleanup_batch( | ||
| cutoff, batch_size, offset | ||
| ) | ||
|
|
||
| return total | ||
|
|
There was a problem hiding this comment.
Using offset in concurrent or sequential batch deletion jobs causes records to be skipped. As records are deleted by earlier batches, the matching dataset shrinks, shifting the remaining records to lower offsets. Consequently, any job running with a non-zero offset will skip a portion of the remaining records, leaving stale logs in the database. A safer and more robust pattern is to have each job delete a batch of size limit at offset=0, and recursively queue the next job if the limit was reached.
_logger.info("Queueing DCI callback logs cleanup job")
self.with_delay(description="Cleanup DCI logs")._cleanup_batch(cutoff, batch_size)
return total
Member
Author
There was a problem hiding this comment.
Confirmed real and addressed in c17bbac — a new test (test_cleanup_deletes_all_old_logs_across_batches) reproduced the skip: with offset batches, half the old logs survived. Batches now run at offset 0 and re-queue themselves while a full batch was deleted, exactly as suggested.
Comment on lines
+190
to
+202
| def _get_partner_identifier(self, partner): | ||
| """Resolve a (identifier_type, identifier_value) for the partner from | ||
| its spp.registry.id records, using the registry priority order.""" | ||
| reg_ids = self.env["spp.registry.id"].search([("partner_id", "=", partner.id)]) | ||
| for id_type in _IDENTIFIER_PRIORITY: | ||
| for reg_id in reg_ids: | ||
| if reg_id.id_type_id.code == id_type and reg_id.value: | ||
| return (reg_id.id_type_id.code, reg_id.value) | ||
| if reg_ids: | ||
| first = reg_ids[0] | ||
| if first.value: | ||
| return (first.id_type_id.code, first.value) | ||
| return None |
There was a problem hiding this comment.
Calling search on spp.registry.id inside a loop for each partner introduces an N+1 query bottleneck, which will severely degrade performance when syncing large batches of registrants. Please optimize this by pre-fetching all spp.registry.id records for the entire batch of partners in a single query, and passing the pre-fetched records to the identifier resolver.
Suggested change
| def _get_partner_identifier(self, partner): | |
| """Resolve a (identifier_type, identifier_value) for the partner from | |
| its spp.registry.id records, using the registry priority order.""" | |
| reg_ids = self.env["spp.registry.id"].search([("partner_id", "=", partner.id)]) | |
| for id_type in _IDENTIFIER_PRIORITY: | |
| for reg_id in reg_ids: | |
| if reg_id.id_type_id.code == id_type and reg_id.value: | |
| return (reg_id.id_type_id.code, reg_id.value) | |
| if reg_ids: | |
| first = reg_ids[0] | |
| if first.value: | |
| return (first.id_type_id.code, first.value) | |
| return None | |
| def _get_partner_identifier(self, partner, reg_ids=None): | |
| """Resolve a (identifier_type, identifier_value) for the partner. | |
| Accepts optional pre-fetched reg_ids to avoid N+1 queries. | |
| """ | |
| if reg_ids is None: | |
| reg_ids = self.env["spp.registry.id"].search([("partner_id", "=", partner.id)]) | |
| for id_type in _IDENTIFIER_PRIORITY: | |
| for reg_id in reg_ids: | |
| if reg_id.id_type_id.code == id_type and reg_id.value: | |
| return (reg_id.id_type_id.code, reg_id.value) | |
| if reg_ids: | |
| first = reg_ids[0] | |
| if first.value: | |
| return (first.id_type_id.code, first.value) | |
| return None |
Member
Author
There was a problem hiding this comment.
Addressed in c17bbac — instead of threading pre-fetched records through the resolver, the lookup now uses the existing partner.reg_ids One2many: Odoo prefetches it across the whole browsed batch, so the per-partner access no longer issues one query per partner.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #221 +/- ##
==========================================
+ Coverage 71.70% 74.71% +3.01%
==========================================
Files 1003 1085 +82
Lines 59245 62197 +2952
==========================================
+ Hits 42483 46473 +3990
+ Misses 16762 15724 -1038
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- replace deprecated <odoo><data> wrappers in 5 data XML files - drop tests import from spp_dci_server_social __init__ (pylint E8130) - remove unused env param in security_warning.js systray item (eslint) - add nosemgrep suppressions for the 16 reviewed sudo() usages, with justification comments; restructure the search_service.py suppression that was misplaced and over-long (also fixes ruff E501)
- spp_dci_compliance: callback-log cleanup batches now chain themselves instead of using offsets; offset batches skipped records because each deletion shifted the survivors to lower offsets (test reproduces this) - spp_dci_client_sr: sync_person_to_local falls back to a namespace-URI match when the identifier type is a urn:, mirroring the callback router - spp_dci_indicators: resolve identifiers via partner.reg_ids so the One2many prefetch covers the whole batch (no N+1 search per partner) - spp_dci_client_compliance: post-migration uses the ORM instead of raw SQL so the ir.config_parameter cache stays in sync; the enabled flag is still only set when absent
| ICP = env["ir.config_parameter"].sudo() # nosemgrep: odoo-sudo-without-context | ||
|
|
||
| token_param = ICP.search([("key", "=", "dci.client_compliance.bearer_token")], limit=1) | ||
| if token_param and token_param.value == "compliance-test-api-key-12345": |
…n router Coverage tests added across four modules (118 new tests, no existing test modified): - spp_dci_compliance 56% -> 92% (verification router, schemas, security warning, endpoint registration, callback log extras) - spp_dci_client_sr 87% -> 96% (sr_record, callback router branches, endpoint registration, signature middleware, service edges) - spp_dci_client_compliance 86% -> 98% (trigger controller edge cases) - spp_dci_indicators fetcher 86% -> 93% (error branches, cron, method materialization edges) Two real bugs surfaced by the new router tests, fixed at the source: - get_callbacks' 'status' query param shadowed fastapi.status, so the ValueError/Exception handlers raised AttributeError instead of clean HTTP 400/500; renamed to status_filter with alias='status' so the wire API is unchanged - spp.dci.callback.log.get_callbacks returned Odoo False for unset Char fields, which Pydantic rejects for str|None; coerced to None (ints left untouched: 0 is legitimate)
Member
Author
Review follow-up: gemini findings + codecov gaps addressedGemini code review (all 4 findings) — commit c17bbac
Codecov gaps — commit f5a83e9118 new tests, no existing test modified:
The new router tests surfaced two real bugs in
All suites green: compliance 100, client_sr 132, client_compliance 27, indicators 65; full pre-commit pass. |
emjay0921
approved these changes
Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two bodies of work, validated end-to-end against a live OpenCRVS (Farajaland) instance.
A. DCI module hardening
spp_dci_*module family (server, clients for CRVS/DR/IBR/SR, compliance) and raise test coverage to ~85%+ line+branch across themgroup_dci_adminmodel_dump()while signing usedmodel_dump(mode="json")→ every validly-signed callback was rejected (dr/ibr/sr)spp.idmodel (→spp.registry.id) broke identifier lookups (sr/dr)hmac.compare_digest, explicitapi_tokensrequired), illegalstatus='part', bulk-uploadpartner.spp_reg_id→ ADR-016 formB. DCI → CEL integration (
spp_dci_indicators+ engine support)is_dci_backed)spp.dci.cel.fetchercalls the registry services per registrant and caches intospp.data.value(TTL); cache-manager routing override keys the cache by the CEL accessor; "Sync DCI Values" server action + disabled-by-default daily cron<registry>.dci.<metric>accessors resolve via a resolver pre-pass; boolean metrics supported (bool RHS coerced for the metric SQL); parameterized methods viaparams_hash—dr.dci.severity('Vision') >= 3,crvs.dci.has_event('death')(translator readsmetric()kwargs intoMetricCompare.params; executor hashes them)verify_birth/check_deathuse the OpenCRVS search format (standard DCI query 500s) and parsedata.reg_records(empty list = no match; previously every queried person was reported deceased)_build_symbol_context, SymbolProvider classes — never invoked by the engine); docs rewritten to describe the real mechanism; branch made lint-clean (ruff/ruff-format/prettier)Working CEL variables
crvs.dci.is_alive,crvs.dci.birth_verified,crvs.dci.has_event('birth'|'death'),dr.dci.has_disability,dr.dci.assessed,dr.dci.vision/hearing/mobility_severe,dr.dci.severity('Vision'|'Hearing'|'Mobility'). IBR/SR andis_marriedship as variables documented “planned, not yet wired”.OpenProject
Test plan
spp_cel_domain573,spp_dci_server305,spp_dci_client_dr144,spp_dci_client_crvs98,spp_dci_client_sr98,spp_dci_server_social63,spp_dci_indicators54,spp_dci_compliance27spp_dci_indicators85% line+branch (resolver 97%, cache-manager 89%, fetcher 82%); client modules 85–91%crvs.dci.is_alive == true→ 25/30,== false→ 5/30;has_event('death')→ exactly the 5 deceased,has_event('birth')→ exactly the 25 birthsspp_dci_indicatorson a fresh DB and run./spp t spp_dci_indicators