Bug 8942 - Translation process breaks javascript in calendar.inc
Summary: Translation process breaks javascript in calendar.inc
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: translate.koha-community.org (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Frédéric Demians
QA Contact: Marcel de Rooy
URL:
Keywords:
: 9152 (view as bug list)
Depends on:
Blocks: 9555
  Show dependency treegraph
 
Reported: 2012-10-18 10:41 UTC by Fridolin Somers
Modified: 2014-05-26 21:04 UTC (History)
9 users (show)

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


Attachments
Bug 8942 Fix translation of JS strings (1.51 KB, patch)
2012-10-18 11:06 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 8942 Fix translation of JS strings (1.51 KB, patch)
2012-10-18 11:09 UTC, Frédéric Demians
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8942 Fix translation of JS strings containing simple/double quote (1.60 KB, patch)
2012-10-19 12:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Proposed patch (1.46 KB, patch)
2012-10-19 13:04 UTC, Frédéric Demians
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8942 Fix translation of JS strings containing simple/double quote (1.69 KB, patch)
2012-10-31 18:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Always use double quotes : main patch (42.61 KB, patch)
2012-11-29 09:13 UTC, Fridolin Somers
Details | Diff | Splinter Review
Always use double quotes : follow-up 1 (2.46 KB, patch)
2012-11-29 09:19 UTC, Fridolin Somers
Details | Diff | Splinter Review
Always use double quotes : follow-up 2 (10.27 KB, patch)
2012-11-29 09:23 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8942: Translation process breaks javascript (41.19 KB, patch)
2012-12-31 14:29 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8942: Translation process breaks javascript (followup 1) (2.61 KB, patch)
2012-12-31 14:35 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8942: Translation process breaks javascript (followup 2) (10.48 KB, patch)
2012-12-31 14:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 8942: Translation process breaks javascript (40.76 KB, patch)
2013-01-30 06:57 UTC, Mason James
Details | Diff | Splinter Review
Bug 8942: Translation process breaks javascript (followup 1) (2.67 KB, patch)
2013-01-30 06:57 UTC, Mason James
Details | Diff | Splinter Review
Bug 8942: Translation process breaks javascript (followup 2) (10.54 KB, patch)
2013-01-30 06:58 UTC, Mason James
Details | Diff | Splinter Review
Bug 8942 : Adding a test to make sure we don't use single quotes for js (1.87 KB, patch)
2013-02-02 07:03 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8942 : Adding a test to make sure we don't use single quotes for js (2.06 KB, patch)
2013-02-05 15:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 8942 : Adding a test to make sure we don't use single quotes for js (2.01 KB, patch)
2013-02-19 08:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
[3.10.x] Bug 8942: Translation process breaks javascript (40.65 KB, patch)
2013-03-29 14:34 UTC, Fridolin Somers
Details | Diff | Splinter Review
[3.10.x] Bug 8942: Translation process breaks javascript (followup 1) (2.63 KB, patch)
2013-03-29 14:35 UTC, Fridolin Somers
Details | Diff | Splinter Review
[3.10.x] Bug 8942: Translation process breaks javascript (followup 2) (10.57 KB, patch)
2013-03-29 14:35 UTC, Fridolin Somers
Details | Diff | Splinter Review
[3.8.x] Bug 8942: Translation process breaks javascript (32.55 KB, patch)
2013-03-29 14:36 UTC, Fridolin Somers
Details | Diff | Splinter Review
[3.8.x] Bug 8942: Translation process breaks javascript (followup 1) (2.63 KB, patch)
2013-03-29 14:36 UTC, Fridolin Somers
Details | Diff | Splinter Review
[3.8.x] Bug 8942: Translation process breaks javascript (followup 2) (9.13 KB, patch)
2013-03-29 14:36 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2012-10-18 10:41:25 UTC
In french translation of calendar.inc :
Because (line 31) :
    currentText: _('Today'),
is translated
    currentText: _('Aujourd'hui'),
additionnal quote brakes the Javascript.

This provokes the use of american format dates in calendar pop-up.

I had a discussion on translate list, the way chosen to repair it is to use HTML escape char sequence "'" instead of single quote in translated strings.
Same thing for double quotes, &, < and >.

So I will correct on translate site but one can follow the resolution on this bug.
Comment 1 Frédéric Demians 2012-10-18 11:06:53 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2012-10-18 11:09:13 UTC Comment hidden (obsolete)
Comment 3 Fridolin Somers 2012-10-18 12:21:55 UTC
Is there not a risk to convert quotes with the fact that TT elements are translated (they are %s) ?
Comment 4 Frédéric Demians 2012-10-18 13:12:43 UTC
> Is there not a risk to convert quotes with the fact that TT elements are
> translated (they are %s) ?

I don't think so. This patch transform only strings coming from JS. I've done a grep -R "&apos" and "&quot" in fr-FR opac & staff templates directory, and I didn't find anything wrong. You can check yourself after applying this patch, and regenerating localized templates.
Comment 5 Kyle M Hall 2012-10-19 12:16:03 UTC
This patch isn't working for me. If I set my language to fr-FR, I still get my dates in MM/DD/YYYY no matter what setting I have for dateformat.
Comment 6 Kyle M Hall 2012-10-19 12:19:09 UTC
Cancel that. I'm stupid ; )

I wasn't thinking about the fact that this is a modification to the translation generator, and that I needed to regenerate my French translations before testing!
Comment 7 Kyle M Hall 2012-10-19 12:19:41 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2012-10-19 12:46:07 UTC
Hi Kyle,

as I said on the mailing list, I am a bit worried the script might translate ' that shoudl not be replaced. Did you check the updated po files for differences? I think maybe we could start comparing a po file with not so many ' (not French ;) ) before and after the patch to find strings where this might be causing problems. I am thinking about program code that shows up in the po files by mistake.
Comment 9 Frédéric Demians 2012-10-19 12:54:26 UTC
> as I said on the mailing list, I am a bit worried the script might
> translate ' that shoudl not be replaced. Did you check the updated po
> files for differences? I think maybe we could start comparing a po
> file with not so many ' (not French ;) ) before and after the patch to
> find strings where this might be causing problems. I am thinking about
> program code that shows up in the po files by mistake.

Well, indeed, you're right to be worried. Replacing simple quote (')
with &apos; seems not to work so well. It seems to be displayed has it
is: the code &apos; not the '.  I don't understand how it worked for
Fridolyn. But the issue is here. And not only in calendar, also in
biblio records merging. I will propose another patch with a test plan.
Katrin, the patch doesn't touch the .po file. It modifies spefically how
the JavaScript translated strings is sent back for inclusion in
templates.
Comment 10 Fridolin Somers 2012-10-19 12:59:13 UTC
(In reply to comment #9)
> is: the code &apos; not the '.  I don't understand how it worked for Fridolyn.

I modified PO files to use &apos; in translated strings :
msgid:"Today"
msgid:"Aujourd&apos;hui"

I will correct it french POs but I encourage you to work on this bugfix.
Comment 11 Frédéric Demians 2012-10-19 13:04:17 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2012-10-19 13:50:42 UTC
Hi Frederic, ah you are right, I made a mistake here. Your second approach limiting the change to Javascript output sounds more secure to me. Thank you!
Comment 13 Owen Leonard 2012-10-31 18:00:09 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2012-11-07 14:33:28 UTC
QA Comment:
Single quote seems to work (but see note below). Double quote does not work.

If I translate:
msgid "Your lists:"
msgstr "Uw "lijsten:"

It results in: 
param1 += "<optgroup label=\""+_("Uw \"lijsten:")+"\">";[% FOREACH addbarshelvesloo IN addbarshelvesloop %]
(Example from opac-results.tt within document ready function section. )

OPAC displays only the word Uw. But does not show \"lijsten.

NOTE:
If I would use already a backslash to escape my quotes in the PO file like this:
msgstr "Uw \'lijsten:"

I would not like to see the backslash in the display.
In other words: Could you improve the regex with lookbehind to prevent adding a backslash that is already there?

Changing status to reflect need for attention.
Comment 15 Jonathan Druart 2012-11-28 10:52:42 UTC
(In reply to comment #14)
> QA Comment:
> Single quote seems to work (but see note below). Double quote does not work.
> 
> If I translate:
> msgid "Your lists:"
> msgstr "Uw "lijsten:"

Hi Marcel,
To me, your example is not good.
You cannot translate "Your lists:" into "Uw "lijsten:", the po file become wrongly formatted.
Have you an example with a correctly formatted po file ?
Comment 16 Fridolin Somers 2012-11-29 09:10:52 UTC
After reflexion, I think it is no good idea to have an automatic quote escape.

For example, if you have a single quote in a double quote string (or opposite) : 
_("Aujourd'hui") will become _("Aujourd\'hui"), the backslash will be displayed.

Also, like Marcel example, there might be problems with HTML added in JS.
For example : 
  $("#id").html("<span title=\"" + _("Dangerous") + "\" />")
will generate
  <span title="Dangerous" />
In this case, the translation must not contain a double quotes, even escaped: 
  $("#id").html("<span title=\"" + _("\"Danger\"") + "\" />")
will generate
  <span title=""Danger"" />, bad HTML.

I've looked at http://wiki.koha-community.org/wiki/Coding_Guidelines.
In JavaScript section, double quotes are always used : _("xxx").
Looking at code, double quotes are used 2629 time and single quotes only 143 times.

So if we always use double quotes, single quotes are not a problem anymore and it was the main problem (it is often used in French).
I've looked at French PO, double quotes are actually never used in JavaScript translation ; it can be managed one by one.

I've looked at old version of calendar, it used double quotes, that's why there was no problem with Today => Aujourd'hui.

I will propose a patch for that.
Comment 17 Fridolin Somers 2012-11-29 09:13:54 UTC Comment hidden (obsolete)
Comment 18 Fridolin Somers 2012-11-29 09:19:04 UTC Comment hidden (obsolete)
Comment 19 Fridolin Somers 2012-11-29 09:23:10 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2012-12-06 11:58:09 UTC
Frederic,
How do you feel about Fridolyn's counterpatches ? 

Just an additional thought: If we resolve all quotes now, how do we prevent them being added again the wrong way in the future? Unit test?
Comment 21 Jonathan Druart 2012-12-10 09:50:49 UTC
(In reply to comment #20)

> Just an additional thought: If we resolve all quotes now, how do we prevent
> them being added again the wrong way in the future? Unit test?

Hi Marcel,
If this rule is accepted, it would be possible to add a regexp in our qa tools in order to catch all simple-quoted strings added in the js files.
Comment 22 Marcel de Rooy 2012-12-10 09:54:32 UTC
(In reply to comment #21)
> Hi Marcel,
> If this rule is accepted, it would be possible to add a regexp in our qa
> tools in order to catch all simple-quoted strings added in the js files.

That would be good. But an automated test would still be safe too..
Comment 23 Katrin Fischer 2012-12-10 09:56:22 UTC
I think an automated test would be preferrable, or have both.
Comment 24 Chris Cormack 2012-12-10 21:41:09 UTC
I agree +1 for a test.
Comment 25 Fridolin Somers 2012-12-11 08:31:38 UTC
(In reply to comment #24)
> I agree +1 for a test.

Can someone help me on that? I have no idea how it works.

My patches seems to be acceptable, so I change status to needs signoff.
Comment 26 Chris Cormack 2012-12-29 09:18:15 UTC
If someone signs off on Fridolyn's patches and if they work fine I'm happy to write a test in xt/ to test no we don't let single quotes sneak back in
Comment 27 Owen Leonard 2012-12-31 14:29:18 UTC Comment hidden (obsolete)
Comment 28 Owen Leonard 2012-12-31 14:35:06 UTC Comment hidden (obsolete)
Comment 29 Owen Leonard 2012-12-31 14:50:58 UTC Comment hidden (obsolete)
Comment 30 Fridolin Somers 2013-01-02 15:26:51 UTC
Thanks a lot for testing Owen.
Comment 31 Fridolin Somers 2013-01-25 15:11:10 UTC
(In reply to comment #26)
> If someone signs off on Fridolyn's patches and if they work fine I'm happy
> to write a test in xt/ to test no we don't let single quotes sneak back in

Patches are signed-off.
Comment 32 Mason James 2013-01-30 06:57:25 UTC Comment hidden (obsolete)
Comment 33 Mason James 2013-01-30 06:57:48 UTC Comment hidden (obsolete)
Comment 34 Mason James 2013-01-30 06:58:07 UTC Comment hidden (obsolete)
Comment 35 Mason James 2013-01-30 07:00:30 UTC
(In reply to comment #34)
> Created attachment 14936 [details] [review]
> Bug 8942: Translation process breaks javascript (followup 2)
> 
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
> 
> Tested all changes. Functionality is unchanged. Thanks for the taking
> the extra effort to move alert strings into variables.
> Signed-off-by: Mason James <mtj@kohaaloha.com>

testing 3 commit(s) (applied to f8686e9 'Bug 9116: Remove some useless tmp var')

passing QA, works well...

nice patch everyone :)
Comment 36 Jared Camins-Esakov 2013-01-31 16:00:25 UTC
This patch has been pushed to master.
Comment 37 Chris Cormack 2013-02-02 07:03:46 UTC Comment hidden (obsolete)
Comment 38 Owen Leonard 2013-02-05 15:48:00 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2013-02-06 15:29:12 UTC
(In reply to comment #37)
> Created attachment 15021 [details] [review]
> Bug 8942 : Adding a test to make sure we don't use single quotes for js

Hi Chris,

Don't you think is better to push the complete filepath in order to avoid ambiguity between OPAC and intranet files?

-        push @files, { name => "$_", lines => \@lines } if @lines;
+        push @files, { name => $File::Find::name, lines => \@lines } if @lines;
Comment 40 Jonathan Druart 2013-02-18 11:16:12 UTC
ping Chris, do you agree ?
Comment 41 Fridolin Somers 2013-02-18 13:21:31 UTC
*** Bug 9152 has been marked as a duplicate of this bug. ***
Comment 42 Chris Cormack 2013-02-18 18:33:23 UTC
(In reply to comment #40)
> ping Chris, do you agree ?

Yep I do .. did you notice that now there is a review link next to each patch, I added that last weekend, you can now do inline code comments :)
I hope that will be useful for doing qa.

Do you want me to resubmit? Im happy if you just fix that line on signing off.
Comment 43 Jonathan Druart 2013-02-19 08:22:37 UTC
(In reply to comment #42)
> (In reply to comment #40)
> > ping Chris, do you agree ?
> 
> Yep I do .. did you notice that now there is a review link next to each
> patch, I added that last weekend, you can now do inline code comments :)
> I hope that will be useful for doing qa.

Yes, I see it. That will be useful!

> Do you want me to resubmit? Im happy if you just fix that line on signing
> off.

I will fix it but I wanted to have your permission first.


The proposed patch for Bug 9555 fixes this test.

Note that a test in koha-qa has been introduced and detect simple-quote strings.

Marked as Passed QA.
Comment 44 Jonathan Druart 2013-02-19 08:22:58 UTC
Created attachment 15524 [details] [review]
Bug 8942 : Adding a test to make sure we don't use single quotes for js

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Tested by adding single quotes to a translation-escaped string.
Test correctly failed it.
Comment 45 Jared Camins-Esakov 2013-02-24 15:29:11 UTC
This patch has been pushed to master.
Comment 46 Fridolin Somers 2013-03-08 15:37:22 UTC
Can it be pushed to stable branches ?
Comment 47 Chris Cormack 2013-03-11 08:50:47 UTC
The patches don't apply to 3.10.x or 3.8.x please rebase and submit if you wish them to be in those branches
Comment 48 Fridolin Somers 2013-03-29 14:34:35 UTC
Created attachment 17030 [details] [review]
[3.10.x] Bug 8942: Translation process breaks javascript
Comment 49 Fridolin Somers 2013-03-29 14:35:10 UTC
Created attachment 17031 [details] [review]
[3.10.x] Bug 8942: Translation process breaks javascript (followup 1)
Comment 50 Fridolin Somers 2013-03-29 14:35:34 UTC
Created attachment 17032 [details] [review]
[3.10.x] Bug 8942: Translation process breaks javascript (followup 2)
Comment 51 Fridolin Somers 2013-03-29 14:36:05 UTC
Created attachment 17033 [details] [review]
[3.8.x] Bug 8942: Translation process breaks javascript
Comment 52 Fridolin Somers 2013-03-29 14:36:32 UTC
Created attachment 17034 [details] [review]
[3.8.x] Bug 8942: Translation process breaks javascript (followup 1)
Comment 53 Fridolin Somers 2013-03-29 14:36:59 UTC
Created attachment 17035 [details] [review]
[3.8.x] Bug 8942: Translation process breaks javascript (followup 2)
Comment 54 Fridolin Somers 2013-03-29 14:37:18 UTC
Patches rebased for 3.10 and 3.8
Comment 55 Chris Cormack 2013-03-29 19:15:57 UTC
Pushed to 3.8.x and 3.10.x I note now that we have a couple of instances of ' in the templates, will fix and push
Comment 56 Fridolin Somers 2013-04-24 13:32:14 UTC
Pushed to 3.8.12 and 3.10.5