Bug 41216

Summary: Resurrect tt_valid.t
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: julian.maurice, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 38720, 41217    
Bug Blocks:    
Attachments: Bug 41216: Restore tt_valid.t
Bug 41216: Try to fix by adding a space between TT tag and attribute
Bug 41216: Correctly fix the translation using I18N
Bug 41216: Restore tt_valid.t
Bug 41216: Try to fix by adding a space between TT tag and attribute
Bug 41216: Correctly fix the translation using I18N
Bug 41216: Correctly fix the translation using I18N
Bug 41216: Fix QA failures
Bug 41216: Fix failing test
Bug 41216: Remove confusion in test output
Bug 41216: Adjust POD
Bug 41216: Restore tt_valid.t
Bug 41216: Try to fix by adding a space between TT tag and attribute
Bug 41216: Correctly fix the translation using I18N
Bug 41216: Fix QA failures
Bug 41216: Fix failing test
Bug 41216: Remove confusion in test output
Bug 41216: Adjust POD
Bug 41216: (QA follow-up): fix path of xt/author/tt_valid.t in comment

Description Jonathan Druart 2025-11-07 12:06:58 UTC

    
Comment 1 Jonathan Druart 2025-11-07 12:59:01 UTC
Created attachment 189316 [details] [review]
Bug 41216: Restore tt_valid.t

HTML1 is gone. Almost...
The previous rule said that we could not have TT tags within HTML tags
because it caused problems with the translation.
It was incorrect.
Bug 38720 removed that rule, and stated that the only problematic use
was a IF with ">".
It was also incorrect.

This one bug states that we cannot have TT tags to construct HTML tags
IF one of the attribute contains a translatable string.
The list of translatable attributes are listed in tmpl_process3.pl.

This patch restores xt/tt_valid.t (and move it to xt/author) and adjust
slightly the test to take into account this last statement.

Test plan:
0. Apply only this first patch
1. Run xt/author/tt_valid.t
=> It fails on mana-subscription-search-result.inc
2. Update po for your prefered language
=> The string "This resource has been reported more than" is not in the
po files

3. Apply the second patch, repeat 1 and 2
=> It still fails: adding a space between the TT tag and the attribute is not enough here
=> The string is picked now! but... install the template and open the
translated version =>

4. Apply the third patch, repeat 1 and 2
=> The tests pass!
=> The string is still picked, install => The template looks good!
Now if you are familiar with Mana you can try to see the translated
string on the UI
Comment 2 Jonathan Druart 2025-11-07 12:59:03 UTC
Created attachment 189317 [details] [review]
Bug 41216: Try to fix by adding a space between TT tag and attribute
Comment 3 Jonathan Druart 2025-11-07 12:59:04 UTC
Created attachment 189318 [details] [review]
Bug 41216: Correctly fix the translation using I18N
Comment 4 Julian Maurice 2025-11-07 13:39:53 UTC
You added a singular version of the text but I don't think it makes sense here, as the warning is displayed only if tr_warning is > 5 (lowWarned)

