Bug 8407 - xt/tt_valid.t is too strict
Summary: xt/tt_valid.t is too strict
Status: RESOLVED LATER
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on: 6458
Blocks: 8385
  Show dependency treegraph
 
Reported: 2012-07-10 13:16 UTC by Jonathan Druart
Modified: 2012-08-01 13:41 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8407: make tt_valid less strict (for non translatable strings) (1.05 KB, patch)
2012-07-10 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8407: make tt_valid less strict (for non translatable strings) (1.10 KB, patch)
2012-07-13 07:35 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8407: make tt_valid less strict (for non translatable strings) (1.26 KB, patch)
2012-07-16 12:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8407: make tt_valid less strict (for non translatable strings) (1.30 KB, patch)
2012-07-25 22:48 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2012-07-10 13:16:19 UTC
A new bug (Bug 8385) highlights a problem already known : 
In Koha, we forbid the following syntax in tt files:
    <option value="foo" [% IF ( option.selected ) %] selected="selected" [% END %]>bar</option>

We prefer :
    [% IF ( option.selected ) %]
        <option value="foo" selected="selected">bar</option>
    [% ELSE %]
        <option value="foo">bar</option>
    [% END %]

For Bug 8385, if I want to make tt_valid.t happy, I should replace 8 lines (with the same structure as previous) with 40 lines.
These new lines get nowhere.
Less readability (discutable), less concise, a lot of useless lines, etc.

Why ?

I don't know exactly. The translator tool crashes if this kind of syntax occurs.

In fact, after few test, it seems the problem existed with HTML::Template, but I don't get through with Template::Toolkit.

It would seem that we can introduce a IF statement to add attributes like selected, checked or disabled (for example).
Given that the string (between IF and END) does not contain translatable strings.

I will propose a patch (for discussion and massive test).
It would be great if several testers could check that this patch does not valid a broken template or a template containing non translatable strings.

Like as Frédéric Demians said me, it is a time bomb and we have to be sure the templates do not passed as valid if they are not.
Comment 1 Jonathan Druart 2012-07-10 13:21:15 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2012-07-12 15:32:24 UTC
Chris, could you give your opinion here ? That did not work at all with HTML::Template::Pro, but it seems it work well with T::T

