refactor: Exchange rate revaluation to handle accounts with zero account balance (backport #33165) #33503
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## version-14-hotfix #33503 +/- ##
=====================================================
+ Coverage 63.91% 64.04% +0.13%
=====================================================
Files 817 818 +1
Lines 58167 58840 +673
=====================================================
+ Hits 37180 37687 +507
- Misses 20987 21153 +166
|
…unt balance (#33165) * refactor: new type for JE - Exchange Gain or Loss * refactor: skip few validations for Exchanage Gain Or Loss type Jour * refactor: ERR create 2 journals for handling zero and non-zero compa 1. Additional check box accounts table to identify accounts with zero balance 2. Accounts with zero balance only in either of the 2 currencies will be handled on separate Journal * refactor: skips few validation for allowing 0 debit/credit * fix: General Ledger presentaion currency * test: fix test case in general ledger * test: fix failing test case in AR report (cherry picked from commit 914b230)
c676545 to
146733c
Compare
|
🎉 This PR is included in version 14.12.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Comment on lines
+186
to
+250
| for d in [x for x in account_details if not x.zero_balance]: | ||
| current_exchange_rate = ( | ||
| d.balance / d.balance_in_account_currency if d.balance_in_account_currency else 0 | ||
| ) | ||
| new_exchange_rate = get_exchange_rate(d.account_currency, company_currency, posting_date) | ||
| new_balance_in_base_currency = flt(d.balance_in_account_currency * new_exchange_rate) | ||
| gain_loss = flt(new_balance_in_base_currency, precision) - flt(d.balance, precision) | ||
| if gain_loss: | ||
| accounts.append( | ||
| { | ||
| "account": d.account, | ||
| "party_type": d.party_type, | ||
| "party": d.party, | ||
| "account_currency": d.account_currency, | ||
| "balance_in_base_currency": d.balance, | ||
| "balance_in_account_currency": d.balance_in_account_currency, | ||
| "zero_balance": d.zero_balance, | ||
| "current_exchange_rate": current_exchange_rate, | ||
| "new_exchange_rate": new_exchange_rate, | ||
| "new_balance_in_base_currency": new_balance_in_base_currency, | ||
| "new_balance_in_account_currency": d.balance_in_account_currency, | ||
| "gain_loss": gain_loss, | ||
| } | ||
| ) | ||
|
|
||
| # Handle Accounts with '0' balance in Account/Base Currency | ||
| for d in [x for x in account_details if x.zero_balance]: | ||
|
|
||
| # TODO: Set new balance in Base/Account currency | ||
| if d.balance > 0: | ||
| current_exchange_rate = new_exchange_rate = 0 | ||
|
|
||
| new_balance_in_account_currency = 0 # this will be '0' | ||
| new_balance_in_base_currency = 0 # this will be '0' | ||
| gain_loss = flt(new_balance_in_base_currency, precision) - flt(d.balance, precision) | ||
| else: | ||
| new_exchange_rate = 0 | ||
| new_balance_in_base_currency = 0 | ||
| new_balance_in_account_currency = 0 | ||
|
|
||
| current_exchange_rate = calculate_exchange_rate_using_last_gle( | ||
| company, d.account, d.party_type, d.party | ||
| ) | ||
|
|
||
| gain_loss = new_balance_in_account_currency - ( | ||
| current_exchange_rate * d.balance_in_account_currency | ||
| ) | ||
|
|
||
| if gain_loss: | ||
| accounts.append( | ||
| { | ||
| "account": d.account, | ||
| "party_type": d.party_type, | ||
| "party": d.party, | ||
| "account_currency": d.account_currency, | ||
| "balance_in_base_currency": d.balance, | ||
| "balance_in_account_currency": d.balance_in_account_currency, | ||
| "zero_balance": d.zero_balance, | ||
| "current_exchange_rate": current_exchange_rate, | ||
| "new_exchange_rate": new_exchange_rate, | ||
| "new_balance_in_base_currency": new_balance_in_base_currency, | ||
| "new_balance_in_account_currency": new_balance_in_account_currency, | ||
| "gain_loss": gain_loss, | ||
| } | ||
| ) |
There was a problem hiding this comment.
@ruthra-kumar I would like to know why iterate twice account_details. Can I refactor it?
There was a problem hiding this comment.
It's not processing all the account_details twice. First loop only processes accounts with balances, second one only processes accounts with zero balance in account/base currency. It's mentioned in the comment as well.
This separation is so that code is easy to reason about.
Comment on lines
+215
to
+232
| if d.balance > 0: | ||
| current_exchange_rate = new_exchange_rate = 0 | ||
|
|
||
| new_balance_in_account_currency = 0 # this will be '0' | ||
| new_balance_in_base_currency = 0 # this will be '0' | ||
| gain_loss = flt(new_balance_in_base_currency, precision) - flt(d.balance, precision) | ||
| else: | ||
| new_exchange_rate = 0 | ||
| new_balance_in_base_currency = 0 | ||
| new_balance_in_account_currency = 0 | ||
|
|
||
| current_exchange_rate = calculate_exchange_rate_using_last_gle( | ||
| company, d.account, d.party_type, d.party | ||
| ) | ||
|
|
||
| gain_loss = new_balance_in_account_currency - ( | ||
| current_exchange_rate * d.balance_in_account_currency | ||
| ) |
There was a problem hiding this comment.
@ruthra-kumar Entries here are supposed to be either balance == 0 or balance_in_account_currency == 0, however
I have seen cases when d.balance < 0 which is normal, I think the logic here could be different. If we calculate current_exchange_rate, this would be negative also, mathematically reasonable but not in business. We could instead set it to zero and apply the same logic as if it were positive. gain_loss = flt(new_balance_in_base_currency, precision) - flt(d.balance, precision). In this case, the condition here is not necessary. Apologize me if I musunderstood the logic.
There was a problem hiding this comment.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an automatic backport of pull request #33165 done by Mergify.
Cherry-pick of 914b230 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Mergify commands and options
More conditions and actions can be found in the documentation.
You can also trigger Mergify actions by commenting on this pull request:
@Mergifyio refreshwill re-evaluate the rules@Mergifyio rebasewill rebase this PR on its base branch@Mergifyio updatewill merge the base branch into this PR@Mergifyio backport <destination>will backport this PR on<destination>branchAdditionally, on Mergify dashboard you can:
Finally, you can contact us on https://mergify.com