Other than that, I followed the test plan and it works well.
Comment 5 Jonathan Druart 2025-11-07 14:08:36 UTC
(In reply to Julian Maurice from comment #4)
> You added a singular version of the text but I don't think it makes sense
> here, as the warning is displayed only if tr_warning is > 5 (lowWarned)
> 
> Other than that, I followed the test plan and it works well.

Yes but I don't know what else we can do. We still want to support languages with multiple plurals.
Comment 6 Julian Maurice 2025-11-07 14:10:10 UTC
(In reply to Jonathan Druart from comment #5)
> Yes but I don't know what else we can do. We still want to support languages
> with multiple plurals.

I forgot about that. So it does make sense, even if the singular version is not used
Comment 7 Julian Maurice 2025-11-07 14:11:58 UTC
Created attachment 189328 [details] [review]
Bug 41216: Restore tt_valid.t

HTML1 is gone. Almost...
The previous rule said that we could not have TT tags within HTML tags
because it caused problems with the translation.
It was incorrect.
Bug 38720 removed that rule, and stated that the only problematic use
was a IF with ">".
It was also incorrect.

This one bug states that we cannot have TT tags to construct HTML tags
IF one of the attribute contains a translatable string.
The list of translatable attributes are listed in tmpl_process3.pl.

This patch restores xt/tt_valid.t (and move it to xt/author) and adjust
slightly the test to take into account this last statement.

Test plan:
0. Apply only this first patch
1. Run xt/author/tt_valid.t
=> It fails on mana-subscription-search-result.inc
2. Update po for your prefered language
=> The string "This resource has been reported more than" is not in the
po files

3. Apply the second patch, repeat 1 and 2
=> It still fails: adding a space between the TT tag and the attribute is not enough here
=> The string is picked now! but... install the template and open the
translated version =>

4. Apply the third patch, repeat 1 and 2
=> The tests pass!
=> The string is still picked, install => The template looks good!
Now if you are familiar with Mana you can try to see the translated
string on the UI

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 8 Julian Maurice 2025-11-07 14:11:59 UTC
Created attachment 189329 [details] [review]
Bug 41216: Try to fix by adding a space between TT tag and attribute

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 9 Julian Maurice 2025-11-07 14:12:01 UTC
Created attachment 189330 [details] [review]
Bug 41216: Correctly fix the translation using I18N

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 10 Tomás Cohen Arazi (tcohen) 2025-12-01 12:43:17 UTC
Some are false positives (french words treated like english typos), but I think we need to address them.

[FAIL] koha-tmpl/intranet-tmpl/prog/en/includes/mana/mana-subscription-search-result.inc
   FAIL	  valid_template
		parse error - /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/includes/mana/mana-subscription-search-result.inc line 43: unexpected end of directive
  [% SET tr_title = I18N.tnx('This resource has already been reported', 'This resource has been reported more than {count} times, take care!', tr_warning, { count = tr_warning } %]
[WARN] t/db_dependent/misc/translator/xgettext.pl.t
   WARN	  spelling
		 attribut ==> attribute
		 attribut ==> attribute
[FAIL] xt/author/tt_valid.t
   FAIL	  file_permissions
		File must have the exec flag
   FAIL	  forbidden_patterns
		forbidden pattern: Use https:// instead of http:// for URLs (line 16)
   WARN	  spelling
		 attibutes ==> attributes
Comment 11 Jonathan Druart 2025-12-01 13:57:36 UTC
Created attachment 190046 [details] [review]
Bug 41216: Correctly fix the translation using I18N

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Amended-by: Add missing closing parenthesis
Comment 12 Jonathan Druart 2025-12-01 13:57:38 UTC
Created attachment 190047 [details] [review]
Bug 41216: Fix QA failures

Patch from commit 523c425
Comment 13 Jonathan Druart 2025-12-01 13:57:56 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #10)
> Some are false positives (french words treated like english typos), but I
> think we need to address them.

Oops, sorry about that!
Comment 14 Victor Grousset/tuxayo 2026-01-04 18:50:43 UTC
Thanks to keep refining this.


> => The string is picked now! but... install the template and open the
translated version =>

I see nothing suspicious. I'm on commit "Try to fix by adding a space between TT tag and attribute" (restarted all), switch language and I'm doing a search of reports in Mana.

I have in po files:
fr-FR-staff-prog.po
12632:msgid "This resource has been reported more than %s times, take care!"


Anything else to do to have the expected UI issues (how should the issues look?)
Comment 15 Jonathan Druart 2026-01-05 14:22:50 UTC
With only the first 2 patches, if you `koha-translate --update fr-FR --dev kohadev`

koha-tmpl/intranet-tmpl/prog/fr-FR/includes/mana/mana-subscription-search-result.inc

41  <tr %]="%]" %]class="[% tr_class | html %]" [%="[%" end="END" id="row[% subscription.id | html %]" if="IF" title="Cette ressource a été signalé plus de [% tr_warning | html %] fois, faites attention !" tr_cla    ss="tr_class" tr_warning="tr_warning">
Comment 16 Victor Grousset/tuxayo 2026-01-06 11:00:33 UTC
Ok, it's not about opening the page in the UI and it blows up.

I have the same template translated line as you. So back on track :)


-----

testing notes:

> 4. Apply the third patch, repeat 1 and 2
> => The tests pass!

It will still fail but it's because it picks the fr-FR versions. It's a false positive.

After installing the language, the test passes on mana-subscription-search-result.inc

But it still fails on koha-tmpl/opac-tmpl/bootstrap/fr-FR/modules/opac-detail.tt
Just pointing to in case that's suspicious.

-------------

> Now if you are familiar with Mana you can try to see the translated
> string on the UI


Since it's using I18N, is there and additional step to see a translated string?
- gulp po:update --lang fr-FR
- find the string in fr-FR-messages.po and fill the translated part (ensure it's not fuzzy)
- misc/translator/translate install fr-FR
- restart_all

But the translated template still has the original string >_<
(while the rest is using the existing translations)
Comment 17 Jonathan Druart 2026-01-06 12:26:49 UTC
(In reply to Victor Grousset/tuxayo from comment #16)
> Ok, it's not about opening the page in the UI and it blows up.
> 
> I have the same template translated line as you. So back on track :)
> 
> 
> -----
> 
> testing notes:
> 
> > 4. Apply the third patch, repeat 1 and 2
> > => The tests pass!
> 
> It will still fail but it's because it picks the fr-FR versions. It's a
> false positive.
> 
> After installing the language, the test passes on
> mana-subscription-search-result.inc
> 
> But it still fails on
> koha-tmpl/opac-tmpl/bootstrap/fr-FR/modules/opac-detail.tt
> Just pointing to in case that's suspicious.

Hum? The test does not process files outside of the git repo, fr-FR files should not be tested.


> > Now if you are familiar with Mana you can try to see the translated
> > string on the UI
> 
> 
> Since it's using I18N, is there and additional step to see a translated
> string?
> - gulp po:update --lang fr-FR
> - find the string in fr-FR-messages.po and fill the translated part (ensure
> it's not fuzzy)
> - misc/translator/translate install fr-FR
> - restart_all
> 
> But the translated template still has the original string >_<
> (while the rest is using the existing translations)

The translation is done on the fly/at runtime, you won't see the translated version in the template.
Comment 18 Victor Grousset/tuxayo 2026-02-06 23:33:12 UTC
> The translation is done on the fly/at runtime, you won't see the translated version in the template.

Thanks Jonathan. I managed to display the string and the translation works :)

In the mean time, a file was introduced that make the test fail:
#   Failed test at xt/author/tt_valid.t line 93.
#          got: '1'
#     expected: '0'
# t/lib/plugins/Koha/Plugin/TestValuebuilder/test_valuebuilder_popup.tt: 8

content of the file:
=================
<!DOCTYPE html>
<!-- prettier-ignore-start -->
<html>
    <head>
        <title>[% plugin_name | html %] - Valuebuilder</title>
        <meta charset="UTF-8" />
    </head>
    <body>
        <h1>[% plugin_name | html %]</h1>
        <p>This is a test valuebuilder popup for field: [% field_id | html %]</p>

        <form>
            <label for="test_value">Enter a value:</label>
            <input type="text" id="test_value" value="Test Value from Plugin" />
[...]
=================


hmmm, where would the "we cannot have TT tags to construct HTML tags IF one of the attribute contains a translatable string." be?


----------


I think this part of the POD is obsolete:
```
[1] Test if TT files contain TT directive within HTML tag. For example:

  <li[% IF

This kind of construction MUST be avoided because it breaks Koha translation
process.
```
Comment 19 Jonathan Druart 2026-02-09 12:01:21 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> > The translation is done on the fly/at runtime, you won't see the translated version in the template.
> 
> Thanks Jonathan. I managed to display the string and the translation works :)

Thanks for the comment.

> In the mean time, a file was introduced that make the test fail:
> #   Failed test at xt/author/tt_valid.t line 93.
> #          got: '1'
> #     expected: '0'
> # t/lib/plugins/Koha/Plugin/TestValuebuilder/test_valuebuilder_popup.tt: 8

Fixed, it was the missing id and class on body.

> I think this part of the POD is obsolete:
> ```
> [1] Test if TT files contain TT directive within HTML tag. For example:
> 
>   <li[% IF
> 
> This kind of construction MUST be avoided because it breaks Koha translation
> process.
> ```

Adjusted.
Comment 20 Jonathan Druart 2026-02-09 12:03:01 UTC
Created attachment 192744 [details] [review]
Bug 41216: Fix failing test

Patch from commit 00348d4
Comment 21 Jonathan Druart 2026-02-09 12:03:02 UTC
Created attachment 192745 [details] [review]
Bug 41216: Remove confusion in test output

Clearly show the check's description in the test's failure
Comment 22 Jonathan Druart 2026-02-09 12:03:04 UTC
Created attachment 192746 [details] [review]
Bug 41216: Adjust POD

Patch from commit 1ec69a6
Comment 23 Victor Grousset/tuxayo 2026-02-11 04:27:18 UTC
Created attachment 192955 [details] [review]
Bug 41216: Restore tt_valid.t

HTML1 is gone. Almost...
The previous rule said that we could not have TT tags within HTML tags
because it caused problems with the translation.
It was incorrect.
Bug 38720 removed that rule, and stated that the only problematic use
was a IF with ">".
It was also incorrect.

This one bug states that we cannot have TT tags to construct HTML tags
IF one of the attribute contains a translatable string.
The list of translatable attributes are listed in tmpl_process3.pl.

This patch restores xt/tt_valid.t (and move it to xt/author) and adjust
slightly the test to take into account this last statement.

Test plan:
0. Apply only this first patch
1. Run xt/author/tt_valid.t
=> It fails on mana-subscription-search-result.inc
2. Update po for your prefered language
=> The string "This resource has been reported more than" is not in the
po files

3. Apply the second patch, repeat 1 and 2
=> It still fails: adding a space between the TT tag and the attribute is not enough here
=> The string is picked now! but... install the template and open the
translated version =>

4. Apply the third patch, repeat 1 and 2
=> The tests pass!
=> The string is still picked, install => The template looks good!
Now if you are familiar with Mana you can try to see the translated
string on the UI

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 24 Victor Grousset/tuxayo 2026-02-11 04:27:20 UTC
Created attachment 192956 [details] [review]
Bug 41216: Try to fix by adding a space between TT tag and attribute

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 25 Victor Grousset/tuxayo 2026-02-11 04:27:23 UTC
Created attachment 192957 [details] [review]
Bug 41216: Correctly fix the translation using I18N

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Amended-by: Add missing closing parenthesis
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 26 Victor Grousset/tuxayo 2026-02-11 04:27:25 UTC
Created attachment 192958 [details] [review]
Bug 41216: Fix QA failures

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 27 Victor Grousset/tuxayo 2026-02-11 04:27:27 UTC
Created attachment 192959 [details] [review]
Bug 41216: Fix failing test

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 28 Victor Grousset/tuxayo 2026-02-11 04:27:30 UTC
Created attachment 192960 [details] [review]
Bug 41216: Remove confusion in test output

Clearly show the check's description in the test's failure

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 29 Victor Grousset/tuxayo 2026-02-11 04:27:32 UTC
Created attachment 192961 [details] [review]
Bug 41216: Adjust POD

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 30 Victor Grousset/tuxayo 2026-02-11 04:27:34 UTC
Created attachment 192962 [details] [review]
Bug 41216: (QA follow-up): fix path of xt/author/tt_valid.t in comment

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