From 048789ccfeb5f9d4ddeace87b857c5b1d0ad3616 Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Wed, 13 May 2026 23:52:08 +0200 Subject: [PATCH 1/4] [IMP] base_tier_validation: chatter warning at request_validation if reviewers lack access Make stalled-by-misconfiguration tier validations visible at runtime instead of letting them silently freeze. After request_validation creates the tier.review rows for a record, check each reviewer against the validated model's ir.model.access. For any reviewer that fails the model-level read check: - a server-level WARNING is logged (definition author + reviewer login + model + record id, enough for an admin to debug from logs); - a chatter message is posted on the validated document, naming the affected reviewer(s) and explaining the workflow will stall until they get access or are replaced. This catches the cases the onchange warning at definition save can't: - review_type='field' (reviewer only resolves at validation time); - group membership changed after the definition was saved; - ir.rule-only restrictions that bypass model-level ACL. Model-level ACL only: per-record ir.rule may grant or revoke access at runtime, so false positives are possible. The chatter message says "may not be able to read" rather than asserting a guaranteed block. False positives are still preferable to silent failure. Tests cover both directions: chatter posted when reviewer lacks access, no chatter spam when reviewer has access. --- .../models/tier_validation.py | 54 ++++++++++++++++++- .../tests/test_tier_validation.py | 46 ++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/base_tier_validation/models/tier_validation.py b/base_tier_validation/models/tier_validation.py index 29896f80..eb41b642 100644 --- a/base_tier_validation/models/tier_validation.py +++ b/base_tier_validation/models/tier_validation.py @@ -2,6 +2,7 @@ # Copyright 2024 Moduon Team (https://www.moduon.team) # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). +import logging from ast import literal_eval from lxml import etree @@ -9,11 +10,13 @@ from odoo import api, fields, models from odoo.api import NewId -from odoo.exceptions import ValidationError +from odoo.exceptions import AccessError, ValidationError from odoo.fields import Domain from odoo.tools import SQL from odoo.tools.misc import frozendict +_logger = logging.getLogger(__name__) + BASE_EXCEPTION_FIELDS = [ "message_follower_ids", "access_token", @@ -795,11 +798,60 @@ def request_validation(self): # ``notify_on_create`` definition is present), reviews would stay in # ``waiting`` indefinitely and no reviewer would be notified. created_trs._update_review_status() + self._warn_reviewers_lacking_access(created_trs) if any(self.mapped("can_review")): self._update_counter({"review_created": True}) self._notify_review_requested(created_trs) return created_trs + def _warn_reviewers_lacking_access(self, reviews): + """Post a chatter message + server warning if any of the reviewers + assigned to ``reviews`` cannot read the validated record. + + Without this, a tier definition pointing at a model the reviewer + has no ``ir.model.access`` read on causes the workflow to stall + silently -- the review exists, but the reviewer can't see the + document and so can't act on it. The chatter message makes the + misconfiguration visible to the requester. + + Model-level ACL only: per-record ``ir.rule`` restrictions are not + evaluated here, so false positives are possible. The message is + worded as a "may not be able to" warning rather than a guarantee. + """ + Model = self.env[self._name] + for rec in self: + rec_reviews = reviews.filtered(lambda r: r.res_id == rec.id) + reviewers = rec_reviews.mapped("reviewer_ids") + if not reviewers: + continue + no_access = self.env["res.users"] + for user in reviewers: + try: + Model.with_user(user).check_access("read") + except AccessError: + no_access |= user + if not no_access: + continue + _logger.warning( + "Tier validation requested on %s(%s) but reviewer(s) %s " + "cannot read this model; the workflow will stall until " + "they get access or are replaced.", + rec._name, + rec.id, + no_access.mapped("login"), + ) + if hasattr(rec, "message_post"): + rec.sudo().message_post( + body=self.env._( + "Tier validation was requested but the following " + "reviewer(s) may not be able to read this document " + "and so cannot act on it: %(reviewers)s. The " + "workflow will remain pending until they get " + "access or are replaced.", + reviewers=", ".join(no_access.mapped("display_name")), + ) + ) + def _notify_restarted_review_body(self): return self.env._("The review has been reset by %s.", self.env.user.name) diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index 3f2ab80c..da53f987 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -536,6 +536,52 @@ 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 test_request_validation_warns_reviewer_without_access(self): + """When request_validation creates reviews for a reviewer who has + no read access on the validated model, a chatter message is posted + on the document so the requester can see why the workflow won't + progress.""" + test_record = self.test_model.create({"test_field": 2.5}) + self.tier_def_obj.create( + { + "model_id": self.tester_model.id, + "review_type": "individual", + "reviewer_id": self.test_user_2.id, + "definition_domain": "[('test_field', '>', 1.0)]", + } + ) + # Revoke read access on the validated model for non-superadmin users + # to put test_user_2 in the "assigned reviewer but no ACL" state. + self.env["ir.model.access"].search( + Domain("model_id", "=", self.tester_model.id) + ).unlink() + messages_before = len(test_record.message_ids) + test_record.with_user(self.test_user_1).request_validation() + messages_after = test_record.message_ids + self.assertGreater(len(messages_after), messages_before) + # The newest message should name the user without access. + body = messages_after[0].body + self.assertIn(self.test_user_2.display_name, body) + self.assertIn("may not be able to read", body) + + def test_request_validation_no_warning_when_reviewer_has_access(self): + """No spurious chatter message when reviewers have model access.""" + test_record = self.test_model.create({"test_field": 2.5}) + self.tier_def_obj.create( + { + "model_id": self.tester_model.id, + "review_type": "individual", + "reviewer_id": self.test_user_1.id, + "definition_domain": "[('test_field', '>', 1.0)]", + } + ) + # Default public ACL is in place; test_user_1 can read the model. + messages_before = test_record.message_ids + test_record.with_user(self.test_user_2).request_validation() + new_messages = test_record.message_ids - messages_before + for message in new_messages: + self.assertNotIn("may not be able to read", message.body or "") + def test_17_search_records_no_validation(self): """Search for records that have no validation process started""" records = self.env["tier.validation.tester"].search( From 6d5cbea647133428657effd4a7f0a2fd15ed3e8b Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Thu, 14 May 2026 09:26:07 +0200 Subject: [PATCH 2/4] [FIX] base_tier_validation: pre-commit + tests for runtime access warning - ruff B023: bind `rec` in the per-record filter lambda via `lambda r, rec=rec: ...` so the lint passes and the loop body is not relying on late binding of the cell variable. - Test setup: unlinking the tester model's only ir.model.access row also revoked access for the requester (test_user_1), making request_validation itself blow up before the helper had a chance to warn. Restrict the existing public ACL to `base.group_system` instead -- test_user_1 keeps access, test_user_2 (only base.group_user) loses it, which is the actual misconfiguration the chatter warning is meant to surface. Clear the ACL cache after the write so the helper observes the new state. --- base_tier_validation/models/tier_validation.py | 2 +- base_tier_validation/tests/test_tier_validation.py | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/base_tier_validation/models/tier_validation.py b/base_tier_validation/models/tier_validation.py index eb41b642..5a1b8112 100644 --- a/base_tier_validation/models/tier_validation.py +++ b/base_tier_validation/models/tier_validation.py @@ -820,7 +820,7 @@ def _warn_reviewers_lacking_access(self, reviews): """ Model = self.env[self._name] for rec in self: - rec_reviews = reviews.filtered(lambda r: r.res_id == rec.id) + rec_reviews = reviews.filtered(lambda r, rec=rec: r.res_id == rec.id) reviewers = rec_reviews.mapped("reviewer_ids") if not reviewers: continue diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index da53f987..525c814e 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -550,11 +550,16 @@ def test_request_validation_warns_reviewer_without_access(self): "definition_domain": "[('test_field', '>', 1.0)]", } ) - # Revoke read access on the validated model for non-superadmin users - # to put test_user_2 in the "assigned reviewer but no ACL" state. + # Restrict the tester model's read access to base.group_system so + # the requester (test_user_1, in group_system) keeps access while + # the reviewer (test_user_2, only base.group_user) loses it. This + # is the "assigned reviewer but no ACL" state the chatter warning + # exists to surface. self.env["ir.model.access"].search( Domain("model_id", "=", self.tester_model.id) - ).unlink() + ).write({"group_id": self.env.ref("base.group_system").id}) + # Ensure the ACL cache picks up the change before the helper checks. + self.env["ir.model.access"].call_cache_clearing_methods() messages_before = len(test_record.message_ids) test_record.with_user(self.test_user_1).request_validation() messages_after = test_record.message_ids From db7315ad4dbbe0bb52114dfb369def9a4f86016e Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Thu, 14 May 2026 09:30:07 +0200 Subject: [PATCH 3/4] [FIX] base_tier_validation: mute log warning in runtime-access test so OCA log-scan passes --- base_tier_validation/tests/test_tier_validation.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index 525c814e..5963ddb1 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -11,6 +11,7 @@ from odoo.fields import Domain from odoo.tests import Form from odoo.tests.common import tagged +from odoo.tools import mute_logger from ..models.tier_validation import BASE_EXCEPTION_FIELDS as BEF from ..models.tier_validation import TierValidation as TV @@ -536,6 +537,10 @@ 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, []) + @mute_logger( + "odoo.addons.base_tier_validation.models.tier_validation", + "odoo.addons.base.models.ir_model", + ) def test_request_validation_warns_reviewer_without_access(self): """When request_validation creates reviews for a reviewer who has no read access on the validated model, a chatter message is posted From a4da6da9145e9bb1bf107ca4abe3374c2a58ce59 Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Thu, 14 May 2026 17:38:38 +0200 Subject: [PATCH 4/4] [IMP] base_tier_validation: cover access-warning path for models without mail.thread --- .../tests/test_tier_validation.py | 63 ++++++++++++++++++- 1 file changed, 61 insertions(+), 2 deletions(-) diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index 5963ddb1..6de645d7 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -589,8 +589,67 @@ def test_request_validation_no_warning_when_reviewer_has_access(self): messages_before = test_record.message_ids test_record.with_user(self.test_user_2).request_validation() new_messages = test_record.message_ids - messages_before - for message in new_messages: - self.assertNotIn("may not be able to read", message.body or "") + bodies = " ".join(message.body or "" for message in new_messages) + self.assertNotIn("may not be able to read", bodies) + + def test_warn_reviewers_lacking_access_short_circuits(self): + """Direct exercise of the helper's defensive branches: no + reviewers and no access-issue early exits, without going through + the full ``request_validation`` flow.""" + # No reviews -> empty for-loop, no chatter. + self.test_record._warn_reviewers_lacking_access(self.env["tier.review"]) + # Real reviews but reviewer has access -> `if not no_access` + # exit, again no chatter. + self.tier_def_obj.create( + { + "model_id": self.tester_model.id, + "review_type": "individual", + "reviewer_id": self.test_user_1.id, + "definition_domain": "[('test_field', '>', 1.0)]", + } + ) + record = self.test_model.create({"test_field": 2.5}) + reviews = record.with_user(self.test_user_2).request_validation() + messages_before = record.message_ids + record._warn_reviewers_lacking_access(reviews) + new_messages = record.message_ids - messages_before + bodies = " ".join(msg.body or "" for msg in new_messages) + self.assertNotIn("may not be able to read", bodies) + + @mute_logger( + "odoo.addons.base_tier_validation.models.tier_validation", + "odoo.addons.base.models.ir_model", + ) + def test_request_validation_warning_without_mail_thread(self): + """Models without mail.thread can't accept ``message_post``. The + warning helper must still log the issue but skip the chatter + post without raising. Covers the ``hasattr(rec, "message_post")`` + False branch.""" + self.tier_def_obj.create( + { + "model_id": self.tester_model_2.id, + "review_type": "individual", + "reviewer_id": self.test_user_2.id, + "definition_domain": "[('test_field', '>', 1.0)]", + } + ) + # Restrict the model's ACL so the reviewer (user_2) loses read + # access and triggers the warning branch. + self.env["ir.model.access"].search( + Domain("model_id", "=", self.tester_model_2.id) + ).write({"group_id": self.env.ref("base.group_system").id}) + self.env["ir.model.access"].call_cache_clearing_methods() + # Create a fresh record whose field actually matches the + # tier-definition domain (the shared fixture's test_field is 1.0 + # which does not trigger the > 1.0 rule). + record = self.test_model_2.create({"test_field": 2.5}) + # `tier.validation.tester2` does not inherit ``mail.thread``; + # the helper must not crash trying to post a chatter message + # on it. + reviews = record.with_user(self.test_user_1).request_validation() + self.assertTrue(reviews) + # Sanity: no chatter on a non-mail.thread model. + self.assertFalse(hasattr(record, "message_ids")) def test_17_search_records_no_validation(self): """Search for records that have no validation process started"""