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

8.0 account_financial_report_webkit fix for multilingual Chart of Acc… #123

Merged

Conversation

luc-demeyer
Copy link

This PR ensures that the user language is correctly passed in the context which is required for users with the "translate=True" on the account.account name field.

@pedrobaeza
Copy link
Member

Reviewing code, seems 👍, but not tested

@luc-demeyer
Copy link
Author

@pedrobaeza
Thanks for the fast review.
I have tested partner reports, trial balance, general ledger with and without comparison.

@BT-ojossen
Copy link

In this file "account_financial_report_webkit/report/common_reports.py" we need also extension with the context. Please have a look in #121.

@luc-demeyer
Copy link
Author

@BT-ojossen
I tested all reports but maybe I forgot a specific use case.
Can you let me know which report is affected by not having the context passed in the "common_reports.py" ?

@BT-ojossen
Copy link

@luc-demeyer
Hi Luc, I guess you're right, it should be ok like this.

@luc-demeyer
Copy link
Author

I think this one is ready for merge.

@luc-demeyer
Copy link
Author

cf. #133 (comment)
Can someone commit this PR ?

@pedrobaeza
Copy link
Member

Needs another review at least. I'm not comfortable enough with only my code review to merge by myself.

@@ -259,7 +261,7 @@ def compute_partner_balance_data(self, data, filter_report_type=None):
# get details for each accounts, total of debit / credit / balance
accounts_by_ids = self._get_account_details(
account_ids, target_move, fiscalyear, main_filter, start, stop,
initial_balance_mode)
initial_balance_mode, context=lang_ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Why not use context=data['form']['used_context'] (or a similar form) instead of this limited context which only contains lang ?
data['form']['used_context'] is defined here : https://github.com/odoo/odoo/blob/8.0/addons/account/wizard/account_report_common.py#L183 and lang key is already defined.

Copy link
Author

Choose a reason for hiding this comment

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

This is a very minimal change. By only adding context=lang_ctx to those places in the code where there was no context passing before I fix the multi-language problem of the account.account,name field without risk to break something else.
I tested all reports with this change and everything work fine, hence I prefer that we merge it this way.
Seen the fact that many methods within the standard account module react differently depending on what is in the context, I think we risk to break a set of reports which work fine for several years now (and we all know how sensitive accountants are when this module would start to return wrong financial data).

Copy link
Member

Choose a reason for hiding this comment

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

@luc-demeyer I need this (luc-demeyer#1) for a customer project. Can you have a look please ? Thanks

@droelneo
Copy link

Tested and ok !

@pedrobaeza
Copy link
Member

@luc-demeyer, are you going to include the patch by @damdam-s?

@pedrobaeza
Copy link
Member

#121 is a patch for solving the same problem, please judge which one is best.

@BT-ojossen
Copy link

+1 Please merge

@melroy89
Copy link

melroy89 commented Oct 3, 2017

@pedrobaeza
@sbidoul
Please merge? Plus sync to all new releases up to and including v11.

@pedrobaeza
Copy link
Member

This has conflicts. Forward porting this patch should be done by a contributor.

@luc-demeyer
Copy link
Author

merging this fix should imho not be an issue.
Forward port is indeed another issue.

@lasley
Copy link

lasley commented Oct 31, 2017

@luc-demeyer Merge is not possible at the moment - there are conflicts (the branch needs to be rebased).

@luc-demeyer luc-demeyer force-pushed the 8.0-webkit_report_account_multilang_fix branch from 889fc1e to 179543d Compare November 1, 2017 19:03
@luc-demeyer
Copy link
Author

@lasley
rebased hence can be merged now.

@lasley lasley merged commit ab5dd74 into OCA:8.0 Nov 1, 2017
rconjour pushed a commit to EssentNovaTeam/account-financial-reporting that referenced this pull request Mar 16, 2020
[FIX] Bug OCA#115 on account_auto_fy_sequence: auto-create fiscal-year spec...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants