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.

Sign up for GitHub

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

Jump to bottom

[16][REF] account_ecotax split #454

Open
wants to merge 16 commits into
base: 16.0
Choose a base branch
from
Open

[16][REF] account_ecotax split #454

wants to merge 16 commits into from

Conversation

florian-dacosta
Copy link

This PR aims to replace this one : #428 as explained here : #453

Going from the module here : #428
to account_ecotax_tax of this PR does not require migration script, just the installation of the new account_ecotax_tax module should be enough.

mourad-ehm and others added 13 commits October 29, 2024 15:10
This was referenced Nov 18, 2024
Open
Closed
@florian-dacosta florian-dacosta force-pushed the 16-account-ecotax-split branch 2 times, most recently from 6af346d to e68d793 Compare November 25, 2024 09:10
@florian-dacosta florian-dacosta mentioned this pull request Nov 25, 2024
Open
@florian-dacosta
Copy link
Author

I think the PR is ready, reviews are very welcome @gurneyalex @mourad-ehm @hparfr @bealdav @yankinmax @epanisset @PaulGoubert

I've added basic override of the invoice report in account_ecotax to display the ecotax at line level and invoice level.
I've done the same in account_ecotax_tax, but the result is a bit worse because as the ecotax is a tax, it is harder to override this part.
This part can be improved by whoever needs it I guess.

gurneyalex
gurneyalex approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@gurneyalex gurneyalex left a comment

Choose a reason for hiding this comment

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

Looks good, I'll make some manual tests

account_ecotax/models/ecotax_line_mixin.py Outdated Show resolved Hide resolved
account_ecotax/models/ecotax_line_product.py Outdated Show resolved Hide resolved
account_ecotax/readme/DESCRIPTION.rst Outdated Show resolved Hide resolved
account_ecotax_tax/readme/DESCRIPTION.rst Outdated Show resolved Hide resolved
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@florian-dacosta
[16][REF] split account_ecotax into account_ecotax and account_ecotax…
e2bb4b7 
…_tax

The goal is to be able to choose between the implementation with and without using the odoo tax mechanism.
The advantages of the implementation based on Odoo tax mechanims are :
- Possibility to choose if product price include or exclude the ecotax amounts
- Isolate the ecotax amounts into a specifc accounting account
The disadvantage is that it adds a small layer of complexity and you have to manage the tax configuration and see all those ecotax taxes on your invoices
And a major difference which can be good or not depending on your use cases, the ecotax amounts are not in the turnover when using Odoo tax mechanism
@florian-dacosta
[IMP] account_ecotax : remove dependency on AccountTestInvoicingCommo…
bf2dd62 
…n from tests

The dependency does not really ease the present test but forces us to put the test as post-installed which cant work as account_ecotax_tax changes the account_ecotax behavior
Instead of isolating the tests of account_ecotax, it seems better to get rid of the AccountTestInvoicingCommon dependency. It also speed up the tests.
@florian-dacosta florian-dacosta force-pushed the 16-account-ecotax-split branch from 0659a4f to 721647d Compare December 5, 2024 11:59
Abraao-Levasseur
Abraao-Levasseur reviewed Dec 11, 2024
View reviewed changes
Copy link

@Abraao-Levasseur Abraao-Levasseur left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@gurneyalex The module works fine with 3 main settings :

  • The tax created with the field eco tax ticked ,ranked upper the others and included into the price.
  • The eco tax classification into accounting / configuration / eco tax classification
  • The eco tax classification set into the product into the accounting tab

However, i suggest to remove modules account_ecotax_tax & account_ecotax_sale_tax to keep the untaxed amount not deduced of the ecotax amount :
If not, the tax amount is not so clear to understand.

=> with model account_ecotax_tax & account_ecotax_sale_tax OCA :
image

=> without model account_ecotax_tax & account_ecotax_sale_tax : ( i unistalled the modules to display like below)
image

I suggest also to add ecotax amount into the invoice template into line an total level :
image

@Abraao-Levasseur Abraao-Levasseur mentioned this pull request Dec 11, 2024
Open
@florian-dacosta
Copy link
Author

@Abraao-Levasseur
The invoice pdf report already include the ecotax amounts (I just fixed a small mistake so it is display in the invoice totals)
Just tested on runbot, with only account_ecotax installed :
image

With account_ecotax_tax, it is different, since ecotax is considered as a tax, you should create a dedicated tax group named "ecotax" or something and it will be displayed along with the vat
I think the report for account_ecotax_tax could be improved in the future... But outside the scope of this PR (I don't plan to do it at least)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Abraao-Levasseur Abraao-Levasseur Abraao-Levasseur left review comments

@gurneyalex gurneyalex gurneyalex approved these changes

@mourad-ehm mourad-ehm mourad-ehm approved these changes

Assignees
No one assigned
Labels
approved ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@florian-dacosta @OCA-git-bot @gurneyalex @mourad-ehm @Abraao-Levasseur @bealdav @rvalyi