feat(spp_programs): deduplication — configure by method, recompute on each run, bulk return to draft - #445
Conversation
…raft Deduplication and eligibility checks put a membership into "duplicated" or "not eligible" and nothing took it out again. back_to_draft() existed, but only as a button on the membership form, so resolving a duplicate meant opening each record one at a time (OP#1170). Surfaces it where validators actually work: - a row button on the membership list, which is what a program's Duplicates smart button opens; - a "Back to Draft" server action bound to that list, so a whole selection can be cleared at once - the shape spp_programs already uses for "Reset to Draft" on entitlements, including filtering the selection and complaining only when nothing in it qualifies; - a row button on the Participation list of both individual and group registrants, where the inline list gets no action menu of its own. Validator-level in every place, matching the existing form button rather than the officer-level Pause and Exit buttons beside it. Clearing a duplicate flag from a list should not be easier than doing it on the record. back_to_draft() is also guarded now. It wrote "draft" over any state at all, which was fine while one button behind a state modifier was the only caller and is not fine with four. A mixed selection is refused whole rather than half-applied.
Deduplication only ever added the flag. A membership marked "duplicated" stayed that way even once the clash behind it was fixed, because a membership already in that state was never re-evaluated out of it. Correct the ID or the phone number, press Deduplicate again, and nothing happened - which is what left validators with almost no way back to draft (OP#796). Clear the program's flags before the managers run, so the run recomputes rather than accumulates: whoever still clashes is flagged again a moment later, and whoever no longer does is left in draft. Both entry points go through it, the program's button and the membership's. It is one transaction, so a run that fails part-way leaves the flags as they were. The notification was wrong once a run could also un-flag: it derived "new" by subtracting counts, which goes negative as soon as something is resolved. It now compares the recordsets and reports new, still flagged and no longer duplicate. Two things found and deliberately left alone: last_deduplication is declared on the membership and written nowhere, and the duplicate record's duplicate/not_duplicate state is set by nothing, read by nothing and reachable from no button.
The button acts on the membership state, so it reads as following from Status rather than crowding the registrant at the start of the row. The registrant-side participation lists already placed it there. Refs OP#1170
Duplicate Detection was the last section of the program configuration page still rendered as a bare group under Optional Features, with the wrapper's manager_ref_id edited inline. That is a Reference field: it asks the user to pick a model and then find or create a record of it, which exposes the wrapper/concrete split and lets a manager belonging to another program be wired in by mistake. It is now a card beside Compliance, Payment and the rest, and adding a method goes through a dialog asking for the method and a name — the same shape Compliance and Payment got in #952/#953, with the extra step deduplication needs because it has three methods where those have one. The card body lists every configured method with its own cog rather than a summary line: a program may legitimately check by shared members and by phone. The header Edit is therefore offered only when there is exactly one method — a single button cannot open two, and opening them in a dialog list is worse still, because a dialog list cannot drill into a form and the rows look clickable while doing nothing. Refs OP#1171
… removed methods "Add a line" survived on the card. deduplication_manager_ids is a Many2many, and for those the list renderer reads `"link" in activeActions ? link : create`, so the list's create="0" and the create domain were both dead letters. The row that survived opened the link picker: every deduplication manager in the database, other programs' included, ready to be mis-wired into this one. The domain now covers 'link'. 'unlink' is left alone, so the row's x still removes a method. Removing a method that way drops the relation but keeps the wrapper, whose program_id still names the program. The duplicate check searched on that, so a method the card no longer showed still refused to be added back. It now asks the program's own field, and adding first sweeps wrappers that no program links.
The Add dialog asked for a method and a name, which is not enough for the ID document method. Its check is `id_type_id in supported_id_document_type_ids`, so a manager created with that list empty matches nothing and reports no duplicates at all — the card says Configured, Deduplicate runs, and nothing is ever flagged. QA asked for the list to appear on selecting ID document (OP#1171 finding 5). The dialog now shows the ID document types when that method is selected, with the same widget and options as the manager's own form, and requires at least one. action_create_manager refuses an empty list as well, because the view's required only binds the client and a programmatic caller would otherwise create the silent no-op. The manager's own form — reached from the card's cog — now requires the field too. Without that, the same empty manager could still be saved from there. Existing records keep what they have until someone edits them. Six tests in the wizard's suite created the ID method without types, which is the state now refused; they set them the way a user would, and one test deliberately omits them to assert the refusal.
19.0.2.3.0 — a minor, since it adds the deduplication setup wizard model and its access rows alongside the behaviour fixes. Covers OP#1171, OP#796 and OP#1170; the bump is required for the new model, the ACL rows and the view changes to load at all on an existing database.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #445 +/- ##
==========================================
- Coverage 76.45% 75.72% -0.73%
==========================================
Files 624 551 -73
Lines 42045 36322 -5723
==========================================
- Hits 32145 27506 -4639
+ Misses 9900 8816 -1084
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Thorough review against OP#745/#796/#1170/#1171. This is careful, well-tested work — I verified the client-side mechanism claims against the actual Odoo 19.0 web source and they all hold, and the test suite is genuinely strong (including the ACL test run as a real non-superuser manager, which most wizard PRs forget). Two must-fixes, both small, then this is good to go.
Verified — the claims all check out
- The
link-vs-createanalysis is exactly right.web/static/src/views/list/list_renderer.js:1075andx2many_field.js:139both read"link" in activeActions ? link : createfor a Many2many, so denyinglinkwas the only way to kill the "Add a line" row. The domain-valued options are evaluated vianew Domain(action).contains(evalContext)(relational_utils.js:129), and a program id is never< 0, so both keys stay denied.no_openis a real list attribute (list_arch_parser.js:220).unlinkuntouched, so the ✕ survives. - The wizard's create path matches
source_mixin.create:_spp_wrapper_modelbuilds the wrapper, and_spp_program_m2m_fieldis indeed load-bearing for a Many2many — without it the program never picks the manager up. - Recompute semantics are correct:
_reset_duplicate_flags→ managers re-run, all in one transaction so a failed run rolls the flags back; the set algebra behind the new/still/resolved message is right; the recompute tests (resolve-clears, unresolved-stays, run-twice-stable, membership-level path) pin all four behaviours. back_to_draftguard: recordset-safe, refuses mixed selections whole, and the server action filters before calling so one stray row doesn't block forty-nine — consistent with the entitlement Reset to Draft shape. Group bindings on the row buttons and the server action match the stated validator-level intent, andUserErroris available in the server-action eval context.- ACL coherence: the Add button is gated by
can_edit_configuration(admin/manager only), and the wizard rows cover exactly those. The sweep spares wrappers linked by other programs and survives a dangling Reference — both tested. - CI: green on the true head (
a3a06cbe); I checked thetest (spp_programs)log — all 52 tests across the three new/extended classes started and none were skipped. 713 tests, 0 failures. The 3 commits the branch is behind (#435/#432/#444) don't touch spp_programs, and 19.0.2.3.0 is the correct next number for a new model + ACLs.
Must fix 1 — @api.depends on _compute_banner_layout_helpers was not extended
The banners tuple gained ("deduplication_manager_ids", "deduplication"), but the decorator above it still lists only eligibility/entitlement/cycle/compliance/payment. So deduplication_manager_count/_display/_detail never invalidate when methods are added or removed within a transaction. Your own tests are the tell: test_the_count_drives_the_card_zero_state and the edit-path tests each need a manual self.program.invalidate_recordset() to pass — with the depends in place they wouldn't. The web client survives because every request gets a fresh env, but any server-side reader (automation, another compute, a future test without the workaround) gets a stale count. One-line fix:
@api.depends(
...,
"deduplication_manager_ids",
"deduplication_manager_ids.manager_ref_id",
)Must fix 2 — the sweep can delete a method another program still runs
_sweep_removed_methods checks whether the leftover wrapper is linked by any program, then unlinks the concrete. But source_mixin.unlink() (source_mixin.py:44-49) cascades to every wrapper whose manager_ref_id points at that concrete — search([("manager_ref_id", "=", manager_ref)]), all programs included. So if two wrappers point at one concrete — wrapper A, unlinked leftover on this program; wrapper B, linked to program B — the sweep deletes the concrete and the cascade silently takes program B's method with it. Program B's card goes back to "not configured" and its dedup runs stop, with no error anywhere.
Two wrappers sharing a concrete is precisely what the old Reference-field UI let users produce (it's the mis-wiring OP#1171 exists to stop), so upgraded databases can hold this shape. Suggested guard, mirroring the mixin's own lookup:
concrete = leftover.manager_ref_id
if concrete and concrete.exists():
ref = f"{concrete._name},{concrete.id}"
other_wrappers = self.env["spp.deduplication.manager"].search([("manager_ref_id", "=", ref)]) - leftover
(leftover if other_wrappers else concrete).unlink()
else:
leftover.unlink()(and a test with two wrappers on one concrete would pin it — the existing test_the_sweep_spares_a_method_another_program_uses covers the shared-wrapper case, not the shared-concrete one).
Nits — take or leave
- The wizard's validator ACL row (
access_spp_deduplication_setup_wizard_validator, create=1) grants a path validators can never use:can_edit_configurationis admin/manager-only, so the button never shows for them,action_add_deduplication_managerreturns False, and the concrete-model ACLs would block the create anyway. Either drop the row or leave a comment saying why validators need it. _("This program already has a %s deduplication method.") % dict(...)interpolates outside_(), unlike the%(name)s-inside-_()style the rest of the module uses — the placeholder value escapes translation extraction.- The membership-level
deduplicate_beneficiarieskeeps the old single-number notification rather than the new new/still/resolved breakdown — fine for a per-record button, just noting the asymmetry is deliberate so nobody "fixes" it later.
…ner depends The sweep deleted the concrete manager to clean up a leftover wrapper, but source_mixin.unlink() resolves wrappers by manager_ref_id across every program (get_managers_for_unlink), so a concrete reached by a second wrapper took that program's row with it — its card silently returned to "not configured" and its dedup runs stopped. Two wrappers on one concrete is the shape the Reference-field UI this wizard replaces allowed, so upgraded databases can hold it. When the concrete is shared, drop this program's relation row and leave the method alone. _compute_banner_layout_helpers gained a deduplication banner but not the matching @api.depends, so the count/display/detail fields never invalidated within a transaction. The web client hid it behind a fresh env per request; server-side readers got a stale count. The eight invalidate_recordset() calls the tests needed to pass are removed with it — they were the symptom. Also drops the deduplication wizard's validator ACL row, which granted a path Finance Validators cannot reach (the Add button is admin/manager only), and moves the duplicate-method message to the %(name)s-inside-_() style the rest of the module uses so the value stays translatable.
|
Both must-fixes addressed in Must fix 1 —
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approved — both must-fixes from the previous review are resolved in 6823bb37, verified end-to-end:
Must-fix 1 (banner @api.depends): deduplication_manager_ids + deduplication_manager_ids.manager_ref_id added, and — the part I checked hardest — all eight invalidate_recordset() calls are removed from the tests rather than kept alongside the fix, so the suite now genuinely proves the depends works.
Must-fix 2 (sweep cascade): verified the guard against the model layer: the wrapper (spp.manager.mixin) has no unlink override, so leftover.unlink() in the shared branch removes only this program's relation row, while the non-shared branch keeps the original concrete-cascade behavior. The Reference equality search ("model,id" string) is the correct idiom, and the dangling-Reference branch stays untangled from the shared-concrete decision. If one program somehow holds two leftovers on the same concrete, the loop converges correctly (first pass drops a wrapper, second pass deletes the concrete). The new test test_the_sweep_spares_a_method_reached_through_another_wrapper pins the exact failure shape — and I take the author's word, with the pasted assertion error, that it fails with the guard reverted.
Nits: validator ACL row dropped (agreed it was a copy-paste artifact — every other validator wizard row in that file is entitlement-flow), translation moved inside _(), membership-level message deliberately left as is.
CI on the true head 6823bb37: fully green; the spp_programs job log shows all 22 wizard tests starting (no skips), 714 tests, 0 failed, 0 errors.
Version chain re-verified after today's 19.0 merge-in: base moved to 19.0.2.2.1 (#436, and #241 took 2.2.0), this PR takes 19.0.2.3.0 which still sorts above; the HISTORY diff against 19.0 is exactly the 2.3.0 block, nothing lost in the merge.
19.0 released spp_programs 19.0.2.3.0 (deduplication, #445), which lands above the 2.2.2 / 2.2.3 this branch claimed. Left alone the manifest would regress to 2.2.3 and — worse — the #353 role-repointing migration in migrations/19.0.2.2.3 would never run on a database already upgraded to 2.3.0, since Odoo only runs scripts where installed < script <= target. The batch's two spp_programs fixes move above 2.3.0: #336 to 19.0.2.3.1 and #353 to 19.0.2.3.2, with the migration directory renamed to match. 19.0's 2.3.0 entry keeps its number and its code is untouched.
19.0 released spp_programs 19.0.2.3.0 (deduplication, #445), above the 19.0.2.2.4 this branch claimed. Left alone the manifest would regress and the new Tier-3 access-control entries would never be applied to a database already upgraded to 2.3.0. This branch's spp_programs entry moves to 19.0.2.3.3, above the 19.0.2.3.1 / 19.0.2.3.2 that the security batch-2 branch (#422) takes, so the two stack cleanly with #422 merging first. The access-control file conflict is a both-sides append: 19.0's deduplication wizard entries and this branch's Tier-3 registry-read entries are both kept. The two new entries are also renamed from access_spp_cycle_registry_registry_read / access_spp_cycle_membership_registry_registry_read, which doubled a word through a generation slip; nothing references the old identifiers and neither has shipped.
Resolves the overlap with #445, which promoted Duplicate Detection to a card while this branch promoted Notifications. Both cards are kept; the old "Optional Features" group that held each of them beforehand is gone, along with the closing tags it left behind. Two collisions the text merge could not see: spp.program had ended up with two write() methods in one class -- the configuration-isolation guard from this branch and the operation-lock guard from 19.0. Python keeps the later definition, so the isolation guard was silently gone and its test failed. Folded into one method, with a note that anything else hooking writes belongs there too. #445's sweep test built its fixture by linking one program's manager into another, which is precisely what this branch forbids. That shape can still exist in databases predating the rule, and sparing it is what the test is for, so it now writes the relation row directly instead of going through write(). Also drops the manager setup wizard's validator ACL row, matching the decision made for the deduplication wizard in #445: the Add button that opens it is admin/manager only, so the row granted a path validators cannot reach. spp_programs takes 19.0.2.4.0. 765 tests, 0 failures.
2 participants
Covers four tickets, all QA-passed: OP#1171, OP#796, OP#1170 and their parent OP#745.
Why is this change needed?
Adding a deduplication method meant editing the wrapper's
manager_ref_id— a Reference field, which asks the user to pick a model and then find or create a record of it. That exposes internal plumbing, and picking an existing record belonging to another program silently mis-wires the manager: it keeps running against the program it was created for while this program's form claims it is configured (OP#1171).Two more faults sat behind it:
duplicatedstayed that way after the clash behind it was fixed, because a membership already in that state was never re-evaluated out of it (OP#796).back_to_draft()existed only as a button on the membership form, so clearing duplicates meant opening records one at a time (OP#1170).How was the change implemented?
id_type_id in supported_id_document_type_ids, so a method created with none set matched nothing and reported no duplicates at all — the card said Configured and Deduplicate ran clean. Required in the dialog and on the method's own form, and refused server-side.link, notcreate— socreate="0"on the list was never read, and the row it left opened a picker listing every program's methods. Both keys are denied;unlinkis untouched, so the ✕ still removes a method.spp_programsalready uses for Reset to Draft on entitlements.New unit tests
spp_programs/tests/test_deduplication_setup_wizard.py(21 tests) covers the dialog end to end: each method can be added, the same one cannot be added twice, a removed one can be added back, the sweep spares a method another program links, a dangling Reference does not crash the Add button, the ID method is refused without types and the chosen types reach the manager. Plus arch assertions that no card offers an add row and that Edit appears only when exactly one method is configured.test_deduplication.pyandtest_program_membership.pycover the recompute and the bulk return.Unit tests executed by the author
Full
spp_programssuite after merging19.0in: 713 tests, 0 failed, 0 errors.How to test manually
Related links
Reviewer notes
spp_programsbumped to 19.0.2.3.0 (minor: adds the setup-wizard model and its ACL rows) with changelog entries, per the in-PR convention.link-vs-createdistinction is the one thing here that arch-level tests cannot prove, since it lives in the web client. QA checked it in the browser; the tests assert the attributes that drive it.