-
-
Notifications
You must be signed in to change notification settings - Fork 681
[18.0][FIX] account_financial_report: journal ledger move_line_ids_taxes_data #1418
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
base: 18.0
Are you sure you want to change the base?
[18.0][FIX] account_financial_report: journal ledger move_line_ids_taxes_data #1418
Conversation
We solve two problems. First, move_line_ids_taxes_data was never propagated, thus always empty. Second, tax name and description get from sql queries are json values. We now handle translations.
|
ping @DorianMAG |
flotho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Thanks for this usefull PR.
Code could be optimized.
You defined L206 the lang variable. Its value is either en_US or another one, never None AFAICS.
Though, you don't need anymore to switch case on lang in L221 and L223
Could you change this ?
Regards
Hi, thanks for your review. The thing is you don't necessarily have a translation for your current language. |
| move_line_ids_taxes_data[move_line_id][account_tax_id] = { | ||
| "name": tax_name, | ||
| "description": tax_description, | ||
| "name": tax_name.get(lang) or tax_name.get("en_US"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Thanks for this usefull PR. Code could be optimized. You defined L206 the lang variable. Its value is either en_US or another one, never None AFAICS. Though, you don't need anymore to switch case on lang in L221 and L223 Could you change this ?
RegardsHi, thanks for your review.
The thing is you don't necessarily have a translation for your current language. If you look in DB, even though a language is installed, translatable fields will have no other key than "en_US" until they are actually translated once.
Thanks for the answer.
I thought I've understood what you want to do.
What I'm proposing is to change this :
"name": tax_name.get(lang) or tax_name.get("en_US"),
By
"name": tax_name.get(lang),
You can do this because you defined a lang variable with a default value that will be en_US. So the test you made is redundant regarding the possible values in the lang variable.
Am I missing something?
regards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say I have French and English installed on my DB and my current language is French.
- lang = "fr_FR"
- tax_name might be: {"en_US": "Tax 21%"}
Hence, tax_name.get(lang) will return False, which will eventually end up as an empty column in the report.
What I have implemented is a fallback on English translation. This is similar to what the ORM does, if you open the form view of the tax the name won't be empty if you have no French translation, the English name will be displayed instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your clear answer.
I missed this one that data in the json could be "de"synchronized from installed languages.
So evrything is OK
LGTM
In the journal ledger report, only move lines with
tax_line_idhad their taxes displayed.The case where taxes come from
move_line_ids_taxes_dataseems to have been forgotten.We solve two problems.
First,
move_line_ids_taxes_datawas never propagated, thus always empty.Second, tax name and description get from sql queries are json values. We now handle translations.