[ADD] hr_payslip_line_manually, [IMP] payroll (hr.payslip & hr.salary.rule) #51

ghost · 2022-08-15T22:42:03Z

In previous payroll software, I registered payslip lines and contract lines (recurring payslip lines) directly. So I developed this for Odoo a few years ago when I transitioned to Odoo.

Usage:

Create salary rules, with selection field for payslip or contract (see demo rules).
From payslip or contract, go to Salary Rules section, and add a line with the values you want for the payslip line.
You may use the same salary rule on multiple lines, with different analytic accounts.

Questions:

Do you think this is useful?

hr.payslip _get_payslip_lines() has an extra loop to create multiple lines for the same salary rule. There are several other changes also in payroll. I don't think they impact existing implementations, but I don't know. Are the changes acceptable?

Model hr.analytic.qty.rate.amount has the fields model and res_id. Both payslip and contract has a field with one2many relation to hr.analytic.qty.rate.amount. This doesn't work so well in version 14.0. Do you know any other Odoo module with similar structure?

nimarosa · 2022-08-16T15:35:41Z

Hello @appstogrow, it's not clear to me which is a use case of this PR. I saw the demo examples but it didn't make more clear to me about the usage. Can you make an example so i can understand more? It seems interesting but i couldn't get it looking at the code only.

About the question, hr.payslip _get_payslip_lines(), i don't think that an extra loop makes any harm. As i saw, you are using contract and analytic code for differentiating lines codes, so it works like the multiple contract implementation, so i think it works okay. Will test it tomorrow if i can.

About hr.analytic.qty.rate.amount, what do you see that don't work well on 14.0? If you are saying this because the checks are failing, i think the build fails because payslip_line is a inherited model of salary_rule so it copy the fields added in salary_rule and that maybe could be fixed easily.

Hope you get back with examples, it looks interesting module but couldn't figure out yet the usage.

ghost · 2022-08-21T12:34:39Z

I have salary rules "Monthly wage" (for payslip) and "Fixed monthly wage" (for contract).
In contract I register fixed monthly wage per department (analytic account).
In payslip I register one-time montly wage per department (analytic account).
"Compute Sheet" will create payslip lines the way they are defined in contract and payslip.

ghost · 2022-08-22T19:44:45Z

See the description here.

Why is indentation not allowed in a code demo in the description?

nimarosa · 2022-08-22T19:49:46Z

@appstogrow i can't see in the link the part you are referring. But i think you are talking about pre-commit failing? pre-commit has some weird rules i suppose mandatory by oca code guidelines. Sometimes it fails in cases when it's wrong indentation. Think we have to adapt and fix all the error pre-commit says :/

nimarosa · 2022-08-22T19:54:26Z

@appstogrow
I made a fast look at the code Henrik. I think it's not good to implement the feature of the new module in payroll and leave it there even if the module is uninstalled. I understand that you have done this because it's not possible to inherit and call super() on that functions without overriding them.

I think we can work in a refactor of the function for allow them to be inherited and modified by another modules without the need to edit them in the main one. It's on my plans to do it.

What do you think of wait for me to refactor that function to merge this module? Because i think that if we add more code to the function it will make it more and more large and we will end up with a huge function that supports other modules even if you don't install them.

ghost · 2022-08-23T11:09:23Z

@nimarosa I just read your comments now, after suggesting a 15.0-dev branch in issue #58.

Very nice that you would like to refactor the very huge function _get_payslip_lines() 👍 💯
This PR can wait for the refactoring.

nimarosa · 2022-08-23T12:18:31Z

@appstogrow Great, this week I will have more than one PR ready so we will have it soon I hope.

nimarosa · 2022-09-09T15:42:13Z

@appstogrow Hi Henrik, with #62 merged i think you can continue with this PR. I will wait for you to finish to introduce new PRs so we can get this merged before.

If you need help, let me know.

ghost · 2022-09-13T22:31:32Z

I have done some changes in payroll - what do you think?

hr.salary.rule
_compute_rule() returning tuple ---------> _compute_values() returning dict

hr.payslip
_compute_payslip_line()
values = rule._compute_values(localdict)
localdict["result_rules"].dict[rule.code] = BaseBrowsableObject(values)

ghost · 2022-09-13T22:32:23Z

How can we change the state from Draft to Open?

nimarosa · 2022-09-14T00:31:59Z

How can we change the state from Draft to Open?

@apps2grow What are you referring to?

nimarosa

@appstogrow I made some comments, maybe i'm missing something or it's not finished?

Payslip and salary_rule changes in payroll look good to me, but made some comments of improvements that we could make.

Also you should run pre-commit before you push, that way you will not get the error.

payroll/models/hr_salary_rule.py

payslip_line_manually/models/hr_payslip.py

payslip_line_manually/models/hr_salary_rule.py

ghost · 2022-09-15T16:07:07Z

Thank you @nimarosa for reviewing. I answer to everything here.

pre-commit still fails, because of documentation with demo code. I think this is ok.

hr.salary.rule _compute_rule() returned a tuple. Changing to a dict will break a custom implementation of the method. IMO renaming the method does not make the situation worse. I just renamed to _compute_payslip_line_values() which I think is a good name. _compute_rule() is also good name. I am flexible.

Currently, I don't need separate methods to compute fixed values or percentage values. I think it we should not make such methods until we expect that there is a need for them.

hr.payslip.line.manually has a field for salary rule. When computing the rule, the code needs the records belonging to the rule. But the code doesn't know which rule is computing. Therefore I need localdict["rule"]. @nimarosa, do you see another way to do this?

Thank you for pointing out that overriding methods ideally should call super(). The new module hr_payslip_line_manually can now do this for _compute_rule() / _compute_payslip_line_values() but not for _compute_payslip_line(). First we would need to change the structure of _compute_payslip_line(), and I think we should do that in another PR.

@mtelahun Would you also like to review?

mtelahun · 2022-09-15T20:00:45Z

