Bug 30991 - [% ELSE %]0[% END %] will break translations if used for assigning variables
Summary: [% ELSE %]0[% END %] will break translations if used for assigning variables
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-17 20:49 UTC by Katrin Fischer
Modified: 2023-12-28 20:44 UTC (History)
5 users (show)

See Also:
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 (7.79 KB, patch)
2022-06-22 10:05 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates (7.84 KB, patch)
2022-06-22 20:37 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 30991: Fix remaining instances of [% ELSE %]0[% END %] in templates (7.84 KB, patch)
2022-06-25 15:59 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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; %];