[18.0][IMP] base_tier_validation: Add cancel status to review#1250
[18.0][IMP] base_tier_validation: Add cancel status to review#1250victoralmau wants to merge 1 commit intoOCA:18.0from
Conversation
Related to OCA#1245 TT61255
|
Hi @LoisRForgeFlow, |
| # If there are waiting reviews that should be approved sequentially, | ||
| # they must be marked as canceled | ||
| waiting_reviews = self.review_ids.filtered( | ||
| lambda r: r.status == "waiting" and r.approve_sequence |
There was a problem hiding this comment.
approve_sequence shouldn't be higher than the current one?
There was a problem hiding this comment.
The sequence works “backwards” from what we would think (definition 1 > sequence 30, definition 2 > sequence 20, definition 3 > sequence 10); In any case, I don't think it's necessary to do that validation because if it's a sequence approval, review 2 cannot be rejected without completing review 1; furthermore, if by chance it could be done, the pending reviews should still be canceled, don't you think? (review 2 is rejected, review 1 and review 3 should be canceled).
There was a problem hiding this comment.
As I don't know this in-depth, again I let @LoisRForgeFlow to decide.
There was a problem hiding this comment.
Yes, this is a long standing issue to be honest. I agree that it should be changed to ease understanding. Some ideas have been proposed in this issue to make the transition. #1082 (comment)
There was a problem hiding this comment.
Okay, thanks for the clarification and for pointing out the issue, but now that it's clear, do you think it's necessary in this PR to change this filter to only “cancel” revisions with a lower sequence? In my opinion, no, because “by sequence” a revision 2 cannot be rejected if revision 1 has not been completed; therefore, the following revisions (revision 3 and revision 4, for example) will always have to be canceled.
4 participants
Add cancel status to review
Related to #1245
Please @pedrobaeza and @LoisRForgeFlow can you review it?
@Tecnativa TT61255