[19.0][FIX] base_tier_validation: auto-promote first review to pending - #21
Merged
Merged
Conversation
After ``request_validation()`` the lowest-sequence review (or every review when ``approve_sequence`` is not used) must reach the ``pending`` state immediately so the reviewer can act on it and so that ``notify_on_pending`` notifications fire. Before this commit ``_update_review_status`` was only called when: - the requesting user was themself the first reviewer (so the record had ``can_review=True`` and ``_update_counter`` was invoked), or - at least one tier definition had ``notify_on_create=True``. In every other case (the common one: setting only ``notify_on_pending``, validation requested by someone who is not the tier-1 reviewer) all reviews stayed in ``waiting`` indefinitely and no reviewer was notified. ``next_review`` also leaked the empty recordset to its ``Char`` field, producing ``tier.review()`` in the "This Record needs to be validated" banner. Restore the original behaviour from the ``base_tier_validation_waiting`` merge (994b3f7) by always promoting the available review(s) right after ``request_validation`` creates them, and coerce ``next_review`` to ``False`` when no review is pending. Tests updated accordingly: the previously-needed manual ``_update_review_status()`` calls are removed, and an explicit regression assertion is added for the ``next_review`` value.
Contributor
|
Hi @LoisRForgeFlow, |
added 3 commits
May 12, 2026 17:26
When approve_sequence=True, only the reviewer at the lowest review.sequence may be notified via notify_on_pending. The next reviewer in the chain must stay unsubscribed and must not receive a message until their predecessor has approved. Adds test_19a_notify_on_pending_sequence_negative covering both the positive (first reviewer notified) and negative (second reviewer NOT subscribed, NOT notified) sides of that contract, so a future regression of the auto-promote / notify-on-pending plumbing is caught immediately.
…py defs The negative test was failing on CI because test_field=5.0 also matched definition_2/definition_3 from common.py (domain test_field > 3.0), which pushed the new def_first to a non-minimum review sequence so it never auto-promoted. Use test_field=2.5 (matches no common.py def) and restrict the new definitions to exactly that value. Also: - Add a HISTORY.md entry for 19.0.1.0.1 - Run ruff-format / oca-gen-addon-readme regenerated artefacts
…iew_available The tier-validation subtypes are marked default=False in mail_data.xml, so a plain message_subscribe(partner_ids=...) leaves the partner subscribed only to default subtypes. The subsequent message_post(subtype_xmlid='mt_tier_validation_requested', ...) then routes to nobody -- notified_partner_ids ends up empty even though the reviewer's definition has notify_on_pending=True. Mirror what _notify_review_requested already does: pass subtype_ids to message_subscribe and short-circuit when there is no reviewer to notify (avoid posting a stray subtype message that nobody is subscribed to).
Adds test_19b_notify_review_available_no_op_when_no_users which calls _notify_review_available directly with a review whose definition has notify_on_pending=False. Asserts no follower is added and no chatter message is posted -- covering the if not users_to_notify: continue guard that codecov flagged.
This was referenced May 12, 2026
Contributor
|
Hello @bosd , I see your point and it's a good catch indeed.
|
This was referenced May 13, 2026
LoisRForgeFlow
approved these changes
May 13, 2026
LoisRForgeFlow
left a comment
Contributor
There was a problem hiding this comment.
/ocabot merge patch
Contributor
|
On my way to merge this fine PR! |
Contributor
|
Congratulations, your PR was merged at 26bb76e. Thanks a lot for contributing to OCA. ❤️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After
request_validation()reviews can stay inwaitingforever when neither of these is true:can_review=True), ornotify_on_create=True.Concrete repro:
approve_sequence=True, sequence 10 and 20, different reviewers, andnotify_on_pending=True(only).Observed:
waiting.This Record needs to be validated. tier.review()-- because_compute_next_reviewleaks the empty recordset to itsCharfield when no review is pending.Expected (and the behaviour originally introduced when merging
base_tier_validation_waiting-- see 994b3f7):pending.notify_on_pendingreviewers are notified.Next: <definition name>.The 19.0 migration (66f964a) refactored the auto-promote side-effect out of
_compute_can_reviewinto a separate_update_review_statusmethod, but only wired it back into the two narrow paths above. The tests for this flow (test_19_waiting_tierandtest_20_no_sequence) were updated to call_update_review_statusmanually instead of catching the regression.Fix
created_trs._update_review_status()unconditionally insiderequest_validationso the available review(s) are promoted topendingright away._update_review_statusis idempotent (skips non-waiting), so the existing_update_counterpath stays correct.next_reviewtoFalsewhen no review is pending (so theCharfield doesn't end up holding the repr of an empty recordset)._update_review_status()calls fromtest_11_add_comment,test_19_waiting_tier,test_20_no_sequenceand updatetest_28_computed_state_fieldto assert the auto-promoted state. Add an explicit regression assertion for thenext_reviewvalue intest_19.Test plan
pytest base_tier_validation/tests(full module suite) greennotify_on_pending=True, validation requested by a non-reviewer -- first reviewer reachespendingand receives the notification; banner readsNext: <definition name>.CC @LoisRForgeFlow