Bug 18634 - Missing empty line at end of opac.pref / colliding translated preference sections
Summary: Missing empty line at end of opac.pref / colliding translated preference sect...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Blou
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-18 21:04 UTC by Blou
Modified: 2018-12-03 20:04 UTC (History)
11 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:
Circulation function:


Attachments
Bug 18634 - Missing empty line at end of opac.pref (835 bytes, patch)
2017-05-18 21:05 UTC, Blou
Details | Diff | Splinter Review
Sample test code (517 bytes, text/plain)
2017-05-23 14:45 UTC, Marcel de Rooy
Details
File causing the problem (309.16 KB, text/x-gettext-translation)
2017-05-23 14:48 UTC, Blou
Details
Translated fr-CA opac.pref (32.30 KB, patch)
2017-05-23 15:25 UTC, Blou
Details | Diff | Splinter Review
Translated fr-CA opac.pref (33.34 KB, patch)
2017-05-24 12:51 UTC, Blou
Details | Diff | Splinter Review
French search for 'paypal' (160.17 KB, image/png)
2017-06-13 18:38 UTC, Blou
Details
English search for 'paypal' (109.94 KB, image/png)
2017-06-13 18:39 UTC, Blou
Details
Bug 18634: Handle colliding translation for preference sections (1.79 KB, patch)
2017-06-14 13:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18634: Handle colliding translation for preference sections (1.89 KB, patch)
2017-06-16 19:17 UTC, Blou
Details | Diff | Splinter Review
Bug 18634: Handle colliding translation for preference sections (1.90 KB, patch)
2017-07-06 11:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18634: Handle colliding translation for preference sections (1.96 KB, patch)
2017-07-06 11:46 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2017-05-18 21:04:51 UTC
The subject line says it all.

When this line is missing, all Payment(PayPal) sysprefs don't show up in translated pages.
Comment 1 Blou 2017-05-18 21:05:53 UTC
Created attachment 63550 [details] [review]
Bug 18634 - Missing empty line at end of opac.pref
Comment 2 Blou 2017-05-18 21:07:57 UTC
It's a shame to have a commit line just for that, but really, that's the reason this is not translatable.  Or rather, it translates but don't show up.
Comment 3 Blou 2017-05-18 21:09:30 UTC
Sponsored by : CCSR
Comment 4 Marc Véron 2017-05-19 05:26:45 UTC
I could not reproduce on current master with de-DE and it-IT

However an empty line 826 seems to be superfluous. Other *.pref-files do not have an empty line at the end. So I think it is better to just remove line 826.

What do you think about?
Comment 5 Blou 2017-05-19 12:19:02 UTC
For me, I enter "paypal" in the system preferences search and get 6 entries under Payment in English.  I get nothing in French.
Adding the empty line fixes it.

Now, is it useless and senseless?  I agree.  I looked at circulation.pref for comfort before commiting, and saw the "semi-emtpy" line.

I looked at fr-CA-pref.po for weirdness, could not find any.  The # OPAC > Payments where right in the middle, all happy and comfy.

If ONE wouldn't show up, I'd get it.  But all six?  It really points to opac.prefs, but I have no idea why (beside this dummy end line).

