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.0][IMP] account_reconcile_oca: Improve multicurrency management #694

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

Conversation

etobella
Copy link
Member

@etobella etobella commented Sep 13, 2024

Tries to fix #692 #693
@florian-dacosta Can you check it please?

@florian-dacosta
Copy link
Contributor

@etobella Thanks for the PR!
Well, it fix the example I gave in the issue yes, thanks, but it does not fix all cases, like the one in the second issue.
Let me show you what I have with the same setup from #693

Default screen when going to reconcile UI, we have a rounding issue
1-bfore-reconcile

After entering the 121000 account in manual tab :
2-accounting-entry

@etobella
Copy link
Member Author

@florian-dacosta Round 2
image
I added a rounding factor in order to remove this possible error (the rounding should only be used on comparison)

@florian-dacosta
Copy link
Contributor

florian-dacosta commented Sep 13, 2024

@etobella I still don't get why there is a rounding to find an amount that is written in the amount field of the account.bank.statement.line. Why the 259 200 needs to be computed and not read for the line ?

Anyway, I've tested the later version
I still get this rounding issue before chosing the invoice.
And once validated, the state /payment on the invoice is not ok.

Go to reconcile UI - Not OK
1-rec-ui

Choose the invoice - OK
2-after-choosing-invoice

Reconciled invoice after validation - Not OK
3-invoice-partial

The bank accounting entry is missing the currency on the 121000 line
4-accounting-entry

@etobella
Copy link
Member Author

We have a rounding issue because the reconcile is done on the currency of the company. Also we cannot use that amount, because we might have previous lines.

The only option I can see is to do the reconcile in the amount of the statement line.

This is more work to do and I just wanted to add a patch, but seems we need to do a real refactoring (it is good to have so many tests)

@PaulGoubert
Copy link

Thank you for your help with these issues, @etobella.
As previously mentioned by @florian-dacosta, we're in a rush on this topic due to our customer's situation—they've been expecting to use it properly for 6 months now. I hope you can tackle these topics and help us resolve the issues.

@etobella
Copy link
Member Author

@PaulGoubert @florian-dacosta Round 3

I have changed the flow in order to compare using the currency of the line. It has no sense to move from EUR to USD to EUR, so it should remove some of this errors.

@etobella
Copy link
Member Author

The only issue I found is that it is not showing properly when reconciling with the Foraign Exchange Loss

Before Reconciling
image

After reconciling
image

The foreign loss has been lost, but it is correctly reconciled
image

@etobella
Copy link
Member Author

I applied a fix for this. I am not sure if it gets all the possible cases, but at least it is an approach

@florian-dacosta
Copy link
Contributor

florian-dacosta commented Sep 15, 2024

@etobella Thanks for your work, it is really helpull !
About the rounding case, with last version of the code, here is what I have :

  1. First click on the reconcile button on journal
    1-start-reconciliation

  2. It give me the reconciliation screen with the rounding error in the suspense account :
    2-before-begining-reconcile-ui

If I choose the counter part, all is fine, then when I reconcile, as you saif, all is fine, the invoice is reconciled correctly, the accounting entries are correct.

  1. Right after, the view of the statement line in the reconciliation UI was correct. I am not sure how it work, but with a new tab/after restarting the server and going back on the statement line, I have really strange data :
    3-after-reconciled-view

For now the most annoying for me is the display in 1. Even if all goes fine after, it is very confusing for the user.

I applied a fix for this. I am not sure if it gets all the possible cases, but at least it is an approach

I am not sure what you refer to ?
If it is about the last created issue (695), I have just updated it because I had made a mistake in the screen shot of the use case.
I have tested again right now, and it is still actual.

@etobella
Copy link
Member Author

1st point should be solved now.

@florian-dacosta
Copy link
Contributor

1st point should be solved now.

It is, thanks
I have made a PR on yours to fix different other issues, Take a look when you can

@florian-dacosta
Copy link
Contributor

@etobella Could you rebase please ?

etobella and others added 2 commits October 1, 2024 15:11
We will use currency of the line in order to get the suspense and max line value
Fix the case of payment with a foreign currency set on bank statement line

improve tests around multi-currency
self.manual_partner_id and self.manual_partner_id.name_get()[0] or False
self.manual_partner_id
and self.manual_partner_id.name_get()[0]
or [False, False]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool([False, False]) => True

Are you sure you want this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the noise here.
I'll make it short here. When there are not partner at all, in the data, the value [False, False] is set here :
https://github.com/OCA/account-reconcile/blob/16.0/account_reconcile_oca/models/account_bank_statement_line.py#L405
and here :
https://github.com/OCA/account-reconcile/blob/16.0/account_reconcile_oca/models/account_bank_statement_line.py#L896

So when checking if the line has changed, and there is no partner at all, comparing the reconcile_data partner part ([False, False])with False will always be considered different while no changed occured (no partner was set).

So I think we could leave this as is. (meaning comparing [False, False] with line.get("partner_id")

Copy link

@PabloMartFL PabloMartFL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the PR on runboat and it resolves the problems in the issues:

  • For the test, I created a company in EUR and created a Bank in EUR.
  • The currency rate are the following:

Captura desde 2024-10-04 12-23-36

The suspended balance apears correct and when you select the invoices that fully match with the statement, it doesnt correct the invoice amount.
Also, in the account move, the amount in currency is displayed correcly

https://www.awesomescreenshot.com/video/32216430?key=6538e7535b46717a2905cfcb08d9a052

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