search

OCA / hr-expense

Human Resources Expenses OCA modules for Odoo
GNU Affero General Public License v3.0
50 stars 103 forks source link

[16.0][FIX] hr_expense_invoice: taxes propagation #244

Closed edlopen closed 3 months ago

edlopen commented 3 months ago

There is an error in the expense sheet when we have set the taxes. Also if we try to create the expense sheet without taxes and then correct it in the invoice we will get the error that the total of the invoice does not match the total of the expense.

At the moment we create an invoice from the expense sheet the information of the tax fields is deleted, leaving the records incoherent.

@moduon MT-5202

pedrobaeza commented 3 months ago

As already said in the issue, this is not the intention, as you can assure the untaxed amount + taxes computed in the expense module is the same as the one computed in the account one. That's why the field is disabled when you add an invoice. Closing.

pedrobaeza commented 2 months ago

As said, you must put 23 $ and put no taxes by yourself, if you are going to do such workflow. If not, then you should change subtotal + clean taxes when creating the vendor bill. That should be the improvement, and not this one.

rafaelbn commented 2 months ago

Hello @pedrobaeza , @OCA/human-resources-maintainers ,

We hope this message finds you well. We are writing to address the recent closure of our issue and PR (#245 and #244) regarding the hr_expense_invoice module. We respectfully disagree with the decision to close these contributions so quickly, as we believe the problem at hand is significant and warrants further discussion.

Issue Summary

The problem occurs when attempting to create a vendor bill from multiple expenses that have taxes applied. The current behavior leads to validation errors due to inconsistent totals when taxes are involved. We have documented the issue with detailed videos demonstrating the problem:

Concerns About Conduct and Process

We believe that the manner in which our contributions were handled does not align with the respectful and constructive environment encouraged by OCA. Specifically:

  1. Lack of Constructive Feedback: The contributions were closed without detailed feedback or an opportunity for us to discuss potential solutions. According to the OCA Contribution Guidelines, maintainers are expected to provide clear and constructive feedback. Rapid closures without detailed explanations hinder collaboration and the ability to improve contributions.
  2. Rapid Closure: The issues were closed very quickly, suggesting that they may not have been thoroughly reviewed. The guidelines emphasize the importance of collaboration and open discussion to resolve disagreements. A more detailed review process and a dialogue to understand the rationale behind the contributions would be more aligned with OCA’s collaborative ethos.
  3. Encouragement of Open Discussion: OCA promotes open discussion and constructive feedback. By closing the PR and issue so swiftly, the opportunity for a collaborative resolution was missed. This goes against the spirit of OCA’s guidelines which advocate for an open, respectful, and constructive environment.

Request for Reconsideration

We kindly request that this issue be reopened for further review. We are committed to working collaboratively to find a suitable solution and believe that our contributions can help improve the module for all users. To facilitate this, we have opened a new issue #248 to continue the discussion.

Next Steps

To facilitate a constructive discussion, we propose the following:

Additional Points

We understand the importance of maintaining high standards for code contributions. Our intention is to collaborate effectively and ensure that the solutions implemented are robust and beneficial for the wider Odoo community. We believe that by reopening the discussion, we can work together to address the issues and improve the hr_expense_invoice module.

We appreciate your attention to this matter and look forward to working together to resolve this issue.

Thank you for your understanding and cooperation.

Best regards,
Rafael Blasco

pedrobaeza commented 2 months ago

Sorry, you write too much and most of it is noise. You have already opened another issue at #248, so the issue is tracked. The proposed PR was incorrect, so it's correct to be kept closed. If you do a new PR with the correct approach, I will review it ASAP. If not, I will analyze and propose the right one when I'm able to.

yajo commented 2 months ago

you must put 23 $ and put no taxes by yourself, if you are going to do such workflow.

Why should we record the expense without taxes if it has taxes?

rafaelbn commented 2 months ago

Hello @pedrobaeza , @OCA/human-resources-maintainers ,

Please could you reopen this PR so I could test it? @pedrobaeza you closed to fast (18 minutes) that I didn't have time to review it in runboat 😢

Please 🙏🏼 , thank you!

I opened a new issue https://github.com/OCA/hr-expense/issues/248 because you closed the last want in a record time of 2 minutes. But this PRs could solve it and I cannot test it.

PD: I don't think this complaint OCA Code of conduct https://odoo-community.org/oca-code-of-conduct

References:

https://github.com/OCA/hr-expense/issues/245 (Issue closed in 2 minutes - Fastest ever - Does this matter Promotes contributions and really compliance with OCA rules about how to welcome new-contributors) https://github.com/OCA/hr-expense/pull/244 (PR closed in 18 minutes)

pedrobaeza commented 2 months ago

Here you have the complete bugfix: #249

@yajo data in the expense is normalized (including taxes removal) when a vendor bill is created as a prevention measure, as now the detail is in the vendor bill, and there, you can split concepts, maybe the bill may have several taxes, etc, so the way to match is to:

The real problem here changing vendor bill data is that the ORM doesn't recompute all the involved fields, so let's do the check in a later stage.

yajo commented 2 months ago

Thanks for the explanation.

It would have been much more constructive to start giving the explanation before anything else. We would have lost a lot less time, you wouldn't have had the need to write the PR yourself (actually you didn't need it, we would have done it too), and there would have been no friction.

Please keep that in mind for the future, for the sake of having a healthy community where contributors feel welcomed and not scorned.

Let's review #249.

pedrobaeza commented 2 months ago

It was explained in https://github.com/OCA/hr-expense/pull/244#issuecomment-2160970573

pedrobaeza commented 2 months ago

And in https://github.com/OCA/hr-expense/issues/245#issuecomment-2160932286