:(
Comment 6 Marc Véron 2017-05-19 19:25:33 UTC
Hi Blou
I just did a git pull and then wanted to apply and sign off your patch.
But now it seems that already without your patch the last line 826 has vanished.
Maybe it was fixed by another patch?
Comment 7 Josef Moravec 2017-05-22 10:11:25 UTC
(In reply to Marc Véron from comment #6)
> Hi Blou
> I just did a git pull and then wanted to apply and sign off your patch.
> But now it seems that already without your patch the last line 826 has
> vanished.
> Maybe it was fixed by another patch?

The same for me, but opac.pref was touch last time about 3 month ago
Comment 8 Marcel de Rooy 2017-05-23 13:37:45 UTC
(In reply to Marc Véron from comment #6)
> Hi Blou
> I just did a git pull and then wanted to apply and sign off your patch.
> But now it seems that already without your patch the last line 826 has
> vanished.
> Maybe it was fixed by another patch?

No this is about git and whitespace settings..
Comment 9 Marcel de Rooy 2017-05-23 13:41:27 UTC
(In reply to Blou from comment #5)
> For me, I enter "paypal" in the system preferences search and get 6 entries
> under Payment in English.  I get nothing in French.
> Adding the empty line fixes it.
> 
> Now, is it useless and senseless?  I agree.  I looked at circulation.pref
> for comfort before commiting, and saw the "semi-emtpy" line.
> 
> I looked at fr-CA-pref.po for weirdness, could not find any.  The # OPAC >
> Payments where right in the middle, all happy and comfy.
> 
> If ONE wouldn't show up, I'd get it.  But all six?  It really points to
> opac.prefs, but I have no idea why (beside this dummy end line).
> 
> :(

Several other .pref files do not have this empty line at the end.
Comment 10 Marcel de Rooy 2017-05-23 14:36:36 UTC
Just looked a bit deeper in the code:

misc/translator uses YAML::Syck::LoadFile on a pref file

admin/preferences first uses gettemplate on a pref file, runs template->output and feeds that into YAML::Syck::Load

Seems your problem is related to the second route.
In order to know what happens with your french prefs, please examine the output of $data=gettemplate(...) and then $data->output in a test script, making sure that gettemplate reads your french file.
Comment 11 Marcel de Rooy 2017-05-23 14:37:51 UTC
Could you add your french pref file as an attachment here too?
Comment 12 Marcel de Rooy 2017-05-23 14:45:28 UTC
Created attachment 63655 [details]
Sample test code
Comment 13 Blou 2017-05-23 14:48:04 UTC
Created attachment 63656 [details]
File causing the problem
Comment 14 Blou 2017-05-23 14:49:38 UTC
Thanks Marcel,
I joined the file I have on my laptop.  I had moved from the problem to other issues, but I'll be happy to do your test as soon as I find 10 minutes, sooner rather than later.
Comment 15 Marcel de Rooy 2017-05-23 15:05:49 UTC
(In reply to Blou from comment #14)
> Thanks Marcel,
> I joined the file I have on my laptop.  I had moved from the problem to
> other issues, but I'll be happy to do your test as soon as I find 10
> minutes, sooner rather than later.

Hi. Could you also attach the actual opac.pref ?
Comment 16 Blou 2017-05-23 15:25:28 UTC
Created attachment 63660 [details] [review]
Translated fr-CA opac.pref
Comment 17 Marcel de Rooy 2017-05-24 11:23:24 UTC
(In reply to Blou from comment #16)
> Created attachment 63660 [details] [review] [review]
> Translated fr-CA opac.pref

Blou,
When I am copying your opac.pref over my english opac.pref, and I am searching for paypal, I do find all paypal preferences, regardless of adding or removing an empty line to this file?
Comment 18 Blou 2017-05-24 12:22:11 UTC
:(  of course, I sent you the one "fixed" with the extra line before translation, so I guess the resulting one doesn't cause trouble.

gimme a sec
Comment 19 Marcel de Rooy 2017-05-24 12:24:00 UTC
(In reply to Blou from comment #18)
> :(  of course, I sent you the one "fixed" with the extra line before
> translation, so I guess the resulting one doesn't cause trouble.

I always want the files with the problems :)
Comment 20 Blou 2017-05-24 12:51:36 UTC
Created attachment 63690 [details] [review]
Translated fr-CA opac.pref

Version that hide 'paypal' results.  (I just tested again before submitting)
Comment 21 Jonathan Druart 2017-06-13 15:13:52 UTC
Can we have a test plan and screenshot please?
That will ease the understanding.
Comment 22 Blou 2017-06-13 18:38:43 UTC
Created attachment 64267 [details]
French search for 'paypal'
Comment 23 Blou 2017-06-13 18:39:28 UTC
Created attachment 64268 [details]
English search for 'paypal'
Comment 24 Blou 2017-06-13 18:45:54 UTC
This problem has been known by Eric Bégin for many years.  I guess usually we do not care, or it doesn't trigger.  I've never done the deep dive like Marcel did to find the cause.

All I know is that it happens in many cases for them (the users).  On the CCSR's branch I just added a empty line at the bottom of EVERY .pref files, and it resolved all their missing parameters problems.

As far as test case
1) translate install fr-CA
2) search for 'paypal' in the sysprefs screen.
Comment 25 Marcel de Rooy 2017-06-14 13:06:06 UTC
Have debugged LangInstaller a *bit*:
And found that this part in the PO file seems to be the source of the problems:

# OPAC > Payments
msgid "opac.pref Payments"
msgstr "Vie privée"

# OPAC > Policy
msgid "opac.pref Policy"
msgstr "Politique"

# OPAC > Privacy
msgid "opac.pref Privacy"
msgstr "Vie privée"

When you install fr-CA both tabs Payments and Privacy are translated to Vie privee. In the code in LangInstaller, in the for loop around line 335 (for my $section) this causes an overwrite of the Payments hash by the Privacy hash.

Please create a unique translated key for each tab, and your problem should be gone.
Comment 26 Marcel de Rooy 2017-06-14 13:20:23 UTC
But this does not mean by definition that we should be satisfied with this overwriting hash key problem in the code. Will give it a little try still.
Comment 27 Marcel de Rooy 2017-06-14 13:41:57 UTC
Created attachment 64283 [details] [review]
Bug 18634: Handle colliding translation for preference sections

Problem on this report was caused by translating the tabs Privacy
and Payments by the same string. This caused overwriting a hash entry.

This patch tests if the key already exists and if so, it merges the
entries instead of overwriting the old contents.

Test plan:
[1] Make sure that e.g. Privacy and Payments translate to e.g Vie privee.
[2] Run translate install fr-CA (or the language you altered)
[3] Without this patch you should loose preferences from either Privacy or
    Payments. With this patch, they should be merged.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with fr-CA.
Comment 28 Marcel de Rooy 2017-06-14 13:44:52 UTC
Blou,
Could you test the latest patch attached?
If you agree, I will remove the first one.
Comment 29 Blou 2017-06-16 19:17:42 UTC
Created attachment 64414 [details] [review]
Bug 18634: Handle colliding translation for preference sections

Problem on this report was caused by translating the tabs Privacy
and Payments by the same string. This caused overwriting a hash entry.

This patch tests if the key already exists and if so, it merges the
entries instead of overwriting the old contents.

Test plan:
[1] Make sure that e.g. Privacy and Payments translate to e.g Vie privee.
[2] Run translate install fr-CA (or the language you altered)
[3] Without this patch you should loose preferences from either Privacy or
    Payments. With this patch, they should be merged.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with fr-CA.

Signed-off-by: Blou <philippe.blouin@inlibro.com>
Reset the .po files, reproduced the problem.  Applied the patch and suddenly 'paypal' appeared.
Comment 30 Blou 2017-06-16 19:20:25 UTC
Comment on attachment 63550 [details] [review]
Bug 18634 - Missing empty line at end of opac.pref

My patch was a solution, but not a fix to the problem.  Marcel's patch is the real fix (IMO, at least :) )
Comment 31 Marcel de Rooy 2017-06-19 06:29:14 UTC
Thanks Blou. Updating status.
Comment 32 Julian Maurice 2017-07-06 11:45:36 UTC
Created attachment 64843 [details] [review]
Bug 18634: Handle colliding translation for preference sections

Problem on this report was caused by translating the tabs Privacy
and Payments by the same string. This caused overwriting a hash entry.

This patch tests if the key already exists and if so, it merges the
entries instead of overwriting the old contents.

Test plan:
[1] Make sure that e.g. Privacy and Payments translate to e.g Vie privee.
[2] Run translate install fr-CA (or the language you altered)
[3] Without this patch you should loose preferences from either Privacy or
    Payments. With this patch, they should be merged.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with fr-CA.

Signed-off-by: Blou <philippe.blouin@inlibro.com>
Reset the .po files, reproduced the problem.  Applied the patch and suddenly 'paypal' appeared.
Comment 33 Julian Maurice 2017-07-06 11:46:15 UTC
Created attachment 64844 [details] [review]
Bug 18634: Handle colliding translation for preference sections

Problem on this report was caused by translating the tabs Privacy
and Payments by the same string. This caused overwriting a hash entry.

This patch tests if the key already exists and if so, it merges the
entries instead of overwriting the old contents.

Test plan:
[1] Make sure that e.g. Privacy and Payments translate to e.g Vie privee.
[2] Run translate install fr-CA (or the language you altered)
[3] Without this patch you should loose preferences from either Privacy or
    Payments. With this patch, they should be merged.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with fr-CA.

Signed-off-by: Blou <philippe.blouin@inlibro.com>
Reset the .po files, reproduced the problem.  Applied the patch and suddenly 'paypal' appeared.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 34 Julian Maurice 2017-07-06 11:48:35 UTC
There is a problem in how we handle translation here. IMO keys of a hash shouldn't be translatable.
But the patch definitely fixes a problem, so passing QA.
Comment 35 Jonathan Druart 2017-07-06 18:50:32 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 36 Fridolin Somers 2017-07-12 07:43:55 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 37 Katrin Fischer 2017-07-15 18:15:08 UTC
This patch has been pushed to 16.11.x and will be in 16.11.10.
Comment 38 Mason James 2017-07-31 11:33:58 UTC
Pushed to 16.05.x, for 16.05.15 release