[14.0] [IMP] payroll: payslip refactoring

[nimarosa]

Hello, in this PR i will be adding commits with the refactoring that i'm doing in hr_payslip.

Here is the list of what is changed so far (will be editing when i add more commits):

Commit: [IMP] payroll: refactor onchange methods

• Now the get_inputs method takes into account the current struct_id selected. Before that, the function was always taking the struct_id from contract what it's inconvenient because the user couldn't change the struct_id from the payslip.
• "struct_id" has his own onchange method now, this allows to recompute the inputs table without recomputing all the dynamic fields and losing current data.
• "date_to" and "date_start" fields has his own onchange method now, this allows to recompute the worked_days_lines without recomputing all the dynamic fields and losing current data.
• New method "_get_employee_contracts" allow to extract the contract fetching logic from the onchange method. This allow not having to call onchange_employee if we want to fetch the contracts in another function.
• Full refactoring of the onchange_employee method. Before this, this method is used for all the information fetching like worked_hours and inputs. The problem in doing it in only one method is that it's difficult to inherit from another modules and also complicates to change the functions get_inputs and get_worked_days_lines that are constantly inherited by other modules to bring more data to the payslip. Now this method is only in charge of validating the necessary fields for the dynamic fetch and autoasign the dynamic values (contract and struct_id) when we change the employee.

Commit: [IMP] payroll: refactor of the method get_worked_day_lines[IMP] payroll: refactor of the method get_worked_day_lines

• New method "_compute_worked_days" extracted from get_worked_day_lines. Now is simpler to inherit this method if someone wants to make changes in how it calculate worked_hours.
• New method "_compute_leave_days" extracted from get_worked_day_lines. Now is simpler to inherit this method if someone wants to make changes in how it calculate worked_hours.
• Refactor of the method "get_worked_day_lines". Now this method only encapsulates the required fields for the data fetching from other models and then call the individual methods for doing such task. This provide more support for custom modules inheriting individual data fetching methods without need to override or copy get_worked_day_lines. Also if anyone wants to add new functions that fetch data from other sources, it will be more easy from another modules.

Commit: [IMP] payroll: Add domain to contract in many2one tables

• Adds domain to contract_id fields in worked_day_lines and input_lines in the payslip form. Before that all contracts were a possible option which not make sense because the payslip is only for one employee. Now the dropdown only shows the contacts for that employee and dates.

Commit: [IMP] payroll: Add daterange widget for dates

Here we add daterange widget which i think makes sense for this type of views. Now you can select the date with a daterange widget which is useful for preventing errors in the dates.

Commit: [IMP] payroll: Refactor get_payslip_vals and add struct_id selection …

This commit refactor the get_payslip_vals used in batch run payslip calculation. Also, we add the possibility to select the salary_structure in the run form, that way we provide the user the option to choose to make the payslips for a structure different of the selected in the contract. Before that, if you use batch computation of payslips, you can only do it for the structure defined on the contract, now, the user can choose which structure use for all payslips at once.

Will be making more commits soon refactoring other areas of the hr_payslip file. Keep you posted and let you know when is ready for review. If someone wants to test it and give advice is welcomed.

Regards.

[OCA-git-bot]

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

[nimarosa]

Also @pedrobaeza can you aprobé my GitHub user in weblate for Spanish and Spanish (Argentina)? I send two mails but no one approved me yet. So I can also make the translation of modules.

[pedrobaeza]

Done

[nimarosa]

Added commit.

**Commit: ** [IMP] payroll: refactor of the method get_worked_day_lines[IMP] payroll: refactor of the method get_worked_day_lines

• New method "_compute_worked_days" extracted from get_worked_day_lines. Now is simpler to inherit this method if someone wants to make changes in how it calculate worked_hours.
• New method "_compute_leave_days" extracted from get_worked_day_lines. Now is simpler to inherit this method if someone wants to make changes in how it calculate worked_hours.
• Refactor of the method "get_worked_day_lines". Now this method only encapsulates the required fields for the data fetching from other models and then call the individual methods for doing such task. This provide more support for custom modules inheriting individual data fetching methods without need to override or copy get_worked_day_lines. Also if anyone wants to add new functions that fetch data from other sources, it will be more easy from another modules.

[nimarosa]

@appstogrow @pedrobaeza Hello guys, i'm back from vacation. Could you review this refactoring to see what else we can improve in my approach? I plan to finish it this week. I have another changes regarding refactoring the method used in payslip run batches but you can review what is done so far to see what you think.

Also don't worry about the number of commits, i will squash everything to make it clear when it's ready.

[ghost]

I tested with Anita Oliver in demo data. Added global leave and individual time off. Payslip got 3 lines as expected: worked days, global leaves, time off.

I changed the contract, with start day towards the end of the month, after all the leaves. Payslip got only one line as expected, with just a few worked days.

@mtelahun Would you like to run any tests?

[nimarosa]

@appstogrow Great, you can also test that now when you change the struct_id (salary_structure) it recalculate the inputs table acording to the concepts included in that salary_structure. Before that, when you change struct_id the module won't update the inputs. That's wrong because if you want to manually change the salary_structure the user would expect the inputs to change to the concepts included in that structure, not to maintain the old ones.

Before this refactoring, the only way to get the inputs updating is to change the default salary_structure in contract, which is inconvenient.

Also like you tested we now update the worked_day_lines when the date changes, without updating anything else. That's better because before when you change the date, all the form is re-rendered so you lost input values if you have edited them.

@mtelahun It would be great if you want to include some tests, because i don't write tests myself and i'm not too familiar with testing in odoo. If we don't add tests, anyways i tested it a lot of times and i think it works great but if you want to contribute with testing you will be welcome.

Will try to add more refactoring in this PR so we can make all in one.

[ghost]

Tested in payslip:

• Changing date_from or date_to will update Worked Days lines.
• Changing struct_id will update Other Inputs lines.

What about adding a button, or include in Compute Sheet, to recompute worked days and other inputs? If the contract, salary rules, leaves or resource calendar changes, a recomputation might be needed.

[nimarosa]

@appstogrow We could add a button because in some use cases, the user might want to add manually or change some value in worked_day_lines, so if we update in compute_sheet we will drop support to manual input because we will be always recalculating the data.

Will work in the button and commit here.

[nimarosa]

@appstogrow It's done, please test it and tell me what you think. The button is called "Refetch Payslip Data" and should only appear in draft.

[nimarosa]

What do you guys think in updating the name field string that is auto-generated? In my opinion it's too long the name "Salary Slip of Mitchell Admin for june-2022", in other languages like spanish, is more longer too.

I think the name field should only contain the month, "june-2022" or "2022-06" because the employee name is implied in the employee_id field, so it's redundant for me.

Maybe we could add more useful information like a string that can be "june-2022 Monthly" => This will be a concatenation of the month-year and the schedule_pay field present in the contract. I don't know in another countries but in mine, we are used to show in the payslip the schedule_pay of the period we are doing payroll for.

What do you think?

[nimarosa]

Commit: [IMP] payroll: Add domain to contract in many2one tables

• Adds domain to contract_id fields in worked_day_lines and input_lines in the payslip form. Before that all contracts were a possible option which not make sense because the payslip is only for one employee. Now the dropdown only shows the contacts for that employee and dates.

[ghost]

The button Refetch Payslip Data works well.
The contract filters work well, I can only select contracts of the employee of the payslip.

The payslip name: I think "Salary Slip of Mitchell Admin for june-2022" is good. If the computation is in a separate method, maybe you would like to override the method in another module, or add a configuration setting to instruct how to compute the name?

[nimarosa]

@appstogrow I took your suggestion, i extracted the name computation method outside the onchange_employee so anyone can inherit that method and compute the name the way they want. Please test.

Actually all this changes are working in individual payslip creation, i will then refactor the get_payslip_vals used in payslip batch runs to also adapt this new changes there.

[mtelahun]

@nimarosa I am in general agreement that the payroll module could benefit from more refactoring; however, I don't have time to test it today. I will get to it tomorrow.

[mtelahun]

@nimarosa I'm not a big fan of the long name, either. I don't use the default payslip report so it doesn't affect me much, but it is annoying in the UI (especially in the bread crumbs). Maybe something like "Name: Year Period". The name is useful in the bread crumbs. i.e if the name was "Michael Telahun: 2022 June" in the bread crumbs it would be: "Employee Payslips / Michael Telahun: 2022 June / ...

[nimarosa]

@mtelahun Hello, about the naming thing. I extracted the name computation method into a separated method _compute_name so developers can inherit that function and adapt the naming method to our needs. In the main module as other suggested we will leave it like it is and then if anyone wants to change it, they can easily inherit the function.

Maybe in the future we can work in a setting or config to let the user define that in the UI, but i think it's not a priority now.
I also change it in my own modules like you do.

[nimarosa]

