[REF][8.0][hr_payroll_account] Get partner_id with a function to inheritable case

[moylop260]

Proposal to add a REF - More info here: #3274
OPW ticket 616565

[mart-e]

Hello,

Are you trying to fix a bug ? Can you give a way to reproduce ?
Thanks

[moylop260]

@mart-e,
Sorry I saved before of add description.
(Now has description)

[moylop260]

@mart-e
What do you think of these PR?

[mart-e]

Hello,

I agree it could be useful to compute the salary based on the partner. However this would be for master as not a bug fix (but you can keep this PR I will rebase while merging).

Thanks to new API you don't need to do things like x = foo_id and foo_id.bar_id and foo_id.bar_id.id, you can just do x = foo_id.bar_id.id, it will work even if foo_id or bar_id is an empty recordset (.id is False).
This should simplify your code.

If I understand correctly the 5 lines condition to assign res[payslip_line.id] (please avoid too long oneliners), you first try with the account_debit and fallback on the account_credit. That doesn't sound right if you have both accounts defined on your rule.

[moylop260]

Hello @mart-e
Thank for you reply.

I see many refactory case in this code, but I'm using the current original code for no change nothing in this PR.
This one only make a minimal changes to add method to get partner, but is the same current copy&paste code.

IMHO changes of logic or new api should be in other PR.
But if you consider that we can change it now.
We can help

[mart-e]

I don't mean to refactor the whole computation (as you say keep the diff minimum) but in the parts you do rewrite, you can reduce the code to improve the readability.

Anyway the part that worries me is the fallback on credit if no debit. Instead of adding a field, wouldn't it be better to extract it in a method with a parameter credit/debit ?

[moylop260]

@mart-e
IMHO add a new field to check credit/debit is a REFactory and rewrite to reduce the current code is a REFactory.
The current code from original addons is hard readeable and there is no field with a parameter credit/debit.
I'm total agree with you but this PR not intended to create a REFactory of any cases.
Only there is a partner_id field inheritable with same code.

Now,
If you like I can work in a second PR to fix "hard readeable" and new fields and add unit test to verify it.

What do you think?

[mart-e]

Ok to keep the existing code but it has not the same behaviour than the existing code.

Let's say your debit_account is neither receivable nor payable, in current code, you do not get a partner for the debit line.
With your code, you fallback on the account_credit.type in ('receivable', 'payable')) condition and set the employee_id.address_home_id as partner if this is true.

The difference is that we evaluated different conditions in case of debit or credit account, while with your patch, it's one global evaluation.

Well I admit it's quite specific but that was the reason I suggested a separated method instead of a computed field.

[moylop260]

@mart-e,
I got it

Then if I add a field function to partner_debit_id and other one to patner_credit_id
Would you be okay?

[moylop260]

Do you feel fine if I add test with this cases: table of cases of test?

[mart-e]

I don't get why do you absolutely need to have fields and could not call a method _get_partner_id or something ?
Having two fields with the same partner (returning False instead on some conditions) is a bit strange.

It's usually a good idea to add tests (thanks for the suggestion) but I am not sure it will be very useful for this refactroring. So you can but no need to do the full table.

[moylop260]

@mart-e
Im confused, sorry.
Can you help me to write your comment into line code as this one, please?

What is the section that I need change it?

I want to help but I don't understand your point.

[mart-e]

Replace this by something like

if credit_account:
    res[payslip_line.id] = (payslip_line.salary_rule_id.register_id.partner_id or ayslip_line.salary_rule_id.account_credit.type in ('receivable', 'payable')) and partner_id or False
else:
    res[payslip_line.id] = (payslip_line.salary_rule_id.register_id.partner_id or ayslip_line.salary_rule_id.account_debit.type in ('receivable', 'payable')) and partner_id or False

[moylop260]

@mart-e
I got it
Thank you!

We will work with this changes.

[jorgenaranjoVauxoo]

Hello @mart-e

I did make the requested changes, you can check the PR again.

Thank you!

[mart-e]

if you are supporting only one id, don't make your argument a list, use a single id as parameter and return the partner_id

[mart-e]

Thanks for the update.
@moylop260 @jorgenaranjo could you also update Vauxoo's CLA ?

[moylop260]

Hello @mart-e
cla update: #7039

[mart-e]

Thanks, merged the CLA update.
Made a few changes (recordset as parameter) and applied on master at 075d80a. Will be merged in master soon.

[moylop260]

@mart-e,
Thank you

[jorgenaranjoVauxoo]

@mart-e
The change for receive the id in _get_partner_id function was made.

[mart-e]

Actually the patch was arleady ready at 075d80afb94a2819e29e1cbb1c815593e65b4268 but I was just waiting for the master branch to be back online.
Now it's done so I have merged it in master at 6b56091
Thanks