Bug 27045 - Exports using CSV profiles with tab as separator don't work correctly
Summary: Exports using CSV profiles with tab as separator don't work correctly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-17 21:14 UTC by Katrin Fischer
Modified: 2023-06-08 22:26 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.05, 21.11.12, 21.05.19


Attachments
Bug 27045: Fix items lost report export if delimiter is tab (1.97 KB, patch)
2020-11-20 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27045: Fix other exports (1.83 KB, patch)
2020-11-20 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27045: Fix items lost report export if delimiter is tab (2.65 KB, patch)
2022-07-16 11:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27045: Fix other exports using CSV profiles (2.53 KB, patch)
2022-07-16 11:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27045: Fix items lost report export if delimiter is tab (2.71 KB, patch)
2022-07-16 11:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27045: Fix other exports using CSV profiles (2.59 KB, patch)
2022-07-16 11:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27045: (follow-up) Fix delimiter in header rows (1.38 KB, patch)
2022-07-16 11:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27045: Fix items lost report export if delimiter is tab (2.76 KB, patch)
2022-07-20 14:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27045: Fix other exports using CSV profiles (2.64 KB, patch)
2022-07-20 14:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27045: (follow-up) Fix delimiter in header rows (1.44 KB, patch)
2022-07-20 14:38 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2020-11-17 21:14:29 UTC
When choosing tabulation as the delimiter on a CSV profile for the lost item report, it's exported as shown below:

title\tauthor\tstocknumber\tcallnumber
"E Street shuffle :"\"Heylin, Clinton."\\

Tabs works best as default for Excel, so it would be great to see this fixed.

Reads similar like other older closed bugs like bug 17590 and bug 7251.
Comment 1 Katrin Fischer 2020-11-17 21:14:48 UTC
Originally found in 20.05.03, but confirmed in master now.
Comment 2 Jonathan Druart 2020-11-20 14:33:43 UTC
Created attachment 113890 [details] [review]
Bug 27045: Fix items lost report export if delimiter is tab
Comment 3 Jonathan Druart 2020-11-20 14:33:47 UTC
Created attachment 113891 [details] [review]
Bug 27045: Fix other exports
Comment 4 Jonathan Druart 2020-11-20 14:35:15 UTC
Hum, I lost myself. I first thought it was because we were not using Text::CSV::Encoded. But found (after I rewrote itemslost.pl export) that it was because of the way we handle "\t".
I think those 2 patches fixes the problem for all the CSV exports, but have only tried with itemslost so far.
Comment 5 Katrin Fischer 2020-11-20 23:38:26 UTC
Is this ready for testing now?
Comment 6 Jonathan Druart 2020-11-21 07:28:40 UTC
I don't know, see previous comment. Feel free to test it!
Comment 7 Fridolin Somers 2021-05-12 08:45:17 UTC
  my $delimiter = $csv_profile->csv_separator;
  $delimiter = "\t" if $delimiter eq "\\t";

Ah strange :

In system preference stored value is string "tabulation" :
https://git.koha-community.org/Koha-community/Koha/src/commit/99c10bf4484635e9befdc9c09a56c0f7278c7976/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref#L50

We see in other places :
  $delimiter = "\t" if $delimiter eq 'tabulation';
https://git.koha-community.org/Koha-community/Koha/src/commit/65542edbe021d58ca6ef1332a8e3ca760b198391/reports/guided_reports.pl#L903

Looks like use of CSVdelimiter is wrong in many places where preference is fetched in TT.
I create a  new bug report for that.

Maybe we shoud add a method C4::Context->csv_delimiter() ?
And add a method to TT plugin.
Comment 8 Katrin Fischer 2021-07-14 09:04:57 UTC
In this case it's independent of the preference - for CSV profile the delimiter is chosen in configuration. But in general both (pref and CSV profiles) need fixing.
Comment 9 Katrin Fischer 2022-07-16 11:15:59 UTC
Created attachment 137790 [details] [review]
Bug 27045: Fix items lost report export if delimiter is tab