Hi @appstogrow, I can't really comment on the module itself because I don't understand the problem you are trying to solve. I can understand that this is something you needed to solve a problem. I'm just saying I don't understand what that is. However, I have some general comments on things I noticed in the code. - It seems to me it replicates large parts of the payroll code. For example hr.payslip.line.manually replicates some the same fields as hr.payslip.line. It also seems _compute_payslip_line() in hr.payslip is replicated again (I don't know if this is intentional or a mistake). The whole concept of hr.payslip.line.manually as a separate model escapes me. Surely it should inherit from hr.payslip.line? - Also, I don't understand what analytic accounts have to do with manual payslip lines. Shouldn't they also apply to hr.payslip.line? And if analytic accounts are not specific to manual payslip lines shouldn't it be in a separate module: hr_payslip_line_analytic? - In the demo data the condition python fields for both rules are virtually identical. This to me is a clear sign that there must be a better way to handle this. For example a fourth 'condition_select' selection ("manual_payslip_line_rule", "Manual payslip line rule match"). Then the module can do the matching that is repeated now in every rule in the 'condition_python' field. - As I've mentioned previously I am not a fan of complicated code in salary rule python fields and the code in the demo rules makes me really uneasy. - The __manifest__ currently says it depends on payroll, but shouldn't it include 'account_analytic' also? And if so, then it should also depend on payroll_account, yes? I have a "feeling" that there must be a better way to do what you want to do but without understanding your problem I am not able to offer a better solution 😬

…

On 15/09/2022 19:07, Henrik Norlin wrote: @mtelahun <https://github.com/mtelahun> Would you also like to review? — Reply to this email directly, view it on GitHub <#51 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4YOHAO5HDL76KMCEMUGOTV6NCTLANCNFSM56TWWJVA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

nimarosa · 2022-09-16T01:26:53Z

@appstogrow

pre-commit still fails, because of documentation with demo code. I think this is ok.

Maybe we can put this documentation in usage.rst? It's really necessary for it to be in demo xml? I think people will not look for documentation there.

hr.salary.rule _compute_rule() returned a tuple. Changing to a dict will break a custom implementation of the method. IMO renaming the method does not make the situation worse. I just renamed to _compute_payslip_line_values() which I think is a good name. _compute_rule() is also good name. I am flexible.

I think it's better to leave the same name "_compute_rule()" if there is not any necessity to change it. I have some modules that inherit this method. It's not a trouble to change them but maybe people using the module who are not aware of this change will see their custom modules affected. If not crucial for you, I prefer we leave the current name, for me it's a good one.

Currently, I don't need separate methods to compute fixed values or percentage values. I think it we should not make such methods until we expect that there is a need for them.

I think that for good coding practices and because all refactoring we are doing in the past PRs will be great to separate each computation mode like you've done with _compute_python. You gave me the idea with your changes and I think it's a better approach to break the function in more smaller ones so they can be easily inherited if needed.

I don't currently need this either but I think is good for the module. If you want to do it in this PR it's okay, if not, maybe I will include it in my next one as part of all the refactoring process of payroll.

I think we are close to be ready for start the migration to version 15.0 so I think that all refactoring we can merge to 14.0 before that, will be great to have a strong module for 15.0 and 16.0 in the future. More maintainable and easy to develop. That's why I suggested this.
But feel free to leave it out if you don't want to do it and I'll include it in next PRs.

hr.payslip.line.manually has a field for salary rule. When computing the rule, the code needs the records belonging to the rule. But the code doesn't know which rule is computing. Therefore I need localdict["rule"]. @nimarosa, do you see another way to do this?

I think like @mtelahun i still don't understand what this new module currently does. Will try to look it more in depth and give you input, maybe we can crack a better way to archive it.

Thank you for pointing out that overriding methods ideally should call super(). The new module hr_payslip_line_manually can now do this for _compute_rule() / _compute_payslip_line_values() but not for _compute_payslip_line(). First we would need to change the structure of _compute_payslip_line(), and I think we should do that in another PR.

The problem with that is that if you override functions without using super, any user that wish to use your module will not be able to use any other module that inherits that functions. That makes your module very incompatible with all private payroll modules/localizations.

Henrik, can you try to explain to us in a simple way, what is the use case or the problem your module solves? Maybe some images or videos will be helpful.

I will install it tomorrow and try to understand it on the functional side, but I think that if we can understand more we can help you to improve it in a compatible way when you don't need too much override or repeated code.

Look forward for your input.

ghost · 2022-09-16T08:04:35Z

It is USAGE.rst which is failing in pre-commit.

I will change the name back to _compute_rule().

I agree "it's a better approach to break the function in more smaller ones". One function should do only one thing. _compute_rule() has code for 3 scenarios, and I would like to introduce a 4th scenario: select a method from a list of available methods. Maybe we should introduce a hook so developers can introduce scenarios in custom modules. This needs more thought, and therefore I don't do more changes to _compute_rule() now.

Please try out the hr_payslip_line_manually module with demo. Create a payslip and select "Manual Lines Structure". Then create a one2many line in Payslip and/or Contract, and Compute Sheet.

if you override functions without using super, any user that wish to use your module will not be able to use any other module that inherits that functions

That is correct, and it is a temporary solution. Currently, the new module CANNOT call super(). So we have these alternatives:
A) Merge this PR after I change the name back to _compute_rule() and get the latest improvements into payroll.
B) Merge this PR after I change the name back to _compute_rule() and remove hr_payslip_line_manually since you think it is not ready yet. IMO it is not perfect, but ready to use.
C) Do refactoring of _compute_payslip_line() in this PR.

I have documented hr_payslip_line_manually the best I can. Please read the documentation files.

ghost · 2022-09-16T09:23:21Z

Now I have changed the name back to _compute_rule().

nimarosa · 2022-09-16T14:32:19Z

@appstogrow Hello, i'm trying the module, but when i try to add a manual payslip line in contract or payslip i get this error:

Please save changes, then refresh the page (F5). If needed, use the menu Payroll / Employee Payslips, or the menu Employees / Employees / Contracts.

Is this expected? I can't add the lines.

nimarosa

@appstogrow

Hello Henrik, i've done a new review.

I tried the module, but i get the error that i described in my last comment every time i try to create a "manual_line". So i couldn't test much of it.

As saw, i understand the module is for making payslip lines manually in contract and/or payslip. So the payslip will compute the lines that you configured, no?

I can't think a use case but if you need it and use it, we can include it here in payroll.

I think that maybe you can move the changes in "payroll" to other PR? So we can review them and test it extensively. Then we can merge your module the way you want it to function. Despite that, i made some suggestions of your module code, that allows you calling super() in the right way in _compute_payslip_line. Please check it, that way you will edit the dict and don't override the compute method.

Regards

nimarosa · 2022-09-16T21:16:45Z

hr_payslip_line_manually/__manifest__.py

+    "license": "AGPL-3",
+    "version": "14.0.1.0.0",
+    "website": "https://github.com/OCA/payroll",
+}


Maybe you want to add the maintainer key so you can maintain the module?

nimarosa · 2022-09-16T21:17:05Z

hr_payslip_line_manually/__manifest__.py

+    "name": "Manual payslip lines, with analytic account",
+    "summary": "Easily create and use salary rules",
+    "author": "AppsToGROW, Odoo Community Association (OCA)",
+    "category": "Uncategorized",


Use "Payroll" category.

nimarosa · 2022-09-16T21:33:34Z

hr_payslip_line_manually/models/hr_payslip.py

+        copy=True,
+    )
+
+    def _compute_payslip_line(self, rule, localdict, lines_dict):


I think you can just do this here:

def _compute_payslip_line(self, rule, localdict, lines_dict): localdict, lines_dict = super()._compute_payslip_line(rule, localdict, lines_dict) values = localdict["result_rules"].dict.get[rule.code] current_key = (rule.code+ "-"+ str(localdict["contract"].id)) new_key = (rule.code + "-" + str(localdict["contract"].id) + "-" + str(values.get("analytic_account_id"))) lines_dict[new_key] = lines_dict.pop(current_key) lines_dict[new_key].dict["analytic_account_id"] = values.get("analytic_account_id") return localdict, lines_dict

I didn't test it but surely you can do this without copying the whole method, just edit the returning values and the values you can get from "result_rules" you don't need to calculate them again.

Sorry gitgub broken my indentation. Please copy the code to python file to read it easily.

nimarosa · 2022-09-16T21:34:41Z

hr_payslip_line_manually/models/hr_payslip_line_manually.py

+    _description = "hr.payslip.line.manually"
+
+    @api.model
+    def _compute_default_model(self):


This is the function that gives me the error. Do you know why?

nimarosa · 2022-09-16T21:36:55Z

hr_payslip_line_manually/models/hr_salary_rule.py

+                values["analytic_account_id"]: result_dict.get("result_analytic")
+                result_list.append(values)
+            return result_list
+        return super(HrSalaryRule, self)._compute_rule_python(localdict)


Not sure but this will generate a loop. You call super two times in the function.

nimarosa · 2022-09-16T21:38:36Z

hr_payslip_line_manually/models/hr_salary_rule.py

+            return result_list
+        return super(HrSalaryRule, self)._compute_rule_python(localdict)
+
+    def _satisfy_condition_python(self, localdict):


Sorry, I still don't get why this is necessary.

nimarosa · 2022-09-16T21:41:38Z

payroll/models/hr_payslip.py

-        localdict["result_rules"].dict[rule.code] = BaseBrowsableObject(
-            {"quantity": qty, "rate": rate, "amount": amount, "total": rule_total}
-        )
+        localdict["result_rules"].dict[rule.code] = BaseBrowsableObject(values)


I think you are leaving out including "total" in "result_rules". Original code is:

localdict["result_rules"].dict[rule.code] = BaseBrowsableObject( {"quantity": qty, "rate": rate, "amount": amount, "total": rule_total} )

It's okay for me to make a dict, but you should add the value values["total"] to values dict.

nimarosa · 2022-09-16T21:45:31Z

payroll/models/hr_salary_rule.py

@@ -250,6 +234,16 @@ def _compute_rule(self, localdict):
                    % (self.name, self.code, repr(ex))
                )

+    def _compute_rule_python(self, localdict):


Why leaving safe_eval out of _compute_rule_python?

nimarosa · 2022-09-16T21:46:15Z

payroll/models/hr_salary_rule.py

@@ -288,3 +281,7 @@ def _satisfy_condition(self, localdict):
                    )
                    % (self.name, self.code, repr(ex))
                )
+
+    def _satisfy_condition_python(self, localdict):


Here you do the safe_eval inside function. Which approach are we going to use? both "python" functions must behave very similar.

nimarosa · 2022-09-16T22:08:53Z

I agree "it's a better approach to break the function in more smaller ones". One function should do only one thing. _compute_rule() has code for 3 scenarios, and I would like to introduce a 4th scenario: select a method from a list of available methods.

Definitely we should implement this in next PRs. I take note of it for next one, so we can work in that.

nimarosa · 2022-09-16T22:15:41Z

That is correct, and it is a temporary solution. Currently, the new module CANNOT call super(). So we have these alternatives:
A) Merge this PR after I change the name back to _compute_rule() and get the latest improvements into payroll.
B) Merge this PR after I change the name back to _compute_rule() and remove hr_payslip_line_manually since you think it is not ready yet. IMO it is not perfect, but ready to use.
C) Do refactoring of _compute_payslip_line() in this PR.

Please check if my suggestion works for allowing you to call super() in _compute_payslip_line()

If we can't figure it out, maybe you can start a new PR with changes to "payroll" and maybe in that one we could implement the separation of methods of salary rule computation modes. Then we can merge this PR with only hr_payslip_line_manually in it, implemented the way it suits your needs, and then we can improve the module when we merge new changes to payroll that allow you to do it with super() but in the meantime, you have the module available in OCA. What do you think?

Anyways, i think the solution i pointed out could work with some adjustments.

So in resume:

New PR for the "payroll" module changes, so in the migrations we can easily migrate the base module and also have more detailed commit history of the changes.
In this PR, you could try my suggestion, if it works great. If not, we can merge the module but make an note in readme description that it overrides functions.
We introduce new PRs that continue improving the module and gives you the tools to update hr_payslip_line_manually later using correct inheriting.

Hope this works for you. Thanks.

ghost · 2022-09-18T12:49:51Z

Thank you @nimarosa and @mtelahun for reviewing!
Based on your responses plus my own, I have done more changes, and saved them in a new commit.
When #69 is merged, I will make a new PR with the payroll changes.

How can we set this PR in state draft?

nimarosa · 2022-09-21T21:39:13Z

@appstogrow If you want you can continue fixing the conflicts and checks in this PR since #70 is merged.
I will wait for you and then i have others PR prepared.

Tell me if you need any input.

ghost · 2022-09-22T14:01:57Z

@nimarosa Go ahead with your next PR. I am quite busy, and this PR may wait.

ghost · 2022-10-11T07:30:45Z

@nimarosa Yes currently you need to do ca what it says: follow the menu, refresh and then edit. I am still looking for a solution for this. @mtelahun I saw your message now, didn't see it this morning. On Sunday I will answer and provide a more clear description of the module and the purpose. fre. 16. sep. 2022, 16:32 skrev Nicolas Rodriguez Sande < ***@***.***>:

…

@appstogrow <https://github.com/appstogrow> Hello, i'm trying the module, but when i try to add a manual payslip line in contract or payslip i get this error: Please save changes, then refresh the page (F5). If needed, use the menu Payroll / Employee Payslips, or the menu Employees / Employees / Contracts. Is this expected? I can't add the lines. — Reply to this email directly, view it on GitHub <#51 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADXMQTYIHDDHYT4EXIG7YFDV6SAH5ANCNFSM56TWWJVA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

nimarosa · 2023-02-10T15:39:54Z

@appstogrow @norlinhenrik Hello Henrik, will this be finished o should i close it?

norlinhenrik · 2023-02-12T14:18:53Z

We can close this PR. I think it is better to create a new PR for hr_payslip_line_manually when I take time for it.

ghost mentioned this pull request Aug 23, 2022

15.0-dev branch for payroll #58

Closed

ghost marked this pull request as draft August 23, 2022 11:11

This was referenced Aug 29, 2022

[IMP] payroll: Improve views, add child functionalities, allow compute of name #60

Merged

[IMP] Payroll: Payslip refactoring and misc functionalies #62

Merged

ghost mentioned this pull request Sep 5, 2022

Employee with multiple contracts #65

Closed

nimarosa added work in progress no stale Use this label to prevent the automated stale action from closing this PR/Issue. labels Sep 13, 2022

ghost changed the title ~~[ADD] payroll_analytic_qty_rate_amount, [IMP] payroll~~ [ADD] payslip_line_manually, [IMP] payroll (hr.payslip & hr.salary.rule) Sep 13, 2022

nimarosa requested changes Sep 14, 2022

View reviewed changes

ghost changed the title ~~[ADD] payslip_line_manually, [IMP] payroll (hr.payslip & hr.salary.rule)~~ [ADD] hr_payslip_line_manually, [IMP] payroll (hr.payslip & hr.salary.rule) Sep 15, 2022

ghost marked this pull request as ready for review September 15, 2022 16:07

nimarosa requested changes Sep 16, 2022

View reviewed changes

nimarosa removed the no stale Use this label to prevent the automated stale action from closing this PR/Issue. label Sep 16, 2022

norlinhenrik added 3 commits October 26, 2022 17:33

[ADD] hr_payslip_line_manually

b8837a5

_compute_payslip_line() -> 3 methods, new module calling super()

0734cb7

[FIX] payroll_payslip_line_manually: pre-commit

b329097

ghost closed this Feb 12, 2023

This pull request was closed.

[ADD] hr_payslip_line_manually, [IMP] payroll (hr.payslip & hr.salary.rule) #51

[ADD] hr_payslip_line_manually, [IMP] payroll (hr.payslip & hr.salary.rule) #51

Conversation

ghost commented Aug 15, 2022

nimarosa commented Aug 16, 2022 • edited Loading

ghost commented Aug 21, 2022

ghost commented Aug 22, 2022

nimarosa commented Aug 22, 2022

nimarosa commented Aug 22, 2022

ghost commented Aug 23, 2022

nimarosa commented Aug 23, 2022

nimarosa commented Sep 9, 2022

ghost commented Sep 13, 2022

ghost commented Sep 13, 2022

nimarosa commented Sep 14, 2022 • edited Loading

nimarosa left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Sep 15, 2022

mtelahun commented Sep 15, 2022 via email

nimarosa commented Sep 16, 2022

ghost commented Sep 16, 2022 • edited by ghost Loading

ghost commented Sep 16, 2022

nimarosa commented Sep 16, 2022

nimarosa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimarosa commented Sep 16, 2022

nimarosa commented Sep 16, 2022 • edited Loading

ghost commented Sep 18, 2022

nimarosa commented Sep 21, 2022

ghost commented Sep 22, 2022

ghost commented Oct 11, 2022 via email

nimarosa commented Feb 10, 2023

norlinhenrik commented Feb 12, 2023

nimarosa commented Aug 16, 2022 •

edited

Loading

nimarosa commented Sep 14, 2022 •

edited

Loading

nimarosa left a comment •

edited

Loading

ghost commented Sep 16, 2022 •

edited by ghost

Loading

nimarosa commented Sep 16, 2022 •

edited

Loading