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

[FIX] account: revert move subjected to cash basis #32023

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@kebeclibre
Copy link
Contributor

kebeclibre commented Mar 21, 2019

Before this commit, when reverting a move
that will be subjected to the creation of a cash basis move
the process failed saying that some entries were already reconciled

That was because the process tried to create the cash basis move during
the revert.
This is wrong, because no real cash is dealt with.

After this fix, the move lines of the original entry are reconciled
only with their revert counterpart

OPW 1938809
closes #30972

@robodoo robodoo added the seen 🙂 label Mar 21, 2019

@C3POdoo C3POdoo added the RD label Mar 21, 2019

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-reverse-caba-lpe branch from 8c515a0 to 1e6dc83 Mar 22, 2019

@robodoo robodoo added the CI 🤖 label Mar 22, 2019

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-reverse-caba-lpe branch from 1e6dc83 to 8b13738 Mar 22, 2019

@robodoo robodoo removed the CI 🤖 label Mar 22, 2019

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-reverse-caba-lpe branch from 8b13738 to e8598e9 Mar 22, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Mar 22, 2019

other solution at #31480
which checks the case by checking if there are invoice on the move we revert
except that this way, manual moves will probably fail to be reverted

@kebeclibre kebeclibre changed the title 12.0 reverse caba lpe [FIX] account: revert move subjected to cash basis Mar 22, 2019

@kebeclibre kebeclibre requested a review from qdp-odoo Mar 22, 2019

@robodoo robodoo added the CI 🤖 label Mar 22, 2019

@moylop260
Copy link
Contributor

moylop260 left a comment

👍
You have an extra case covered.
Thanks!

closes #31168

@robodoo robodoo removed the CI 🤖 label Mar 25, 2019

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-reverse-caba-lpe branch 4 times, most recently from aad74fd to 3db257c Mar 25, 2019

@kebeclibre kebeclibre requested a review from smetl Mar 25, 2019

@robodoo robodoo added the CI 🤖 label Mar 25, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Mar 25, 2019

@smetl
@qdp-odoo
Commits 0 and 1 go together and are different implementations
Commits 2 is just about tests

Commit 3 is much more intresting
I am pretty sure we don't need to make an exchange diff entry for cash basis reconciliation of taxes lines
Am I right ?

@@ -1601,6 +1603,8 @@ def create_tax_cash_basis_entry(self, percentage_before_rec):
if line.account_id.reconcile:
#setting the account to allow reconciliation will help to fix rounding errors
to_clear_aml |= line
if percentage_after < 1.0:
to_clear_aml = to_clear_aml.with_context(skip_full_reconcile_check=True)

This comment has been minimized.

Copy link
@hbto

hbto Mar 26, 2019

Contributor

@kebeclibre

I am highly concerned with the wired 1.0 percent value, can this be linked to something like units of the currency rounding?

Regards

This comment has been minimized.

Copy link
@kebeclibre

kebeclibre Mar 27, 2019

Author Contributor

@hbto
percentage is a plain float, without unit.
It represents the proportion of a move being reconcilied
So logically, when the proportion is not one we are not supposed even to to check whether the reconciliation will be full.
However, I definitely admit I don't like the fix :D

This comment has been minimized.

Copy link
@hbto

hbto Mar 28, 2019

Contributor

@kebeclibre When I refer to units of rounding Currency I mean,

X times the value of the rounding of the currency of company.

regards

@moylop260

This comment has been minimized.

Copy link
Contributor

moylop260 commented Mar 27, 2019

@smetl @qdp-odoo
Your opinion is welcome here

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 27, 2019

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-reverse-caba-lpe branch from 865c355 to 202a695 Mar 27, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 27, 2019

moylop260 added a commit to vauxoo-dev/odoo that referenced this pull request Mar 27, 2019

@moylop260

This comment has been minimized.

Copy link
Contributor

moylop260 commented Apr 8, 2019

@nim-odoo

This PR look fine for us
Could we merge it?

kebeclibre added some commits Mar 21, 2019

[FIX] account: revert move subjected to cash basis
Before this commit, when reverting a move
that will be subjected to the creation of a cash basis move
the process failed saying that some entries were already reconciled

That was because the process tried to create the cash basis move during
the revert.
This is  wrong, because no real cash is dealt with.

After this fix, the move lines of the original entry are reconciled
only with their revert counterpart

OPW 1938809
closes #30972
[FIX] account: tests reconciliation helper class
Before this commit, test reconciliation was inherited by
test reconciliation widget.
It made the actual test_xx functions of the former
being executed twice

After this commit, we create an intermediary test class for setUp and helpers
And the tests for each class execute only once
[FIX] account: cash basis reconciliation almost all amount
In multicurrency

Do an invoice with cash basis lines in the foreign currency
Make a payment for almost all the invoice
"almost" refers to the point where virtually all taxes will be paid

i.e., paying 90 over 100, that contains a tax of 5%
The tax paid will be an amount almost equal (to a few cents) to the
total amount of the tax
It is not negligible, but if the currency rates are in the right configuration
(i.e. 0.005888)

Before this commit: the Cash Basis reconciliation will be considered as full and will trigger
the creation of the Exchange Diff Entry.
In turn, paying the rest of the invoice will pop up an error saying that some entries are already reconciled
(with the Exchange Diff entry)
It is not *that* that an exchange rate entry has been created the first time
after all, a percentage sufficiently close to 100 has been paid
But it blocks subsequent reconciliation, hence this fix

After this commit, if the cash basis entry doesn't match at least 100% of the paid move
then, we force to not check for full reconciliation

OPW 1953027
closes #31168

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-reverse-caba-lpe branch from 202a695 to 787d8f1 Apr 10, 2019

@robodoo robodoo removed the CI 🤖 label Apr 10, 2019

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Apr 10, 2019

robodoo rebase-ff

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

kebeclibre commented Apr 10, 2019

robodoo r+

@robodoo robodoo added the r+ 👌 label Apr 10, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Apr 10, 2019

Merge method set to rebase and fast-forward

@robodoo robodoo added the CI 🤖 label Apr 10, 2019

@robodoo robodoo closed this in c43acc3 Apr 10, 2019

@robodoo robodoo added merged 🎉 and removed merging 👷 labels Apr 10, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Apr 10, 2019

Merged, thanks!

@KangOl KangOl deleted the odoo-dev:12.0-reverse-caba-lpe branch Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.