Skip to content

[19.0][IMP] base_tier_validation: clearer notify_on_pending chatter body#22

Open
bosd wants to merge 1 commit into
OCA:19.0from
bosd:19.0-imp-tier-pending-notification-body
Open

[19.0][IMP] base_tier_validation: clearer notify_on_pending chatter body#22
bosd wants to merge 1 commit into
OCA:19.0from
bosd:19.0-imp-tier-pending-notification-body

Conversation

@bosd
Copy link
Copy Markdown
Contributor

@bosd bosd commented May 12, 2026

Problem

The chatter message posted when a tier reaches pending (from _notify_review_available) has two UX problems users have reported:

  1. Wrong attribution on later tiers. The body reads "A review has been requested by Administrator" on tier 1 and then, after tier 1 is approved and tier 2 is auto-promoted, "A review has been requested by Tier-1-Reviewer". The previous reviewer never pressed Request Validation -- they pressed Validate -- so crediting them for requesting the next tier is misleading. Cause: self.env.user.name inside _notify_review_available resolves to whichever user happened to trigger the promotion, not the original requester. On the second tier that's the tier-1 approver.

  2. No way to tell who the message is for. Chatter system notifications do not render the "Notified to" pill the way user-typed comments do. A recipient seeing "A review has been requested by Administrator" in their inbox cannot tell whether the message was meant for them or for somebody else.

Fix

  • Pass the affected tier_reviews to _notify_requested_review_body; source the requester from review.requested_by (set once at request_validation time, never reassigned) so the attribution is consistent across all tiers.
  • Reuse the existing tier.review.todo_by field for the assignee label. It already handles every review type:
    • individual / Python expression / res.users field: name(s), abbreviated past 3 with "(and N more)".
    • group: "Group ".
    • res.groups field: same logic via user_ids.
  • New wording: "Review pending for Mike, requested by Administrator."

Backwards-compatible: _notify_requested_review_body() still works with no arguments and returns the previous short body for any downstream overrider that may call it that way (the reminder activity body, custom modules that rendered the requester body themselves, etc).

Why not switch to message_type='comment' to get the pill

  • Flips the message into the Send Message lane: it triggers unread counters for every follower (not just the assignee), counts as a human comment in chatter history, and runs external partner email routing rules. Internal-user followers may receive an actual email instead of just an inbox bubble.
  • Audit-wise it then reads as "Administrator wrote a message" instead of "tier-validation system fired a workflow event", which is wrong.
  • Most Odoo workflows (sale approval, account_followup reminders, etc.) deliberately use message_type='notification' and put the recipient context in the body. This PR follows that convention.

Why not schedule a mail.activity per reviewer (per-user TODO)

That would land the review in each reviewer's My Activities widget, which is the strongest UX. But it adds:

  • a new mail.activity type (or reuse the existing reminder activity for a different purpose),
  • cleanup hooks in _validate_tier, _rejected_tier and restart_validation to avoid stale activities,
  • additional test coverage for the full happy-path + reject + restart matrix.

Out of scope here; happy to follow up with that as a separate PR if there is appetite.

Test plan

  • test_19a_notify_on_pending_sequence_negative extended to verify:
    • tier-1 message body contains the assignee (review_first.todo_by) and the original requester (env.user.display_name from when request_validation ran),
    • tier-2 message body contains the second assignee (review_second.todo_by) and the original requester -- and does not contain the tier-1 approver's name (regression guard for the attribution bug).
  • test_20_no_sequence updated to assert both the assignee and the requester are present in the body.

Depends on

PR #21 ([FIX] base_tier_validation: auto-promote first review to pending). Branched off it so the changes compose; can be rebased onto 19.0 once that lands.

CC @LoisRForgeFlow

@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 12, 2026
@bosd bosd changed the title [IMP] base_tier_validation: clearer notify_on_pending chatter body [19.0][IMP] base_tier_validation: clearer notify_on_pending chatter body May 12, 2026
The chatter message posted when a tier reaches ``pending`` has two
problems:

1. ``self.env.user.name`` is wrong on second-and-later tier
   promotions. ``_notify_review_available`` runs inside
   ``_validate_tier -> _update_counter -> _update_review_status``,
   where ``env.user`` is the *approver* of the previous tier, not the
   person who originally pressed *Request Validation*. The message
   therefore credits the wrong user.

2. Chatter system notifications do not render the "Notified to" pill
   the way user-typed comments do, so a recipient seeing
   *"A review has been requested by Administrator"* in their inbox
   cannot tell whether the message was meant for them or for somebody
   else.

Source the requester from ``review.requested_by`` (set once at
``request_validation`` time) and weave the assignee into the body by
reusing ``tier.review.todo_by`` -- so the new wording reads, for
example, *"Review pending for Mike, requested by Administrator."*

``todo_by`` already handles every review type (individual user,
group, ``res.users`` / ``res.groups`` field) and tracks the same
abbreviation rules as the Reviews table, keeping the chatter message
and the table consistent.
@bosd bosd force-pushed the 19.0-imp-tier-pending-notification-body branch from 32e083c to 481aa54 Compare May 13, 2026 15:32
jaydeep32 pushed a commit to archeti-org/tier-validation that referenced this pull request May 14, 2026
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.

2 participants