Commit 45c03413 authored by Jérome Perrin's avatar Jérome Perrin

hal_json_style: prevent a translation of worklist with document count

When viewing the worklist page, a request is made to traverse portal_workflow,
which translate global worklist actions including the document count.

The non regression test was failing with:

    [u'Draft To Validate (1)'] != ['Draft To Validate']

and in real usage, several messages were added to Localizer.
parent f6d0f4c0
Pipeline #15471 failed with stage
......@@ -343,6 +343,14 @@ portal_absolute_url = portal.absolute_url()
preference_tool = portal.portal_preferences
Base_translateString = portal.Base_translateString
def translateWorklistActionName(worklist_action_name):
"""Translate a worklist action name, that may contain the document count.
Instead of translating "Draft to Validate (123)", we translate "Draft to Validate".
"""
return Base_translateString(re.sub(r' \(\d+\)$', '', worklist_action_name))
preferred_html_style_developer_mode = preference_tool.getPreferredHtmlStyleDevelopperMode()
preferred_html_style_translator_mode = preference_tool.getPreferredHtmlStyleTranslatorMode()
......@@ -1519,7 +1527,10 @@ def calculateHateoas(is_portal=None, is_site_root=None, traversed_document=None,
'href': '%s' % view_action['url'],
'name': view_action['id'],
'icon': view_action['icon'],
'title': Base_translateString(view_action['title']),
'title': Base_translateString(
translateWorklistActionName(view_action['title'])
if 'worklist_id' in view_action
else view_action['title']),
  • This commit in general works good, but I cannot understand the patch here. Shouldn't it be something like:

              'title': translateWorklistActionName(
                view_action['title']
                ) if 'worklist_id' in view_action else Base_translateString(
                  view_action['title']
                ),
    

    I could not find some broken case yet, but this translateWorklistActionName within Base_translateString looks sneaky

  • By looking at this patch, it seems there's a double translation, but by looking at the test it seems the test should have catched it, so I don't know but it seems you are right.

  • Test is difficult to read, but would it catch it?

    I mean, you made the patch here so that we do not translate Draft To Validate (1), but we do Draft To Validate. So reading the test, I think it checks this. But does it check if we translated the translation of Draft To Validate? Meaning if we add to Localizer something like Brouillon à valider, which is what I think the issue would be here.

  • I was assuming that there is no translation for Draft To Validate in this message catalog, so the first call will return Draft To Validate and the second call would also translate Draft To Validate, which should be catched by this assertion

    
        self.assertEqual(
            [x[1][0] for x in gettext_mock.mock_calls if x[1][0].startswith('Draft To Validate')],
            ['Draft To Validate'])
    

    that should fail because in the first argument we have a Draft To Validate twice.

    Maybe we are adding a message in another test here and that's why the test won't fail:

    https://lab.nexedi.com/nexedi/erp5/blob/45c03413ac6c7762027a4b7908ef907c870968a7/bt5/erp5_hal_json_style/TestTemplateItem/portal_components/test.erp5.testHalJsonStyle.py#L2802


    let's try to change the test to make it detect the double translation https://lab.nexedi.com/nexedi/erp5/pipelines/24554

    Edited by Jérome Perrin
  • mmm but I do not get how it should be caught.

    Meaning, if in our Localizer we have no translation for Draft To Validate defined, then we will still have one key Draft To Validate, no new will be added.

    In on the other hand we have a translation, let's say in fr it is Brouillon à valider, then in our Localizer we will have 2 keys, Draft To Validate and Brouillon à valider. But Draft To Validate will still appear once.

    Btw, I could reproduce, with current code we get Brouillon à valider in Localizer

    let's try to change the test to make it detect the double translation https://lab.nexedi.com/nexedi/erp5/pipelines/24554

    ah, I did not know you could do a pipeline for a patch. It runs the tests? But still this would need new test I think (if we actually want to add it to tests)

  • I thought it would be catched because the gettext_mock would be called twice by the Base_translateString(Base_translateString('Draft to Validate'))

  • ah, I did not know you could do a pipeline for a patch. It runs the tests? But still this would need new test I think (if we actually want to add it to tests)

    just by pushing the commit in for_testrunner_1 branch, the commit is annotated automatically

Please register or sign in to reply
})
global_action_type = ("view", "workflow", "object_new_content_action",
......@@ -2321,7 +2332,7 @@ def calculateHateoas(is_portal=None, is_site_root=None, traversed_document=None,
'href': url_template_dict["jio_search_template"] % {
"query": make_query({"query": query})
},
'name': Base_translateString(re.sub(r' \(\d+\)$', '', action['name'])),
'name': translateWorklistActionName(action['name']),
'count': action['count']
}
......
......@@ -16,6 +16,7 @@ import json
import re
import urllib
import mock
from zope.globalrequest import setRequest # pylint: disable=no-name-in-module, import-error
from Acquisition import aq_base
from Products.ERP5Form.Selection import Selection, DomainSelection
......@@ -2872,6 +2873,33 @@ return msg"
self.assertEqual(result_dict['_debug'], "worklist")
@simulate('Base_getRequestUrl', '*args, **kwargs',
'return "http://example.org/bar"')
@simulate('Base_getRequestHeader', '*args, **kwargs',
'return "application/hal+json"')
@simulate('Base_translateString', 'msg, catalog="ui", encoding="utf8", lang="wo", **kw',
code_string)
@createIndexedDocument()
@changeSkin('Hal')
def test_getHateoasWorklist_portal_workflow_worklist_translation(self, document):
# Non regression test that traversing portal_workflow does not translate
# worklists with the actual count of document
fake_request = do_fake_request("GET")
default_gettext = self.portal.Localizer.erp5_ui.gettext
def gettext(message, **kw):
return default_gettext(message, **kw)
with mock.patch.object(self.portal.Localizer.erp5_ui.__class__, 'gettext', side_effect=gettext) as gettext_mock:
self.portal.web_site_module.hateoas.ERP5Document_getHateoas(
REQUEST=fake_request,
mode='traverse',
relative_url='portal_workflow'
)
self.assertEqual(
[x[1][0] for x in gettext_mock.mock_calls if x[1][0].startswith('Draft To Validate')],
['Draft To Validate'])
@simulate('Base_getRequestUrl', '*args, **kwargs',
'return "http://example.org/bar"')
@simulate('Base_getRequestHeader', '*args, **kwargs',
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment