diff --git a/base_tier_validation/models/tier_definition.py b/base_tier_validation/models/tier_definition.py index b36a82f69..cdcd2dc14 100644 --- a/base_tier_validation/models/tier_definition.py +++ b/base_tier_validation/models/tier_definition.py @@ -2,6 +2,7 @@ # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). from odoo import api, fields, models +from odoo.exceptions import AccessError from odoo.fields import Domain @@ -115,6 +116,75 @@ def onchange_review_type(self): self.reviewer_id = None self.reviewer_group_id = None + def _reviewers_without_model_access(self, model_name, users): + """Return the subset of ``users`` that cannot read ``model_name``. + + The check is model-level only (``ir.model.access``). It does not + evaluate per-record ``ir.rule`` restrictions, so callers should + treat a non-empty result as "*probably* no access" rather than a + guarantee -- record rules can grant or revoke access dynamically + at runtime. + """ + Model = self.env.get(model_name) + if Model is None: + return self.env["res.users"] + no_access = self.env["res.users"] + for user in users: + try: + Model.with_user(user).check_access("read") + except AccessError: + no_access |= user + return no_access + + def _reviewers_to_check_for_access(self): + """Return the user recordset whose model access we can check ahead + of time for this definition. Skip review types where the reviewer + only resolves at validation time (``field`` reads off the record).""" + self.ensure_one() + if self.review_type == "individual": + return self.reviewer_id + if self.review_type == "group": + return self.reviewer_group_id.user_ids + return self.env["res.users"] + + @api.onchange("review_type", "reviewer_id", "reviewer_group_id", "model_id") + def _onchange_warn_reviewer_access(self): + """Advisory warning when an assigned reviewer cannot read the model. + + Non-blocking: legitimate workflows (admin is about to grant the + group, ir.rules expose specific records, ...) still save. The + warning just makes it impossible to misconfigure this silently. + """ + # Read the model name off the m2o directly rather than via the + # stored related ``self.model`` -- on ``.new()`` records the + # related can still be empty depending on cache state. + model_name = self.model_id.model + if not (model_name and self.review_type): + return + users = self._reviewers_to_check_for_access() + if not users: + return + no_access = self._reviewers_without_model_access(model_name, users) + if not no_access: + return + return { + "warning": { + "title": self.env._("Reviewer may lack access"), + "message": self.env._( + "The following reviewer(s) may not be able to read " + "'%(model)s' records and so cannot act on the reviews " + "this tier will create: %(reviewers)s.\n\n" + "This is a best-effort check against model-level access " + "rights only -- record rules may still grant or revoke " + "access at runtime. Make sure these users belong to a " + "group with read access on the target model, or pick " + "different reviewers.", + model=self.model_id.name or model_name, + reviewers=", ".join(no_access.mapped("display_name")), + ), + } + } + @api.depends("review_type", "model_id") def _compute_domain_reviewer_field(self): models = self.mapped("model") diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index 3f2ab80cf..00c3958b2 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -536,6 +536,105 @@ def test_16b_review_user_count_no_model_access(self): result = self.test_user_2.with_user(self.test_user_2).review_user_count() self.assertEqual(result, []) + def _revoke_tester_model_access(self): + """Make the tester model unreadable for non-admin users by + unlinking its public ACL and clearing the ACL cache so the next + check_access actually re-evaluates against the new state.""" + self.env["ir.model.access"].search( + Domain("model_id", "=", self.tester_model.id) + ).unlink() + self.env["ir.model.access"].call_cache_clearing_methods() + + def test_definition_onchange_warns_when_reviewer_lacks_access(self): + """Setting an individual reviewer with no read access on the target + model returns a non-blocking onchange warning.""" + # Revoke first so the per-record check_access cache for test_user_2 + # never gets populated with a stale "allowed" result. + self._revoke_tester_model_access() + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "individual", + "reviewer_id": self.test_user_2.id, + } + ) + warning = definition._onchange_warn_reviewer_access() + self.assertIsNotNone(warning) + self.assertIn(self.test_user_2.display_name, warning["warning"]["message"]) + + def test_definition_onchange_warns_when_group_member_lacks_access(self): + """Group reviewer: if any member of the assigned group lacks read + access on the target model, the onchange warns and names them.""" + # Put test_user_2 in a fresh group; revoke the public ACL. + group = self.env["res.groups"].create( + {"name": "Tier Reviewers", "user_ids": [Command.link(self.test_user_2.id)]} + ) + self._revoke_tester_model_access() + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "group", + "reviewer_group_id": group.id, + } + ) + warning = definition._onchange_warn_reviewer_access() + self.assertIsNotNone(warning) + self.assertIn(self.test_user_2.display_name, warning["warning"]["message"]) + + def test_definition_onchange_skips_field_review_type(self): + """The 'field' review type cannot be checked ahead of time -- the + reviewer only resolves at validation time -- so the onchange + returns no warning even if the ACL would block.""" + self._revoke_tester_model_access() + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "field", + } + ) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + + def test_definition_onchange_returns_nothing_when_no_problem(self): + """The onchange must stay silent when there's nothing to warn + about. Covers the three early-return branches: + + - no model selected yet (``model_id`` empty); + - no reviewer set yet on a model that has one; + - reviewer has model access (default ACL in place). + """ + # No model -> early return on `if not (model_name and ...):`. + definition = self.tier_def_obj.new({"review_type": "individual"}) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + # No reviewer -> early return on `if not users:`. + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "individual", + } + ) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + # Reviewer with access (default public ACL is in place) -> the + # access check finds nothing to flag and the onchange returns + # None at the `if not no_access:` exit. + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "individual", + "reviewer_id": self.test_user_2.id, + } + ) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + + def test_reviewers_without_model_access_unknown_model(self): + """When the target model name doesn't resolve (e.g. a stale + reference after an uninstall), the helper returns an empty + res.users recordset without raising.""" + result = self.tier_def_obj._reviewers_without_model_access( + "this.model.does.not.exist", self.test_user_2 + ) + self.assertFalse(result) + self.assertEqual(result._name, "res.users") + def test_17_search_records_no_validation(self): """Search for records that have no validation process started""" records = self.env["tier.validation.tester"].search(