fix(roles): complete OP#951 menu audit — role group updates + menu gating#192
fix(roles): complete OP#951 menu audit — role group updates + menu gating#192emjay0921 wants to merge 12 commits into
Conversation
…(PR-A additive) Per the role/menu audit on OP#951, extend roles in 7 modules with the additional viewer-tier groups they need so menu visibility stays correct once the Hazard / GIS Reports / GRM menu roots are gated in a follow-up. This PR is intentionally additive — no role loses access: - spp_hazard, spp_gis_report, spp_area, spp_service_points: new data/user_roles.xml extending spp_user_roles roles with the relevant viewer-tier group (group_hazard_viewer, group_gis_report_user, group_area_viewer, group_service_points_viewer). - spp_programs/data/user_roles.xml: Program Viewer additionally implies group_registry_viewer + group_approval_viewer; Program Viewer / Validator / Cycle Approver all additionally imply group_hazard_viewer + group_gis_report_user. Adds spp_hazard + spp_gis_report to depends. - spp_change_request_v2/data/user_roles.xml: all 3 CR roles swap group_registry_read (Tier-3, no menu) for group_registry_viewer (Tier-2, with menu) and add group_hazard_viewer. Adds spp_hazard to depends. - spp_farmer_registry/data/user_roles.xml: Farm User + Farm Manager additionally imply group_hazard_viewer + group_gis_report_user. Adds spp_hazard + spp_gis_report to depends. System Admin (group_spp_admin) transitively implies every domain manager via existing extensions in each module's groups.xml, so no admin-role change is needed. Module versions bumped (all 7); HISTORY.md updated per module.
![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 implements a security and menu audit (OP#951) across several modules, updating user roles to ensure correct visibility for Registry, Hazard, GIS Reports, and Service Points. Feedback highlights that role updates within noupdate="1" blocks require migration scripts for existing installations, and several modules need explicit dependencies on spp_user_roles. Additionally, the documentation formatting in spp_programs needs correction as table width changes broke RST syntax and manual edits were incorrectly applied to an auto-generated HTML file.
| Command.link(ref('spp_registry.group_registry_viewer')), | ||
| Command.link(ref('spp_approval.group_approval_viewer')), | ||
| Command.link(ref('spp_hazard.group_hazard_viewer')), | ||
| Command.link(ref('spp_gis_report.group_gis_report_user')), |
There was a problem hiding this comment.
These changes are located within a block in a file that already exists in the module. In Odoo, records marked with noupdate="1" are not updated during a standard module upgrade. Consequently, existing roles in the database will not receive these new group memberships. To ensure these changes are applied to existing installations, you should use a migration script or a call to update the roles programmatically.
| Command.link(ref('spp_registry.group_registry_viewer')), | ||
| Command.link(ref('spp_hazard.group_hazard_viewer')), |
There was a problem hiding this comment.
The modifications to these role records are inside a block. Since this file is already part of the module's data, these changes will not be applied to existing databases during a standard upgrade. A migration script or a call is required to ensure existing roles are updated to align with the menu audit.
| +----------------------------------+----------------------------------+ | ||
| | Model | Description | | ||
| +==================================+==================================+ |
There was a problem hiding this comment.
The table formatting for "Key Models" has been broken by reducing the column width. Literal strings (model names) are now split across multiple lines, which breaks the RST literal syntax (backticks) and makes the documentation unreadable in the generated HTML. Please increase the column width to accommodate the longest model names without splitting them.
| +----------------------------------+----------------------------------+ | |
| | Model | Description | | |
| +==================================+==================================+ | |
| +---------------------------------------------+----------------------------------+ | |
| | Model | Description | | |
| +=============================================+==================================+ |
| <tr><td>`` | ||
| spp.program.membership.manager``</td> |
There was a problem hiding this comment.
This auto-generated file appears to have been manually edited to include RST-style backticks (``) instead of proper HTML tags. Manual changes to files in static/description/ are discouraged as they will be overwritten by the `oca-gen-addon-readme` tool. Additionally, RST syntax is not valid HTML and will not render correctly in the browser. Please fix the source `README.rst` and regenerate this file using the tool.
References
- Manual changes to files auto-generated by the oca-gen-addon-readme tool will be overwritten; any necessary changes should be made in the source file or the upstream tool.
| "security/groups.xml", | ||
| "security/ir.model.access.csv", | ||
| "data/impact_type_data.xml", | ||
| "data/user_roles.xml", |
There was a problem hiding this comment.
The newly added data/user_roles.xml references external IDs from the spp_user_roles module (e.g., spp_user_roles.global_role_registry_viewer), but spp_user_roles is not listed as a dependency in the manifest. While it may be transitively included via other modules, it is best practice to explicitly declare dependencies on any module whose data you reference to ensure correct loading order and prevent future breakages.
| "security/ir.model.access.csv", | ||
| # Data | ||
| "data/gis_report_category_data.xml", | ||
| "data/user_roles.xml", |
There was a problem hiding this comment.
The newly added data/user_roles.xml references external IDs from the spp_user_roles module, but spp_user_roles is not listed as a dependency in the manifest. Please add it to the depends list to ensure correct loading order.
| ], | ||
| "data": [ | ||
| "data/vocabularies.xml", | ||
| "data/user_roles.xml", |
There was a problem hiding this comment.
The newly added data/user_roles.xml references external IDs from the spp_user_roles module, but spp_user_roles is not listed as a dependency in the manifest. Please add it to the depends list to ensure correct loading order.
Modules spp_area and spp_service_points had data/user_roles.xml listed
before security/groups.xml in __manifest__.py, so when the role file
tried to ref('group_area_viewer') / ref('group_service_points_viewer')
the group records didn't exist yet, raising:
ValueError: External ID not found in the system: spp_area.group_area_viewer
Move user_roles.xml after the security/ entries so the local group XIDs
resolve. spp_hazard and spp_gis_report already had the correct order.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #192 +/- ##
=======================================
Coverage 71.70% 71.70%
=======================================
Files 942 942
Lines 55563 55563
=======================================
Hits 39844 39844
Misses 15719 15719
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ier-3 swap (OP#951 PR-B/C) Completes the OP#951 menu audit on top of the additive role changes in 82d62ae / f8b920e / 100ba93. Two structural changes: PR-B — top-level menu gating (3 modules): - spp_hazard/views/menu.xml: add groups='spp_hazard.group_hazard_viewer' to hazard_main_menu_root. Was previously visible to every logged-in user (no groups= attribute). - spp_gis_report/views/menu.xml: add groups='spp_gis_report.group_gis_report_user' to menu_gis_report_root. Same — previously ungated. - spp_grm/views/grm_ticket_menu.xml: add groups='spp_grm.group_grm_viewer' to spp_grm_ticket_main_menu. Same — previously ungated. Combined with PR-A's role additions, the audit's no-entries (Hazard / GIS Reports / GRM hidden from Finance, Support, Support Manager, Local Support, Registry Viewer for GIS, etc.) now take effect. PR-C — Global Program Cycle Approver Registry menu hide: - spp_programs/data/user_roles.xml: replace spp_registry.group_registry_viewer (Tier-2, gates menu) with spp_registry.group_registry_write (Tier-3, ACL-only) on Cycle Approver's implied_ids. Tier-3 transitively implies group_registry_read, so read+write data access via Programs cross-references is preserved — only the dedicated Registry top-level menu disappears, matching the audit's no for this role. Modules bumped: spp_grm 19.0.2.0.0 -> 19.0.2.0.1 with HISTORY entry. spp_hazard/spp_gis_report/spp_programs HISTORY entries extended under their existing 19.0.2.0.1 / 19.0.2.1.1 sections from PR-A. QA note: spp_programs/data/user_roles.xml uses noupdate='1'. The Cycle Approver group surgery only takes effect on FRESH install (resetdb + reinstall). Existing deployments need a migration script to call rec.implied_ids = [Command.unlink(ref('group_registry_viewer')), Command.link(ref('group_registry_write'))] manually. For QA via the standard wipe-and-reload flow this is not a concern.
emjay0921
changed the title
…1 menu audit OP#951 menu audit specifies that Global Program Manager should NOT see the Studio top-level menu, but spp_studio/data/user_roles.xml extended the role with group_studio_viewer which gates the menu's visibility. Remove the extension file entirely and de-register it from the manifest data list. System Admin keeps Studio visibility via spp_security.group_spp_admin → group_studio_manager (wired in spp_studio/security/groups.xml), so the only effective change is that Program Manager no longer sees the Studio menu. Migration caveat (same as PR-C): noupdate='1' on the previous extension means existing prod deployments retain group_studio_viewer on Program Manager users until a separate migration script unlinks it. Fresh installs (the wipe-and-reload QA flow) get the correct behavior automatically.
Out of the box the Odoo Apps top-level menu (base.menu_management) has no `groups` attribute and is visible to every logged-in user. The OP#951 role/menu audit specifies that only System Admin should see Apps; every other role is annotated `Apps no`. Verified on the live MIS instance (port 32772) by logging in as the test_registry_viewer user: Apps was still visible because the menu was ungated, not because the role had base.group_system. Override base.menu_management to set group_ids = base.group_system. System Admin is the only OpenSPP role that pulls in base.group_system (via spp_user_roles.global_role_admin → Command.link(base.group_system)), so this single override enforces the audit without touching individual roles.
…override Move the base.menu_management group_ids write out of groups_admin.xml and into a new _gate_apps_menu function called from post_init_hook. XML record overrides for cross-module Many2many fields are unreliable: the override can be wiped when the owning module (base) gets upgraded, silently reverting our gating. Hooks are idempotent and re-apply on every spp_security install/upgrade, which is the same pattern this module already uses to wire spp_security.group_spp_admin into base.group_system.implied_ids. No behavior change in steady state — the hook produces the same effect as the XML record did, just more robustly.
…g menuitem override (OP#951) Earlier commits in this PR attempted the gating via spp_security: first as an XML record override (e70a983), then as a post_init_hook (cc11557). Both produced empty group_ids on a freshly installed DB — verified on port 32774 with spp_security at 19.0.2.0.1. Root cause: spp_base_common's views/main_view.xml already overrides base.menu_management with its own <menuitem> (to set the OpenSPP web icon). spp_base_common loads AFTER spp_security in the dep graph, and in Odoo 19 a <menuitem> without an explicit `groups` attribute resets group_ids on the target record. So any group_ids that spp_security's hook or XML override writes earlier gets clobbered later by spp_base_common's reload. Fix: gate the menu where it's already declared. Add `groups="base.group_system"` to the existing menuitem in spp_base_common, and revert the spp_security gymnastics (version bump, HISTORY entry, _gate_apps_menu hook, migration script). System Admin is the only OpenSPP role that pulls in base.group_system (via spp_user_roles.global_role_admin → Command.link(base.group_system)), so this single attribute hides Apps from every other role and matches the OP#951 audit's `Apps: no` rows. Modules touched: - spp_base_common: + groups attr on Apps menuitem; bump 19.0.2.0.0 → 19.0.2.0.1 + HISTORY entry - spp_security: revert version bump, drop _gate_apps_menu hook and associated comment, no migration script needed
1 participant
Why is this change needed?
OP#951 (Registry role-based left-hand menu audit) produced a spreadsheet specifying, per role, which top-level menus each user role should see. Cross-referencing the audit against the current
res.users.roleandir.ui.menudefinitions surfaced three categories of work that all need to land together for the audit to take effect:groups=attribute and are visible to every logged-in user. Without gating, no audit "no" can be enforced.This PR ships all three together. Split internally into 3 implementation rounds (A → B → C) but landing as one commit chain so the audit is delivered atomically — splitting into separate PRs would have left half-applied behavior between merges and required two QA rounds.
How was the change implemented?
PR-A — Additive role group adds (7 modules)
Followed the existing pattern (
spp_programs/data/user_roles.xmlextends roles defined inspp_user_roles):spp_hazard/data/user_roles.xml(new): grantsgroup_hazard_viewerto Registry Viewer, Program Manager, Global Registrar, Local Registrar.spp_gis_report/data/user_roles.xml(new): grantsgroup_gis_report_userto Program Manager.spp_area/data/user_roles.xml(new): grantsgroup_area_viewerto Global Support, Global Support Manager, Local Support.spp_service_points/data/user_roles.xml(new): grantsgroup_service_points_viewerto Global Registrar, Local Registrar.spp_programs/data/user_roles.xml(edit):group_registry_viewer,group_approval_viewer,group_hazard_viewer,group_gis_report_usergroup_hazard_viewer,group_gis_report_usergroup_hazard_viewer,group_gis_report_userspp_hazardandspp_gis_reportto module dependencies.spp_change_request_v2/data/user_roles.xml(edit): all 3 CR roles swapspp_registry.group_registry_read(Tier-3 ACL, no menu) forspp_registry.group_registry_viewer(Tier-2, with menu) and addspp_hazard.group_hazard_viewer. Addsspp_hazardto deps.spp_farmer_registry/data/user_roles.xml(edit): Farm User + Farm Manager getgroup_hazard_viewer+group_gis_report_user. Addsspp_hazard+spp_gis_reportto deps.System Admin is unchanged —
spp_security.group_spp_adminalready transitively implies every domain manager via the existing per-module extensions.PR-B — Top-level menu gating (3 modules)
spp_hazard/views/menu.xml: addgroups="spp_hazard.group_hazard_viewer"tohazard_main_menu_root.spp_gis_report/views/menu.xml: addgroups="spp_gis_report.group_gis_report_user"tomenu_gis_report_root.spp_grm/views/grm_ticket_menu.xml: addgroups="spp_grm.group_grm_viewer"tospp_grm_ticket_main_menu.Combined with PR-A's role additions, the audit's "no" entries (Hazard / GIS Reports / GRM hidden from Finance, Support, Support Manager, Local Support, Registry Viewer for GIS, etc.) now take effect. Every role keeps the menus the audit says it should keep, because PR-A pre-emptively wired the viewer groups.
PR-C — Cycle Approver Registry-menu hide
spp_programs/data/user_roles.xml: onglobal_role_program_cycle_approver, replacespp_registry.group_registry_viewer(Tier-2, gates Registry menu) withspp_registry.group_registry_write(Tier-3, ACL-only).group_registry_writetransitively impliesgroup_registry_read, so the role keeps full read+write data access via Programs cross-references — only the dedicated Registry top-level menu disappears. This matches the audit's "Registry: no" for this role + the user's decision to retain data-access rights.8 module versions bumped (patch); HISTORY.md updated per module.
New unit tests
None — the change is data-only (XML role records + menu attributes). No new Python logic.
Unit tests executed by the author
Per-module
pre-commit run --files <PR-scoped paths>passed locally (CI-matched container).How to test manually
Per the OP#951 spreadsheet, test on a
./spp start --wipeinstance:admin, navigate to Settings → Users → Roles, open each affected role, and verify the Implied Groups tab matches the audit (e.g. Program Viewer should now list Registry: Viewer, Approval: Viewer, Hazard: Viewer, GIS Reports: User alongside its existing Programs: Viewer).Migration caveat for prod: PR-C uses
noupdate="1"so the Cycle Approver registry-group surgery only fires on fresh install. Existing deployments with users already in Cycle Approver retaingroup_registry_viewerand need a separate migration script to apply the swap on upgrade. For the wipe-and-reload QA flow this is automatic.Related links