Bug 28327

Summary: System preference CSVdelimiter special case for tabulation
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: ToolsAssignee: Fridolin Somers <fridolin.somers>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: didier.gautheron, jonathan.druart+koha, m.de.rooy, nick
Version: master   
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
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 27486    
Bug Blocks: 28363    
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

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 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 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 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 2021-05-12 13:17:13 UTC
Created attachment 120869 [details] [review]
Bug 28327: (QA follow-up) Add POD
Comment 8 Nick Clemens 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 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 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 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 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 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.