diff --git a/base_tier_validation/models/tier_validation.py b/base_tier_validation/models/tier_validation.py index 29896f80..5a1b8112 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, rec=rec: 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..6de645d7 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,120 @@ 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 + 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)]", + } + ) + # 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) + ).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 + 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 + 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""" records = self.env["tier.validation.tester"].search(