Merge lp:~savoirfairelinux-openerp/ocb-server/translate_warnings_1297525-trunk into lp:ocb-server

Status: Rejected
Rejected by: Holger Brunn (Therp) on 2014-11-24
Proposed branch: lp:~savoirfairelinux-openerp/ocb-server/translate_warnings_1297525-trunk
Merge into: lp:ocb-server
Diff against target: 69 lines (+9/-7)
3 files modified
openerp/osv/orm.py (+1/-1)
openerp/service/wsgi_server.py (+6/-4)
openerp/tools/translate.py (+2/-2)
To merge this branch: bzr merge lp:~savoirfairelinux-openerp/ocb-server/translate_warnings_1297525-trunk
Reviewer Review Type Date Requested Status
Holger Brunn (Therp) 2014-03-25 Disapprove on 2014-11-24
Stefan Rijnhart (Opener) Abstain on 2014-05-02
Sandy Carter (http://www.savoirfairelinux.com) Needs Information on 2014-04-30
OpenERP Community Reviewer/Maintainer 2014-04-30 Pending
Review via email: mp+212736@code.launchpad.net

Description of the change

When searching a frame for context, the _() looks for self.localcontext but misses self.context.
The integration of this patch allows for web warnings (such as except_orm) to be translated by _()

To post a comment you must log in.
review: Approve (code review)
5296. By Mohammed Shekha(Open ERP) on 2014-04-06

[FIX]Base, Tools Translation: Fixed the issue of translation, warnings, except_orm and which is raised through OpenERPSession class send method or raised xmlrpclib.Fault in xmlrpc_handle_exception_legacy will also not be translatanble, also there is no way through which translation terms which are used outside any module like used in tools and service will not be registered in pot when translation is exported, we need to add those path explicitly.

5297. By El Hadji Dem (http://www.savoirfairelinux.com) on 2014-04-06

[FIX] Fixed instance of ValidateError not being translated

Two commits have been added:
r5296 from OpenERP's support
r5297 from our team for a case I missed of error message translation

Thanks for the proposal!

Strings like 'AccessError' and 'ValidateError' are not proper English, so they cannot be translated. Either we accept the current strings as fixed codes (in lieu of a proper exception type) and they should not be passed to gettext, or we should replace them with translatable strings such as Access Error and Validation Error.

review: Needs Fixing

@Stefan they give a satisfactory message when translated (no update to .po needed)

I don't want to change too much from the original code, but if you feel it's really needed, I can replace them with your suggestions.

review: Needs Information

Yes, I would like you to take the opportunity to improve this as well. I would prefer to see proper English here, but I am not sure if its value is checked on any occasion in case the exception is caught (in lieu of a proper exception subclassing, I can imagine).

5298. By Sandy Carter (http://www.savoirfairelinux.com) on 2014-04-30

[IMP] Use english translatable string in error popups

Done.

Please make sure this doesn't have funny side effects and translates properly.

OK, here's a reason not to change the labels and keep them untranslatable:

    hr_attendance/test/attendance_process.yml: assert e[0]=='ValidateError', e

Apparently, these labels are used in tests.

The tests fail anyway in any other language, especially in yml tests
Should I fix that assert too.

Well, if that is possible. What kind of solution were you thinking of to fix the test?

Basically, if I change the labels, I would change the message in the tests.
Ideally, an assertRaises should be used there, but I have no idea if yml tests support that.

Can you be sure that if a translator on Launchpad changed the translation of this term, it does not make the tests fail?

The en_US translation I mean.

It would make the tests fail, but this problem is widespread in OE.
For example test_20_message_post (openerp.addons.mail.tests.test_mail_features.test_mail) fails when a translator changes the value. It just doesn't fail in en_US because the values haven't changed. The same is not true for other locales.

A solution to this would be to use _() in the assert. I am not sure if that's possible in yml.

If we're talking about changing en_US why not revert my last commit and translate "ValidateError" as "Validate Error"?

I guess it's hard for us to decide without input from the upstream developers. I really don't want to break ocb tests, and to prevent that it would need a coordinated merge between the two projects. So I would therefore still favour having 'ValidateError' to be restored and untranslatable for ocb, but maybe change it to a translatable 'Validation Error' in proposals on upstream trunk server and the tests in addons?

I have already made some MPs for upstream, but the code is pretty different.

Not sure what you mean there. Do you think my suggestions are not a good idea?

Sorry. I didn't explain very well.
I have MPs for OCB, 7.0 and trunk for this bug.
OE trunk doesn't have both MP (server and web) because it too much, therefore web doesn't have an MP.
As for server, commits up to r5297 ([FIX] Fixed instance of ValidateError not being translated) have been proposed.

I think we need a reviewer with a fresh look on this.

review: Abstain
Holger Brunn (Therp) (hbrunn) wrote :

Development for 7.0 has moved to github on https://github.com/OCA/ocb - please move your merge proposal there if it is still valid.

(I close and reject this in order to have a cleaner overview for 6.1 MPs which indeed have to be done on launchpad)

review: Disapprove

Unmerged revisions

5298. By Sandy Carter (http://www.savoirfairelinux.com) on 2014-04-30

[IMP] Use english translatable string in error popups

5297. By El Hadji Dem (http://www.savoirfairelinux.com) on 2014-04-06

[FIX] Fixed instance of ValidateError not being translated

5296. By Mohammed Shekha(Open ERP) on 2014-04-06

[FIX]Base, Tools Translation: Fixed the issue of translation, warnings, except_orm and which is raised through OpenERPSession class send method or raised xmlrpclib.Fault in xmlrpc_handle_exception_legacy will also not be translatanble, also there is no way through which translation terms which are used outside any module like used in tools and service will not be registered in pot when translation is exported, we need to add those path explicitly.

5295. By Sandy Carter (http://www.savoirfairelinux.com) on 2014-03-25

Also look for context in self

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openerp/osv/orm.py'
2--- openerp/osv/orm.py 2014-04-07 10:57:40 +0000
3+++ openerp/osv/orm.py 2014-04-30 15:28:06 +0000
4@@ -1559,7 +1559,7 @@
5 )
6 self._invalids.update(fields)
7 if error_msgs:
8- raise except_orm('ValidateError', '\n'.join(error_msgs))
9+ raise except_orm(_('Validate Error'), '\n'.join(error_msgs))
10 else:
11 self._invalids.clear()
12
13
14=== modified file 'openerp/service/wsgi_server.py'
15--- openerp/service/wsgi_server.py 2013-11-25 10:38:42 +0000
16+++ openerp/service/wsgi_server.py 2014-04-30 15:28:06 +0000
17@@ -44,6 +44,7 @@
18 import openerp
19 import openerp.modules
20 import openerp.tools.config as config
21+from openerp.tools.translate import _
22 import websrv_lib
23
24 _logger = logging.getLogger(__name__)
25@@ -138,17 +139,18 @@
26 return response
27
28 def xmlrpc_handle_exception_legacy(e):
29+ code_string = u"%s -- %s\n\n%s"
30 if isinstance(e, openerp.osv.osv.except_osv):
31- fault = xmlrpclib.Fault('warning -- ' + e.name + '\n\n' + e.value, '')
32+ fault = xmlrpclib.Fault(code_string % (_("warning"), e.name, e.value), '')
33 response = xmlrpclib.dumps(fault, allow_none=False, encoding=None)
34 elif isinstance(e, openerp.exceptions.Warning):
35- fault = xmlrpclib.Fault('warning -- Warning\n\n' + str(e), '')
36+ fault = xmlrpclib.Fault(code_string % (_("warning"), _("Warning"), e), '')
37 response = xmlrpclib.dumps(fault, allow_none=False, encoding=None)
38 elif isinstance(e, openerp.exceptions.AccessError):
39- fault = xmlrpclib.Fault('warning -- AccessError\n\n' + str(e), '')
40+ fault = xmlrpclib.Fault(code_string % (_("warning"), _("Access Error"), e), '')
41 response = xmlrpclib.dumps(fault, allow_none=False, encoding=None)
42 elif isinstance(e, openerp.exceptions.AccessDenied):
43- fault = xmlrpclib.Fault('AccessDenied', str(e))
44+ fault = xmlrpclib.Fault(_('Access Denied'), str(e))
45 response = xmlrpclib.dumps(fault, allow_none=False, encoding=None)
46 elif isinstance(e, openerp.exceptions.DeferredException):
47 info = e.traceback
48
49=== modified file 'openerp/tools/translate.py'
50--- openerp/tools/translate.py 2014-04-11 16:37:46 +0000
51+++ openerp/tools/translate.py 2014-04-30 15:28:06 +0000
52@@ -196,7 +196,7 @@
53 lang = ctx.get('lang')
54 s = frame.f_locals.get('self', {})
55 if not lang:
56- c = getattr(s, 'localcontext', None)
57+ c = getattr(s, 'localcontext', None) or getattr(s, 'context', None)
58 if c:
59 lang = c.get('lang')
60 if not lang:
61@@ -863,7 +863,7 @@
62 path_list = [root_path,] + apaths
63
64 # Also scan these non-addon paths
65- for bin_path in ['osv', 'report' ]:
66+ for bin_path in ['osv', 'report', 'tools', 'service' ]:
67 path_list.append(os.path.join(config.config['root_path'], bin_path))
68
69 _logger.debug("Scanning modules at paths: ", path_list)