Bug 30991

Summary: [% ELSE %]0[% END %] will break translations if used for assigning variables
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: I18N/L10NAssignee: Alex Buckley <alexbuckley>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: alexbuckley, arthur.suzuki, f.demians, jonathan.druart, lucas
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30958
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30569
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31563
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.04
Attachments: Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates
Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates
Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates

Description Katrin Fischer 2022-06-17 20:49:04 UTC
See bug 30958 and bug 23713:

The construct with the 0 will break on translation as it's removed in the translated templates:

> The problematic line (below) in the English template is:
> var OD_password_required = [% IF
> Koha.Preference('OverDrivePasswordRequired') %]1[% ELSE %]0[% END %];
> 
> Which is translated to the following in the en-NZ (and other languages)
> template:
> var OD_password_required = [% IF
> Koha.Preference('OverDrivePasswordRequired') %]1[% ELSE %][% END %];

We should verify that there are not more occurences of the bad pattern or improve our translation scripts:

(In reply to Owen Leonard from comment #3)
> There are a couple of other instances that I saw which should also be
> addressed. I searched the NZ templates for "1[% ELSE %][% END %]"
Comment 1 Alex Buckley 2022-06-22 10:05:37 UTC
Created attachment 136408 [details] [review]
Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates

The construct of [% ELSE %]0[% END %] breaks translations as it is
translated as [% ELSE %][% END %]. Note: No 0 in the ELSE statement.

This patchset either removes occurances of a lone 0 in template ELSE
statements, or splits it over multiple lines so the 0 is not removed in
the translated templates.

Test plan:
1. Install the en-NZ translation
2. Search the translated templates for '[% ELSE %][% END %]' and confirm
there are are instances of that
3. Apply patch
4. Update your en-NZ translation
5. Repeat step 2 and confirm there are no more instances of [% ELSE %][%
END %] in the translated templates

Note: I removed the [% ELSE %] statement from opac-bottom.inc as that
statement was empty in the en translation so it didn't look to be
needed.

Sponsored-by: Catalyst IT, New Zealand
Comment 2 Lucas Gass 2022-06-22 20:37:35 UTC
Created attachment 136429 [details] [review]
Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates

The construct of [% ELSE %]0[% END %] breaks translations as it is
translated as [% ELSE %][% END %]. Note: No 0 in the ELSE statement.

This patchset either removes occurances of a lone 0 in template ELSE
statements, or splits it over multiple lines so the 0 is not removed in
the translated templates.

Test plan:
1. Install the en-NZ translation
2. Search the translated templates for '[% ELSE %][% END %]' and confirm
there are are instances of that
3. Apply patch
4. Update your en-NZ translation
5. Repeat step 2 and confirm there are no more instances of [% ELSE %][%
END %] in the translated templates

Note: I removed the [% ELSE %] statement from opac-bottom.inc as that
statement was empty in the en translation so it didn't look to be
needed.

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 3 Katrin Fischer 2022-06-25 15:59:51 UTC
Created attachment 136554 [details] [review]
Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates

The construct of [% ELSE %]0[% END %] breaks translations as it is
translated as [% ELSE %][% END %]. Note: No 0 in the ELSE statement.

This patchset either removes occurances of a lone 0 in template ELSE
statements, or splits it over multiple lines so the 0 is not removed in
the translated templates.

Test plan:
1. Install the en-NZ translation
2. Search the translated templates for '[% ELSE %][% END %]' and confirm
there are are instances of that
3. Apply patch
4. Update your en-NZ translation
5. Repeat step 2 and confirm there are no more instances of [% ELSE %][%
END %] in the translated templates

Note: I removed the [% ELSE %] statement from opac-bottom.inc as that
statement was empty in the en translation so it didn't look to be
needed.

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 4 Tomás Cohen Arazi 2022-06-25 18:31:33 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 5 Lucas Gass 2022-07-29 16:25:23 UTC
Backported to 22.05.x for 22.05.04
Comment 6 Arthur Suzuki 2022-08-11 12:47:57 UTC
conflicts when trying to apply to 21.11.x. please provide backport if needed.
Comment 7 Jonathan Druart 2022-11-25 07:23:39 UTC
I do understand what we are doing on bug 23713.

But here we are simply splitting the rule on different lines. That is not fixing the problem in the long term.
Comment 8 Jonathan Druart 2022-11-25 07:25:22 UTC
I mean, it's not solid for future changes, another author can put the statement back on one line.
Comment 9 Katrin Fischer 2022-12-04 12:53:23 UTC
(In reply to Jonathan Druart from comment #8)
> I mean, it's not solid for future changes, another author can put the
> statement back on one line.

Maybe this would be a little more solid?

-    var OD_password_required = [% IF Koha.Preference('OverDrivePasswordRequired') %]1[% ELSE %]0[% END %];
+    [%- IF Koha.Preference('OverDrivePasswordRequired') -%]
+        var OD_password_required = 1;
+    [%- ELSE -%]
+        var OD_password_required = 0;
+    [%- END -%]

I have an open issue with those 0 too: 
Bug 31563 - Numbers on claims tab not showing in translated templates
Comment 10 David Cook 2023-02-10 03:34:53 UTC
Comment on attachment 136554 [details] [review]
Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates

Review of attachment 136554 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
@@ +1307,5 @@
> +    [%- IF Koha.Preference('OverDrivePasswordRequired') -%]
> +        var OD_password_required = 1;
> +    [%- ELSE -%]
> +        var OD_password_required = 0;
> +    [%- END -%]

I know it's after the fact, but would something like the following be a viable alternative with the translation tools?

var OD_password_required = [% IF Koha.Preference('OverDrivePasswordRequired'); '1'; ELSE; '0'; END; %];