OCA / l10n-switzerland

Odoo Swiss localization
GNU Affero General Public License v3.0
53 stars 164 forks source link

[12.0][MIG] l10n_ch_hr_payroll - bis #647

Closed robinkeunen closed 2 years ago

robinkeunen commented 2 years ago

Taking over this stale PR https://github.com/OCA/l10n-switzerland/pull/567

I already fixed the linting errors and cleanup up the readmes. But I'm stuck with the tests.

This module adds the fields python_amount and python_rate to model hr.payslip.line. These fields are computed from amount and rate. These fields were added here: https://github.com/OCA/l10n-switzerland/commit/7eda5ca858d7537e6d0e4a1828d5b294d55e3d11

The tests break at the assertion(s) on the line.total value.

In the following extract, the expected total value is computed from python_amount and python_rate.

self.assertEqual(line.python_amount, round(calc_lpp, 2))  # passes
self.assertEqual(line.python_rate, -8.650)                # passes
self.assertEqual(line.total, -50.71)                      # fails

My first fix idea was to override and fix

@api.depends('quantity', 'amount', 'rate')
def _compute_total(self):
    for line in self:
        line.total = float(line.quantity) * line.amount * line.rate / 100

to take the new fields into account but I'm afraid it might break a lot of behaviours in the standard modules.

What confuses me is that I do not see any use of python_amount and python_rate elsewhere except in a view override.

Any ideas ?

oca-clabot commented 2 years ago

Hey @robinkeunen, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet. You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla Here is a list of the users:

Appreciation of efforts, OCA CLAbot

robinkeunen commented 2 years ago

Moving forward, I tried :

 @api.depends('quantity', 'python_rate', 'python_amount')
 def _compute_total(self):
     for line in self:
         line.total = float(line.quantity) * line.python_amount * line.python_rate / 100

This breaks many tests.

Also, leaving the _compute_total untouched and commenting the failing test breaks many other test that were not reached before.

=> I'm tempted to just remove (or factor out in a new, alpha, module) python_amount and python_rate. So that we can move forward and merge at least all the salary rules and other swiss payroll localisations.

What's your POV on this ?

robinkeunen commented 2 years ago

@OCA/local-switzerland-maintainers can you take a look this ?

github-actions[bot] commented 2 years ago

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.