feat(registry): consolidate registry configuration into a Registry Settings section - #440
Conversation
…y config (#1009) - New 'Registry Settings' section in the Settings app (spp_registry), mirroring 'Programs Settings', carrying the 'Restrict Registry Edits to Admin Only' toggle. - The toggle keeps both legacy config-parameter keys (spp_farmer_registry.registry_admin_only_crud and spp_starter.registry_admin_only_crud) in sync via get_values/set_values, so the spp_farmer_registry and spp_starter_sp_mis controllers keep reading their own key — no migration, no enforcement change. Removed the now-duplicate 'Farmer Registry Settings' and 'SP-MIS Settings' sections and their field definitions. - Relocated the API V2 and Import Match menus out of Registry > Configuration into a new 'Registry' group under the Settings app (Settings > Registry), and updated the API V2 security compliance manifest to match.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #440 +/- ##
==========================================
- Coverage 75.01% 72.77% -2.24%
==========================================
Files 524 556 +32
Lines 35301 37851 +2550
==========================================
+ Hits 26480 27547 +1067
- Misses 8821 10304 +1483
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.
The consolidation design is right — one toggle, dual legacy keys kept in sync so neither starter's controller changes, and the Settings-app grant done through menu group_ids with Command.link rather than an implied_ids escalation, which is exactly the mechanism our security posture wants. The central toggle also stores "False" explicitly in set_values, so the OP#1142 off-is-unrepresentable trap is avoided. compliance.yaml matches the menu move. All of that verified out.
Three findings, one of them functional:
1. MAJOR — the granted personas can open Registry Settings but cannot save it
res.config.settings.execute() refuses non-administrators (odoo/addons/base/models/res_config.py:366 — if not self.env.is_admin(): raise AccessError(...)), and is_admin means superuser or base.group_erp_manager (res_users.py:1181). Neither spp_security.group_spp_admin nor spp_registry.group_registry_config_admin has or implies that group — the implication deliberately runs the other way (base.group_system implies group_spp_admin, spp_security/security/groups_admin.xml:38).
So the grant delivers, to exactly the personas it exists to serve, a settings form that throws "Only administrators can change the settings" on Save. The relocated Configuration menus (API V2, Import Match) genuinely work — they're ordinary actions with their own gates — but General Settings is reach-without-use. QA passed because the walkthrough ran as admin/admin (a base.group_system user), and the persona checks in the test guide verified reachability, not saving; the PR's three unit tests cover the key-sync logic only.
Options, in order of preference:
- Hide
menu_registry_settings_generalfrom non-erp-manager users (leave the Configuration menus granted as they are) and say in the section help that changing the toggle requires a system administrator. Honest UI, no security change. - Give the toggle a dedicated, writable form outside
res.config.settings, gated ongroup_registry_config_admin— more work, but delivers the ticket's intent for those personas. - Granting
base.group_erp_managerto these groups is a real privilege escalation and would need Edwin's explicit sign-off — recommend against, consistent with the #327/#329 fixes.
Whichever way: please add a test that saves the settings as each persona, so this class of gap can't pass green again.
2. Blocker — structural collision with approved PR #411
This PR deletes spp_starter_sp_mis/models/res_config_settings.py. #411 (approved, awaiting a merge gate) rewrote that file — and its res_partner.py imports REGISTRY_ADMIN_ONLY_CRUD_PARAM from it. Merged in the wrong order, spp_starter_sp_mis fails to load with an ImportError; merged in the other order, the branches conflict textually. Recommended sequence: #411 lands first, then this PR rebases — keep the constant (move it into res_partner.py, or keep a slim res_config_settings.py holding only the constant), and delete only the field and set_values override that the central toggle supersedes. Your dual-key design means #411's migration, noupdate fix and enforcement all remain fully valid underneath this PR.
3. Blocker — no version bumps on the five released modules
spp_registry, spp_farmer_registry, spp_starter_sp_mis, spp_api_v2, spp_import_match — all touched, none bumped, and this PR is almost entirely data (menus re-parented, sections removed, new view/menu records). Your own QA guide documents the failure mode: partial upgrades leave stale sections and menus behind. Per the settled convention: bumps + HISTORY in the PR, numbers reconciled at merge time (note spp_registry now has its own ordering: #418 pending at 19.0.2.1.5, plus #437 and this PR).
Minor
get_values's "any explicit True wins" is fine under the documented at-most-one-key assumption; a pre-#440 database with both starters and drifted keys re-syncs on first save — acceptable, worth one line in HISTORY.- The
base.menu_administrationoverride lives inspp_registrydata, so the links linger if the module is ever uninstalled — cosmetic, no action needed.
With #1 resolved (option 1 is a small change), the bumps added, and the #411 sequencing agreed, this is an approve.
Resolves the three review findings. The Save gap: res.config.settings.execute() refuses anyone who is not an Odoo administrator, and is_admin means superuser or base.group_erp_manager — which neither group_spp_admin nor group_registry_config_admin has or implies. Offering General Settings to those personas therefore produced a form that threw on Save. The menu is now gated on base.group_erp_manager, the section help says a Settings administrator is required, and five tests pin it: each persona is refused by execute(), an erp_manager saves and both legacy keys follow, the menu carries the group that can save, the relocated configuration menus stay available to both personas, and the help says so. Granting group_erp_manager to those groups would have been a privilege escalation, so the menu follows the permission rather than the reverse. The #411 collision: this branch deleted spp_starter_sp_mis's res_config_settings, which #411 rewrote and whose constant res_partner imports. Now that #411 is merged, the file is kept slimmed to that constant, with only the field and set_values the central toggle supersedes removed — so #411's migration, noupdate fix and enforcement all stand. Its DESCRIPTION.md no longer advertises the field that moved, and points at the new location. Version bumps with changelog entries on all five touched modules, since this is almost entirely data: spp_registry 19.0.2.2.1 (above #437's 2.2.0), spp_farmer_registry 19.0.2.0.4, spp_starter_sp_mis 19.0.2.1.1, spp_api_v2 19.0.2.1.1, spp_import_match 19.0.2.0.3.
Five modules' changelog entries, plus spp_starter_sp_mis's description, which still advertised the toggle field that moved into Registry Settings.
|
Thanks — all three addressed. Pushed as 1. The Save gap — fixed, and the wall is higher than either of us thoughtTook option 1: Worth correcting the diagnosis, because it changes which group is right. You identified That message is misleading — two groups in this database are named "Administrator" — so I resolved the ACL row directly: it grants Five tests, per your ask that this cannot pass green again:
2. The #411 collision — resolved in the order you recommended#411 has since merged, so this branch rebased onto it. Its 3. Version bumps — all five, with changelog entries
MinorNoted on the Suites
Back to you. Not merged: the group change and the slimmed file are both worth your eyes rather than my assumption that "option 1" landed where you meant it. |
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Re-review of the three requested changes — all addressed, and one of them better than asked. Approving.
1. The Save gap — fixed, and your correction to my diagnosis is right
I verified the ACL you resolved: base/security/ir.model.access.csv line 131 grants res.config.settings create to base.group_system alone, so the wall is indeed at create, before the execute() gate I identified — and an erp-manager-only gating would have moved the persona one step further into the same dead end, exactly as you said. group_system implies erp_manager, so it clears both. Good catch; the menu now follows the real permission.
The five tests are exactly what I asked for and I confirmed all of them (plus the three key-sync tests) ran in the CI test (spp_registry) job — started, none skipped, 253 tests 0 failed. The persona-refusal test pinning both gates in one create(...).execute() call is the regression net this needed.
2. The #411 reconciliation — verified intact
REGISTRY_ADMIN_ONLY_CRUD_PARAM survives in spp_starter_sp_mis/models/res_config_settings.py and res_partner.py:6 still imports it; only the superseded field and set_values override are gone. #411's migration and data/config_parameters.xml are untouched, both starter modules still ship install defaults for their keys (so the OP#1142 missing-row drift cannot recur), and the central toggle's dual-key write matches what each controller reads. The DESCRIPTION.md update is a nice touch.
3. Version bumps — all five verified against 19.0
Each module takes the next free number except spp_registry's deliberate leapfrog to 2.2.1 over the open #418 (2.1.5) and #437 (2.2.0) — the right call given merge-order uncertainty, and we'll renumber whichever of those lands after this. Changelog entries present, READMEs regenerated.
Notes, not blockers
- The substantive fixes travelled inside the
Merge branch '19.0'commit rather than their own commits, and the "regenerate READMEs" commit also carries test and view edits. The tree is right, which is what I reviewed — but standalone fix commits would have made this re-review diffable commit-by-commit. Worth doing next time. - The
base.menu_administrationgroup_idsextension is additive viaCommand.linkas before — still the correct non-escalating shape.
Approved. Merge waits on Edwin's authorization, per process.
Resolves the spp_registry version ladder after #440 landed 19.0.2.2.1: this branch's #1136 entry moves 19.0.2.2.0 -> 19.0.2.2.2, and the migration directory is renamed to match. A 19.0.2.2.0 script would be skipped on any site already upgraded to 2.2.1 (Odoo runs scripts where installed < script <= new), leaving the old constraint in place and the fix inert. README.rst and index.html regenerated.
2 participants
Why is this change needed?
Registry configuration lived in two places (OP#1009):
The ticket asks for one Registry Settings section in Settings, equivalent to Programs Settings, with the registry configuration moved there — including superuser configuration such as API V2, which sat under Registry → Configuration.
How was the change implemented?
spp_registry, mirroring Programs Settings, carrying Restrict Registry Edits to Admin Only.get_values/set_valueskeepspp_farmer_registry.registry_admin_only_crudandspp_starter.registry_admin_only_crudin sync, so the Farmer Registry and SP-MIS controllers keep reading their own parameter unchanged — no migration and no change to how the restriction is enforced. The now-duplicate Farmer Registry Settings and SP-MIS Settings sections and their field definitions are removed.base.menu_administration). The API V2 security compliance manifest is updated to match.spp_security.group_spp_adminandspp_registry.group_registry_config_admin. Additive viaCommand.link, so stock system / access-rights visibility is preserved and every section under Settings still enforces its own group gate.res_config_settings_views.xml, which the manifest loads beforemain_view.xmlso the reference resolves.New unit tests
spp_registry/tests/test_res_config_settings.py— the Registry Settings section reads and writes the toggle, and both legacy parameter keys stay in sync in each direction, which is the property that replaces a data migration.Unit tests executed by the author
After merging
19.0in, all five affected modules installed together against current19.0—spp_registry,spp_starter_sp_mis,spp_farmer_registry,spp_api_v2,spp_import_match: 1069 tests, 0 failed, 1 error.That one error is pre-existing on
19.0and unrelated to this branch:spp_registry/tests/test_phone_number.py::test_unparseable_phone_falls_back_to_originalexpects"abcxyz"to fall through tophone_sanitized, whilespp_base_common/models/phone_number.py:41raisesPhone number must not contain letters.from itscreate()override. Both files are unchanged here and untouched by this branch. It stays hidden in CI becausespp_registrydoes not depend onspp_base_common, so the per-module matrix never installs them together; it surfaces only in a combined install like the one above. Raising it separately.How to test manually
registry_admin_only_crud— both keys follow the toggle in both directions.Related links