Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[14.0] [FIX] Fixes in hr_payslip defaults and typo errors, more #27

Merged
merged 1 commit into from
Feb 8, 2022

Conversation

nimarosa
Copy link
Contributor

Hello, here i have some minor fixes that we are dragging from previus versions:

  • Fixed typo in leave calculation
  • Worked days should not compute leaves by default, that is because leaves are computed in a separated work_hour entry, so we need to compute the full worked days and then in another salary rule we can subtract the leaves, otherwise it has no point in computing the leaves separated from the worked time. In many countries the default way is to show in the payslip the full worked hours (normal hours without leaves) and the subtract the corresponding leaves from the total.
    TODO: Also maybe we need to express the leaves as a negative number, i will update this PR with that.
  • I deleted the default starting data, because as i'm the developer of the only one Argentina Localization for payroll, i found out that this default values ends in the user mistaking in the salary rule creating. I think it's better to remove the default values and leave the user (or localization) create it.

I will add more minor fixes to this PR and i hope it can be merged this time... Payroll development for odoo has been abandoned for a long time and i hope we can continue developing this module.

If you need maintainers i want to help. I'm actively developing payroll addons for my country and i have a lot of custom fixes and extra modules we could add to this repo.

Please let me know if you need me to adjust anything in the PR for this to be merged.

@nimarosa nimarosa changed the title [FIX] Fixes in hr_payslip defaults and typo errors, more [14.0] [FIX] Fixes in hr_payslip defaults and typo errors, more Jan 25, 2022
@pedrobaeza
Copy link
Member

Put them as demo data?

@nimarosa
Copy link
Contributor Author

@pedrobaeza They are already in demo data, i removed them from production data because like i said, i dont see the point of populating a production database. Most of my users get confused and it really don't help because every country has they own rules for payroll.

I can add them as demo data if you want, but there is a lot of demo data already loaded in demo.

@pedrobaeza
Copy link
Member

It's OK for me then. Please check pre-commit.

cc @AntoniRomera @apps2grow

@nimarosa
Copy link
Contributor Author

@pedrobaeza pre-commit ready.

@nimarosa
Copy link
Contributor Author

Okay so i added this to the demo data because the data is required for tests.

@ghost
Copy link

ghost commented Jan 28, 2022

Thank you @nimarosa for these improvements. I confirm that the demo data is installing correctly on demo, and not installed when not using demo. This change might require a migration script for existing instances, so it is good that the default branch is still 13.0.

Feel free to add yourself as a maintainer. Just add a new commit to this PR.

I am not really into testing, but in general I think it is good to write tests.

@nimarosa
Copy link
Contributor Author

@appstogrow mantainer key added

@nimarosa
Copy link
Contributor Author

@appstogrow Yes i don't write tests either. Maybe when we do more improvements we can write some. For now i think this is ready to merge if you want.

@ghost
Copy link

ghost commented Jan 30, 2022

@nimarosa In the OCA, one PR cannot have changes in more than one module. So in this PR you can only add yourself as a maintainer for the payroll module.

@ghost
Copy link

ghost commented Jan 30, 2022

Could you also try to squash the commits together into one commit?

@nimarosa
Copy link
Contributor Author

@appstogrow i've removed the mantainer key in payroll_account (will add later in another commit to this module). And also do the squash. Please tell me if it's okay because i don't often rebase branches so i might be wrong in the process.

Hope everything it's okay to merge now.

@ghost
Copy link

ghost commented Jan 31, 2022

@nimarosa Yes I can see that the maintainer key is fixed. 👍
I see 10 commits now, I expected to see 1. Here is an article about squashing.. Then git push <your_repo> <your_branch_name> --force

author nicolasrsande <nicolasrsande@gmail.com> 1643135061 -0300
committer nicolasrsande <nicolasrsande@gmail.com> 1643668459 -0300

fix typo in leave calculation

default for worked days should not compute leaves, because leaves are calculated separately

we should not add data because it interfers with custom localization payroll modules

fix pre-commit

add demo data so the test can be executed

leaves should be computed in negative value to help creating salary rules

Add mantainer key

remove mantainer in payroll_account

14.0-minor-fixes

fix typo in leave calculation

we should not add data because it interfers with custom localization payroll modules

fix pre-commit

add demo data so the test can be executed

leaves should be computed in negative value to help creating salary rules

Add mantainer key

remove mantainer in payroll_account

14.0-payroll-minor-fixes
@nimarosa
Copy link
Contributor Author

@appstogrow I think it's ready now. I see only one commit.

@ghost
Copy link

ghost commented Feb 1, 2022

👍

@nimarosa
Copy link
Contributor Author

nimarosa commented Feb 3, 2022

When can we merge this?

@ghost
Copy link

ghost commented Feb 8, 2022

@pedrobaeza Would you like to merge this PR?

@pedrobaeza pedrobaeza added this to the 14.0 milestone Feb 8, 2022
@pedrobaeza
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-27-by-pedrobaeza-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5a9a27b into OCA:14.0 Feb 8, 2022
@OCA-git-bot
Copy link
Contributor

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

@nimarosa nimarosa deleted the 14.0-minor-fixes branch June 2, 2022 23:56
mtelahun added a commit to trevi-software/payroll that referenced this pull request Jun 17, 2022
mtelahun added a commit to trevi-software/payroll that referenced this pull request Jun 17, 2022
mtelahun added a commit to trevi-software/payroll that referenced this pull request Jun 17, 2022
mtelahun added a commit to trevi-software/payroll that referenced this pull request Jun 17, 2022
mtelahun added a commit to trevi-software/payroll that referenced this pull request Aug 12, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants