fix(spp_user_roles): prevent validation error when assigning groups to a new role#177
fix(spp_user_roles): prevent validation error when assigning groups to a new role#177anthonymarkQA wants to merge 4 commits into19.0from
Conversation
![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 workaround for an Odoo cache-clearing bug by overriding the create method in the ResUsersRoleCustomSPP model and updates the implied_ids field display in the XML view. Feedback includes a recommendation to narrow the scope of the create override to specifically target implied_ids to prevent interference with standard mechanisms, and a correction for a likely invalid field name (full_name) in the view definition.
| domain="[('role_id', '=', False)]" | ||
| > | ||
| <list create="0"> | ||
| <field name="full_name" string="Group" /> |
There was a problem hiding this comment.
The field full_name does not exist in the standard Odoo res.groups model. Unless it is added by a custom module not shown in this context, this will cause a traceback when loading the view. You should likely use name or display_name instead.
| <field name="full_name" string="Group" /> | |
| <field name="name" string="Group" /> |
| groups_vals_list = [] | ||
| group_fields = set(self.env["res.groups"]._fields) - {"name"} | ||
| for vals in vals_list: | ||
| group_vals = {} | ||
| for field in group_fields: | ||
| if field in vals: | ||
| group_vals[field] = vals.pop(field) | ||
| groups_vals_list.append(group_vals) | ||
|
|
||
| new_records = super().create(vals_list) | ||
|
|
||
| for record, group_vals in zip(new_records, groups_vals_list): | ||
| if group_vals: | ||
| record.group_id.write(group_vals) |
There was a problem hiding this comment.
The current implementation of the create override is broader than necessary. By popping all fields belonging to res.groups (except name), you risk interfering with the standard Odoo _inherits mechanism for fields that don't suffer from the cache-clearing bug. It also makes the creation fragile if other modules add required fields to res.groups. It is recommended to target only the problematic fields, specifically implied_ids, as mentioned in the PR description and mirroring the write workaround in base_user_role.
| groups_vals_list = [] | |
| group_fields = set(self.env["res.groups"]._fields) - {"name"} | |
| for vals in vals_list: | |
| group_vals = {} | |
| for field in group_fields: | |
| if field in vals: | |
| group_vals[field] = vals.pop(field) | |
| groups_vals_list.append(group_vals) | |
| new_records = super().create(vals_list) | |
| for record, group_vals in zip(new_records, groups_vals_list): | |
| if group_vals: | |
| record.group_id.write(group_vals) | |
| implied_ids_list = [vals.pop("implied_ids", None) for vals in vals_list] | |
| new_records = super().create(vals_list) | |
| for record, implied_ids in zip(new_records, implied_ids_list): | |
| if implied_ids: | |
| record.group_id.write({"implied_ids": implied_ids}) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #177 +/- ##
==========================================
- Coverage 71.61% 71.47% -0.14%
==========================================
Files 933 932 -1
Lines 55357 54830 -527
==========================================
- Hits 39643 39191 -452
+ Misses 15714 15639 -75
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
CI's ruff (B905) flagged `zip(new_records, groups_vals_list)` for omitting the explicit `strict=` parameter. The two iterables are built side-by-side in the same loop above the call, so they're guaranteed equal length — make it explicit with `strict=True` to satisfy the linter and turn an undetectable mismatch into a clear runtime error if the invariant ever breaks.
OCA xml-view-dangerous-replace-low-priority hook flagged the position="replace" used to add a domain and an embedded list to the `implied_ids` field on the role form. Replace is reserved as a last resort; the same effect can be achieved with position="attributes" (for the new domain) plus position="inside" (to embed the list). Functionally identical for OP#979 — the field still restricts the group dropdown to free groups and the inline list still has create disabled, but the inheritance is now safer for downstream views.
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
CI fully green after the two follow-ups (zip(strict=True) for ruff B905 and attributes + inside instead of position=replace for the OCA xml-view-dangerous-replace check). Functionality identical to the original fix — group dropdown restricted to free groups, inline create disabled to prevent OP#979's empty-row-creates-blank-group trap.
2 participants
Clicking "Add a line" in the Groups tab triggered inline creation of a
new res.groups record. Since res.groups.name is required and left blank,
Odoo threw a validation error and blocked the save entirely.
Two changes:
so "Add a line" opens a search dialog to select existing groups instead
of creating an inline row
directly to group_id after creation, mirroring the existing write()
workaround in base_user_role for the same Odoo _inherits cache-clearing bug
Why is this change needed?
Users unable to create a role and assign a group in one save. previously users must create an empty role first then save, then add a group into it. which is inneficient
How was the change implemented?
How was the change implemented?
1.
spp_user_roles/views/role.xmlReplaced the
implied_idsfield override with one that includes an explicitinner
<list create="0">. Settingcreate="0"on the inner list tells Odooto replace the "Add a line" inline row with an "Add" button that opens a
search/select dialog. This ensures only existing
res.groupsrecords can belinked to a role — no blank inline records are ever created.
2.
spp_user_roles/models/role.pyAdded a
create()override that mirrors the existingwrite()workaroundalready present in
base_user_role. Theres.users.rolemodel uses Odoo's_inheritsdelegation tores.groups, which means fields likeimplied_idsbelong to the parent
res.groupsrecord. On create, passingimplied_idsthrough the
_inheritsmechanism triggers an Odoo cache-clearing bug(documented in the upstream
write()override) that causes the value to bedropped. The fix extracts
implied_idsfrom vals before callingsuper().create(), lets the role and its associatedgroup_idbe createdfirst, then writes
implied_idsdirectly togroup_id— bypassing the bug.New unit tests
Unit tests executed by the author
How to test manually
Related links
https://projects.acn.fr/projects/acn-eng/work_packages/979/activity