When choosing tabulation as the delimiter for a CSV profile
for the lost item report, the tabs appear in the file as \t
instead of as proper tabs.

title\tauthor\tstocknumber\tcallnumber
"E Street shuffle :"\"Heylin, Clinton."\\

To test:
* Upate the existing sample lost item CSV profile to use tabs as
  separator
* Make sure you have some lost items in your database or create some
* Go to Reports > Lost items
* Run the report
* Check all or some of the checkboxes in the result list
* Export using the link on top and the CSV profile
* Verify the tabs are not exported correctly
* Apply patch
* Verify tabs now are proper tabs instead of \t
Comment 10 Katrin Fischer 2022-07-16 11:16:07 UTC
Created attachment 137791 [details] [review]
Bug 27045: Fix other exports using CSV profiles

This patch corrects the export of the 2 other reports
using CSV profiles:

* Late issues (serials)
* Basket (acquisitions)

To test:
1) Late issues
* Update the late issues sample report to use tab as separator
* Create a subscription
* Go to serial collection and 'generate next' to get some late issues
* Go to Claims
* Export the late issues and verify format is correct
* Verify exported file has tabs

2) Basket summary
* Create an order with several order lines
* Create an SQL type CSV profile for basket export using tab as separator
  Example: aqorders.quantity|aqordres.listprice|Title=biblio.title
* Export the basket using your configured CSV profile
* Verify exported file has tabs
Comment 11 Katrin Fischer 2022-07-16 11:18:28 UTC
Hi Joubu,

I've rebased the patch set, cleaned up the descriptions and added test plans. There is only one problem: this works almost perfectly, but the header rows for the basket and late serial issues still show the \t, while the data has the proper tabs.

Example:

aqorders.quantity\taqordres.listprice\tTitle
1	12.000000	"Perl best practices /"

Could you help?
Comment 12 Katrin Fischer 2022-07-16 11:25:14 UTC
Created attachment 137792 [details] [review]
Bug 27045: Fix items lost report export if delimiter is tab

When choosing tabulation as the delimiter for a CSV profile
for the lost item report, the tabs appear in the file as \t
instead of as proper tabs.

title\tauthor\tstocknumber\tcallnumber
"E Street shuffle :"\"Heylin, Clinton."\\

To test:
* Upate the existing sample lost item CSV profile to use tabs as
  separator
* Make sure you have some lost items in your database or create some
* Go to Reports > Lost items
* Run the report
* Check all or some of the checkboxes in the result list
* Export using the link on top and the CSV profile
* Verify the tabs are not exported correctly
* Apply patch
* Verify tabs now are proper tabs instead of \t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2022-07-16 11:25:19 UTC
Created attachment 137793 [details] [review]
Bug 27045: Fix other exports using CSV profiles

This patch corrects the export of the 2 other reports
using CSV profiles:

* Late issues (serials)
* Basket (acquisitions)

To test:
1) Late issues
* Update the late issues sample report to use tab as separator
* Create a subscription
* Go to serial collection and 'generate next' to get some late issues
* Go to Claims
* Export the late issues and verify format is correct
* Verify exported file has tabs

2) Basket summary
* Create an order with several order lines
* Create an SQL type CSV profile for basket export using tab as separator
  Example: aqorders.quantity|aqordres.listprice|Title=biblio.title
* Export the basket using your configured CSV profile
* Verify exported file has tabs

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2022-07-16 11:25:23 UTC
Created attachment 137794 [details] [review]
Bug 27045: (follow-up) Fix delimiter in header rows

The header rows still showed \t because the newly defined
variable wasn't used there.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2022-07-16 11:26:19 UTC
(In reply to Katrin Fischer from comment #11)
> Hi Joubu,
> 
> I've rebased the patch set, cleaned up the descriptions and added test
> plans. There is only one problem: this works almost perfectly, but the
> header rows for the basket and late serial issues still show the \t, while
> the data has the proper tabs.
> 
> Could you help?

Never mind - found the fix myself :)
Comment 16 Nick Clemens 2022-07-20 14:38:37 UTC
Created attachment 137933 [details] [review]
Bug 27045: Fix items lost report export if delimiter is tab

When choosing tabulation as the delimiter for a CSV profile
for the lost item report, the tabs appear in the file as \t
instead of as proper tabs.

title\tauthor\tstocknumber\tcallnumber
"E Street shuffle :"\"Heylin, Clinton."\\

To test:
* Upate the existing sample lost item CSV profile to use tabs as
  separator
* Make sure you have some lost items in your database or create some
* Go to Reports > Lost items
* Run the report
* Check all or some of the checkboxes in the result list
* Export using the link on top and the CSV profile
* Verify the tabs are not exported correctly
* Apply patch
* Verify tabs now are proper tabs instead of \t

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2022-07-20 14:38:41 UTC
Created attachment 137934 [details] [review]
Bug 27045: Fix other exports using CSV profiles

This patch corrects the export of the 2 other reports
using CSV profiles:

* Late issues (serials)
* Basket (acquisitions)

To test:
1) Late issues
* Update the late issues sample report to use tab as separator
* Create a subscription
* Go to serial collection and 'generate next' to get some late issues
* Go to Claims
* Export the late issues and verify format is correct
* Verify exported file has tabs

2) Basket summary
* Create an order with several order lines
* Create an SQL type CSV profile for basket export using tab as separator
  Example: aqorders.quantity|aqordres.listprice|Title=biblio.title
* Export the basket using your configured CSV profile
* Verify exported file has tabs

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2022-07-20 14:38:45 UTC
Created attachment 137935 [details] [review]
Bug 27045: (follow-up) Fix delimiter in header rows

The header rows still showed \t because the newly defined
variable wasn't used there.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Tomás Cohen Arazi 2022-07-20 14:54:26 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 20 Fridolin Somers 2022-07-22 08:35:12 UTC
Please have a look at Bug 28327 where I try to centralize the tabulation special case.
Comment 21 Katrin Fischer 2022-07-22 08:49:53 UTC
(In reply to Fridolin Somers from comment #20)
> Please have a look at Bug 28327 where I try to centralize the tabulation
> special case.

This doesn't depend on the pref, but is a per CSV profile setting. So my impresison was they complement each other.
Comment 22 Fridolin Somers 2022-07-27 18:48:21 UTC
(In reply to Katrin Fischer from comment #21)
> (In reply to Fridolin Somers from comment #20)
> > Please have a look at Bug 28327 where I try to centralize the tabulation
> > special case.
> 
> This doesn't depend on the pref, but is a per CSV profile setting. So my
> impresison was they complement each other.

In Bug 28327 I add a method csv_delimiter() that take a value to convert and fallsback to syspref if no value.
So it may be used for CSV profiles.
Comment 23 Lucas Gass 2022-08-23 20:12:19 UTC
Backported to 22.05.x for 22.05.05
Comment 24 Arthur Suzuki 2022-08-30 09:30:20 UTC
thx pushed to 21.11.x for 21.11.12
Comment 25 Victor Grousset/tuxayo 2022-09-14 19:31:43 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.
Comment 26 Katrin Fischer 2022-09-14 20:43:24 UTC
(In reply to Victor Grousset/tuxayo from comment #25)
> Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.
> 
> Nothing to document, marking resolved.

It's needed, we noticed this in 20.11 :)
Comment 27 Victor Grousset/tuxayo 2022-09-16 00:24:46 UTC
Ok, will backport before next release :)
Comment 28 Victor Grousset/tuxayo 2022-09-19 20:09:01 UTC
Backported: Pushed to 21.05.x branch for 21.05.19