Skip to content

[19.0][IMP] base_tier_validation: reminder cron — batch limit, recordset semantics, orphan guard#39

Open
bosd wants to merge 1 commit into
OCA:19.0from
bosd:19.0-imp-base_tier_validation-reminder-cron-limit
Open

[19.0][IMP] base_tier_validation: reminder cron — batch limit, recordset semantics, orphan guard#39
bosd wants to merge 1 commit into
OCA:19.0from
bosd:19.0-imp-base_tier_validation-reminder-cron-limit

Conversation

@bosd
Copy link
Copy Markdown
Contributor

@bosd bosd commented May 14, 2026

Summary

Forward-port of OCA/server-ux#1281 (authored by @lancelot / Noviat) onto 19.0.

The upstream PR's title only says "increase review limit", but the actual diff fixes three things in one ~15-line change. Worth keeping all three because #2 below is the kind of silent recordset bug that bites you in prod without an error to trace.

1. Batch limit 1 -> 50 in _get_review_needing_reminder

Per cron run, each tier.definition returned at most ONE pending review to remind. With many definitions × few pending reviews that's fine; with one definition × hundreds of pending reviews, you only catch up 1 per run — at default 4 runs/day that's 25 days to remind 100 people. Bumped to 50 per definition per run.

2. _send_review_reminder made recordset-aware

_send_review_reminder previously assumed self was a singleton without calling ensure_one() — calling it on a recordset would silently process only the first review and ignore the rest. Now iterates for rev in self properly. This is what makes #1 actually deliver — without it, bumping the limit would still only send one reminder per cron tick.

3. Skip orphaned reviews (record.exists() guard)

A tier.review row pointing at a deleted document (orphaned because the original record was deleted without cleaning up the review) would crash the cron with Record does not exist or has been deleted. Now those rows are silently skipped — the cron survives.

Forward-port note

The v18 commit used the legacy list-of-tuples domain syntax in _get_review_needing_reminder; v19 already uses the Domain(...) builder for the same lookup. Trivial conflict resolved by keeping the v19 Domain(...) shape and only carrying over the limit=1 -> limit=50 change. All other lines applied cleanly.

Credit

Original commit: da31cda5 by @lancelot.

Test plan

Manual: create a tier definition with notify_reminder_delay=1, generate ≥2 pending reviews older than 1 day, run the reminder cron. Before: only one gets a reminder. After: all of them up to 50/run.

@OCA-git-bot
Copy link
Copy Markdown
Contributor

Hi @LoisRForgeFlow,
some modules you are maintaining are being modified, check this out!

@OCA-git-bot OCA-git-bot added mod:base_tier_validation Module base_tier_validation series:19.0 labels May 14, 2026
@bosd bosd force-pushed the 19.0-imp-base_tier_validation-reminder-cron-limit branch from e7179c0 to d06a380 Compare May 14, 2026 10:05
@bosd bosd force-pushed the 19.0-imp-base_tier_validation-reminder-cron-limit branch from d06a380 to 5790db4 Compare May 14, 2026 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mod:base_tier_validation Module base_tier_validation series:19.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants