-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
base: 16.0
Are you sure you want to change the base?
Conversation
Hi @alexis-via, @etobella, |
ad5f240
to
137b4f3
Compare
What do you think of this other approach Tecnativa@91022a0 ? |
This other approach seems interesting for the 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 |
With the approach indicated in the use case at #779 this does not happen. It is important to note that it happened (line data was changed) because the “onchange” was done and |
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.
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. |
Ok, I have updated the commit Tecnativa@91022a0 to allow what you comment and to allow |
Looks good to me |
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. |
…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
137b4f3
to
7755101
Compare
@victoralmau
What happens is that the I'll keep this PR as draft for now and will need to do more tests, but all reviews are welcome. |
Anyway, I am afraid this could lead to regression as during the whole process of reconciliation, the |
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