Skip to content

[18.0][FIX] base_tier_validation_forward: fix multiple reviews in 'pending' after forward#1186

Merged
OCA-git-bot merged 2 commits intoOCA:18.0from
StefanRijnhart:18.0-fix-base_tier_validation_forward-approval_sequence_violation
Mar 4, 2026
Merged

[18.0][FIX] base_tier_validation_forward: fix multiple reviews in 'pending' after forward#1186
OCA-git-bot merged 2 commits intoOCA:18.0from
StefanRijnhart:18.0-fix-base_tier_validation_forward-approval_sequence_violation

Conversation

@StefanRijnhart
Copy link
Member

@StefanRijnhart StefanRijnhart commented Nov 10, 2025

How to reproduce:

  • Create two reviews for the same model with different sequences and Approve by sequence enabled.
  • Request validation on a record. Let the user of the first review forward the
    first validation.

Desired result:

The new (forward) review is in state 'pending', and the remaining review is in state 'waiting'.

Actual result:

Both the new (forward) review and the remaining review are in state 'pending'.

Suggested solution:

_compute_can_review has to be deferred during the fowarding process. It was called explicitely afterwards already.

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot
Copy link
Contributor

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

@StefanRijnhart StefanRijnhart marked this pull request as draft November 24, 2025 10:40
@StefanRijnhart StefanRijnhart marked this pull request as ready for review December 3, 2025 17:31
@StefanRijnhart StefanRijnhart force-pushed the 18.0-fix-base_tier_validation_forward-approval_sequence_violation branch from 9b8c6b5 to b9c9254 Compare December 3, 2025 17:32
By ordering only by sequence, the order is not deterministic when there are
forwards, because the forwards are created with the same sequence as the
forwarded reviews.
… after forward

How to reproduce:

* Create two reviews for the same model with different sequences and `Approve by
sequence` enabled.
* Request validation on a record. Let the user of the first review forward the
first validation.

Desired result:

The new (forward) review is in state 'pending', and the remaining review is in
state 'waiting'.

Actual result:

Both the new (forward) review and the remaining review are in state 'pending'.

Suggested solution:

_compute_can_review has to be deferred during the fowarding process. It was
called explicitely afterwards already.
@StefanRijnhart StefanRijnhart force-pushed the 18.0-fix-base_tier_validation_forward-approval_sequence_violation branch from b9c9254 to 0cb0f2e Compare March 3, 2026 08:16
Copy link
Contributor

@espo-tony espo-tony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@len-foss len-foss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit hacky, but this is because the module's design is pushing the limits of the ORM. So AFAICT it's the best solution we can get given the constraints we have 👍

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, indeed is a bit hacky but the issue is real. Thanks for providing appropriate test cases.

@LoisRForgeFlow
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 18.0-ocabot-merge-pr-1186-by-LoisRForgeFlow-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 01bd40d into OCA:18.0 Mar 4, 2026
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 8760d3f. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants