Bug 18331 - Translated CSV exports need to be fixed once and for all
Summary: Translated CSV exports need to be fixed once and for all
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 16914 18694
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-24 13:27 UTC by Katrin Fischer
Modified: 2017-12-07 22:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18331: Fix CSV export (once and for all!) (15.56 KB, patch)
2017-06-12 20:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18331: Force tt to insert newline to empty_line.inc (736 bytes, patch)
2017-08-03 15:54 UTC, Marc Véron
Details | Diff | Splinter Review
[FOLLOW-UP] Fix CSV export (once and for all!) (1.20 KB, patch)
2017-08-04 19:35 UTC, Lee Jamison
Details | Diff | Splinter Review
Bug 18331: Fix CSV export (once and for all!) (15.44 KB, patch)
2017-08-07 15:53 UTC, Lee Jamison
Details | Diff | Splinter Review
Bug 18331: Force tt to insert newline to empty_line.inc (789 bytes, patch)
2017-08-07 15:53 UTC, Lee Jamison
Details | Diff | Splinter Review
Fix CSV export (once and for all!) (1.24 KB, patch)
2017-08-07 15:53 UTC, Lee Jamison
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18331: Fix CSV export (once and for all!) (15.51 KB, patch)
2017-08-08 08:59 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18331: Force tt to insert newline to empty_line.inc (859 bytes, patch)
2017-08-08 08:59 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18331: Followup - Fix CSV export (once and for all!) (1.33 KB, patch)
2017-08-08 08:59 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18331: Followup - fix path to orders_by_budget csv header (1.02 KB, patch)
2017-08-08 08:59 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18331: Fix CSV export (once and for all!) (15.57 KB, patch)
2017-08-10 14:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18331: Force tt to insert newline to empty_line.inc (917 bytes, patch)
2017-08-10 14:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18331: Followup - Fix CSV export (once and for all!) (1.39 KB, patch)
2017-08-10 14:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18331: Followup - fix path to orders_by_budget csv header (1.08 KB, patch)
2017-08-10 14:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18331: POST_CHOMP everywhere! (1.88 KB, patch)
2017-08-14 13:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18331: Fix CSV export (once and for all!) (15.65 KB, patch)
2017-08-15 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18331: Force tt to insert newline to empty_line.inc (1001 bytes, patch)
2017-08-15 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18331: Followup - Fix CSV export (once and for all!) (1.47 KB, patch)
2017-08-15 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18331: Followup - fix path to orders_by_budget csv header (1.16 KB, patch)
2017-08-15 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18331: POST_CHOMP everywhere! (1.96 KB, patch)
2017-08-15 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2017-03-24 13:27:21 UTC
CSV exports of late orders and item search are broken again if you are using them from a 'complete' translation.

The theory is that if the strings are not translated, then the file is only copied (preserves line breaks) and not translated (kills line breaks). 

Both late orders and item search csv expots are effected.

For item search there is one string in itemsearch_item.csv.inc:

%s %s %s %s "%s %svon %s%s", "%s", "%s", "%s", "%s", "%s", "%s", "%s", "%s", "%s", "%s", "%s" 

Easiest way to test: 

- Install German
- Test CSV export - it should be broken.
- Look for the strings in itemsearch_item.csv in the staff po file and "untranslate" them. This is easiest done by adding "fuzzy".
- Reinstall German
- CSV shoudl be fixed
Comment 1 Jonathan Druart 2017-06-12 20:50:38 UTC
Created attachment 64239 [details] [review]
Bug 18331: Fix CSV export (once and for all!)

I am going to try and explain the problem here in order to make it a
have-to-read for next changes.

There are several things to test when and most of the time we break
something when we try to fix something else.

You have to test different CSV export, not only one. There are not all
processed the same way. For instance acqui/csv/basket.tt does not
contain any strings to translate and the translate script will copy the
original file as it.
But reports/csv/orders_by_budget.tt will not! Indeed it contains "TOTAL"
which will be translated. The generated tt file for the translated
language will remove all the carriage returns! That's why we cannot rely
on TT for newlines (contrary to bug 16914 assumption).

There are two possible methods:
1/ Fix the translate script
=> Hum... nope
2/ Remove all carriage returns and make them explicit by including an
file that only contains 1 carriage return

