From 5790db4dff0804f12915c54e1126a0922a5cce59 Mon Sep 17 00:00:00 2001 From: Lancelot Date: Fri, 10 Apr 2026 12:57:08 +0200 Subject: [PATCH] [IMP] base_tier_validation: increase review limit for reminder cron --- base_tier_validation/__manifest__.py | 2 +- .../models/tier_definition.py | 2 +- base_tier_validation/models/tier_review.py | 24 ++++--- .../tests/test_tier_validation_reminder.py | 63 +++++++++++++++++++ 4 files changed, 79 insertions(+), 12 deletions(-) diff --git a/base_tier_validation/__manifest__.py b/base_tier_validation/__manifest__.py index 3d2ad910..e826d37b 100644 --- a/base_tier_validation/__manifest__.py +++ b/base_tier_validation/__manifest__.py @@ -3,7 +3,7 @@ { "name": "Base Tier Validation", "summary": "Implement a validation process based on tiers.", - "version": "19.0.1.0.2", + "version": "19.0.1.0.3", "development_status": "Mature", "maintainers": ["LoisRForgeFlow"], "category": "Tools", diff --git a/base_tier_validation/models/tier_definition.py b/base_tier_validation/models/tier_definition.py index b36a82f6..74a57371 100644 --- a/base_tier_validation/models/tier_definition.py +++ b/base_tier_validation/models/tier_definition.py @@ -154,7 +154,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 bed4caaf..d37ec1a2 100644 --- a/base_tier_validation/models/tier_review.py +++ b/base_tier_validation/models/tier_review.py @@ -189,16 +189,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 08979a58..88d2e5b6 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)