[ADD] grap_pos_change_sale_move: Aggregate policy

[remytms]

Add a new parameter on res.company to chose how move.line are
aggregated when closing a pos.session.

Three options can be selected:

1. "Standard", it means that nothing change to the default Odoo behaviour.
2. "Group by account and tax", it means that:
   • product move.line are aggregated by account and by taxes.
   • tax move.line are aggregated by tax.
   • only one counter part move.line are written for the whole pos.session
3. "Group by account, tax and partner", it means that:
   • product move.line are aggregated by account, by taxes and by partner.
   • tax move.line are aggregated by tax and by partner.
   • counter part move.line are aggregated by partner.

This is a first attempt. Readme should be changed later when these
changes will be validated.

Coop IT Easy linked task

[remytms]

I see now that you have split the module grap_pos_change_sale_move into two module. I will change this PR accordingly. But the idea will be the same. So comments are welcome.

[legalsylvain]

Hi @remytms thanks for this work !

• In the first sight, LGTM. If I understand correctly, your improvement doesn't change the current behaviour, as the default setting is account_tax.
• regarding the place of the setting we have 3 options : by pos.config, by res.company or by account.journal : In the Odoo Spirit, settings are on pos.config generally, but I can understand that it's a mess to configure many pos.config and that makes no sense to have some pos.config with one setting and others with other setting. Maybe the setting on the sale account.journal could be more in the odoo spirit. (it's a configuration of accountant). What do you think ? (not a blocking point).
• Do you think this module is so generic to be shared in the OCA/pos repo ? if yes, I think we should rename it to pos_change_sale_move. Don't you think ?

Just a question :

• I don't why you changed some vat excl vat incl settings on the demo data.

[remytms]

Hi @legalsylvain ,

Thanks for this quick review !

• In the first sight, LGTM. If I understand correctly, your improvement doesn't change the current behaviour, as the default setting is account_tax.

Yes, the goal was to add functionality but not modify the default behaviour of this module.

• regarding the place of the setting we have 3 options : by pos.config, by res.company or by account.journal : In the Odoo Spirit, settings are on pos.config generally, but I can understand that it's a mess to configure many pos.config and that makes no sense to have some pos.config with one setting and others with other setting. Maybe the setting on the sale account.journal could be more in the odoo spirit. (it's a configuration of accountant). What do you think ? (not a blocking point).

I agree that this config is lonely on the res.company. My point of view is that putting this config on pos.config will allow user to chose some PoS that should aggregate, and other not, but I don't know if such a case make sens in real live. But it seams that there is already some accounting configuration on the pos.config (https://github.com/odoo/odoo/blob/12.0/addons/point_of_sale/models/pos_config.py#L133).

• Do you think this module is so generic to be shared in the OCA/pos repo ? if yes, I think we should rename it to pos_change_sale_move. Don't you think ?

I think it can be good like this. But to be more generic, I think one option should be to have the default Odoo behaviour. Sort of bypass for this module, especially if the configuration is put on the pos.config.

• I don't why you changed some vat excl vat incl settings on the demo data.

I have to check that the aggregation works fine when there is multiple taxes applied on the same product. I didn't achieve to have the good amount on the tax move.line, because it seams that Odoo tries to compute the vat_excl price based on the vat_incl price. But that give wrong amount of tax.

Ex: Product with 5% and 20% VAT : 10€ (VAT excl) -> 12,5€ (VAT incl) But when odoo tries to get the vat_excl price from the vat_incl, it seams to compute like this: VAT 5% -> 12,5 / 1,05 and VAT 20% -> 12,5 / 1,20. But that's not correct.

So I moved to tax based on the vat_excl price. So that all price are given VAT excl, and things are computed well.

[legalsylvain]

Ex: Product with 5% and 20% VAT : 10€ (VAT excl) -> 12,5€ (VAT incl) But when odoo tries to get the vat_excl price from the vat_incl, it seams to compute like this: VAT 5% -> 12,5 / 1,05 and VAT 20% -> 12,5 / 1,20. But that's not correct.

I don't think so !

10€ VAT excl = 12 VAT incl. (10 * 1.2)

[remytms]

I agree with your example, but it states that there is only one tax that is applied to the product. I'm talking about a product with two different taxes on the same product. I created such a product in the test data: (https://github.com/grap/grap-odoo-custom-account/pull/25/files#diff-6960beb0255a39910bdea429eb48d6c7f2f7cebbc3d83b24e43e60e4f9616d16R46). So the 5% tax and the 20% tax apply on the base price. 12,5€ (incl taxes) = 10 € + (10 € * 0,05) + (10 € * 0,20).

Do you get my point ?

I edited the computation of the price incl taxes

[codecov]

Codecov Report

Merging #25 (0e21100) into 12.0 (29e6557) will decrease coverage by 6.55%.
The diff coverage is 97.77%.

❗ Current head 0e21100 differs from pull request most recent head bd6dc59. Consider uploading reports for the commit bd6dc59 to get more accurate results

@@            Coverage Diff             @@
##             12.0      #25      +/-   ##
==========================================
- Coverage   84.10%   77.54%   -6.56%     
==========================================
  Files          55       27      -28     
  Lines        1082      726     -356     
  Branches      175        0     -175     
==========================================
- Hits          910      563     -347     
- Misses        139      163      +24     
+ Partials       33        0      -33     

Impacted Files	Coverage Δ
grap_pos_change_sale_move/models/pos_order.py	95.38% <97.56%> (+10.09%)	⬆️
grap_pos_change_sale_move/models/pos_config.py	100.00% <100.00%> (ø)
grap_account_export_ebp/models/res_company.py	70.00% <0.00%> (-10.00%)	⬇️
grap_account_export_ebp/models/account_move.py	81.25% <0.00%> (-4.47%)	⬇️
grap_pos_change_sale_move/models/__init__.py
...rap_pos_change_sale_move_test/tests/test_module.py
...payment_move/tests/test_pos_change_payment_move.py
...code/wizards/wizard_res_partner_add_export_code.py
grap_pos_change_payment_move/models/__init__.py
grap_account_export_partner_code/tests/__init__.py
... and 38 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[remytms]

@legalsylvain I've made some little changes on this module (commits seams explicit). I also upgraded the README and added a migration file.

Module will be fully tested by accountant at BEES coop. I will let you know what's the results of theses tests.

[legalsylvain]

Hi @remytms. what is the result of your test ?
thanks !

[carmenbianca]

This PR has been in production for us for a while. It recently disappeared from our test instance due to merge conflicts. I've fixed the conflicts and force-pushed.

@remytms Please take a look at this PR when you're back from holidays :)

[remytms]

rebased on top of the 12.0 branch.

[remytms]

@legalsylvain As @carmenbianca suggested this PR has been in production for a while and accountant seams to be happy with it.

[legalsylvain]

Hi @remytms : Is it OK if I let your PR open ? And when I migrate in V16, I integrate your work ?
It saves me review / test time, and you can continue to work on your branch.

[remytms]

Yes, it's OK. You can ping me to review the migration if you want.