Is there not a risk to convert quotes with the fact that TT elements are translated (they are %s) ?

> 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.

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.

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!

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.

> 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 ' seems not to work so well. It seems to be displayed has it
is: the code ' 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.

(In reply to comment #9)
> is: the code ' not the '.  I don't understand how it worked for Fridolyn.

I modified PO files to use ' in translated strings :
msgid:"Today"
msgid:"Aujourd'hui"

I will correct it french POs but I encourage you to work on this bugfix.

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!

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.

(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 ?

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.

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?

(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.

(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..

I think an automated test would be preferrable, or have both.

I agree +1 for a test.

(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.

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

Thanks a lot for testing Owen.

(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.

(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 :)

This patch has been pushed to master.

(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;

ping Chris, do you agree ?

*** Bug 9152 has been marked as a duplicate of this bug. ***

(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.

(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.

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.

This patch has been pushed to master.

Can it be pushed to stable branches ?

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

Created attachment 17030 [details] [review]
[3.10.x] Bug 8942: Translation process breaks javascript

Created attachment 17031 [details] [review]
[3.10.x] Bug 8942: Translation process breaks javascript (followup 1)

Created attachment 17032 [details] [review]
[3.10.x] Bug 8942: Translation process breaks javascript (followup 2)

Created attachment 17033 [details] [review]
[3.8.x] Bug 8942: Translation process breaks javascript

Created attachment 17034 [details] [review]
[3.8.x] Bug 8942: Translation process breaks javascript (followup 1)

Created attachment 17035 [details] [review]
[3.8.x] Bug 8942: Translation process breaks javascript (followup 2)

Patches rebased for 3.10 and 3.8

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

Pushed to 3.8.12 and 3.10.5