Bug 18331 - CSV exports need to be fixed once and for all
Summary: CSV exports need to be fixed once and for all
Status: Needs Signoff
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: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 16914 18694
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-24 13:27 UTC by Katrin Fischer
Modified: 2017-06-14 14:56 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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

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