Skip to content

Commit

Permalink
[FIX] base: propagate changes on implied groups update
Browse files Browse the repository at this point in the history
Steps to reproduce:

1. Add a stored computed field on `res.users` that depends on `groups_id`
2. Add a constraint on `res.users` that depends on `groups_id`
3. Give the user a group
4. Modify the given group and add an implied group

Result:

- The computed field is not recomputed
- The constraint is not checked

The reason for this is that the users groups are updated with a raw SQL query,
and the ORM is not aware of the changes.

This is a regression since 5f12e24. This issue was partially detected and fixed
in 459e6dc, but only for a single constraint.

This commit makes sure that the ORM properly propagates the changes to computed
fields and all constraints, not just the one that was explicitly checked.

closes odoo#177301

Signed-off-by: Raphael Collet <rco@odoo.com>
  • Loading branch information
ivantodorovich committed Dec 10, 2024
1 parent 2bcc305 commit 7f52166
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 3 deletions.
19 changes: 18 additions & 1 deletion odoo/addons/base/models/res_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from odoo.osv import expression
from odoo.service.db import check_super
from odoo.tools import is_html_empty, partition, collections, frozendict, lazy_property
from odoo.tools.misc import OrderedSet

_logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -1319,6 +1320,8 @@ def write(self, values):
res = super(GroupsImplied, self).write(values)
if values.get('users') or values.get('implied_ids'):
# add all implied groups (to all users of each group)
updated_group_ids = OrderedSet()
updated_user_ids = OrderedSet()
for group in self:
self._cr.execute("""
WITH RECURSIVE group_imply(gid, hid) AS (
Expand All @@ -1339,8 +1342,22 @@ def write(self, values):
FROM res_groups_users_rel r
JOIN group_imply i ON (r.gid = i.hid)
WHERE i.gid = %(gid)s
RETURNING gid, uid
""", dict(gid=group.id))
self._check_one_user_type()
updated = self.env.cr.fetchall()
gids, uids = zip(*updated) if updated else ([], [])
updated_group_ids.update(gids)
updated_user_ids.update(uids)
# notify the ORM about the updated users and groups
updated_groups = self.env['res.groups'].browse(updated_group_ids)
updated_groups.invalidate_recordset(['users'])
updated_groups.modified(['users'])
updated_users = self.env['res.users'].browse(updated_user_ids)
updated_users.invalidate_recordset(['groups_id'])
updated_users.modified(['groups_id'])
# explicitly check constraints
updated_groups._validate_fields(['users'])
updated_users._validate_fields(['groups_id'])
return res

def _apply_group(self, implied_group):
Expand Down
35 changes: 33 additions & 2 deletions odoo/addons/base/tests/test_user_has_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def test_two_user_types_implied_groups(self):
"""Contrarily to test_two_user_types, we simply add an implied_id to a group.
This will trigger the addition of the relevant users to the relevant groups;
if, say, this was done in SQL and thus bypassing the ORM, it would bypass the constraints
and thus give us a case uncovered by the aforementioned test.
and field recomputations, and thus give us a case uncovered by the aforementioned test.
"""
grp_test = self.env["res.groups"].create(
{"name": "test", "implied_ids": [Command.set([self.grp_internal.id])]})
Expand All @@ -175,9 +175,40 @@ def test_two_user_types_implied_groups(self):
'groups_id': [Command.set([grp_test.id])]
})

with self.assertRaises(ValidationError):
with self.assertRaisesRegex(ValidationError, "The user cannot have more than one user types"), self.env.cr.savepoint():
grp_test.write({'implied_ids': [Command.link(self.grp_portal.id)]})

self.env["ir.model.fields"].create(
{
"name": "x_group_names",
"model_id": self.env.ref("base.model_res_users").id,
"state": "manual",
"field_description": "A computed field that depends on groups_id",
"compute": "for r in self: r['x_group_names'] = ', '.join(r.groups_id.mapped('name'))",
"depends": "groups_id",
"store": True,
"ttype": "char",
}
)
self.env["ir.model.fields"].create(
{
"name": "x_user_names",
"model_id": self.env.ref("base.model_res_groups").id,
"state": "manual",
"field_description": "A computed field that depends on users",
"compute": "for r in self: r['x_user_names'] = ', '.join(r.users.mapped('name'))",
"depends": "users",
"store": True,
"ttype": "char",
}
)

grp_additional = self.env["res.groups"].create({"name": "additional"})
grp_test.write({'implied_ids': [Command.link(grp_additional.id)]})

self.assertIn(grp_additional.name, test_user.x_group_names)
self.assertIn(test_user.name, grp_additional.x_user_names)

def test_demote_user(self):
"""When a user is demoted to the status of portal/public,
we should strip him of all his (previous) rights
Expand Down

0 comments on commit 7f52166

Please sign in to comment.