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

[REF] account: Add test for issue#30972 #31480

Open
wants to merge 1 commit into
base: 12.0
from

Conversation

Projects
None yet
6 participants
@keylor2906
Copy link
Contributor

keylor2906 commented Feb 28, 2019

Description of the issue/feature this PR addresses:
Avoid the creation of a cash basis entry when reversing an invoice journal entry.

Steps to reproduce:
Previous configurations:

  1. Active cash basis on the accounting settings
  2. Configure the Tax Due = Based on Payment on the purchase tax
  3. Configure the tax account as Allow Reconciliation
  4. Generate a new supplier invoice with the same tax configured.
  5. Validate the invoice
  6. Go to the journal entry on the invoice and try to generate a Reverse Entry

Current behavior before PR:
A cash basis journal entry is created when reversing the invoice journal entry.
And an error message You are trying to reconcile some entries that are already reconciled. is returned when trying to generate the reversal.

Desired behavior after PR is merged:
Not create cash basis journal entry when reversing the invoice journal entry. The cash basis entries should be only created on payments. And not getting error message when trying to generate the reversal.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@moylop260

This comment has been minimized.

Copy link
Contributor

moylop260 commented Feb 28, 2019

Unittest for case #30972

@robodoo robodoo added the CI 🤖 label Feb 28, 2019

@moylop260

This comment has been minimized.

Copy link
Contributor

moylop260 commented Feb 28, 2019

@hbto @luistorresm
What do you think?

@hbto

This comment has been minimized.

Copy link
Contributor

hbto commented Feb 28, 2019

@moylop260 @keylor2906

Test must be done for both Supplier Invoices & Customer Invoices,

I can see that the test is done for one of them only and

this piece of code is suspicious

cash_basis = debit_moves and debit_moves[0].account_id.internal_type in ('receivable', 'payable') and **not debit_moves[0].invoice_id** or False

because it is considering only one side for the invoices (debit_moves)

@keylor2906 keylor2906 force-pushed the vauxoo-dev:12.0-add-test-revert-invoice-dev-keylor2906 branch Feb 28, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 28, 2019

@keylor2906

This comment has been minimized.

Copy link
Contributor Author

keylor2906 commented Mar 1, 2019

@hbto Added the test for Customer Invoices.

Regards.

@keylor2906 keylor2906 force-pushed the vauxoo-dev:12.0-add-test-revert-invoice-dev-keylor2906 branch Mar 1, 2019

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

@moylop260

This comment has been minimized.

Copy link
Contributor

moylop260 commented Mar 8, 2019

@JulioSerna
Could you check it, please?

@@ -823,7 +823,7 @@ def _reconcile_lines(self, debit_moves, credit_moves, field):
"""
(debit_moves + credit_moves).read([field])
to_create = []
cash_basis = debit_moves and debit_moves[0].account_id.internal_type in ('receivable', 'payable') or False
cash_basis = debit_moves and debit_moves[0].account_id.internal_type in ('receivable', 'payable') and not debit_moves[0].invoice_id or False

This comment has been minimized.

@hbto

hbto Mar 14, 2019

Contributor

why only the debit_move and not the credit_move?

This comment has been minimized.

@hbto

hbto Mar 14, 2019

Contributor

does it work for both the in_invoice and the out_invoice?

it is quite weird for me.

Regards

This comment has been minimized.

@keylor2906

keylor2906 Mar 15, 2019

Author Contributor

Being honest, I don't really know why the debit_moves and not the credit_moves, I only did the change based on the previous criteria for creating a cash basis move (debit_moves) and works for both cases, in and out invoices.

@hbto

This comment has been minimized.

Copy link
Contributor

hbto commented Mar 14, 2019

Other than that comment I am ok with this PR

[REF] account: Add new test to validate the correct revert of an invo…
…ice account.move when the tax cash basis account has set reconcile True.

[REF] account: Not create cash basis move when reversing an invoice

@keylor2906 keylor2906 force-pushed the vauxoo-dev:12.0-add-test-revert-invoice-dev-keylor2906 branch to 35f2e50 Mar 15, 2019

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

@hbto

This comment has been minimized.

Copy link
Contributor

hbto commented Mar 15, 2019

👍 given the explanation of @keylor2906 and based on the fact that this commit has the regarding unit tests that support it, I agree on this PR.

Regards

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

@JulioSerna

This comment has been minimized.

Copy link
Contributor

JulioSerna commented Mar 15, 2019

LGTM 👍

@moylop260

This comment has been minimized.

Copy link
Contributor

moylop260 commented Mar 15, 2019

@qdp-odoo @smetl
May I ask you to check this PR, Please?

@kebeclibre

This comment has been minimized.

Copy link
Contributor

kebeclibre commented Mar 22, 2019

@moylop260
Check out my PR, I use a context key, which I don't like to do at all, especially in accounting.
But it has the advantage of:

  • being accurate: we really are in a context were no cash basis is necessary since the is no cash flow
  • working with any move, even manual ones

Your fix is smart, but I'm afraid it will work only for invoice moves and not for any random move

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.