Description
Katrin Fischer
2017-03-24 13:27:21 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 Upping severity a bit Guys, please try your hardest to test this patch, I am fed up with fixing this bug every release! 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. I have retested and it works for me (late orders). 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 Did you update the po file correctly? It happens in English (see comment #4) I replaced empty_string with [% '' %] and I got the same result (i.e. it works). (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 ? I'd like somebody else to test as we get different behaviours with the same code. Hmm, there must be a difference. Just to exclude: Same version of Template Toolkit? About page: Template (2.22) 2.26 Updated Template Toolkit to 2.27, same result. 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 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 :( 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. 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 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? pmvers Template => 2.24 Try to remove the content of template_cache_dir or disable it (and restart memcached + plack). 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 -%] 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 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 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 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> 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> 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> 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> 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> 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> 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! 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> 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> 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> 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> 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. 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 :) Created attachment 65956 [details] [review] Bug 18331: POST_CHOMP everywhere! 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. 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> 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> 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> 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> Created attachment 66015 [details] [review] Bug 18331: POST_CHOMP everywhere! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 17.11, thanks to everybody involved! Pushed to 17.05.x, will be in 17.05.04 Thanks for your job, a real never ending bug 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! |