feat: add spp_cr_type_assign_program module#187
Merged
gonzalesedwin1123 merged 12 commits into19.0from May 5, 2026
Merged
Conversation
Add a new module declaring its manifest, dependencies, and readme fragments. Module installs cleanly but defines no behaviour yet — detail model, apply strategy, view, CR-type record, and conflict rule land in subsequent commits.
…ram domain Define `spp.cr.detail.assign_program` with `program_id`, a computed `allowed_program_ids` filtered to active programs whose `target_type` matches the registrant, and `created_membership_id` for the apply-time audit back-reference. ACL rows match the cr_user / cr_validator(_hq) / cr_manager pattern used by other detail models. Tests cover both target-type branches and the active/inactive program filter.
…tion Implement `spp.cr.apply.assign_program` with explicit `validate()`, `apply()`, and `preview()`. Validation rejects: missing program, disabled registrant, inactive program, target_type mismatch (defensive guard against direct ID writes that bypass the form domain), and existing memberships for the same `(registrant, program)` pair — intercepting the DB unique constraint with a friendlier message. On apply, creates a draft `spp.program.membership` and stores its ID on the detail for the audit trail.
Add the detail form view (with a prominent Beneficiary section and a callout explaining that the registrant itself is the program beneficiary) and the `spp.change.request.type` data record that wires the type into the standard Create-Change-Request wizard. The type is non-editable via Studio, system-defined, and has conflict detection enabled — the rule itself lands in the next commit.
…ssignments Add a `scope='custom'` conflict rule on the new CR type and the `_check_custom_conflicts` override on `spp.change.request` that narrows the registrant-scoped match to candidates whose detail's `program_id` equals ours. Two CRs assigning the same registrant to the *same* program block; two CRs for the same registrant on *different* programs proceed independently.
Approve and apply a draft CR end-to-end, asserting that `cr.action_apply()` creates the program membership and back-links it on the detail. Complements the unit-level apply tests by exercising the strategy through the CR's own apply pipeline (preview snapshot, sudo, audit logging) instead of calling `apply()` directly.
Run the oca-gen-addon-readme pre-commit hook to produce README.rst and static/description/index.html from the readme/ fragments.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #187 +/- ##
==========================================
+ Coverage 71.63% 71.67% +0.04%
==========================================
Files 933 942 +9
Lines 55369 55478 +109
==========================================
+ Hits 39664 39765 +101
- Misses 15705 15713 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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 the spp_cr_type_assign_program module, which adds a new change request type for enrolling registrants into programs. The implementation includes a detail model for program selection, an apply strategy to generate draft membership records, and custom conflict detection logic. Feedback focuses on optimizing performance by batching database queries in computed fields and conflict checks to avoid N+1 patterns. Additionally, a non-standard readonly attribute on a form view was identified for removal.
Two N+1 query patterns identified by review: - `_compute_allowed_program_ids` ran one search per detail record. Cache results by `registrant_target_type` so a recordset of N details runs at most 2 queries (one per target type). - `_check_custom_conflicts` called `get_detail()` per candidate, each triggering its own load. Replace with one `search` over the candidate detail IDs filtered by `program_id`. `check_same_type_only=True` on our rule guarantees candidates share our detail model; defensive guards still skip candidates whose detail model or detail_res_id is missing.
…UserError `validate()` checks the (registrant, program) pair is unique before `apply()` creates the membership, but a concurrent transaction can insert the same pair between the two operations. The DB unique constraint on `spp.program.membership(partner_id, program_id)` fires and surfaces to the user as a raw `psycopg2.errors.UniqueViolation` plus a poisoned transaction (subsequent ORM calls fail with `InFailedSqlTransaction`). Wrap the create in `cr.savepoint()` so the parent transaction stays usable, mute the noisy SQL error log, and translate `UniqueViolation` into the same friendly UserError the validate() path produces. Add a test that mocks `spp.program.membership.create` to raise `UniqueViolation`, asserting the strategy raises UserError with the expected message. Also add a test for the conflict-hook short-circuit: when `_check_custom_conflicts` is called with a rule that isn't ours, candidates must be returned unchanged so other modules' custom conflict logic isn't suppressed. Document the deliberate UI-staleness of `_compute_allowed_program_ids` when a program transitions active <-> ended mid-form: the apply strategy revalidates `state == 'active'`, so staleness is UI-only.
…in the form Address UX-review findings on PR #187 around beneficiary disambiguation and missing affordances: 1. Surface `registrant_target_type` as a visible labelled field ("Beneficiary type") next to `registrant_id` in the Beneficiary group. Single highest-leverage change for users coming from the household form: makes "Group" vs "Individual" enrolment explicit instead of inferred. 2. Rewrite the explanatory alert to: - name the registrant as the beneficiary (not just "the registrant above"), - tie the enrollment shape to the new visible Beneficiary type field, - direct users who actually want to enroll a household *member* to start the CR from the member's own record. 3. Add an `alert-warning` shown only when no active programs match the beneficiary's target type — replaces the silent empty-dropdown dead end with concrete guidance to ask a Program Manager. `aria-live="polite"` so screen readers announce the state when it appears. 4. Rename the post-apply group "Result" -> "Created Membership". The `created_membership_id` field is already openable (no `no_open: True` set), so the user can navigate from here to the membership record without an extra smart button. 5. Enrich the `program_id` help text to surface the filter rule (active + matching target type) and the post-apply state (Draft, Program Manager activates). 6. Mirror the in-form guidance in `readme/DESCRIPTION.md` so the module description and the in-form alert tell the same story.
… the dropdown Move duplicate prevention from "discovered at apply, after a wasted approval cycle" to "discovered at form-fill." Extend `_compute_allowed_program_ids` with `registrant_id.program_membership_ids` in its depends, then subtract the registrant's existing memberships from the cached active-matching set. The per-target-type cache stays — it saves the expensive program search; the per-registrant exclusion is a cheap Python set subtraction. Refine the empty-state warning copy to cover both reasons the dropdown can be empty (no active programs match the beneficiary's type, or the registrant is enrolled in all of them). The existing validate() check and the savepoint-protected create remain the safety net for races between form-fill and apply. Add `test_d5_allowed_programs_excludes_already_enrolled` asserting the exclusion.
…irect line Remove the sentence advising users to "open that member's record and start a change request from there." Verified there is no in-form path to start a CR from a partner record: - `spp_change_request_v2/models/res_partner.py` adds no smart button - `spp_change_request_v2/views/` has no `res.partner` view inheritance - `action_cr_create_wizard` has no `binding_model`, so it does not appear in the partner form's Action menu The wizard's `default_get` would pre-fill `registrant_id` from `active_model='res.partner'` context, but nothing actually opens it that way. Telling users to "start a CR from there" is misleading. Drop the line from the in-form info alert and from `readme/DESCRIPTION.md`. The disambiguating copy on the "registrant IS the beneficiary" semantic stays. Regenerate README.rst and static/description/index.html to match.
emjay0921
approved these changes
May 5, 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
2 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.
Why is this change needed?
OpenSPP needs a way to record program enrollment as an auditable change request — covered by the standard CR approval, document, and conflict workflow rather than the direct
spp.assign.program.wizardshortcut. This module adds a single CR type,assign_program, that does exactly that.The CR's registrant is the program beneficiary. There is no "select a member of the household" step:
target_type='group'(the household itself is enrolled)target_type='individual'(the individual is enrolled, whether or not they belong to a household)Standalone individuals (registrants not in any household) are supported.
How was the change implemented?
New module
spp_cr_type_assign_program(Layer 2 capability, depends onspp_change_request_v2andspp_programs) bundling:spp.cr.detail.assign_program—program_idwith a live, computedallowed_program_idsfiltered bystate='active'ANDtarget_typematching the registrant;created_membership_idrecords the resulting membership for the audit trail.spp.cr.apply.assign_program— explicitvalidate()/apply()/preview(). Validation rejects: missing program, disabled registrant, inactive program, target-type mismatch (defensive guard against direct ID writes that bypass the form domain), and existing memberships for the same(registrant, program)pair (intercepting the DB unique constraint with a friendlier message).data/cr_types.xmlwithtarget_type='both',apply_strategy='custom', non-Studio-editable, system-defined.custom+ a_check_custom_conflictsoverride onspp.change.requestthat narrows the registrant-scoped match to candidates whose detail'sprogram_idequals ours. Two CRs assigning the same registrant to the same program block; two CRs for the same registrant on different programs proceed independently.cr_user/cr_validator(_hq)/cr_managermatching the convention used by other detail models inspp_change_request_v2.The new beneficiary semantics, decision history (group-targeted vs individual-targeted vs flexible), and design tradeoffs are written up in
docs/plans/SPP_CR_TYPE_ASSIGN_PROGRAM_PLAN.md(lives in the local docs symlink, not in this repo).History is split into 7 logical commits (skeleton → detail+ACL → strategy → view+type → conflicts → integration test → generated README) so each commit is independently reviewable and installable.
New unit tests
spp_cr_type_assign_program/tests/test_assign_program.py— 15 tests:registrant_target_typefor group + individual, andallowed_program_idsfiltering for each (active vs. inactive, group vs. individual programs).spp.program.membershiprecords.UserError.(registrant, program)rejected with friendly message rather than raw DB unique-constraint error.action_apply()→ membership exists with correct partner/program/state and is back-referenced on the detail.(registrant, program)→ second isconflict_status='blocked'with first listed inconflicting_cr_ids.conflict_status='none'(proves the custom hook narrows correctly).Unit tests executed by the author
Pre-commit (
ruff,ruff-format,prettier,pylint_odoo, OpenSPP compliance hooks, bandit, semgrep) all pass on every changed file.How to test manually
ODOO_INIT_MODULES=spp_cr_type_assign_program docker compose --profile ui up -dadmin/admin), open Change Requests → All Requests → New.spp.program.membershipexists in Programs → the relevant program → Beneficiaries, with state Draft.(registrant, program)pair: the second CR should be blocked with a "Duplicate program assignment" conflict notice.Related links
spp_cr_types_base,spp_cr_types_advancedspp.assign.program.wizardinspp_programs/wizard/assign_to_program_wizard.py