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

[16][account_reconcile_oca] FIX amounts inconsistencies between statement line reconcile_data and accounting entries #784

Draft
wants to merge 2 commits into
base: 16.0
Choose a base branch
from

Conversation

florian-dacosta
Copy link
Contributor

Tries to fix #779 (which is only one case, but other exists, like simply updating a statement line amount after a wrong creation)
Also add a test to show the issue (in current implementation)

Instead of skipping the synchronisation with the accounting entries, which seems like a really dangerous approach, we try to detect if there is inconsistencies in amount (should NOT happen most of the time), and only if there are, then we get rid of the reconcile_data info.

Far from perfect for the user that may discover the reset of what he has reconcile only when he tries to validate. (Because the write is done only when clickng on a button or another line.
Also, it clean the reconcile data kind of silently, which is not ideal either.
That is only a first proposal, to be improved...

Any comment/idea @etobella @victoralmau @pedrobaeza

@OCA-git-bot
Copy link
Contributor

Hi @alexis-via, @etobella,
some modules you are maintaining are being modified, check this out!

@florian-dacosta florian-dacosta force-pushed the 16-fix-reconcile-data-consistency-synchronizing branch from ad5f240 to 137b4f3 Compare January 20, 2025 17:29
@victoralmau
Copy link
Member

victoralmau commented Jan 21, 2025

What do you think of this other approach Tecnativa@91022a0 ?
IMO you should not synchronize the moves, this happens by modifying the partner_id field so it should be avoided.

@florian-dacosta
Copy link
Contributor Author

What do you think of this other approach Tecnativa@998286e ?

This other approach seems interesting for the manual_partner_id use case (I did not test it yet).
But I am afraid similar issues to what is described here : #779
could happen with other fields, because all fields of the statement line could be modified.
You would have to manage at least payment_ref and partner_id as well for instance (I guess skip sync, which I don't like because it is hard to anticipate all the consequences and then manage it in the _synchronize_to_moves override.

I did the current proposal in order to manage all cases.

You would also have to manage the case of the fields that have a direct impact on the amounts and clean/reset the reconcile_data after write on currency_id, amount_currency, foreign_currency_id, amount
Else you will still have case with inconsistencies in reconcile_data.

@victoralmau
Copy link
Member

With the approach indicated in the use case at #779 this does not happen.
ejemplo
Can you check it too? (or if there is anything else that should be taken into account)

It is important to note that it happened (line data was changed) because the “onchange” was done and partner_id was set (in the reconciliation kanban view). If you modify any other field (payment_ref, partner_id or amount) in the tree view the move (and lines) will be re-synchronized (something that IMO is correct).

@florian-dacosta
Copy link
Contributor Author

The exact use case described in #779 does not happen with your fix.

What I am saying is that this approach is not complete because I can easily reproduce a similar bug, another way.
Example :

  1. Create statement line in our EUR bank journal (while company currency is USD)
  2. Click on the liquidity line in the reconciliation widget (so that Odoo save data in reconcile_data field)
  3. Change the currency rate
  4. change the partner_id from the bank statement line LIST view
  5. Inconsistencies between accounting entries and reconcile_data info.
    Capture vidéo du 2025-01-21 09-35-40.webm

Same kind of issues will happen with other change in fields. The issue, IMO is that the reconcile_data is not recomputed after such changes.

@victoralmau
Copy link
Member

Ok, I have updated the commit Tecnativa@91022a0 to allow what you comment and to allow reconcile_data_info to be synchronized when needed.

@florian-dacosta
Copy link
Contributor Author

Looks good to me

@victoralmau
Copy link
Member

Ok, feel free to apply those changes (or as many as necessary) in this PR when you check that everything works correctly and there are no side effects.

victoralmau and others added 2 commits January 21, 2025 14:32
…er_id without onchange or subsequent changes with the _synchronize_to_moves() method.

We do not define partner_id with the value of manual_partner_id to prevent _synchronize_to_moves()
from making changes to the account.move.line leaving unintended values and/or data.

Re-define the value of reconcile_data_info if the _synchronize_to_moves() method has changed
anything on the lines.

Related to OCA#779

TT52634
We need to look at the reconcile_data to find the partner as the manual_partner_id can contain the information of an auxiliary line
@florian-dacosta florian-dacosta force-pushed the 16-fix-reconcile-data-consistency-synchronizing branch from 137b4f3 to 7755101 Compare January 21, 2025 14:26
@florian-dacosta
Copy link
Contributor Author

florian-dacosta commented Jan 21, 2025

@victoralmau
I still had one issue : detecting the write on manual_partner_id is not reliable.
Indeed, in the following case, it does not work :

  1. In reconciliation widget, create Statement line
  2. Click on liquidity line and set the partner in the manual operation tab
  3. click on the suspense line and fill the account (in manual operation tab)
  4. Validate
    The partner is not set on the accounting line.

What happens is that the manual_partner_id is empty when written on the statement line. I guess because it takes the value of the suspense line form, which is empty...
I have updated your proposal to base this logic on the partner_id of the liquidity line of the reconcile_data...

I'll keep this PR as draft for now and will need to do more tests, but all reviews are welcome.

@florian-dacosta
Copy link
Contributor Author

Anyway, I am afraid this could lead to regression as during the whole process of reconciliation, the partner_id of the account.bank.statement.line could be wrong if you changed it (because there is not more write on the account.bank.statement.line on this field) and I saw that sometimes it is checked in the code (like self.partner_id where self is a statement_line) the fact that we do not write the partner_id make this kind of check unreliable...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants