Bug 24661 - Inclusion of locale-related javascript files is broken
Summary: Inclusion of locale-related javascript files is broken
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 21156
Blocks: 24662
  Show dependency treegraph
 
Reported: 2020-02-14 08:57 UTC by Julian Maurice
Modified: 2020-02-25 14:29 UTC (History)
6 users (show)

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


Attachments
Bug 24661: Fix inclusion of locale-related javascript files (1.54 KB, patch)
2020-02-14 09:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 24661: Fix inclusion of locale-related javascript files (1.62 KB, patch)
2020-02-14 11:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24661: (RM follow-up) Check lang is defined (1.09 KB, patch)
2020-02-25 14:29 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2020-02-14 08:57:04 UTC
Bug 21156 adds 2 js files (Gettext.js, i18n.js) + 1 generated js file (locale_data.js). On master, their URL are broken and the files are not loaded.

Steps to reproduce:
1. Checkout master
2. Open Firefox/Chrome devtools, network tab, and go to any page in the staff interface (except the main page)
3. You should see 3 404 errors for the files mentioned above.
Comment 1 Julian Maurice 2020-02-14 09:03:57 UTC
Created attachment 98913 [details] [review]
Bug 24661: Fix inclusion of locale-related javascript files

Bug 21156 adds 2 js files (Gettext.js, i18n.js) + 1 generated js file
(locale_data.js). Their URL are broken and the files are not loaded.

Use Template::Toolkit plugin Asset to include these files, and exclude
locale_data.js when language is english, because obviously translations
are not needed in this case.

Test plan:
1. Just make sure the files mentioned above are loaded correctly when in
   English and in other languages as well
Comment 2 Martin Renvoize 2020-02-14 11:56:35 UTC
Created attachment 98930 [details] [review]
Bug 24661: Fix inclusion of locale-related javascript files

2 js files (Gettext.js, i18n.js) + 1 generated js file (locale_data.js)
were added by bug 21156. Their URL are broken and the files are not loaded.

Use Template::Toolkit plugin Asset to include these files, and exclude
locale_data.js when language is english, because obviously translations
are not needed in this case.

Test plan:
1. Just make sure the files mentioned above are loaded correctly when in
   English and in other languages as well

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2020-02-14 11:57:00 UTC
Thanks for the quick fix Julian.. works as expected.. going for a fast QA on this one!
Comment 4 Martin Renvoize 2020-02-14 11:57:34 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 5 Bernardo Gonzalez Kriegel 2020-02-14 12:29:47 UTC
(In reply to Martin Renvoize from comment #4)
> Nice work everyone!
> 
> Pushed to master for 20.05

Too fast! I was about to sign this :)

Anyway, I found the following, not related with this bug, that pops doing a language update (also create and install) to do the testing:

koha-tmpl/intranet-tmpl/prog/js/checkouts.js:569: warning: unterminated string
koha-tmpl/intranet-tmpl/prog/js/checkouts.js:858: warning: unterminated string
koha-tmpl/intranet-tmpl/prog/js/checkouts.js:904: warning: unterminated string
koha-tmpl/intranet-tmpl/prog/js/checkouts.js:911: warning: unterminated string
koha-tmpl/intranet-tmpl/prog/js/checkouts.js:1095: warning: RegExp literal terminated too early
Comment 6 Julian Maurice 2020-02-14 12:55:03 UTC
It looks like xgettext does not like ES6 template literals.

https://savannah.gnu.org/bugs/?50920

From what I understand, support for template literals was added in gettext 0.20 (still not packaged in debian) but is still buggy in latest released version 0.20.1 and a fix is present in master.
Comment 7 Bernardo Gonzalez Kriegel 2020-02-14 13:19:07 UTC
(In reply to Julian Maurice from comment #6)
> It looks like xgettext does not like ES6 template literals.
> 
> https://savannah.gnu.org/bugs/?50920
> 
> From what I understand, support for template literals was added in gettext
> 0.20 (still not packaged in debian) but is still buggy in latest released
> version 0.20.1 and a fix is present in master.

Then we have a little problem with that. Current (19.10/18.04) ubuntu has 0.19.8.1
Comment 8 Julian Maurice 2020-02-14 13:31:19 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #7)
> Then we have a little problem with that. Current (19.10/18.04) ubuntu has
> 0.19.8.1

Yes, same version in Debian unstable, so we're stuck with this version for some time.

One possible workaround is to declare translatable strings as soon as possible in the file (before the first template literal), since xgettext parse these strings correctly.
Comment 9 Marcel de Rooy 2020-02-21 07:59:08 UTC
Warning from qa tools:
File not found : /js/locale_data.js at /usr/share/koha/Koha/Template/Plugin/Asset.pm line 84.

koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-close.inc:    [% Asset.js(lang _ '/js/locale_data.js') | $raw %]
Cannot find this file. Not sure if this construction works in template toolkit btw.
Comment 10 Marcel de Rooy 2020-02-21 08:20:57 UTC
(In reply to Marcel de Rooy from comment #9)
> Warning from qa tools:
> File not found : /js/locale_data.js at
> /usr/share/koha/Koha/Template/Plugin/Asset.pm line 84.
> 
> koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-close.inc:    [%
> Asset.js(lang _ '/js/locale_data.js') | $raw %]
> Cannot find this file. Not sure if this construction works in template
> toolkit btw.

The construction is fine btw. Not sure why qa tools doesnt like it..
Comment 11 Julian Maurice 2020-02-21 08:31:06 UTC
> The construction is fine btw. Not sure why qa tools doesnt like it..

Probably because lang is not defined so it looks for absolute path /js/locale_data.js
Not sure why qa tools has to process templates.
Comment 12 Jonathan Druart 2020-02-21 09:46:55 UTC
(In reply to Julian Maurice from comment #11)
> > The construction is fine btw. Not sure why qa tools doesnt like it..
> 
> Probably because lang is not defined so it looks for absolute path
> /js/locale_data.js
> Not sure why qa tools has to process templates.

It's coming from xt/tt_valid.t (or similar code from the qa script)
Comment 13 Jonathan Druart 2020-02-25 11:49:57 UTC
Jenkins has a lot of those lines, coming from xt/author/translatable-templates.t, I do not get the warnings when run locally however, which is a bit weird.

https://jenkins.koha-community.org/job/Koha_Master_D9/1172/consoleFull

What about the following changes?

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-close.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-close.inc
index 1ad6197288..94f86ad2f6 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-close.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-close.inc
@@ -26,6 +26,7 @@
 var Koha = {};
 </script>

+[% SET lang = 'en' UNLESS lang.defined %]
 [% IF lang != 'en' %]
     [% Asset.js(lang _ '/js/locale_data.js') | $raw %]
 [% END %]
Comment 14 Jonathan Druart 2020-02-25 11:50:30 UTC
Or even

[% IF lang.defined AND lang != 'en' %]
Comment 15 Julian Maurice 2020-02-25 11:56:43 UTC
(In reply to Jonathan Druart from comment #14)
> Or even
> 
> [% IF lang.defined AND lang != 'en' %]

I never used the .defined VMethod of Template::Toolkit, but it should be ok. I'm not sure it is even needed. I think you can simply write

[% IF lang && lang != 'en' %]

Since no language code can evaluate to false
Comment 16 Martin Renvoize 2020-02-25 13:58:10 UTC
(In reply to Julian Maurice from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > Or even
> > 
> > [% IF lang.defined AND lang != 'en' %]
> 
> I never used the .defined VMethod of Template::Toolkit, but it should be ok.
> I'm not sure it is even needed. I think you can simply write
> 
> [% IF lang && lang != 'en' %]
> 
> Since no language code can evaluate to false

Looks good to me, nice and terse.
Comment 17 Martin Renvoize 2020-02-25 13:59:12 UTC
(In reply to Julian Maurice from comment #6)
> It looks like xgettext does not like ES6 template literals.
> 
> https://savannah.gnu.org/bugs/?50920
> 
> From what I understand, support for template literals was added in gettext
> 0.20 (still not packaged in debian) but is still buggy in latest released
> version 0.20.1 and a fix is present in master.

Hmm.. do we need to discuss this bit a bit further...  are you suggesting a coding guideline fix or am I misunderstanding?
Comment 18 Jonathan Druart 2020-02-25 14:20:14 UTC
(In reply to Martin Renvoize from comment #17)
> (In reply to Julian Maurice from comment #6)
> > It looks like xgettext does not like ES6 template literals.
> > 
> > https://savannah.gnu.org/bugs/?50920
> > 
> > From what I understand, support for template literals was added in gettext
> > 0.20 (still not packaged in debian) but is still buggy in latest released
> > version 0.20.1 and a fix is present in master.
> 
> Hmm.. do we need to discuss this bit a bit further...  are you suggesting a
> coding guideline fix or am I misunderstanding?

See bug 24725.
Comment 19 Martin Renvoize 2020-02-25 14:25:43 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Martin Renvoize from comment #17)
> > (In reply to Julian Maurice from comment #6)
> > > It looks like xgettext does not like ES6 template literals.
> > > 
> > > https://savannah.gnu.org/bugs/?50920
> > > 
> > > From what I understand, support for template literals was added in gettext
> > > 0.20 (still not packaged in debian) but is still buggy in latest released
> > > version 0.20.1 and a fix is present in master.
> > 
> > Hmm.. do we need to discuss this bit a bit further...  are you suggesting a
> > coding guideline fix or am I misunderstanding?
> 
> See bug 24725.

Awesome, thanks Jonathan :)
Comment 20 Martin Renvoize 2020-02-25 14:29:57 UTC
Created attachment 99586 [details] [review]
Bug 24661: (RM follow-up) Check lang is defined

This adds a check that the lang variable is defined before testing
against it. In normal operation this is likely a no-op, but it is
required for compiling templates during QA.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>