[18.0][REF] payroll: use recordsets instead int lists #188
Conversation
|
Hi @appstogrow, @nimarosa, |
|
Hello @dreispt i did a fast review of the code. I think you misunderstood the fix in #83 Child rules can be calculated before and after the parent rule, this is not a limitation and my PR never intended to modify their calculation order. The fix i made it's intended to discard the calculation of child rules if the condition of the parent rule is False. In other words, child rules should not be calculated if the parent rule condition is false (so it won't be shown in the payslip). In normal use, child rules contains auxiliary calculations that can or are related to the parent rule. So if the condition in parent rule don't returns True, it's not necessary to even calculate the child rules, so they are discarded. That don't have anything to do with the order of calculation which is what i think you are proposing here. Rules (child or not) should be calculated according their sequence and that is the only field that decides the order of calculation. For example, you can have a parent rule with 2 child rules and one can be calculated before and other later, it depends of the sequence and that don't change with 83 PR. |
|
@nimarosa Understood - if parent rule is not "activated" then there is no point in activating the child rule. So my PR is fixing something different. |
dreispt
changed the title
|
Hello @dreispt, The problem I think with this patch/fix is that you are changing the way payroll historically works. Why a child rule should be calculated before the parent rule? I think rules should continue to be calculated according to their sequence. I saw cases where child rules need to be calculated after the parent rule, an example of this is: if you need the value of the parent rule to use in the formula of the child rule. So if we merge this, we might be broking some payroll implementations and localizations. I personally don't have a case when I use child rules after the parent, because I use them as aux variables for the parent rule, but I seen cases in my country which use them after parent. I think we don't have to mess with the sequence order. The sequence field is there for a reason: to let the user choose the order of calculation. If we change that for child rules, we might affect users. Maybe I'm wrong I would like to see what other users think. |
dreispt
force-pushed
the
18-dr-payroll-fix-child-rules
branch
from
f1652dc to
8067267
Compare
dreispt
changed the title
|
@nimarosa updated, to honor strict Rule sequence. Tests were not changed. |
There was a problem hiding this comment.
I think the changes look right. I didn't have time to test it in runboat.
So to be clear, this is not affecting calculation order and we still doing it by sequence?
The other changes that use recordsets instead of structs look right to me.
I think you tested if it's not impacting in calculation times in a noticeable way. The reason to use structs is for speed in payslips with a lot of rules to be calculated, but I think now the speed with be the same using recordsets than in prior versions of Odoo.
In case we merge this will be good to port it to 17 too.
| return [] | ||
| # YTI TODO return browse records | ||
| return list(set(structures._get_parent_structure().ids)) | ||
| # TODO: remove, too simple and not used |
There was a problem hiding this comment.
This TODO is asking to remove the line you just changed or im understanding it wrong?
There was a problem hiding this comment.
This method can be removed, as it implements so useful logic at all.
But I hesitated removing it, for backward compatibility.
|
@nimarosa The rule sequence is honored when processing payslips. This is ensured here. High calculation efficiency is on my list of important things to improve. |
I see there was a fix at #83, but in my tests this was not working.I improved the tests for the child rule calculation dependencies, and now they are passing.