Skip to content

Conversation

@edescalona
Copy link

@edescalona edescalona commented Jul 29, 2025

@BinhexTeam

Reasons why the PR was created:

  1. There is a [MIG 17.0] payroll_account #189 for this module, but it has not been continued.
  2. The module is merged into the 17.0 branch, but its content does not indicate that it is a valid module, if there is any reason for this, please let me know.
  3. Backport 18.0

Hi @dreispt @nimarosa , if you could review the PR or clarify the issue with the merged module in case there's a reason I'm not aware of, thank you.

@edescalona edescalona force-pushed the 17.0-mig-payroll_account branch from d410263 to 7ed7f43 Compare September 17, 2025 20:40
@edescalona edescalona marked this pull request as ready for review September 20, 2025 04:05
Saran440 and others added 28 commits September 19, 2025 23:15
Currently translated at 100.0% (24 of 24 strings)

Translation: payroll-14.0/payroll-14.0-payroll_account
Translate-URL: https://translation.odoo-community.org/projects/payroll-14-0/payroll-14-0-payroll_account/ca/
Currently translated at 16.6% (4 of 24 strings)

Translation: payroll-14.0/payroll-14.0-payroll_account
Translate-URL: https://translation.odoo-community.org/projects/payroll-14-0/payroll-14-0-payroll_account/es_AR/
Currently translated at 100.0% (24 of 24 strings)

Translation: payroll-14.0/payroll-14.0-payroll_account
Translate-URL: https://translation.odoo-community.org/projects/payroll-14-0/payroll-14-0-payroll_account/es_AR/
Currently translated at 100.0% (24 of 24 strings)

Translation: payroll-14.0/payroll-14.0-payroll_account
Translate-URL: https://translation.odoo-community.org/projects/payroll-14-0/payroll-14-0-payroll_account/es/
…or the payslip

Until now this module tried to confirm an empty account move. This caused
the account module to throw an exception. This causes problems in
other modules that may not know about payroll_account. For example
in other modules' tests.
Currently translated at 100.0% (24 of 24 strings)

Translation: payroll-14.0/payroll-14.0-payroll_account
Translate-URL: https://translation.odoo-community.org/projects/payroll-14-0/payroll-14-0-payroll_account/es_AR/
Currently translated at 100.0% (24 of 24 strings)

Translation: payroll-14.0/payroll-14.0-payroll_account
Translate-URL: https://translation.odoo-community.org/projects/payroll-14-0/payroll-14-0-payroll_account/es/
…cumentation

[14.0] [IMP] payroll: improve salary rules views and documentation

[14.0] [IMP] payroll: improve salary rules  and categories views

[14.0] [IMP] payroll: improve salary rules views and documentation

[14.0] [IMP] payroll: improve salary rules views and documentation

[IMP] payroll: fix repeated words

[14.0] [IMP] payroll: fix typos
[14.0] [IMP] payroll: change manifest category

[14.0] [IMP] payroll: add migration for new payslip and payslips objects

[14.0] [IMP] payroll: add migration for new payslip and payslips objects

[14.0] [IMP] payroll: add migration for new payslip and payslips objects
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: payroll-16.0/payroll-16.0-payroll_account
Translate-URL: https://translation.odoo-community.org/projects/payroll-16-0/payroll-16-0-payroll_account/
Currently translated at 100.0% (21 of 21 strings)

Translation: payroll-16.0/payroll-16.0-payroll_account
Translate-URL: https://translation.odoo-community.org/projects/payroll-16-0/payroll-16-0-payroll_account/fa/
@edescalona edescalona force-pushed the 17.0-mig-payroll_account branch from 7ed7f43 to 45cfdab Compare September 20, 2025 04:21
Copy link

@rrebollo rrebollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. This module contains deep functional knowledge, so it’s better to wait for more highly ranked reviews, but I can already see good work here. Feel free to address my comment on migrations scripts.

[IMP] payroll_account: Remove folder migrations
@edescalona edescalona force-pushed the 17.0-mig-payroll_account branch from 45cfdab to 146e01e Compare September 21, 2025 14:58
@dreispt
Copy link
Member

dreispt commented Sep 22, 2025

@edescalona #189 was not a viable PR.
I worked on the 18.0 migration, where relevant cleanup was done.
I suggest trying the backport of the 18.0 module to 17.0.

@dreispt
Copy link
Member

dreispt commented Sep 22, 2025

PS: the 18.0 migration was based on 16.0 of course, so you might want to look at the additional commits done for 18.0 and consider cherry-picking them.

@edescalona
Copy link
Author

Hi @dreispt , if I migrated from 18.0, all that's missing is a review of this PR, both functional and code-wise. Thanks for your feedback.

Copy link
Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nimarosa
Copy link
Contributor

Just reviewed it via runboat. It looks like it works okay.
Will merge so we have the module in 17 and then we can make fixes if problems arrises, but look ok for now. Pending testing production.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-205-by-nimarosa-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8ae0228 into OCA:17.0 Sep 22, 2025
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.