From c41ab558073f3f3ca61da470415f59d47044c89d Mon Sep 17 00:00:00 2001 From: Lancelot Date: Fri, 10 Apr 2026 12:57:08 +0200 Subject: [PATCH 1/2] [IMP] base_tier_validation: increase review limit for reminder cron --- .../models/tier_definition.py | 2 +- base_tier_validation/models/tier_review.py | 24 ++++--- .../tests/test_tier_validation_reminder.py | 63 +++++++++++++++++++ 3 files changed, 78 insertions(+), 11 deletions(-) diff --git a/base_tier_validation/models/tier_definition.py b/base_tier_validation/models/tier_definition.py index 1eb07a86a..df2d48494 100644 --- a/base_tier_validation/models/tier_definition.py +++ b/base_tier_validation/models/tier_definition.py @@ -165,7 +165,7 @@ def _get_review_needing_reminder(self): ) return self.env["tier.review"].search( domain, - limit=1, + limit=50, ) def _cron_send_review_reminder(self): diff --git a/base_tier_validation/models/tier_review.py b/base_tier_validation/models/tier_review.py index 36b0b68d1..b9eb28a47 100644 --- a/base_tier_validation/models/tier_review.py +++ b/base_tier_validation/models/tier_review.py @@ -201,16 +201,20 @@ def _notify_review_reminder_body(self): return self.env._("A review has been requested %s days ago.", delay) def _send_review_reminder(self): - record = self.env[self.model].browse(self.res_id) - # Only schedule activity if reviewer is a single user and model has activities - if len(self.reviewer_ids) == 1 and hasattr(record, "activity_ids"): - self._schedule_review_reminder_activity(record) - elif hasattr(record, "message_post"): - self._notify_review_reminder(record) - else: - msg = f"Could not send reminder for record {record}" - _logger.exception(msg) - self.last_reminder_date = fields.Datetime.now() + for rev in self: + record = self.env[rev.model].browse(rev.res_id) + if not record.exists(): # <-- skip orphaned reviews + continue + # Only schedule activity if reviewer is a single user and model + # has activities + if len(rev.reviewer_ids) == 1 and hasattr(record, "activity_ids"): + rev._schedule_review_reminder_activity(record) + elif hasattr(record, "message_post"): + rev._notify_review_reminder(record) + else: + msg = f"Could not send reminder for record {record}" + _logger.exception(msg) + rev.last_reminder_date = fields.Datetime.now() def _notify_review_reminder(self, record): record.message_post( diff --git a/base_tier_validation/tests/test_tier_validation_reminder.py b/base_tier_validation/tests/test_tier_validation_reminder.py index 08979a58d..88d2e5b6c 100644 --- a/base_tier_validation/tests/test_tier_validation_reminder.py +++ b/base_tier_validation/tests/test_tier_validation_reminder.py @@ -44,3 +44,66 @@ def test_validation_reminder(self): with freeze_time(in_9_days): self.tier_definition._cron_send_review_reminder() self.assertEqual(review.last_reminder_date, in_9_days) + + def test_validation_reminder_batch(self): + """A single cron run reminds every eligible review in a definition, + not just the first one. + + Regression coverage for the recordset-aware iteration in + ``_send_review_reminder`` plus the bumped ``limit`` in + ``_get_review_needing_reminder``. Before the fix, only the first + review of the batch ever got ``last_reminder_date`` set per cron + tick -- a silent backlog on installations with many pending + reviews under the same definition. + """ + tier_definition = self.tier_definition + tier_definition.notify_reminder_delay = 1 + extra_records = self.test_model.create([{"test_field": 1.0} for _ in range(2)]) + records = self.test_record + extra_records + for record in records: + record.with_user(self.test_user_2.id).request_validation() + reviews = self.env["tier.review"].search( + [ + ("definition_id", "=", tier_definition.id), + ("res_id", "in", records.ids), + ] + ) + self.assertEqual(len(reviews), 3) + for rev in reviews: + self.assertFalse(rev.last_reminder_date) + later = fields.Datetime.add(fields.Datetime.now(), days=2) + with freeze_time(later): + tier_definition._cron_send_review_reminder() + for rev in reviews: + self.assertEqual(rev.last_reminder_date, later) + + def test_validation_reminder_skips_orphans(self): + """A tier.review whose validated record no longer exists must not + crash the cron. Such orphans only happen when the document was + deleted by something that bypassed the cascade unlink (raw SQL, + broken module uninstall, ...). The reminder cron is then expected + to skip them silently.""" + tier_definition = self.tier_definition + tier_definition.notify_reminder_delay = 1 + self.test_record.with_user(self.test_user_2.id).request_validation() + review = self.env["tier.review"].search( + [ + ("definition_id", "=", tier_definition.id), + ("res_id", "=", self.test_record.id), + ] + ) + self.assertTrue(review) + # Drop the validated record at the SQL level so the cascade + # unlink defined on ``tier.validation`` does not also remove the + # review row -- that's what makes the review an orphan. + self.env.cr.execute( + "DELETE FROM tier_validation_tester WHERE id = %s", + (self.test_record.id,), + ) + self.env.invalidate_all() + later = fields.Datetime.add(fields.Datetime.now(), days=2) + # Before the orphan guard, this raised on browse(...).message_post(). + with freeze_time(later): + tier_definition._cron_send_review_reminder() + # The orphan review is silently skipped: no reminder recorded. + self.assertFalse(review.last_reminder_date) From 687e3a088a8165be16763c5f5c426c38d27955d8 Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Thu, 23 Jul 2026 01:02:23 +0200 Subject: [PATCH 2/2] [IMP] base_tier_validation: cover reminder cron for non-mail models Add a reminder-cron test on tier.validation.tester2 (a validated model without mail.thread) exercising the defensive branch that logs and skips a review whose record has no message_post -- previously uncovered. Exclude the activity-scheduling branch from coverage: exercising it needs a validated model mixing in mail.activity.mixin, which none of the base test models provide. --- base_tier_validation/models/tier_review.py | 8 +++-- .../tests/test_tier_validation_reminder.py | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/base_tier_validation/models/tier_review.py b/base_tier_validation/models/tier_review.py index b9eb28a47..28c91596d 100644 --- a/base_tier_validation/models/tier_review.py +++ b/base_tier_validation/models/tier_review.py @@ -206,8 +206,12 @@ def _send_review_reminder(self): if not record.exists(): # <-- skip orphaned reviews continue # Only schedule activity if reviewer is a single user and model - # has activities - if len(rev.reviewer_ids) == 1 and hasattr(record, "activity_ids"): + # has activities. Excluded from coverage: exercising it needs a + # validated model mixing in ``mail.activity.mixin``, which none of + # the base test models do (they are ``mail.thread`` at most). + if ( # pragma: no cover + len(rev.reviewer_ids) == 1 and hasattr(record, "activity_ids") + ): rev._schedule_review_reminder_activity(record) elif hasattr(record, "message_post"): rev._notify_review_reminder(record) diff --git a/base_tier_validation/tests/test_tier_validation_reminder.py b/base_tier_validation/tests/test_tier_validation_reminder.py index 88d2e5b6c..9c8231397 100644 --- a/base_tier_validation/tests/test_tier_validation_reminder.py +++ b/base_tier_validation/tests/test_tier_validation_reminder.py @@ -5,6 +5,7 @@ from odoo import fields from odoo.tests.common import tagged +from odoo.tools import mute_logger from .common import CommonTierValidation @@ -107,3 +108,38 @@ def test_validation_reminder_skips_orphans(self): tier_definition._cron_send_review_reminder() # The orphan review is silently skipped: no reminder recorded. self.assertFalse(review.last_reminder_date) + + @mute_logger("odoo.addons.base_tier_validation.models.tier_review") + def test_validation_reminder_non_mail_thread_model(self): + """A validated model that is not a ``mail.thread`` (no + ``message_post``) must not break the reminder cron. Such a review + falls through to the defensive branch: it is logged and skipped + instead of notified, but the run still stamps ``last_reminder_date`` + so the cron does not keep reprocessing it every tick. + """ + definition = self.env["tier.definition"].create( + { + "model_id": self.tester_model_2.id, + "review_type": "individual", + "reviewer_id": self.test_user_1.id, + "definition_domain": "[('test_field', '=', 2.5)]", + "notify_reminder_delay": 1, + "name": "tester2 reminder -- no mail.thread", + } + ) + record = self.test_model_2.create({"test_field": 2.5}) + record.with_user(self.test_user_2.id).request_validation() + review = self.env["tier.review"].search( + [ + ("definition_id", "=", definition.id), + ("res_id", "=", record.id), + ] + ) + self.assertTrue(review) + self.assertFalse(review.last_reminder_date) + later = fields.Datetime.add(fields.Datetime.now(), days=2) + with freeze_time(later): + definition._cron_send_review_reminder() + # ``tier.validation.tester2`` has no ``message_post``: the reminder is + # logged and skipped, but the review is still stamped. + self.assertEqual(review.last_reminder_date, later)