feat(spp_dci_demo): add DCI birth verification demo module and Conditional Child Grant#40
feat(spp_dci_demo): add DCI birth verification demo module and Conditional Child Grant#40jeremi wants to merge 10 commits intofeat/dci-client-improvementsfrom
Conversation
Summary of ChangesHello @jeremi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances the DCI integration capabilities by introducing a comprehensive demo module for birth verification. It enhances the DCI client to support specific external registry formats like OpenCRVS, making the system more adaptable. Crucially, it integrates verification directly into the change request workflow, enabling automated processing of new member additions and their enrollment into relevant programs based on verified data. This work also lays a stronger foundation for identity verification within the registry by adding dedicated fields to track how IDs are verified. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a DCI-powered birth verification demo, a new Conditional Child Grant program, and refactors the DCI client for better OpenCRVS compatibility. While the changes are generally well-structured, a critical security vulnerability exists: Personally Identifiable Information (PII), such as names, birth dates, and registration numbers from DCI request/response data, is logged to an audit model and included in exception messages. This PII must be sanitized or masked to comply with data protection principles. Furthermore, a high-severity bug in the auto-enrollment logic prevents new child enrollment, and a medium-severity issue involves a missing system parameter definition. Addressing these points will make this an excellent addition.
| if hasattr(household, "group_membership_ids"): | ||
| for membership in household.group_membership_ids: | ||
| if membership.individual: | ||
| self._enroll_partner_in_program(membership.individual, program) |
There was a problem hiding this comment.
The auto-enrollment logic for the new child is likely to fail. The household record's cache for group_membership_ids is probably stale after the super().apply() call and won't include the newly created member. As a result, the loop over group_membership_ids will skip the new child.
To fix this, you should invalidate the cache for this field on the household record before iterating over the members. This ensures the new child is included in the enrollment process.
| if hasattr(household, "group_membership_ids"): | |
| for membership in household.group_membership_ids: | |
| if membership.individual: | |
| self._enroll_partner_in_program(membership.individual, program) | |
| # Enroll all household members including the new child | |
| household.invalidate_recordset(["group_membership_ids"]) | |
| if hasattr(household, "group_membership_ids"): | |
| for membership in household.group_membership_ids: | |
| if membership.individual: | |
| self._enroll_partner_in_program(membership.individual, program) |
| # Re-raise UserError as-is | ||
| raise | ||
| except Exception as e: | ||
| _logger.exception("Birth verification failed for BRN %s", self.birth_registration_number) |
There was a problem hiding this comment.
The action_verify_birth method logs the Birth Registration Number (BRN) in an exception message using _logger.exception. BRN is considered Personally Identifiable Information (PII). Logging PII in system logs is generally discouraged as it may lead to unauthorized data exposure if log files are not properly secured or are aggregated into less secure logging systems.
There was a problem hiding this comment.
The code attempts to read the system parameter spp_dci_demo.default_crvs_data_source, but this parameter is not defined in any of the module's data files (e.g., data/system_parameters.xml). While the code correctly falls back to searching for a data source, this makes the configuration via system parameter non-functional out of the box. Consider adding this parameter to your data files for completeness and to make the feature fully configurable.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| <record id="param_auto_approve_on_match" model="ir.config_parameter"> | ||
| <field name="key">spp_dci_demo.auto_approve_on_match</field> | ||
| <field name="value">True</field> | ||
| </record> |
There was a problem hiding this comment.
Auto-approval on match feature is not implemented
High Severity
The spp_dci_demo.auto_approve_on_match system parameter is defined and set to True, and the module description explicitly states "CR auto-approved if verification matched", but no code ever reads this parameter or implements the auto-approval logic. The _after_submit hook (mentioned in the PR description as the consolidation point for auto-approval) is never overridden in spp_dci_demo. After a successful birth verification with matching data, submitting the CR will not auto-approve it — it will follow the normal manual approval flow, contradicting the documented behavior and the purpose of the parameter.
Additional Locations (1)
| Returns: | ||
| Dict with normalized person data or None if not found | ||
| """ | ||
| return extract_person_from_dci_response(response) |
There was a problem hiding this comment.
Model method _extract_person_from_dci_response is unused dead code
Low Severity
The _extract_person_from_dci_response method is defined on the model but never called anywhere in the codebase. The only caller of the extraction logic, _check_data_matches_dci_response, directly invokes the standalone utility function extract_person_from_dci_response instead of going through this wrapper method. This is dead code that adds confusion about which extraction path is actually used.
| CRVS = "ns:org:RegistryType:Civil" | ||
| IBR = "ns:org:RegistryType:IBR" | ||
| DISABILITY_REGISTRY = "ns:org:RegistryType:DR" | ||
| FUNCTIONAL_REGISTRY = "ns:org:RegistryType:FR" |
There was a problem hiding this comment.
RegistryType enum value change lacks data migration
Medium Severity
The RegistryType enum values changed from short strings (e.g., "CRVS", "SOCIAL_REGISTRY") to namespaced strings (e.g., "ns:org:RegistryType:Civil", "ns:org:RegistryType:Social"), but there is no data migration for existing spp.dci.data.source records. Existing records still store the old values in the registry_type Selection field, which no longer match any valid option. This causes _get_registry_type() to silently fall back to SOCIAL_REGISTRY for CRVS sources, and crvs_service.py to reject them outright with a ValidationError.
Additional Locations (1)
jeremi
force-pushed
the
feat/dci-client-improvements
branch
from
db3b9a4 to
3ea418e
Compare
Features: - Birth verification via OpenCRVS DCI integration - Auto-approval when DCI data matches CR detail fields - Auto-enrollment of household in configured program on CR apply - Add Child wizard for streamlined UX - Verified BRN registry ID created on apply Technical: - Add search_by_id_opencrvs method for OpenCRVS-specific format - Extract DCI verification logic to utils module - Add system parameters for configuration - Add post_init_hook for auto-configuration Note: DCI data source credentials must be configured manually via Settings > Technical > System Parameters or UI.
…exists Add computed single_dci_data_source field to auto-hide the DCI data source dropdown when there is zero or one active Civil registry, removing an unnecessary selection step from the UI.
…after_submit The DCI demo wizard adds no value over the standard CR flow now that detail forms have Submit buttons. Remove the wizard and its tests, and remove the duplicated _try_auto_approve() from the detail model. Auto- approval now only happens via the _after_submit() hook on the CR model.
…emo household - Add Masters household (Adam + Mary) as demo data for DCI CR flow - Hide Contact Information and Relationship fields in add-member form - Target Conditional Child Grant program in post_init_hook - Add Conditional Child Grant program with first-1,000-days eligibility - Add Health Visit event type for compliance tracking - Configure compliance manager with CEL expression support
…add system parameter Invalidate household group_membership_ids cache before iterating so the newly created child membership is included in auto-enrollment. Add missing spp_dci_demo.default_crvs_data_source system parameter with an empty default to system_parameters.xml so the parameter is defined on module install.
jeremi
force-pushed
the
feat/dci-demo-module
branch
from
eb68e0d to
76e158e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if "sex" in person_data: | ||
| normalized["sex"] = person_data["sex"].lower() | ||
| elif "gender" in person_data: | ||
| normalized["sex"] = person_data["gender"].lower() |
There was a problem hiding this comment.
Null API values cause AttributeError in extraction
Medium Severity
extract_person_from_dci_response calls .lower() on person_data["sex"] and person_data["gender"] without guarding against None. If the API returns {"sex": null}, None.lower() raises AttributeError. The same issue exists at lines 106–107 where name.get("given_name", "") returns None (not "") when the key exists with a null value, causing .strip() to fail. The outer exception handler catches this but incorrectly reports an "error" verification status.
Additional Locations (1)
| "cycle_duration": 30, # Monthly | ||
| # CEL: Households with children under 2 (first 1,000 days) | ||
| # Pattern: Member age check via members.exists() | ||
| "cel_expression": "r.is_group == true and members.exists(m, age_years(m.birthdate) < 2)", |
There was a problem hiding this comment.
Eligibility CEL expression contradicts program description
Low Severity
The Conditional Child Grant cel_expression uses age_years(m.birthdate) < 2 (targeting children aged 0–1 only), but the description says "children aged 0-2" and the demo_points say "0-2 years." The compliance_cel_expression correctly uses <= 2 (ages 0–2). Since age_years returns a floored integer, < 2 excludes 2-year-olds from eligibility despite the description including them. The eligibility expression likely needs <= 2 to match the stated intent.
Additional Locations (2)
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
Code review complete. Well-structured demo module with clean separation of concerns. No blockers found.
2 participants
Summary
spp_dci_demomodule: DCI-powered birth verification via SPDCI/OpenCRVS for the Add Child Member change request flow_after_submit(removed separate wizard)_onchange_invalidate_verificationto reset verification when verified fields are editedspp_mis_demo_v2with event types for child-related life eventsdci_verification.py) for extracting and matching person data from DCI responsesDependencies
feat/dci-client-improvements) for DCI client fixes and approval mixinfeat/api-outgoing-log) for outgoing API audit trailTest plan
test_single_module.sh spp_dci_demotest_single_module.sh spp_mis_demo_v2Note
Medium Risk
Touches core change-request and program demo flows and adds new verification/enrollment behaviors plus OpenCRVS-specific request formatting, which could affect integrations and downstream data creation if misconfigured.
Overview
Adds a new
spp_dci_demomodule that extends the Add Member change request flow with DCI/OpenCRVS birth verification (BRN entry, verify/re-verify action, verification invalidation on edits), surfaces verification status on the parent CR form, and ships demo data plus system parameters and a post-init hook for auto-enrollment configuration.Updates the DCI client and schemas to be SPDCI-spec compliant by switching
RegistryTypevalues to namespaced strings, adding OpenCRVS-specific search/envelope building (search_by_id_opencrvs, optional OpenCRVS expression wrapping, and corrected OpenCRVS envelope fields), and expands tests to cover the new OpenCRVS formats.Enhances registry IDs by adding verification metadata (
verification_method,is_verified, etc.) and UI support (form view + list columns), and extends MIS demo content with a new Conditional Child Grant program (new demo constant, event type, and generator support for compliance manager CEL configuration).Written by Cursor Bugbot for commit a9a731d. This will update automatically on new commits. Configure here.