[19.0][IMP] base_tier_validation: warn at definition save if reviewer lacks model ACL

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")

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(