From f2d8ec32054c1a480e67466aa4fe98c57bafc875 Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Thu, 27 Aug 2026 13:02:43 +0200 Subject: [PATCH] [FIX] base_tier_validation: reviewer systray counter drifting (negative) The reviewer counter in the systray can display a wrong, even negative, value. The counter was mutated by real-time bus deltas (``review_created`` -> ++, ``review_deleted`` -> --), but ``_update_counter`` sent those deltas to ``self.env.user.partner_id`` -- the *acting* user -- instead of the reviewers whose pending count actually changes. So whenever a user acted on a tier-validated record without being one of its reviewers (creating, editing, force-validating), the wrong user's counter was moved, and an active approver accumulated a drift that eventually went negative. The systray's own absolute count is a ``len()`` and can never be negative; only the blind delta could. Fixes: - JS: the systray component now re-fetches the authoritative absolute count (``review_user_count``) on the bus event, instead of a separate service nudging a +/- delta. The dedicated ``tierReviewService`` (whose only job was that delta) is removed; the component already owns ``fetchSystrayReviewer`` and the ORM via ``useService``, so the badge stays live without drift and can never go below zero. - Python: ``_update_counter`` now notifies the reviewers' partners (the users whose pending count changes), falling back to the acting user only when the reviews are already gone (deletions). The re-fetch is correct but it is not free, and the notification now reaches every reviewer rather than one user. Approving a batch of documents therefore fans out to (reviewers x open tabs x documents) recounts of ``review_user_count``, which is enough to occupy every HTTP worker. The recounts are coalesced so that the fan-out cannot turn into a request storm: - a 15 s leading+trailing debounce around the bus handler -- the leading edge keeps an isolated change instant, the trailing edge settles the final value; - an in-flight guard, so a recount that outlasts the debounce window records that something changed and re-runs once instead of stacking concurrent calls on top of a slow one. --- .../models/tier_validation.py | 13 ++++- .../tier_review_menu/tier_review_menu.esm.js | 52 ++++++++++++++++--- .../js/services/tier_review_service.esm.js | 33 ------------ 3 files changed, 58 insertions(+), 40 deletions(-) delete mode 100644 base_tier_validation/static/src/js/services/tier_review_service.esm.js diff --git a/base_tier_validation/models/tier_validation.py b/base_tier_validation/models/tier_validation.py index 782cca05d..16551cb28 100644 --- a/base_tier_validation/models/tier_validation.py +++ b/base_tier_validation/models/tier_validation.py @@ -874,7 +874,18 @@ def restart_validation(self): def _update_counter(self, review_counter): self.review_ids._update_review_status() channel = "base.tier.validation/updated" - self.env.user.partner_id._bus_send(channel, review_counter) + # Notify the reviewers whose pending count actually changes, not the + # acting user. Sending the delta to ``self.env.user`` made unrelated + # users' systray counters drift (even negative) whenever someone acted + # on a tier-validated record without being one of its reviewers. When + # the reviews are already gone (deletions), fall back to the acting + # user. The client recomputes the absolute count on receipt, so the + # exact audience only affects how promptly a reviewer sees the update. + partners = self.review_ids.mapped("reviewer_ids.partner_id") + if not partners: + partners = self.env.user.partner_id + for partner in partners: + partner._bus_send(channel, review_counter) def unlink(self): self.mapped("review_ids").unlink() diff --git a/base_tier_validation/static/src/components/tier_review_menu/tier_review_menu.esm.js b/base_tier_validation/static/src/components/tier_review_menu/tier_review_menu.esm.js index 553796b88..40980ccd2 100644 --- a/base_tier_validation/static/src/components/tier_review_menu/tier_review_menu.esm.js +++ b/base_tier_validation/static/src/components/tier_review_menu/tier_review_menu.esm.js @@ -1,6 +1,7 @@ import {Component, useState} from "@odoo/owl"; import {Dropdown} from "@web/core/dropdown/dropdown"; import {registry} from "@web/core/registry"; +import {useDebounced} from "@web/core/utils/timing"; import {useDiscussSystray} from "@mail/utils/common/hooks"; import {useDropdownState} from "@web/core/dropdown/dropdown_hooks"; import {useService} from "@web/core/utils/hooks"; @@ -16,18 +17,57 @@ export class TierReviewMenu extends Component { this.orm = useService("orm"); this.store = useState(useService("mail.store")); this.action = useService("action"); + this.busService = useService("bus_service"); this.dropdown = useDropdownState(); + this.fetchPending = false; + this.fetchRunning = false; + // Keep the badge live and correct: re-fetch the authoritative count + // whenever a tier review changes server-side, instead of nudging a + // running +/- delta. The absolute value is a len() and so can never go + // negative, which a delta could when an update reaches a user for whom + // the review was never part of their pending count. + // + // That re-fetch is not free: ``review_user_count`` costs a handful of + // queries per pending review, and the notification is broadcast to + // every reviewer. Approving a batch of documents therefore fans out to + // (reviewers x open tabs x documents) recounts, which is enough to + // occupy every HTTP worker. Coalesce the bursts: the leading edge keeps + // an isolated change instant, the trailing edge settles the final value. + this.debouncedFetch = useDebounced(() => this.fetchSystrayReviewer(), 15000, { + immediate: true, + trailing: true, + }); this.fetchSystrayReviewer(); + this.busService.subscribe("base.tier.validation/updated", () => + this.debouncedFetch() + ); + this.busService.start(); } async fetchSystrayReviewer() { - const groups = await this.orm.call("res.users", "review_user_count"); - let total = 0; - for (const group of groups) { - total += group.pending_count || 0; + // A recount can outlast the debounce window on a busy database. Never + // let them overlap: remember that something changed and re-run once, + // instead of stacking concurrent calls on top of a slow one. + if (this.fetchRunning) { + this.fetchPending = true; + return; + } + this.fetchRunning = true; + try { + const groups = await this.orm.call("res.users", "review_user_count"); + let total = 0; + for (const group of groups) { + total += group.pending_count || 0; + } + this.store.tierReviewCounter = total; + this.store.tierReviewGroups = groups; + } finally { + this.fetchRunning = false; + } + if (this.fetchPending) { + this.fetchPending = false; + await this.fetchSystrayReviewer(); } - this.store.tierReviewCounter = total; - this.store.tierReviewGroups = groups; } availableViews() { diff --git a/base_tier_validation/static/src/js/services/tier_review_service.esm.js b/base_tier_validation/static/src/js/services/tier_review_service.esm.js deleted file mode 100644 index bde257bef..000000000 --- a/base_tier_validation/static/src/js/services/tier_review_service.esm.js +++ /dev/null @@ -1,33 +0,0 @@ -import {reactive} from "@odoo/owl"; -import {registry} from "@web/core/registry"; - -export class TierReviewService { - constructor(env, services) { - this.env = env; - this.store = services["mail.store"]; - this.busService = services.bus_service; - } - setup() { - this.busService.subscribe("base.tier.validation/updated", (payload) => { - if (payload.review_created) { - this.store.tierReviewCounter++; - } - if (payload.review_deleted) { - this.store.tierReviewCounter--; - } - }); - this.busService.start(); - } -} - -export const tierReviewService = { - dependencies: ["bus_service", "mail.store"], - - start(env, services) { - const tier_review_service = reactive(new TierReviewService(env, services)); - tier_review_service.setup(); - return tier_review_service; - }, -}; - -registry.category("services").add("tierReviewService", tierReviewService);