[14.0] [ADD] payroll_rule_time_parameter / [IMP] payroll: remove rule_parameters from payroll & [FIX] payslip and payslips objects

[norlinhenrik]

With this PR, payroll depends on base_time_parameter.
@nimarosa if you don't want to depend on server-tools, would you like to copy base_time_parameter into a private repo?

What do you think about these changes in models and fields?

• hr.rule.parameter --> base.time.parameter
  • parameter_version_ids --> value_ids
  • new field: type
• hr.rule.parameter.value --> base.time.parameter.value
  • rule_parameter_id --> parameter_id
  • parameter_value --> value_text
  • new field: value_reference
  • new field: type

Would you prefer base.time.parameter.version and version_ids?

Later I will write migration script for this PR.

[OCA-git-bot]

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

[nimarosa]

Hello @norlinhenrik, if you want to make this working i think you should implement the part of hr_salary_rule_parameter as implemented previously, (see hr_payslip.py file) it adds the function to Payslips object to allow calling it from inside salary rules using payslip.rule_parameter("CODE").

Also the views of the salary rules parameters should be by default filtered to only "Payroll" records type of parameter, so if we use the module in another place, we don't end up with all time parameters of the instance. For this i think we should add a "Model" field that allows us filtering the views.

We can work it out in this PR but we need to replicate current functionality to change hr_salary_rule_parameter to this new dependency.

[nimarosa]

Also check the runboat and tests since they are not passing.

[norlinhenrik]

@nimarosa Now you can test this PR together with the PR for base_time_parameter.

[nimarosa]

@norlinhenrik Can you rebase this so i can test it?

[nimarosa]

@norlinhenrik We have a licenses issue between modules. I suggest we choose LGPL-3 license for both modules this should fix tests.

payroll (LGPL-3) depends on base_time_parameter (AGPL-3)

[pedrobaeza]

Both should be LGPL instead. As payroll comes from Odoo, we can't relicense it.

[nimarosa]

@pedrobaeza @norlinhenrik so we have to make PR changing the license in server tools module and then we retrigger tests here.

[pedrobaeza]

Yes

[nimarosa]

@norlinhenrik Just retriggered the tests. You also have a error in pre-commit.

payroll (Beta) depends on base_time_parameter (Alpha) I think we will have to change alpha status of base_time_parameter

[nimarosa]

/ocabot rebase

[OCA-git-bot]

Congratulations, PR rebased to 14.0.

[nimarosa]

/ocabot rebase

[OCA-git-bot]

Congratulations, PR rebased to 14.0.

[norlinhenrik]

@nimarosa I was thinking to make a test to satisfy covecov. But then I have a question:

Now when hr.rule.parameter is renamed to base.time.parameter, would you also like to rename payslip.rule_parameter(code) to payslip.time_parameter(code)?

[nimarosa]

@norlinhenrik i would prefer leaving the name as rule_parameter since I have a lot of rules that use this method and people are trained to call this method. Changing it now it will have impact in current instances.

What we can do is add a deprecation warning log message and we can change it maybe in the future.

I will try to make time to test this today, so we can merge. A question, is the views filtered by model solved?

[norlinhenrik]

@mtelahun Can we inherit the payroll browsable class in payroll_rule_time_parameter? If not, do you see that as a problem?

@nimarosa localdict without records is much safer. Then contract.env["hr.payslip"] will not be possible.

[nimarosa]

@nimarosa localdict without records is much safer. Then contract.env["hr.payslip"] will not be possible.

The problem is that we already have a bunch of records. You can edit contract and employee if you want and if you want to access payslip you can with env:

I don't test the contract.env. The one I tested and know it can be done because I used it with prior impmemntation is:

payslip.env["hr.payslip"]

Test it, try to call a function or field from that object and it will work.

Edit: actually it will not work from contract because it's an object but it will work from any browsable class object so you can also do worked_days.env["model"] if you want.

So I think the security issue it's not in adding payslip as an object because it can be accessed anyways, we have to work in safe_eval security or in writing more standard methods to move python to another module like you suggested, or even write own rule language. But this is out of scope of this PR, and adding payslip is actually the same in security concerns, it does not add security but didn't increase current risk, we already have objects and environment available in rules. If someone want to maliciously do something with a rule, they will can. That's why we added security groups and probably I will make more PRs trying to improve security. But not adding payslip literally don't change nothing to current security issues. I think @mtelahun will agree on this since it's a known problem and fixing this object (like it was intended to be) will not change it.

[norlinhenrik]

I don't like that employee, contract, and now payslip objects can be accessed (and potentially modified) from salary rules.

@mtelahun
If we can call browsableobject.env["model"], then what is the security benefit of not passing records directly?

@nimarosa It is easy to override the localdict. In payroll_payslip_line_manually I add the rule record:

    def _satisfy_condition_python(self, localdict):
        localdict["rule"] = self
        return super(HrSalaryRule, self)._satisfy_condition_python(localdict)

    def _compute_rule_code(self, localdict):
        localdict["rule"] = self
        return super(HrSalaryRule, self)._compute_rule_code(localdict)

[nimarosa]

I don't like that employee, contract, and now payslip objects can be accessed (and potentially modified) from salary rules.

@mtelahun If we can call browsableobject.env["model"], then what is the security benefit of not passing records directly?

That's the point. The security issues come from safe_eval itself not from adding a record directly. We do it with contract and employee and also have access to all env objects so, why not to add payslip object since we always thought it was there. I didn't realize that the payslip object was a browsable object until I had to call a method. The other thing is they Payslips browsable class object should not be used as payslip object, instead it's there to interact with other payslips (like the sum method does).

@nimarosa It is easy to override the localdict. In payroll_payslip_line_manually I add the rule record:

    def _satisfy_condition_python(self, localdict):

        localdict["rule"] = self

        return super(HrSalaryRule, self)._satisfy_condition_python(localdict)



    def _compute_rule_code(self, localdict):

        localdict["rule"] = self

        return super(HrSalaryRule, self)._compute_rule_code(localdict)

Yes I know. It was my first approach until i realized that with base_time_parameter I can call the function directly from any model. Then i realized that the payslip object was not really the real one.

But this changes are not only motivated by this module (it help a lot because we didn't have to add any code for base_time_parameter to work) but also because accessing the real payslip object allow us to call methods inside it, that return information, like we can do with contract and employee. If we use the browsable version of payslip, we can only access stored fields, not even computed ones. So that's a limitation that is not justified by security issues because the current python implementation has already security issues because of python safe_eval, so I continue thinking that for now the best is to add the correct object like I'm doing.

If then we want to work in security we have to do one of the things I listed earlier, since not adding an object now will not make the system more secure.

[nimarosa]

@norlinhenrik @mtelahun i created a new issue to start working in a new language or framework to write rules that allow us to get rid of safe_eval and security concerns.

In the meantime, like I said, I think that we should finish this PR and let payslip object exists since we have no other option for now and the system is already insecure since it has env access. Adding or removing objects will not change that. We should work on safe eval which is really the problem.

Regards.

[mtelahun]

Yes, it's a difficult problem. That's one of the reasons why I said it should be a problem for another PR. That's also why I don't really object to payslip also being directly available now. It's a hard problem that needs to be solved, but it's going to take time.

…

On 08/11/2022 14:10, Henrik Norlin wrote: I don't like that employee, contract, and now payslip objects can be accessed (and potentially modified) from salary rules. @mtelahun <https://github.com/mtelahun> If we can call |browsableobject.env["model"]|, then what is the security benefit of not passing records directly? @nimarosa <https://github.com/nimarosa> It is easy to override the localdict. In |payroll_payslip_line_manually| I add the rule record: |def _satisfy_condition_python(self, localdict): localdict["rule"] = self return super(HrSalaryRule, self)._satisfy_condition_python(localdict) def _compute_rule_code(self, localdict): localdict["rule"] = self return super(HrSalaryRule, self)._compute_rule_code(localdict) | — Reply to this email directly, view it on GitHub <#87 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4YOHEOBF6DXHC4C3AEALDWHIYJLANCNFSM6AAAAAARHMSPFY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[norlinhenrik]

Good, then localdict["payslip"] will be the record instead of the browsable object. Is there any behavior of the browsable object that the record does not have? If not, then why add the browsable object as localdict["payslips"]?

@nimarosa In the commit [14.0] [IMP] payroll: change manifest category could you please update the category in ALL modules in the repo?

I see that you could update my branch directly, without any PR. How did you do it?

[nimarosa]

@norlinhenrik

Good, then localdict["payslip"] will be the record instead of the browsable object. Is there any behavior of the browsable object that the record does not have? If not, then why add the browsable object as localdict["payslips"]?

"payslips" browsable object, contains a "sum" method, that allow to sum payslip lines totals from other confirmed payslips between a date range. You should pass the line code and at least a from_date, and optionally you can pass to_date. This is useful when you need to operate with other payslips data. It use this method when i need to sum yearly gross for example, or make some average salary calculations from some rule. More useful methods could be added to this browsable object, i have plan to add an average one, and a yearly gross one. So i think it's useful to keep it.

So now we will have two objects:

• payslip: real original payslip object
• payslips: the browsable one, in which we can plug in methods to work with all payslips data, not only the current one. For now, we have the sum method.

@nimarosa In the commit [14.0] [IMP] payroll: change manifest category could you please update the category in ALL modules in the repo?

Yes will do in next commit.

I see that you could update my branch directly, without any PR. How did you do it?

When you create the PR, by default you have an option that says "Allow edits from maintainers". When this setting its activated, it allow more than you to collaborate in your own PR. This is very useful for collaboration in large PRs or features.

But i don't know if anyone can do it. I think i can because i have write access to the repository because the PSC role i have assigned, didn't tried it in other repos (will do and let you know).

I also do it with vscode github extension that let me select the pull request and change to the "pull request branch" and then i make the changes and push. It looks like this:

So i think it's like a virtual branch that github creates and the replicates in yours. I don't know if requires some specific commands because i always do it with vscode.

[norlinhenrik]

Thanks for the explanations.
I suggest you add this to the payroll migration script (not the best scripting but I think it works):

def localdict_convert_payslip_to_payslips(env):
    for rule in env["hr.salary.rule"].sudo().search([]):
        if 'localdict["payslip"]' in rule.condition_python:
            rule.condition_python = rule.condition_python.replace(
                'localdict["payslip"]', 'localdict["payslips"]'
            )
        if "localdict['payslip']" in rule.condition_python:
            rule.condition_python = rule.condition_python.replace(
                "localdict['payslip']", "localdict['payslips']"
            )
        if 'localdict["payslip"]' in rule.amount_python_compute:
            rule.amount_python_compute = rule.amount_python_compute.replace(
                'localdict["payslip"]', 'localdict["payslips"]'
            )
        if "localdict['payslip']" in rule.amount_python_compute:
            rule.amount_python_compute = rule.amount_python_compute.replace(
                "localdict['payslip']", "localdict['payslips']"
            )

[nimarosa]

def localdict_convert_payslip_to_payslips(env):
for rule in env["hr.salary.rule"].sudo().search([]):
if 'localdict["payslip"]' in rule.condition_python:
rule.condition_python = rule.condition_python.replace(
'localdict["payslip"]', 'localdict["payslips"]'
)
if "localdict['payslip']" in rule.condition_python:
rule.condition_python = rule.condition_python.replace(
"localdict['payslip']", "localdict['payslips']"
)
if 'localdict["payslip"]' in rule.amount_python_compute:
rule.amount_python_compute = rule.amount_python_compute.replace(
'localdict["payslip"]', 'localdict["payslips"]'
)
if "localdict['payslip']" in rule.amount_python_compute:
rule.amount_python_compute = rule.amount_python_compute.replace(
"localdict['payslip']", "localdict['payslips']"
)

Actually we should replace payslip with payslips. It's not necessary to call localdict inside rule, you call directly the object. I will adapt the code and add it to the migration in case this change affects anyone.

Edit: @norlinhenrik done. Please check last commit, i'm not so experimented with migration.

[norlinhenrik]

Usually all methods are inside the migration script. I put move_records outside of (14.0.6.0.0) so I could import it also in a hook.

In pre-migration, the models are not loaded yet, so we cannot call env["hr.salary.rule"] in pre-migration. So create a new file post-migration.py:

from openupgradelib import openupgrade


def localdict_convert_payslip_to_payslips(env):
    for rule in env["hr.salary.rule"].sudo().search([]):
        if "payslip" in rule.condition_python:
            rule.condition_python = rule.condition_python.replace("payslip", "payslips")
        if "payslip" in rule.amount_python_compute:
            rule.amount_python_compute = rule.amount_python_compute.replace(
                "payslip", "payslips"
            )


@openupgrade.migrate()
def migrate(env, version):
    localdict_convert_payslip_to_payslips(env)

I have tested that this code works.

[nimarosa]

@norlinhenrik Done now. I think... please check it and thanks for the input.

[norlinhenrik]

post-migration works.
But employee.payslip_count should not become employee.payslips_count.
So please replace "payslip." to "payslips."
This would still change env["hr.payslip.line"] to env["hr.payslips.line"]
So maybe it is better to drop this migration and manually change payslip to payslips when needed?

[nimarosa]

post-migration works.

But employee.payslip_count should not become employee.payslips_count.

So please replace "payslip." to "payslips."

This would still change env["hr.payslip.line"] to env["hr.payslips.line"]

So maybe it is better to drop this migration and manually change payslip to payslips when needed?

Yea I agree. Anyways if the user is using payslip (the old one) to access payslip fields it will continue working after the change. The only case the user must change it manually is if they are using the sum method of the browsable, so I think it's no a problem to leave that migration out.

Will make the changes and finally merge so we move forward.

[nimarosa]

Okay so migration is removed, everything else is working. I think we can merge this one.

/ocabot merge major

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-87-by-nimarosa-bump-major, awaiting test results.

[OCA-git-bot]

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