This second method is implemented in this patch.

How it works: Use the PRE_CHOMP "[%-" and POST_CHOMP "-%]"
(http://www.template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP)
to remove all the newlines that could be added by TT in the original
(en) files. Then include the new_line.inc to add a new line.
That way original and translated files will behave the same way.
The BLOCK in the csv_headers avoid to have the newlines added, in any
cases. For instance: by default we will have an empty line at the end of
the headers, but the translated headers will not have it.

Test plan:
At least 2 signoffs will be needed, please test carefully!
You will need to generate CSV with at least 2 entries!
Test with the non-translated interface (en) and with the language you
want. To test with a translated language you will *have to*:
- cd misc/translator
- perl translate update LANG
- manually edit the po file, remove the fuzzy tags, correctly translate
the string (do not forget the additional %s in the headers)
- perl translate install LANG
1/ Export basket and basketgroup as CSV
2/ Export late orders as CSV
3/ Export items (from the item search) as CSV
4/ Export the 2 reports "Orders by fund" and "Cash register" as CSV
Comment 2 Jonathan Druart 2017-06-12 20:51:38 UTC
Upping severity a bit
Comment 3 Jonathan Druart 2017-06-12 20:53:48 UTC
Guys, please try your hardest to test this patch, I am fed up with fixing this bug every release!
Comment 4 Marc Véron 2017-06-13 05:12:49 UTC
I first tested English CSV export of late orders with two items and got everything on one line.

Just for testing, I
- removed the chomps from line 3, changed to:
[% INCLUDE empty_line.inc %]
-> Fail, same result (one line)

- added an empty line above line 3 (just for testing, it makes no sense with the intention of an empty line include)
[%- USE KohaDates -%]
[%- INCLUDE csv_headers/acqui/lateorders.tt -%]

[% INCLUDE empty_line.inc %]
[%- FOREACH o IN orders -%]
-> Fail CSV-Export with header on one line, contents (both items on one line

- additionally added an empty line above line above the second empty line include:
(...)unt %]","[% o.claimed_date %]"

[% INCLUDE empty_line.inc %]
[%- END -%]
,,Total orders in late, [% orders.size %]

-> CSV-Export OK, 1 header line, 2 lines for 2 orders, 1 line for Total

Result: The trick with the empty line include did not to work.

Then I reseted all, applied the patch again and added the following to the first and only line of koha-tmpl/intranet-tmpl/prog/en/includes/empty_line.inc
[% '' %]

Result: CSV Export worked as expected (for late orders, English)
Then I tested with a translation and it worked as well.
Comment 5 Jonathan Druart 2017-06-13 14:59:39 UTC
I have retested and it works for me (late orders).
Comment 6 Marc Véron 2017-06-13 16:08:10 UTC
Re-tested on current master with patch applied, plack + memcached restarted, page reloaded, 2 late orders, CSV export, open in editor, result (in one line):

ORDER DATE,ESTIMATED DELIVERY DATE,VENDOR,INFORMATION,TOTAL COST,BASKET,CLAIMS COUNT,CLAIMED DATE"2017-06-13 (0 days)","06/13/2017","PROBESHOP","Schokolade Author: Testmann, Hans.","1.00 x 1 = 1.00 (Fund 1)","Probebasket 1 (1)","0","""2017-06-13 (0 days)","06/13/2017","PROBESHOP","Republik im Fokus Author: Hanisch, Ernst.","2.00 x 1 = 2.00 (Fund 1)","Probebasket 1 (1)","0","",,Total orders in late, 2

If I add the [% '' %] to koha-tmpl/intranet-tmpl/prog/en/includes/empty_line.inc I get 4 lines (as expected):
ORDER DATE,ESTIMATED DELIVERY DATE,VENDOR,INFORMATION,TOTAL COST,BASKET,CLAIMS COUNT,CLAIMED DATE
"2017-06-13 (0 days)","06/13/2017","PROBESHOP","Schokolade Author: Testmann, Hans.","1.00 x 1 = 1.00 (Fund 1)","Probebasket 1 (1)","0",""
"2017-06-13 (0 days)","06/13/2017","PROBESHOP","Republik im Fokus Author: Hanisch, Ernst.","2.00 x 1 = 2.00 (Fund 1)","Probebasket 1 (1)","0",""
,,Total orders in late, 2
Comment 7 Jonathan Druart 2017-06-13 16:33:06 UTC
Did you update the po file correctly?
Comment 8 Marc Véron 2017-06-13 17:58:28 UTC
It happens in English (see comment #4)
Comment 9 Jonathan Druart 2017-06-13 19:18:13 UTC
I replaced empty_string with [% '' %] and I got the same result (i.e. it works).
Comment 10 Marc Véron 2017-06-14 06:23:41 UTC
(In reply to Jonathan Druart from comment #9)
> I replaced empty_string with [% '' %] and I got the same result (i.e. it
> works).

...as in my tests from comment #6
What is the next step? - Change koha-tmpl/intranet-tmpl/prog/en/includes/empty_line.inc ?
Comment 11 Jonathan Druart 2017-06-14 12:46:12 UTC
I'd like somebody else to test as we get different behaviours with the same code.
Comment 12 Marc Véron 2017-06-14 13:52:33 UTC
Hmm, there must be a difference.

Just to exclude:
Same version of Template Toolkit?
About page: Template (2.22) 	2.26
Comment 13 Marc Véron 2017-06-14 14:01:59 UTC
Updated Template Toolkit to 2.27, same result.
Comment 14 Marc Véron 2017-06-14 14:56:17 UTC
I can fix and reproduce the issue with following steps:

- Apply patch
- restart plack / memcached with every step as appropriate
- Verify that (on my installation) the CSV export is in one line
- Edit empty_line.inc (using vim)
- Set vim to display line breaks (:set list)
- Add a new line after the existing one
- Delete the first line
- Save
- Do a new CSV export -> Success
- Revert all edits (using u in vim)
- Save
- Do a new CSV export -> Fail, export is again on one line
Comment 15 Katrin Fischer 2017-07-09 17:20:11 UTC
Hi I tested this fresh from start to end and I am seeing no improvement with the patch:

Tested on master:

- translate update de-DE
- translate install de-DE
- Switch to de-DE:
  - item search export broken
  - acq basket export broken
  - late acq orders broken
  - late issues export - OK (uses CSV profiles)

Tested with patch on master:

- apply patch
- restart_all
- translate update de-DE
- open file in poedit - check headers are translated and not fuzzy (orange in poedit)
  - item search for csv - seems unchanged?
  - acq basket was fuzzy - retranslated
  - late acq orders was fuzzy - retranslated
- translate install de-DE
- Test again:
  - item search - still broken
  - acq basket - still broken
  - late acq orders - still broken

I am sorry, but this doesn't seem to be the fix we are looking for :(
Comment 16 Katrin Fischer 2017-07-09 17:34:30 UTC
Ok, reading through the bug a bit more, I also tested with English templates.
I am also seeing trouble with the patch applied:

  - item search - OK!
  - acq basket - still broken
  - late acq orders - still broken

Without patch:

  - item search - OK
  - acq basket - OK
  - late acq orders - OK

Sorry to have on better news.
Comment 17 Jonathan Druart 2017-07-10 20:17:15 UTC
Well, we have a problem here. Still works for me.
Please follow carefully the following, there is something you are or I am missing.

- Apply the patches
- Restart plack and memcached # useless, but just in case...
- perl translate update de-DE
- Edit po/de-DE-staff-prog.po and modify the two following blocks (for the CSV content, then for the headers):

#: intranet-tmpl/prog/en/modules/acqui/csv/lateorders.tt:1
msgid ""
"%s %s %s %s \"%s (%s days)\",\"%s\",\"%s\",\"%s %sAuthor: %s.%s%sPublished "
"by: %s.%s\",\"%s x %s = %s (%s)\",\"%s (%s)\",\"%s\",\"%s\" %s %s ,,Total "
"orders in late, %s "
msgstr ""
"%s %s %s %s \"%s (%s Tage)\",\"%s\",\"%s\",\"%s %sVerfasser: %s.%s%sVeröffentlicht "
"von: %s.%s\",\"%s x %s = %s (%s)\",\"%s (%s)\",\"%s\",\"%s\" %s %s ,,Anzahl "
"verspäteter Bestellungen, %s "

#: intranet-tmpl/prog/en/includes/csv_headers/acqui/lateorders.tt:1
msgid ""
"%sORDER DATE,ESTIMATED DELIVERY DATE,VENDOR,INFORMATION,TOTAL COST,BASKET,"
"CLAIMS COUNT,CLAIMED DATE%s "
msgstr ""
"%sBestelldatum,Berechnetes Lieferdatum,Lieferant,Informaton,Gesamtkosten,"
"Bestellung,Anzahl Reklamationen,Reklamationsdatum%s "

Make sure the fuzzy lines are removed!

- perl translate install de-DE
- Create 2 orders, close the basket
- hit /cgi-bin/koha/acqui/lateorders.pl
- Export the 2 orders (English template), I get:
ORDER DATE,ESTIMATED DELIVERY DATE,VENDOR,INFORMATION,TOTAL COST,BASKET,CLAIMS COUNT,CLAIMED DATE
"2017-07-10 (0 days)","07/13/2017","My Vendor","E Street shuffle : Author: Heylin, Clinton..","111.00 x 1 = 111.00 (Fund 1_2)","My Basket (1)","0",""
"2017-07-10 (0 days)","07/13/2017","My Vendor","Introduction to Attic Greek / Author: Mastronarde, Donald J..Published by: University of California Press,.","22.00 x 1 = 22.00 (Main fund)","My Basket (1)","0",""
,,Total orders in late, 2

- Enable de-DE (pref language)
- Export the 2 orders, I get:
Bestelldatum,Berechnetes Lieferdatum,Lieferant,Informaton,Gesamtkosten,Bestellung,Anzahl Reklamationen,Reklamationsdatum
 "2017-07-10 (0 Tage)","07/13/2017","My Vendor","E Street shuffle : Verfasser: Heylin, Clinton..","111.00 x 1 = 111.00 (Fund 1_2)","My Basket (1)","0",""
 "2017-07-10 (0 Tage)","07/13/2017","My Vendor","Introduction to Attic Greek / Verfasser: Mastronarde, Donald J..Veröffentlicht von: University of California Press,.","22.00 x 1 = 22.00 (Main fund)","My Basket (1)","0",""
 ,,Anzahl verspäteter Bestellungen, 2

% wc /tmp/lateorders_en.csv
  4  55 486 /tmp/lateorders_en.csv
% wc /tmp/lateorders_de.csv
  4  50 541 /tmp/lateorders_de.csv
Comment 18 Katrin Fischer 2017-07-10 21:12:33 UTC
Hi Jonathan, 
It's exactly what I did. I think the really curious thing is that for me it's broken the English templates too. So if we leave out all the po stuff - where could be the difference on this level?
Comment 19 Jonathan Druart 2017-07-11 17:13:38 UTC
pmvers Template => 2.24
Comment 20 Jonathan Druart 2017-07-11 18:49:18 UTC
Try to remove the content of template_cache_dir or disable it (and restart memcached + plack).
Comment 21 Jonathan Druart 2017-08-03 15:41:51 UTC
Not related with the current discussion, but found an error in the current patch:

+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/csv/basket.tt
@@ -1,4 +1,6 @@
-[% INCLUDE csv_headers/acqui/basket.tt %]
+[%- INCLUDE csv_headers/acqui/basket.tt -%]
+[% INCLUDE empty_line.inc -%]

last line should be 
+[%- INCLUDE empty_line.inc -%]
Comment 22 Marc Véron 2017-08-03 15:54:26 UTC
Created attachment 65478 [details] [review]
Bug 18331: Force tt to insert newline to empty_line.inc

See end of comment #4 and IRC:
http://irc.koha-community.org/koha/2017-08-03#i_1958062
Comment 23 Lee Jamison 2017-08-04 19:35:54 UTC
Created attachment 65515 [details] [review]
[FOLLOW-UP] Fix CSV export (once and for all!)

Follow-up on Comment 21 to correct the empty_line.inc
include in basket.tt
Comment 24 Lee Jamison 2017-08-07 15:53:39 UTC
Created attachment 65581 [details] [review]
Bug 18331: Fix CSV export (once and for all!)

I am going to try and explain the problem here in order to make it a
have-to-read for next changes.

There are several things to test when and most of the time we break
something when we try to fix something else.

You have to test different CSV export, not only one. There are not all
processed the same way. For instance acqui/csv/basket.tt does not
contain any strings to translate and the translate script will copy the
original file as it.
But reports/csv/orders_by_budget.tt will not! Indeed it contains "TOTAL"
which will be translated. The generated tt file for the translated
language will remove all the carriage returns! That's why we cannot rely
on TT for newlines (contrary to bug 16914 assumption).

There are two possible methods:
1/ Fix the translate script
=> Hum... nope
2/ Remove all carriage returns and make them explicit by including an
file that only contains 1 carriage return

This second method is implemented in this patch.

How it works: Use the PRE_CHOMP "[%-" and POST_CHOMP "-%]"
(http://www.template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP)
to remove all the newlines that could be added by TT in the original
(en) files. Then include the new_line.inc to add a new line.
That way original and translated files will behave the same way.
The BLOCK in the csv_headers avoid to have the newlines added, in any
cases. For instance: by default we will have an empty line at the end of
the headers, but the translated headers will not have it.

Test plan:
At least 2 signoffs will be needed, please test carefully!
You will need to generate CSV with at least 2 entries!
Test with the non-translated interface (en) and with the language you
want. To test with a translated language you will *have to*:
- cd misc/translator
- perl translate update LANG
- manually edit the po file, remove the fuzzy tags, correctly translate
the string (do not forget the additional %s in the headers)
- perl translate install LANG
1/ Export basket and basketgroup as CSV
2/ Export late orders as CSV
3/ Export items (from the item search) as CSV
4/ Export the 2 reports "Orders by fund" and "Cash register" as CSV

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Leaving as Needs Signoff as 2 signoffs are required per Comment 1
Comment 25 Lee Jamison 2017-08-07 15:53:49 UTC
Created attachment 65582 [details] [review]
Bug 18331: Force tt to insert newline to empty_line.inc

See end of comment #4 and IRC:
http://irc.koha-community.org/koha/2017-08-03#i_1958062

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 26 Lee Jamison 2017-08-07 15:53:52 UTC
Created attachment 65583 [details] [review]
Fix CSV export (once and for all!)

Follow-up on Comment 21 to correct the empty_line.inc
include in basket.tt

https://bugs.koha-community.org/show_bug.cgi?id=18331

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 27 Josef Moravec 2017-08-08 08:59:32 UTC
Created attachment 65620 [details] [review]
[SIGNED-OFF] Bug 18331: Fix CSV export (once and for all!)

I am going to try and explain the problem here in order to make it a
have-to-read for next changes.

There are several things to test when and most of the time we break
something when we try to fix something else.

You have to test different CSV export, not only one. There are not all
processed the same way. For instance acqui/csv/basket.tt does not
contain any strings to translate and the translate script will copy the
original file as it.
But reports/csv/orders_by_budget.tt will not! Indeed it contains "TOTAL"
which will be translated. The generated tt file for the translated
language will remove all the carriage returns! That's why we cannot rely
on TT for newlines (contrary to bug 16914 assumption).

There are two possible methods:
1/ Fix the translate script
=> Hum... nope
2/ Remove all carriage returns and make them explicit by including an
file that only contains 1 carriage return

This second method is implemented in this patch.

How it works: Use the PRE_CHOMP "[%-" and POST_CHOMP "-%]"
(http://www.template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP)
to remove all the newlines that could be added by TT in the original
(en) files. Then include the new_line.inc to add a new line.
That way original and translated files will behave the same way.
The BLOCK in the csv_headers avoid to have the newlines added, in any
cases. For instance: by default we will have an empty line at the end of
the headers, but the translated headers will not have it.

Test plan:
At least 2 signoffs will be needed, please test carefully!
You will need to generate CSV with at least 2 entries!
Test with the non-translated interface (en) and with the language you
want. To test with a translated language you will *have to*:
- cd misc/translator
- perl translate update LANG
- manually edit the po file, remove the fuzzy tags, correctly translate
the string (do not forget the additional %s in the headers)
- perl translate install LANG
1/ Export basket and basketgroup as CSV
2/ Export late orders as CSV
3/ Export items (from the item search) as CSV
4/ Export the 2 reports "Orders by fund" and "Cash register" as CSV

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Josef Moravec 2017-08-08 08:59:36 UTC
Created attachment 65621 [details] [review]
[SIGNED-OFF] Bug 18331: Force tt to insert newline to empty_line.inc

See end of comment #4 and IRC:
http://irc.koha-community.org/koha/2017-08-03#i_1958062

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Josef Moravec 2017-08-08 08:59:39 UTC
Created attachment 65622 [details] [review]
[SIGNED-OFF] Bug 18331: Followup - Fix CSV export (once and for all!)

Follow-up on Comment 21 to correct the empty_line.inc
include in basket.tt

https://bugs.koha-community.org/show_bug.cgi?id=18331

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 30 Josef Moravec 2017-08-08 08:59:43 UTC
Created attachment 65623 [details] [review]
[SIGNED-OFF] Bug 18331: Followup - fix path to orders_by_budget csv header

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 31 Katrin Fischer 2017-08-10 13:59:53 UTC
Tested, each with English and German (translated):
- Acquisitions: Basket export - OK!
- Acquisitions: Basket group export - OK!
- Acquisitions: Late orders - OK!
- Item search - OK!
- Report: Cash register stats - OK! (filed bug 19075 for improving header line)
- Report: Orders by fund - OK!
Comment 32 Katrin Fischer 2017-08-10 14:31:25 UTC
Created attachment 65815 [details] [review]
[SIGNED OFF] Bug 18331: Fix CSV export (once and for all!)

I am going to try and explain the problem here in order to make it a
have-to-read for next changes.

There are several things to test when and most of the time we break
something when we try to fix something else.

You have to test different CSV export, not only one. There are not all
processed the same way. For instance acqui/csv/basket.tt does not
contain any strings to translate and the translate script will copy the
original file as it.
But reports/csv/orders_by_budget.tt will not! Indeed it contains "TOTAL"
which will be translated. The generated tt file for the translated
language will remove all the carriage returns! That's why we cannot rely
on TT for newlines (contrary to bug 16914 assumption).

There are two possible methods:
1/ Fix the translate script
=> Hum... nope
2/ Remove all carriage returns and make them explicit by including an
file that only contains 1 carriage return

This second method is implemented in this patch.

How it works: Use the PRE_CHOMP "[%-" and POST_CHOMP "-%]"
(http://www.template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP)
to remove all the newlines that could be added by TT in the original
(en) files. Then include the new_line.inc to add a new line.
That way original and translated files will behave the same way.
The BLOCK in the csv_headers avoid to have the newlines added, in any
cases. For instance: by default we will have an empty line at the end of
the headers, but the translated headers will not have it.

Test plan:
At least 2 signoffs will be needed, please test carefully!
You will need to generate CSV with at least 2 entries!
Test with the non-translated interface (en) and with the language you
want. To test with a translated language you will *have to*:
- cd misc/translator
- perl translate update LANG
- manually edit the po file, remove the fuzzy tags, correctly translate
the string (do not forget the additional %s in the headers)
- perl translate install LANG
1/ Export basket and basketgroup as CSV
2/ Export late orders as CSV
3/ Export items (from the item search) as CSV
4/ Export the 2 reports "Orders by fund" and "Cash register" as CSV

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Katrin Fischer 2017-08-10 14:31:29 UTC
Created attachment 65816 [details] [review]
[SIGNED OFF] Bug 18331: Force tt to insert newline to empty_line.inc

See end of comment #4 and IRC:
http://irc.koha-community.org/koha/2017-08-03#i_1958062

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2017-08-10 14:31:33 UTC
Created attachment 65817 [details] [review]
[SIGNED OFF] Bug 18331: Followup - Fix CSV export (once and for all!)

Follow-up on Comment 21 to correct the empty_line.inc
include in basket.tt

https://bugs.koha-community.org/show_bug.cgi?id=18331

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2017-08-10 14:31:37 UTC
Created attachment 65818 [details] [review]
[SIGNED OFF] Bug 18331: Followup - fix path to orders_by_budget csv header

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Marcel de Rooy 2017-08-11 08:27:41 UTC
The tt files now include:
Header line
[%- INCLUDE empty_line.inc -%]        <= with final chomp
Row information
[%- INCLUDE empty_line.inc %]         <= without final chomp

But if I understand it correcty, this does not make a difference. I would recommend to add the final chomp to the second include too for consistency (all directives chomp at both sides and the empty line includes care for newlines). The newline is generated by processing the empty line include and that one is not chomped away anymore. Tested with basket.tt. Please let me know if I am mistaken.
Comment 37 Katrin Fischer 2017-08-11 08:33:43 UTC
It would be great if we could get this one in before the 15th and string freeze.
It's well tested now, please don't break it - there is a long history of 'argh' over this problem :)
Comment 38 Jonathan Druart 2017-08-14 13:27:36 UTC
Created attachment 65956 [details] [review]
Bug 18331: POST_CHOMP everywhere!
Comment 39 Jonathan Druart 2017-08-14 13:29:09 UTC
Yes, basically the idea is to have PRE and POST CHOMP everywhere in the csv template files, then use the empty_line include file to add it explicitly.
Comment 40 Marcel de Rooy 2017-08-15 12:40:24 UTC
Created attachment 66011 [details] [review]
Bug 18331: Fix CSV export (once and for all!)

I am going to try and explain the problem here in order to make it a
have-to-read for next changes.

There are several things to test when and most of the time we break
something when we try to fix something else.

You have to test different CSV export, not only one. There are not all
processed the same way. For instance acqui/csv/basket.tt does not
contain any strings to translate and the translate script will copy the
original file as it.
But reports/csv/orders_by_budget.tt will not! Indeed it contains "TOTAL"
which will be translated. The generated tt file for the translated
language will remove all the carriage returns! That's why we cannot rely
on TT for newlines (contrary to bug 16914 assumption).

There are two possible methods:
1/ Fix the translate script
=> Hum... nope
2/ Remove all carriage returns and make them explicit by including an
file that only contains 1 carriage return

This second method is implemented in this patch.

How it works: Use the PRE_CHOMP "[%-" and POST_CHOMP "-%]"
(http://www.template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP)
to remove all the newlines that could be added by TT in the original
(en) files. Then include the new_line.inc to add a new line.
That way original and translated files will behave the same way.
The BLOCK in the csv_headers avoid to have the newlines added, in any
cases. For instance: by default we will have an empty line at the end of
the headers, but the translated headers will not have it.

Test plan:
At least 2 signoffs will be needed, please test carefully!
You will need to generate CSV with at least 2 entries!
Test with the non-translated interface (en) and with the language you
want. To test with a translated language you will *have to*:
- cd misc/translator
- perl translate update LANG
- manually edit the po file, remove the fuzzy tags, correctly translate
the string (do not forget the additional %s in the headers)
- perl translate install LANG
1/ Export basket and basketgroup as CSV
2/ Export late orders as CSV
3/ Export items (from the item search) as CSV
4/ Export the 2 reports "Orders by fund" and "Cash register" as CSV

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2017-08-15 12:40:29 UTC
Created attachment 66012 [details] [review]
Bug 18331: Force tt to insert newline to empty_line.inc

See end of comment #4 and IRC:
http://irc.koha-community.org/koha/2017-08-03#i_1958062

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2017-08-15 12:40:33 UTC
Created attachment 66013 [details] [review]
Bug 18331: Followup - Fix CSV export (once and for all!)

Follow-up on Comment 21 to correct the empty_line.inc
include in basket.tt

https://bugs.koha-community.org/show_bug.cgi?id=18331

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2017-08-15 12:40:38 UTC
Created attachment 66014 [details] [review]
Bug 18331: Followup - fix path to orders_by_budget csv header

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2017-08-15 12:40:42 UTC
Created attachment 66015 [details] [review]
Bug 18331: POST_CHOMP everywhere!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Jonathan Druart 2017-08-15 15:26:44 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 46 Fridolin SOMERS 2017-09-06 13:13:36 UTC
Pushed to 17.05.x, will be in 17.05.04

Thanks for your job, a real never ending bug
Comment 47 Katrin Fischer 2017-09-10 08:35:06 UTC
These patches have been pushed to 16.11.x and will be in 16.11.12.

Let's hope this zombie bug stays dead now!