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

[florian-dacosta]

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]

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

[victoralmau]

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]

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]

With the approach indicated in the use case at #779 this does not happen.

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]

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]

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]

Looks good to me

[victoralmau]

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.

[florian-dacosta]

@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]

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...

[florian-dacosta]

@victoralmau
I have reworked the PR.
Like I said before, I think is not reasonable to remove the _update_move_partner method because if we does, the partner_id of the statement line stays empty during all the reconciliation process (because the write is done only when leaving or pressing a button). It means that all the onchange played after setting the partner (in manual_partner_id field) that check the field partner_id will find an empty partner record while it has been set right before.

Since partner_id is the only field triggering a synchronization toward the related accounting entries that could change in the reconcile widget need to manage this case.
In this version, we continue to write the partner_id during the onchange, but we make sure to only update the partner on the relative accounting entries (and not the amounts).

Also, I kept the logic that reset the reconcile_data_info in case an amount is changed. (because of a write made outside of the reconciliation widget, like from the LIST view for example.
I also did improve a bit an existing tests to avoid regression about the use cases described in #779
This solve the different tests cases I made.

What do you think ?

[victoralmau]

Good approach, solves the initial problem, if something else is found later that needs to be fixed it will be analyzed then, but in the meantime, this is correct IMO.

[pedrobaeza]

/ocabot merge patch

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-784-by-pedrobaeza-bump-patch, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at cbb9a7f. Thanks a lot for contributing to OCA. ❤️