Bug 28462

Summary: TT tag on several lines break the translator tool
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: OPACAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to oldoldoldstable --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: fridolin.somers, kyle, victor, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.02,20.11.09,20.05.15,19.11.22
Bug Depends on: 18776    
Bug Blocks:    
Attachments: Bug 28462: Remove TT tag on several lines - opac-advsearch.tt
Bug 28462: Remove TT tag on several lines - opac-advsearch.tt
Bug 28462: Remove line breaks in TT tags
Bug 28462: Remove TT tag on several lines - opac-advsearch.tt
Bug 28462: Remove line breaks in TT tags
Bug 28462: Remove TT tag on several lines - opac-advsearch.tt
Bug 28462: Remove line breaks in TT tags

Description Jonathan Druart 2021-05-26 14:19:36 UTC
In opac-advsearch.tt
Comment 1 Jonathan Druart 2021-05-26 14:24:28 UTC
Well, not sure it's major actually, it has been there for a while now.
Comment 3 Jonathan Druart 2021-05-26 14:27:35 UTC
Created attachment 121434 [details] [review]
Bug 28462: Remove TT tag on several lines - opac-advsearch.tt

It fixes the translation in case the operators have been translated

Test plan:
1. update/install pt-BR
2. Use pt-BR and hit opac-advsearch.pl
3. Notice the error
Template process failed: file error - parse error - /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/pt-BR/modules/opac-advsearch.tt line 409: unexpected token (e)                                                    [% IF (OpacAdvSearchMoreOptions and OpacAdvSearchMoreOptions.size> 0 e extended_options) ou (OpacAdvSearchOptions and OpacAdvSearchOptions.size> 0 e n expandido_options) %] at /kohadevbox/koha/C4/Templates.pm
4. Apply the patch and repeat 1 and 2
Note that it's now working
Comment 4 Victor Grousset/tuxayo 2021-05-27 17:20:11 UTC
I can't reproduce, somehow the process handles well the TT tag on several lines.
After these commands
misc/translator/translate update pt-BR
misc/translator/translate install pt-BR

=== the translated TT looks like ===
 [% END # / IF expanded_options %]

 [% IF ( OpacAdvSearchMoreOptions and OpacAdvSearchMoreOptions.size > 0 and expanded_options ) or
 ( OpacAdvSearchOptions and OpacAdvSearchOptions.size > 0 and not expanded_options ) %]
 <div class="text-center">
 <fieldset class="action">
 <!-- SEARCH BUTTONS -->
 [% PROCESS searchbuttons %]
 <!-- /SEARCH BUTTONS -->
 </fieldset>
 </div>
 [% END %]
 </div> <!-- / .container-fluid -->
 </div> <!-- / .main -->
</form>

[% INCLUDE 'opac-bottom.inc' %]
[% BLOCK searchbuttons %]
 <p>
 <input accesskey="s" class="btn btn-primary" name="do" title="Pesquisar TEST translated" type="submit" value="Pesquisar" />


=== Analysis ===
The multi line condition isn't translate and thus not broken.

we can see "Pesquisar TEST translated" which comes from the .po
where it's the translation of "Search TEST source"
which has been added to the original template to confirm that the process is working.
Comment 5 Jonathan Druart 2021-06-07 10:00:41 UTC
Victor, what is the value in the PO?
I've written this patch on top of 20.11 IIRC, but I am pretty sure it will fail on master if the tag is translated.

Basically, after the "translate install" the IF TT tags must not appear in the po.
Comment 6 Hakam Almotlak 2021-06-22 19:52:41 UTC
Hey i did the test plan step by step but then i was unable to find any error.
Comment 7 Victor Grousset/tuxayo 2021-06-23 04:32:49 UTC
(In reply to Jonathan Druart from comment #5)
> Victor, what is the value in the PO?

"Pesquisar TEST translated"

I changed the original source to "Search TEST source"
And it showed in the .po and it's translation was filed.
And it ended up in the translated template, thus showing that the whole process is working. But page hasn't been broken.
Comment 8 Victor Grousset/tuxayo 2021-06-23 04:37:45 UTC
Just did a quick minimal retry
- master
- misc/translator/translate update pt-BR
- misc/translator/translate install pt-BR
- enable pt-BR in sysprefs
- in pt-BR, go to OPAC://cgi-bin/koha/opac-search.pl
- no error

¯\ (°_o)/¯
Comment 9 Victor Grousset/tuxayo 2021-06-23 05:07:33 UTC
Wait, I see what you mean.

It's just that the current .po doesn't reproduce the error.
Because the IF statement translation is the same. So it hides the issue.

Ok, so I can reproduce. I just need to alter the translation of the IF statement in the .po to sabotage it.

It works! :D

The old .po content if still there but commented
#, c-format
#~ msgid ""
#~ "%s %s [%% IF ( OpacAdvSearchMoreOptions and OpacAdvSearchMoreOptions.size "
#~ "> 0 and expanded_options ) or ( OpacAdvSearchOptions and "
#~ "OpacAdvSearchOptions.size > 0 and not expanded_options ) %%] "
#~ msgstr ""
#~ "%s %s [%% SABOTAGEEEEEEEEEE ( OpacAdvSearchMoreOptions and "
#~ "OpacAdvSearchMoreOptions.size > 0 and expanded_options ) or "
#~ "( OpacAdvSearchOptions and OpacAdvSearchOptions.size > 0 and not "
#~ "expanded_options ) %%] "


test plan updated
Comment 10 Victor Grousset/tuxayo 2021-06-23 05:08:17 UTC
Created attachment 122309 [details] [review]
Bug 28462: Remove TT tag on several lines - opac-advsearch.tt

It fixes the translation in case the operators have been translated

Test plan:
1. misc/translator/translate update pt-BR
2. In pt-BR-opac-bootstrap.po , find OpacAdvSearchMoreOptions
3. sabotage the translation part, like replacing IF with something else
4. misc/translator/translate install pt-BR
5. enable and use pt-BR and hit opac-advsearch.pl
6. Notice the error
   Template process failed: file error - parse error - /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/pt-BR/modules/opac-advsearch.tt line 409: unexpected token (e)                                                    [% IF (OpacAdvSearchMoreOptions and OpacAdvSearchMoreOptions.size> 0 e extended_options) ou (OpacAdvSearchOptions and OpacAdvSearchOptions.size> 0 e n expandido_options) %] at /kohadevbox/koha/C4/Templates.pm
7. Apply the patch and redo update and install of the language
8. Note that it's now working

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 11 Victor Grousset/tuxayo 2021-06-23 07:05:35 UTC
There are more cases of the issue.

git grep "\bIF\b" misc/translator/po | grep -v '#' | grep '\bpt-BR'

(just taking pt-BR as an example, any language would do)

Here is an example:
https://git.koha-community.org/Koha-community/Koha/src/commit/feaeff330cfa9d49c99a0d70da0df87f5316652b/koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc#L104

That seems to be all long lines split but not in an organized, aligned way.

1. So same fix as for the current patch right?
2. Shall I summit a patch for them here or in a followup?
3. The grep is just searching the "IF" and if we want to ban all multi-line TT tags there are other things to grep right?

(in discussion, so QA doesn't pick it yet, and might skip and forget it)
Comment 12 Jonathan Druart 2021-06-23 07:53:21 UTC
Created attachment 122311 [details] [review]
Bug 28462: Remove line breaks in TT tags

Same as previous patch for other files.
Looks like the problem exists only with IF.
Comment 13 Jonathan Druart 2021-06-23 07:54:40 UTC
Several runs of the following command did it for me:
  perl -p -i -e 's#(^\s*\[%[^%]*)\n#$1#gms' **/*.inc **/*.tt 

Ignoring sample.tt and country-list.inc.
Comment 14 Victor Grousset/tuxayo 2021-06-29 21:50:31 UTC
Created attachment 122498 [details] [review]
Bug 28462: Remove TT tag on several lines - opac-advsearch.tt

It fixes the translation in case the operators have been translated

Test plan:
1. misc/translator/translate update pt-BR
2. In pt-BR-opac-bootstrap.po , find OpacAdvSearchMoreOptions
3. sabotage the translation part, like replacing IF with something else
4. misc/translator/translate install pt-BR
5. enable and use pt-BR and hit opac-advsearch.pl
6. Notice the error
   Template process failed: file error - parse error - /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/pt-BR/modules/opac-advsearch.tt line 409: unexpected token (e)                                                    [% IF (OpacAdvSearchMoreOptions and OpacAdvSearchMoreOptions.size> 0 e extended_options) ou (OpacAdvSearchOptions and OpacAdvSearchOptions.size> 0 e n expandido_options) %] at /kohadevbox/koha/C4/Templates.pm
7. Apply the patch and redo update and install of the language
8. Note that it's now working

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 15 Victor Grousset/tuxayo 2021-06-29 21:50:36 UTC
Created attachment 122499 [details] [review]
Bug 28462: Remove line breaks in TT tags

Same as previous patch for other files.
Looks like the problem exists only with IF.

== test plan ==
1. See the problematic strings in a given language
     git grep "\bIF\b" misc/translator/po | grep -v '#' | grep '\bzh-Hant-TW'
2. update the language
   misc/translator/translate update zh-Hant-TW
3. Recheck the problematic strings, they aren't here anymore
4. Check the diff: git diff
   and search the if's with «/-.* IF » to see how the deleted strings
   look like and that it makes sense.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 16 Katrin Fischer 2021-07-05 20:14:23 UTC
Is this something we could check for in the QA test tools?
Comment 17 Katrin Fischer 2021-07-05 20:15:12 UTC
Created attachment 122589 [details] [review]
Bug 28462: Remove TT tag on several lines - opac-advsearch.tt

It fixes the translation in case the operators have been translated

Test plan:
1. misc/translator/translate update pt-BR
2. In pt-BR-opac-bootstrap.po , find OpacAdvSearchMoreOptions
3. sabotage the translation part, like replacing IF with something else
4. misc/translator/translate install pt-BR
5. enable and use pt-BR and hit opac-advsearch.pl
6. Notice the error
   Template process failed: file error - parse error - /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/pt-BR/modules/opac-advsearch.tt line 409: unexpected token (e)                                                    [% IF (OpacAdvSearchMoreOptions and OpacAdvSearchMoreOptions.size> 0 e extended_options) ou (OpacAdvSearchOptions and OpacAdvSearchOptions.size> 0 e n expandido_options) %] at /kohadevbox/koha/C4/Templates.pm
7. Apply the patch and redo update and install of the language
8. Note that it's now working

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2021-07-05 20:15:16 UTC
Created attachment 122590 [details] [review]
Bug 28462: Remove line breaks in TT tags

Same as previous patch for other files.
Looks like the problem exists only with IF.

== test plan ==
1. See the problematic strings in a given language
     git grep "\bIF\b" misc/translator/po | grep -v '#' | grep '\bzh-Hant-TW'
2. update the language
   misc/translator/translate update zh-Hant-TW
3. Recheck the problematic strings, they aren't here anymore
4. Check the diff: git diff
   and search the if's with «/-.* IF » to see how the deleted strings
   look like and that it makes sense.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Jonathan Druart 2021-07-06 09:05:57 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 20 Jonathan Druart 2021-07-06 10:15:26 UTC
(In reply to Katrin Fischer from comment #16)
> Is this something we could check for in the QA test tools?

We have one already
"""
  commit 594843834db69745d4bd0debc1dfb71433542500
  TT tags should not be defined on several lines
   
   That breaks our translate script
   See bug 18675.
"""
Comment 21 Kyle M Hall 2021-07-09 17:46:15 UTC
Pushed to 21.05.x for 21.05.02
Comment 22 Fridolin Somers 2021-08-02 20:38:33 UTC
Pushed to 20.11.x for 20.11.09
Comment 23 Victor Grousset/tuxayo 2021-08-16 18:24:08 UTC
Backported: Pushed to 20.05.x branch for 20.05.15
Comment 24 wainuiwitikapark 2021-08-17 00:26:31 UTC
Should this be backported to 19.11.x for 19.11.21?

Or if it contains string changes, backported to 19.11.22? As we are currently in a string freeze.
Comment 25 wainuiwitikapark 2021-09-07 01:44:38 UTC
Should this be backported to 19.11.x for 19.11.22?
Comment 26 Victor Grousset/tuxayo 2021-09-07 22:44:56 UTC
I would say the policy for oldoldoldstable should be the same as for oldoldstable, thus by default trying to backport any patches for bugs >= major
Comment 27 wainuiwitikapark 2021-09-08 02:23:50 UTC
Backported to 19.11.x for 19.11.22