(jonathan, I've switched to need signoff)
Comment 3 Chris Cormack 2012-07-12 20:14:52 UTC
These seems to work ok, the template still passes the translated test.

Before the patch
 perl xt/author/translatable-templates.t               
1..2
ok 1 - opac templates are translatable
ok 2 - intranet templates are translatable

 xt/tt_valid.t                            2012-07-13 08:04:57 christopher pts/2
1..2
not ok 1 - TT syntax: not using TT directive within HTML tag
#   Failed test 'TT syntax: not using TT directive within HTML tag'
#   at xt/tt_valid.t line 55.
# Files list: 
# intranet-tmpl/prog/en/modules/admin/searchengine/solr/indexes.tt: 132, 133, 134, 135, 136, 137, 141, 144
ok 2 - Token received same as original put on stack
# Looks like you failed 1 test of 2.


After the patch

 perl xt/tt_valid.t                                    2012-07-13 08:09:14 christopher pts/2
1..2
not ok 1 - TT syntax: not using TT directive within HTML tag
#   Failed test 'TT syntax: not using TT directive within HTML tag'
#   at xt/tt_valid.t line 60.
# Files list: 
# intranet-tmpl/prog/en/modules/admin/searchengine/solr/indexes.tt: 141, 144
ok 2 - Token received same as original put on stack
# Looks like you failed 1 test of 2.


Dropped the errors in the tt down from 8 to 2.

Looking at that file
It appears to still be the checked="checked" lines it doesn't like. 

If we fix that up, Ill do the first signoff.
Comment 4 Jonathan Druart 2012-07-13 07:23:50 UTC
Hi Chris, 
I proposed a patch for Bug 8385. It fixes these 2 errors.
Comment 5 Chris Cormack 2012-07-13 07:35:45 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2012-07-13 07:39:40 UTC
I have signed off, but will leave it needs signoff, I would like to get it a bit more testing.

It seems safe, but If someone could test creating and updating the .po files for another language as well as installing them. I only did english-nz
Comment 7 Katrin Fischer 2012-07-16 06:01:28 UTC
I tested breaking the templates and checking the output of the test.

I think maybe the test needs should be modified to ignore the keywords disabled, checked and selected in a TT directive. Example:

<th [% UNLESS ( selected ) %]title="TESTING"[% END %]>Date</th>

This will pass the test. I am not sure, if it really breaks translations. In my tests "TESTING" didn't show up in the updated po files.

I am not sure, if this is reason to fail this an if it really breaks something. 

Frederic, maybe you can take a look and give your opinion?
Comment 8 Jonathan Druart 2012-07-16 12:12:19 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2012-07-25 14:37:49 UTC
I raise the priority for this bug.

Indeed, Jenkins is not happy currently.

Either this patch corrects and does not introduce regression, or I will submit a patch for Bug 8385 (which will replace 6 lines with 6*5 [useless] lines).

Please test and report any eventual problems.
Comment 10 Chris Cormack 2012-07-25 22:27:21 UTC
Have signed off again, I am happy with it.

It fixes the problems katrin found. Works with Maori and English-NZ

Drops the fails to 1, which bug 8385 fixes
Comment 11 Chris Cormack 2012-07-25 22:48:22 UTC
Created attachment 11139 [details] [review]
Bug 8407: make tt_valid less strict (for non translatable strings)

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 12 Paul Poulain 2012-08-01 12:11:06 UTC
chris, could you describe the exact tests you made.

I did the following:

in memberentrygen.tt, replaced
[% IF ( categoryloo.categorycodeselected ) %]
    <option value="[% categoryloo.categorycode %]" selected="selected" data-typename="[% typeloo.typename %]">[% categoryloo.categoryname %]</option>
 [% ELSE %]
    <option value="[% categoryloo.categorycode %]" data-typename="[% typeloo.typename %]">[% categoryloo.categoryname %]</option>

by

<option value="[% categoryloo.categorycode %]" [% IF ( categoryloo.categorycodeselected ) %]selected="selected"[% END %] data-typename="[% typeloo.typename %]" title="blablabla">[% categoryloo.categoryname %]</option>


in english, no problems.

then ./translate update fr-FR
translate blablabla into bliblibli

then ./translate install fr-FR

switching to english, I get a very nasty:

Template process failed: file error - parse error - /home/paul/koha.dev/koha-community/koha-tmpl/intranet-tmpl/prog/fr-FR/modules/members/memberentrygen.tt line 520: unexpected token (%)
  [% "[%">[% categoryloo.categoryname %] at /home/paul/koha.dev/koha-community/C4/Templates.pm line 127.
 at /home/paul/koha.dev/koha-community/members/memberentry.pl line 842

Checking the french templates, I see:
 [% FOREACH typeloo IN typeloop %] [% FOREACH categoryloo IN typeloo.categoryloop %] [% IF ( loop.first ) %] [% IF ( typeloo.typename_C ) %]<optgroup value="C" label="Enfant">[% END %] [% IF ( typeloo.typename_A ) %]<optgroup value="A" label="Adulte">[% END %] [% IF ( typeloo.typename_S ) %]<optgroup label="Staff"        value="S">[% END %] [% IF ( typeloo.typename_I ) %]<optgroup value="I" label="Collectivité">[% END %] [% IF ( typeloo.typename_P ) %]<optgroup value="P" label="Professionnel">[% END %] [% IF ( typeloo.typename_X ) %]<optgroup value="X" label="Statistiques">[% END %] [% END %] <option data-typename="[% typeloo.typename %]" value="[% categoryloo.categorycode %]" categoryloo.categorycodeselected="categoryloo.categorycodeselected" %]="%]" if="IF" end="END" )=")" title="bliblibli" %]selected="selected" (="(" [%="[%">[% categoryloo.categoryname %]</option>
Comment 13 Paul Poulain 2012-08-01 12:17:14 UTC
(In reply to comment #12)
> switching to english, I get a very nasty:

One must read "switching to french, I get a very nasty:...3

In english, everything is OK
Comment 14 Katrin Fischer 2012-08-01 12:22:54 UTC
I guess that proves that the problem still exists with TT :(
So we shouldn't change the test and fix the templates.
Comment 15 Jonathan Druart 2012-08-01 13:26:29 UTC
Problem is in routine text_replace_tag (tmpl_process3.pl). I don't know how we can fix it :-/

I abandon this bug (for now...) and will submit a patch for Bug 8385 ;-(