Bug 28327

Summary: System preference CSVdelimiter special case for tabulation
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: ToolsAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Kyle M Hall (khall) <kyle>
Severity: normal    
Priority: P5 - low CC: david, didier.gautheron, jonathan.druart, kyle, lucas, m.de.rooy, nick, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27045
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17590
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes the CSV export so that data is correctly exported with a tab (\t) as the separator when this format is selected. This was incorrectly using the word 'tabulation' as the separator. (The default export format is set using the CSVdelimiter system preference.) In addition, the code where this is used was simplified (including several of the default reports, item search export, and the log viewer), and the default for CSVdelimiter was set to the comma separator.
Version(s) released in:
22.11.00,22.05.06,21.11.12
Circulation function:
Bug Depends on: 27486    
Bug Blocks: 28363, 34845    
Attachments: Bug 28327: System preference CSVdelimiter special case for tabulation
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter
Bug 28327: System preference CSVdelimiter special case for tabulation
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter
Bug 28327: (QA follow-up) Add POD
Bug 28327: System preference CSVdelimiter special case for tabulation in templates
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter
Bug 28327: System preference CSVdelimiter special case for tabulation in perl
Bug 28327: use semicolon when preference CSVDelimiter is missing
Bug 28327: (QA follow-up) Add POD
Bug 28135: Replace use of input type number in additem.js
Bug 28327: CSVdelimiter special case for tabulation in templates
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter
Bug 28327: CSVdelimiter special case for tabulation in perl
Bug 28327: use semicolon when preference CSVDelimiter is missing
Bug 28327: (QA follow-up) Add POD
Bug 28327: CSVdelimiter special case for tabulation in templates
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter
Bug 28327: CSVdelimiter special case for tabulation in perl
Bug 28327: (QA follow-up) Add POD
Bug 28327: CSVdelimiter special case for tabulation in templates
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter
Bug 28327: CSVdelimiter special case for tabulation in perl
Bug 28327: (QA follow-up) Add POD
Bug 28327: Unify CSV delimiter special behavior for tabulation
Bug 28327: Add unit tests
Bug 28327: Unify CSV delimiter special behavior for tabulation
Bug 28327: Add unit tests
Bug 28327: Unify CSV delimiter special behavior for tabulation
Bug 28327: Add unit tests
Bug 28327: Unify CSV delimiter special behavior for tabulation
Bug 28327: Add unit tests
Bug 28327: Unify CSV delimiter special behavior for tabulation
Bug 28327: (follow-up) Comma as default CSV delimiter
Bug 28327: Add unit tests
Bug 28327: Unify CSV delimiter special behavior for tabulation
Bug 28327: (follow-up) Comma as default CSV delimiter
Bug 28327: Add unit tests
Bug 28327: Add unit tests
Bug 28327: Add unit tests
Bug 28327: Unify CSV delimiter special behavior for tabulation
Bug 28327: (follow-up) Comma as default CSV delimiter

Description Fridolin Somers 2021-05-12 09:43:11 UTC
System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

Must be fixed when preference is fetch in template.
Comment 1 Fridolin Somers 2021-05-12 10:12:37 UTC
Created attachment 120859 [details] [review]
Bug 28327: System preference CSVdelimiter special case for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in template.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
Comment 2 Fridolin Somers 2021-05-12 10:38:41 UTC
Created attachment 120861 [details] [review]
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter

Run prove t/Koha_Template_Plugin_Koha.t
Comment 3 Nick Clemens (kidclamp) 2021-05-12 11:01:28 UTC
(In reply to Fridolin Somers from comment #2)
> Created attachment 120861 [details] [review] [review]
> Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter
> 
> Run prove t/Koha_Template_Plugin_Koha.t

Why do you add the 'val' passthrough to the routine?
Comment 4 Fridolin Somers 2021-05-12 12:25:24 UTC
(In reply to Nick Clemens from comment #3)
> (In reply to Fridolin Somers from comment #2)
> > Created attachment 120861 [details] [review] [review] [review]
> > Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter
> > 
> > Run prove t/Koha_Template_Plugin_Koha.t
> 
> Why do you add the 'val' passthrough to the routine?

In case we use it with an existing value from DB.
For example in CSV profile we store delimiter (not possible to store \t in DB I  bet).
So we may need to call this method in the future with a separator value, just to transform 'tabulation' into '\t'.
Comment 5 Nick Clemens (kidclamp) 2021-05-12 13:17:06 UTC
Created attachment 120867 [details] [review]
Bug 28327: System preference CSVdelimiter special case for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in template.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens (kidclamp) 2021-05-12 13:17:09 UTC
Created attachment 120868 [details] [review]
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter

Run prove t/Koha_Template_Plugin_Koha.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens (kidclamp) 2021-05-12 13:17:13 UTC
Created attachment 120869 [details] [review]
Bug 28327: (QA follow-up) Add POD
Comment 8 Nick Clemens (kidclamp) 2021-05-12 13:19:05 UTC
This bug is ugly, moving to PQA as very trivial, thanks Frido!
Comment 9 Fridolin Somers 2021-05-12 13:24:24 UTC
Wait I have more patches ;)
Comment 10 Fridolin Somers 2021-05-12 13:39:33 UTC
Created attachment 120870 [details] [review]
Bug 28327: System preference CSVdelimiter special case for tabulation in templates

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in template.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
Comment 11 Fridolin Somers 2021-05-12 13:39:50 UTC
Created attachment 120871 [details] [review]
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter

Run prove t/Koha_Template_Plugin_Koha.t
Comment 12 Fridolin Somers 2021-05-12 13:40:08 UTC
Created attachment 120872 [details] [review]
Bug 28327: System preference CSVdelimiter special case for tabulation in perl

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in perl.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
Comment 13 Fridolin Somers 2021-05-12 13:40:26 UTC
Created attachment 120873 [details] [review]
Bug 28327: use semicolon when preference CSVDelimiter is missing

Some places use comma as value when preference CSVDelimiter is missing
or empty.
From installer/data/mysql/mandatory/sysprefs.sql whe see that defaut
install value is semicolon.

This patch sets semicolon when preference CSVDelimiter is missing.

"We are perl, we love semicolon ;)"
Comment 14 Fridolin Somers 2021-05-12 13:40:42 UTC
Created attachment 120874 [details] [review]
Bug 28327: (QA follow-up) Add POD
Comment 15 Fridolin Somers 2021-05-12 13:43:46 UTC Comment hidden (obsolete)
Comment 16 Fridolin Somers 2021-05-12 13:45:26 UTC
Created attachment 120877 [details] [review]
Bug 28327: CSVdelimiter special case for tabulation in templates

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in template.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
Comment 17 Fridolin Somers 2021-05-12 13:45:45 UTC
Created attachment 120878 [details] [review]
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter

Run prove t/Koha_Template_Plugin_Koha.t
Comment 18 Fridolin Somers 2021-05-12 13:46:01 UTC
Created attachment 120879 [details] [review]
Bug 28327: CSVdelimiter special case for tabulation in perl

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in perl.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
Comment 19 Fridolin Somers 2021-05-12 13:46:17 UTC
Created attachment 120880 [details] [review]
Bug 28327: use semicolon when preference CSVDelimiter is missing

Some places use comma as value when preference CSVDelimiter is missing
or empty.
From installer/data/mysql/mandatory/sysprefs.sql whe see that defaut
install value is semicolon.

This patch sets semicolon when preference CSVDelimiter is missing.

"We are perl, we love semicolon ;)"
Comment 20 Fridolin Somers 2021-05-12 13:46:33 UTC
Created attachment 120881 [details] [review]
Bug 28327: (QA follow-up) Add POD
Comment 21 Fridolin Somers 2021-05-12 13:48:41 UTC
(In reply to Fridolin Somers from comment #9)
> Wait I have more patches ;)

Here they are.

Tell me if your OK with semicolon ';' as default value since it is value in installer/data/mysql/mandatory/sysprefs.sql
Comment 22 Nick Clemens (kidclamp) 2021-05-12 14:17:35 UTC
(In reply to Fridolin Somers from comment #21)
> (In reply to Fridolin Somers from comment #9)
> > Wait I have more patches ;)
> 
> Here they are.
> 
> Tell me if your OK with semicolon ';' as default value since it is value in
> installer/data/mysql/mandatory/sysprefs.sql

If a library has the pref not set, this will change behavior - I would move that to its own bug
Comment 23 Fridolin Somers 2021-05-17 10:32:32 UTC
(In reply to Nick Clemens from comment #22)
> (In reply to Fridolin Somers from comment #21)
> > (In reply to Fridolin Somers from comment #9)
> > > Wait I have more patches ;)
> > 
> > Here they are.
> > 
> > Tell me if your OK with semicolon ';' as default value since it is value in
> > installer/data/mysql/mandatory/sysprefs.sql
> 
> If a library has the pref not set, this will change behavior - I would move
> that to its own bug

OK i'm on it
Comment 24 Fridolin Somers 2021-10-21 22:08:59 UTC
Created attachment 126682 [details] [review]
Bug 28327: CSVdelimiter special case for tabulation in templates

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in template.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
Comment 25 Fridolin Somers 2021-10-21 22:09:24 UTC
Created attachment 126683 [details] [review]
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter

Run prove t/Koha_Template_Plugin_Koha.t
Comment 26 Fridolin Somers 2021-10-21 22:09:47 UTC
Created attachment 126684 [details] [review]
Bug 28327: CSVdelimiter special case for tabulation in perl

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in perl.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
Comment 27 Fridolin Somers 2021-10-21 22:10:05 UTC
Created attachment 126685 [details] [review]
Bug 28327: (QA follow-up) Add POD
Comment 28 Fridolin Somers 2021-10-21 22:11:10 UTC
Fixed a typo in "CSVdelimiter special case for tabulation in perl" and rebase on current master
Comment 29 Nick Clemens (kidclamp) 2021-10-27 11:12:39 UTC
Created attachment 126964 [details] [review]
Bug 28327: CSVdelimiter special case for tabulation in templates

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in template.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Nick Clemens (kidclamp) 2021-10-27 11:12:43 UTC
Created attachment 126965 [details] [review]
Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter

Run prove t/Koha_Template_Plugin_Koha.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Nick Clemens (kidclamp) 2021-10-27 11:12:46 UTC
Created attachment 126966 [details] [review]
Bug 28327: CSVdelimiter special case for tabulation in perl

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch fixed when preference is fetch in perl.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 32 Nick Clemens (kidclamp) 2021-10-27 11:12:49 UTC
Created attachment 126967 [details] [review]
Bug 28327: (QA follow-up) Add POD

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Jonathan Druart 2021-12-01 14:55:20 UTC
It would have been better to have a new method in Koha.Config that could be called from all the other places as well:

admin/aqplan.pl:$del = "\t" if $del eq 'tabulation';
misc/cronjobs/overdue_notices.pl:    $sep_char = "\t" if ($sep_char eq 'tabulation');
misc/cronjobs/overdue_notices.pl:            $delimiter = "\t" if $delimiter eq 'tabulation';
misc/export_borrowers.pl:    $separator = "\t" if ($separator eq 'tabulation');
reports/acquisitions_stats.pl:$sep = "\t" if ($sep eq 'tabulation');
reports/bor_issues_top.pl:$sep = "\t" if ($sep eq 'tabulation');
reports/borrowers_out.pl:$sep = "\t" if ($sep eq 'tabulation');
reports/borrowers_stats.pl:$sep = "\t" if ($sep and $sep eq 'tabulation');
reports/cash_register_stats.pl:        $delimiter = "\t" if $delimiter eq 'tabulation';
reports/cat_issues_top.pl:$sep = "\t" if ($sep eq 'tabulation');
reports/catalogue_stats.pl:$sep = "\t" if ($sep eq 'tabulation');
reports/guided_reports.pl:                $delimiter = "\t" if $delimiter eq 'tabulation';
reports/issues_avg_stats.pl:$sep = "\t" if ($sep eq 'tabulation');
reports/issues_stats.pl:$sep = "\t" if ($sep eq 'tabulation');
reports/orders_by_fund.pl:        $sep = "\t" if ($sep eq 'tabulation');
reports/reserves_stats.pl:$sep = "\t" if ($sep eq 'tabulation');
reports/serials_stats.pl:$sep = "\t" if ($sep eq 'tabulation');
t/Koha_Template_Plugin_Koha.t:    is($plugin->CSVDelimiter(), "\t", "CSVDelimiter() returns \\t when preference is tabulation");
t/Koha_Template_Plugin_Koha.t:    is($plugin->CSVDelimiter('tabulation'), "\t", "CSVDelimiter(arg) returns \\t value when arg is tabulation");
tools/viewlog.pl:            $delimiter = "\t" if $delimiter eq 'tabulation';


With this patch we are adding yet one more occurrences...
Comment 34 Marcel de Rooy 2022-03-18 08:27:10 UTC
From the third patch:

             my $delimiter = C4::Context->preference('CSVDelimiter') || ';';
+            $delimiter = "\t" if $delimiter eq 'tabulation';

         my $delimiter = C4::Context->preference('CSVDelimiter') || ',';
+        $delimiter = "\t" if $delimiter eq 'tabulation';

+            my $delimiter = C4::Context->preference('CSVDelimiter') || ',';
+            $delimiter = "\t" if $delimiter eq 'tabulation';

In addition to last comment of Jonathan, if we are adding a function now to control behavior, we should not need to 'fiddle' anymore like in the above lines. Call the function and thats it.

Which probably means move the function out of the template plugin, and only leave a wrapper there.
Comment 35 Fridolin Somers 2022-03-22 07:54:46 UTC
Created attachment 131995 [details] [review]
Bug 28327: Unify CSV delimiter special behavior for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch adds C4::Context->csv_delimiter to add a uniq metod dealing
with this behavior.
Also create Koha::Template::Plugin::Koha->CSVDelimiter for calls from
Toolkit Templates.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
4) Check with another delimiter
Comment 36 Fridolin Somers 2022-03-22 07:55:14 UTC
Created attachment 131996 [details] [review]
Bug 28327: Add unit tests
Comment 37 Fridolin Somers 2022-03-22 07:55:33 UTC
Created attachment 131997 [details] [review]
Bug 28327: Unify CSV delimiter special behavior for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch adds C4::Context->csv_delimiter to add a uniq metod dealing
with this behavior.
Also create Koha::Template::Plugin::Koha->CSVDelimiter for calls from
Toolkit Templates.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
4) Check with another delimiter
Comment 38 Fridolin Somers 2022-03-22 07:57:27 UTC
I propose to add C4::Context->csv_delimiter.

This changes patch a lot so I have squashed all of them before amending.
Special thanks to you Nick for your help on this.
Comment 39 Jonathan Druart 2022-03-22 10:33:42 UTC
(In reply to Fridolin Somers from comment #38)
> I propose to add C4::Context->csv_delimiter.

Why did you prefer C4::Context over a method in Koha::Config::SysPref?
Comment 40 Fridolin Somers 2022-03-22 19:42:00 UTC
Koha::Config::SysPref is an object representing an existing preference.
How whould we call the new method ?
Remember sometimes the separator value is already existing, we don't need the preference, just transform tabulation.
Comment 41 David Nind 2022-06-19 08:06:22 UTC
Patch no longer applies 8-(..

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28327: Add unit tests
Applying: Bug 28327: Unify CSV delimiter special behavior for tabulation
Using index info to reconstruct a base tree...
M	C4/Context.pm
M	koha-tmpl/intranet-tmpl/prog/en/includes/catalogue/itemsearch_item.csv.inc
M	koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/catalogue/itemsearch.tt
M	reports/cash_register_stats.pl
M	reports/guided_reports.pl
Falling back to patching base and 3-way merge...
Auto-merging reports/guided_reports.pl
Auto-merging reports/cash_register_stats.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/catalogue/itemsearch.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/catalogue/itemsearch_item.csv.inc
Auto-merging C4/Context.pm
CONFLICT (content): Merge conflict in C4/Context.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28327: Unify CSV delimiter special behavior for tabulation
Comment 42 Fridolin Somers 2022-06-20 20:50:43 UTC
Created attachment 136378 [details] [review]
Bug 28327: Add unit tests
Comment 43 Fridolin Somers 2022-06-20 20:51:43 UTC
Created attachment 136379 [details] [review]
Bug 28327: Unify CSV delimiter special behavior for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch adds C4::Context->csv_delimiter to add a uniq metod dealing
with this behavior.
Also create Koha::Template::Plugin::Koha->CSVDelimiter for calls from
Toolkit Templates.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
4) Check with another delimiter
Comment 44 Fridolin Somers 2022-06-20 20:59:52 UTC
Created attachment 136380 [details] [review]
Bug 28327: Add unit tests
Comment 45 Fridolin Somers 2022-06-20 21:00:07 UTC
Created attachment 136381 [details] [review]
Bug 28327: Unify CSV delimiter special behavior for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch adds C4::Context->csv_delimiter to add a uniq metod dealing
with this behavior.
Also create Koha::Template::Plugin::Koha->CSVDelimiter for calls from
Toolkit Templates.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
4) Check with another delimiter
Comment 46 Nick Clemens (kidclamp) 2022-06-21 11:25:48 UTC
(In reply to Fridolin Somers from comment #45)
> Created attachment 136381 [details] [review] [review]
> Bug 28327: Unify CSV delimiter special behavior for tabulation

It looks like you default to ';' but the majority of code you change defaulted to ',' - can you explain what made you choose seimcolon? I would think comma makes a better default
Comment 47 Fridolin Somers 2022-06-23 08:48:42 UTC
(In reply to Nick Clemens from comment #46)
> (In reply to Fridolin Somers from comment #45)
> > Created attachment 136381 [details] [review] [review] [review]
> > Bug 28327: Unify CSV delimiter special behavior for tabulation
> 
> It looks like you default to ';' but the majority of code you change
> defaulted to ',' - can you explain what made you choose seimcolon? I would
> think comma makes a better default

Mmm it is the default value in installer/data/mysql/mandatory/sysprefs.sql.

Actual fallback values is often comma indeed :
 > git grep "CSVDelimiter') ||" | grep -v misc/translator/
koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/acqui/lateorders.tt:[%- SET delimiter = Koha.Preference('CSVDelimiter') || ',' -%]
koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/catalogue/itemsearch.tt:[%- SET delimiter = Koha.Preference('CSVDelimiter') || ',' -%]
misc/cronjobs/fines.pl:my $delim = "\t";    # ?  C4::Context->preference('CSVDelimiter') || "\t";
misc/cronjobs/overdue_notices.pl:    my $sep_char = C4::Context->preference('CSVDelimiter') || ';';
misc/cronjobs/overdue_notices.pl:            my $delimiter = C4::Context->preference('CSVDelimiter') || ';';
misc/export_borrowers.pl:    $separator = C4::Context->preference('CSVDelimiter') || ',';
reports/bor_issues_top.pl:our $sep     = $input->param("sep") || C4::Context->preference('CSVDelimiter') || ',';
reports/cash_register_stats.pl:        my $delimiter = C4::Context->preference('CSVDelimiter') || ',';
reports/guided_reports.pl:            my $delimiter = C4::Context->preference('CSVDelimiter') || ',';
tools/viewlog.pl:        my $delimiter = C4::Context->preference('CSVDelimiter') || ',';

May we change installer ?
Comment 48 David Nind 2022-06-23 13:19:18 UTC
Test fail for t/Koha_Template_Plugin_Koha.t:

prove t/Koha_Template_Plugin_Koha.t
t/Koha_Template_Plugin_Koha.t .. 1/5 
    #   Failed test 'CSVDelimiter() returns comma when preference is empty string'
    #   at t/Koha_Template_Plugin_Koha.t line 99.
    #          got: ';'
    #     expected: ','

    #   Failed test 'CSVDelimiter() returns comma when preference is undefined'
    #   at t/Koha_Template_Plugin_Koha.t line 102.
    #          got: ';'
    #     expected: ','
    # Looks like you failed 2 tests of 8.

#   Failed test 'Koha::Template::Plugin::Koha::CSVDelimiter tests'
#   at t/Koha_Template_Plugin_Koha.t line 115.
# Looks like you failed 1 test of 5.
t/Koha_Template_Plugin_Koha.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/5 subtests 

Test Summary Report
-------------------
t/Koha_Template_Plugin_Koha.t (Wstat: 256 Tests: 5 Failed: 1)
  Failed test:  5
  Non-zero exit status: 1
Files=1, Tests=5,  0 wallclock secs ( 0.03 usr  0.00 sys +  0.42 cusr  0.04 csys =  0.49 CPU)
Result: FAIL

Like Nick, I think comma separated makes more sense to me as the default. (comment #46).

I tested a couple of places and exported as tab separated - now works as per the test plan (properly tab separated, not the word tabulation). Places tested: Item search, Log viewer.
Comment 49 Fridolin Somers 2022-06-24 07:42:46 UTC
Created attachment 136473 [details] [review]
Bug 28327: Add unit tests
Comment 50 Fridolin Somers 2022-06-24 07:43:09 UTC
Created attachment 136474 [details] [review]
Bug 28327: Unify CSV delimiter special behavior for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch adds C4::Context->csv_delimiter to add a uniq metod dealing
with this behavior.
Also create Koha::Template::Plugin::Koha->CSVDelimiter for calls from
Toolkit Templates.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
4) Check with another delimiter
Comment 51 Fridolin Somers 2022-06-24 07:43:24 UTC
Created attachment 136475 [details] [review]
Bug 28327: (follow-up) Comma as default CSV delimiter

Looks like most of existing code wants comma as default value.

Also impact installer/data/mysql/mandatory/sysprefs.sql.
Comment 52 Fridolin Somers 2022-06-24 07:45:51 UTC
Good catch David and Nick, thanks a lot for your help
Comment 53 David Nind 2022-06-24 22:15:23 UTC
Created attachment 136535 [details] [review]
Bug 28327: Add unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 54 David Nind 2022-06-24 22:15:29 UTC
Created attachment 136536 [details] [review]
Bug 28327: Unify CSV delimiter special behavior for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch adds C4::Context->csv_delimiter to add a uniq metod dealing
with this behavior.
Also create Koha::Template::Plugin::Koha->CSVDelimiter for calls from
Toolkit Templates.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
4) Check with another delimiter

Signed-off-by: David Nind <david@davidnind.com>
Comment 55 David Nind 2022-06-24 22:15:35 UTC
Created attachment 136537 [details] [review]
Bug 28327: (follow-up) Comma as default CSV delimiter

Looks like most of existing code wants comma as default value.

Also impact installer/data/mysql/mandatory/sysprefs.sql.

Signed-off-by: David Nind <david@davidnind.com>
Comment 56 David Nind 2022-06-24 22:36:57 UTC
Testing notes:

1. I hope I have got the text for the release notes correct - I wasn't sure how to describe the changes to all the areas where the CSV export is used/was updated (apart from listing them all).

2. These are a couple of things I noted - these are outside the scope of this bug though:
   - In areas where DataTables is used (which has its own export function), is it now necessary to have an output section (output to screen and to file)? For example, the log viewer. 
   - For the log viewer, the columns that are exported when using the output area export file are different to that displayed on the screen:
     . Screen: Date, Librarian, Module,	Action, Object, Info, Interface
     . Output to file from the output selection area: action, barcode, biblioitemnumber, biblionumber, info, interface, module, object, timestamp, user
     . DataTables export: same as for screen

3. I only tested a couple of areas after the patch was applied: item search, log viewer, and the circulation report (after adding a few check outs).
Comment 57 Kyle M Hall (khall) 2022-08-19 11:40:26 UTC
Created attachment 139479 [details] [review]
Bug 28327: Add unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 58 Kyle M Hall (khall) 2022-08-19 18:38:53 UTC
Created attachment 139513 [details] [review]
Bug 28327: Add unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 59 Kyle M Hall (khall) 2022-08-19 18:40:10 UTC
Created attachment 139514 [details] [review]
Bug 28327: Add unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 60 Kyle M Hall (khall) 2022-08-19 18:40:28 UTC
Created attachment 139515 [details] [review]
Bug 28327: Unify CSV delimiter special behavior for tabulation

System preference 'CSVdelimiter' has a special case for tabulation.
Preference value contains string 'tabulation' but string '\t' must be used in CSV file.

This is OK in many places, for exemple Bug 17590.

This patch adds C4::Context->csv_delimiter to add a uniq metod dealing
with this behavior.
Also create Koha::Template::Plugin::Koha->CSVDelimiter for calls from
Toolkit Templates.

Test plan :
1) Set system preference 'CSVdelimiter' = 'tabs'.
2) Create CSV export in impacted pages
3) Check columns are separated by tabulation character and not string 'tabulation'
4) Check with another delimiter

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 61 Kyle M Hall (khall) 2022-08-19 18:40:33 UTC
Created attachment 139516 [details] [review]
Bug 28327: (follow-up) Comma as default CSV delimiter

Looks like most of existing code wants comma as default value.

Also impact installer/data/mysql/mandatory/sysprefs.sql.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 62 Tomás Cohen Arazi (tcohen) 2022-08-19 19:04:51 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 63 Lucas Gass (lukeg) 2022-10-14 19:34:17 UTC
Backported to 22.05.x for 22.05.06
Comment 64 Victor Grousset/tuxayo 2022-10-21 22:18:58 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.
Comment 65 Arthur Suzuki 2022-10-22 22:07:09 UTC
Thanks!

Pushed to 21.11 for 21.11.12
Comment 66 Katrin Fischer 2022-10-23 09:27:32 UTC
Something is odd here, how can this be in 22.11 and 21.11, but not in 22.05?
Comment 67 Katrin Fischer 2022-10-23 09:28:28 UTC
Fixed the Versions from the comments.
Comment 68 Fridolin Somers 2023-03-29 08:11:48 UTC
*** Bug 28363 has been marked as a duplicate of this bug. ***