I will make the refactoring of the function _get_payslip_vals used in payslip batches and i think we are ready to merge for this PR.

If someone has another suggestion please tell me and i can include it here. The idea is to leave a clean and inheritable code for the payslip model. Then we can move forward with the other files.

[nimarosa]

Commit: [IMP] payroll: Add daterange widget for dates

Here we add daterange widget which i think makes sense for this type of views. Now you can select the date with a daterange widget which is useful for preventing errors in the dates.

Commit: [IMP] payroll: Refactor get_payslip_vals and add struct_id selection …

This commit refactor the get_payslip_vals used in batch run payslip calculation. Also, we add the possibility to select the salary_structure in the run form, that way we provide the user the option to choose to make the payslips for a structure different of the selected in the contract. Before that, if you use batch computation of payslips, you can only do it for the structure defined on the contract, now, the user can choose which structure use for all payslips at once.

I think it needs more refactoring in the wizard too, please test this change and i wait for your opinions.

@appstogrow @mtelahun

[ghost]

Payslip select daterange works.
Payslip Batch select structure works.

@nimarosa what other refactoring would you like?
I suggest to discuss additional refactoring in a new issue, come to an agreement, then make a new PR to test and approve.
This was the fourth time I tested this PR. I think it is not a good workflow to add new improvements to an existing open PR.

[nimarosa]

@appstogrow Hello, okay I will finish doing some test I had planned today and submit some minor changes, but for this PR I think we are ready.

I have planned all this refactoring in different PRs but I decided to bring it to one because I think they are highly related. That's why i submitted the changes and leave a log of what each commit does.

Next PR I will submit it when it's done if you want. I wanted to do this way to foment collaboration and ideas during the refactor.

[ghost]

@nimarosa It is ok to collaborate on a PR which is not finished. Then it is good to have it in draft state instead of open. In the beginning you made clear that this PR is work in progress. By now I thought it was ready.

[ghost]

I just tried to merge pr 45, 47 and 50 locally. Both 47 and 50 are modifying hr_payslip.py get_worked_day_lines(). Since #50 is almost ready to merge, pr 47 will have to refactor this method afterwards.

[nimarosa]

@appstogrow Yes I was thinking of that too. I will try to finish this today (I only have one commit ready but pending to sync) so maybe we can merge this and the adapt PR 50 to the refactored method.

Will try to do it today when I get home.

[nimarosa]

@appstogrow This is ready to merge. No more changes remaining for this PR.

@pedrobaeza Can we check this for merge?

[pedrobaeza]

If the rest approve it and CI is green, I'll merge it.

[nimarosa]

@pedrobaeza I just rebased a few commits. CI is running again now.

[ghost]

The code is good.
Please squash to one commit.

[nimarosa]

@appstogrow Everything squashed now. Wait for the one more approval and we are ready to merge.

[pedrobaeza]

@mtelahun do you agree with this change?

[mtelahun]

Yes, @pedrobaeza . I'm in favor of it

[pedrobaeza]

Let's go!

/ocabot merge major

[OCA-git-bot]

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-47-by-pedrobaeza-bump-major, awaiting test results.

[OCA-git-bot]

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

[mtelahun]

The computation method is fine for me. I expect that most people will customize it anyways so it's really not worth our time to try and find the "perfect" wording.

…

On 11/08/2022 20:20, Nicolas Rodriguez Sande wrote: @mtelahun <https://github.com/mtelahun> Hello, about the naming thing. I extracted the name computation method into a separated method _compute_name so developers can inherit that function and adapt the naming method to our needs. In the main module as other suggested we will leave it like it is and then if anyone wants to change it, they can easily inherit the function. Maybe in the future we can work in a setting or config to let the user define that in the UI, but i think it's not a priority now. I also change it in my own modules like you do. — Reply to this email directly, view it on GitHub <#47 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4YOHBE4LLEGCKICHNUL4TVYUY4BANCNFSM52LQ4SMA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[mtelahun]

I can wait until this is merged.

…

On 14/08/2022 17:18, Nicolas Rodriguez Sande wrote: @appstogrow <https://github.com/appstogrow> Yes I was thinking of that too. I will try to finish this today (I only have one commit ready but pending to sync) so maybe we can merge this and the adapt PR 50 to the refactored method. Will try to do it today when I get home. — Reply to this email directly, view it on GitHub <#47 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4YOHAYWQ3K2X73FYDCHCLVZD54DANCNFSM52LQ4SMA>. You are receiving this because you were mentioned.Message ID: ***@***.***>