Description
Fridolin Somers
2021-05-12 09:43:11 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' Created attachment 120861 [details] [review] Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter Run prove t/Koha_Template_Plugin_Koha.t (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? (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'. 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> 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> Created attachment 120869 [details] [review] Bug 28327: (QA follow-up) Add POD This bug is ugly, moving to PQA as very trivial, thanks Frido! Wait I have more patches ;) 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' Created attachment 120871 [details] [review] Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter Run prove t/Koha_Template_Plugin_Koha.t 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' 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 ;)" Created attachment 120874 [details] [review] Bug 28327: (QA follow-up) Add POD Created attachment 120876 [details] [review] Bug 28135: Replace use of input type number in additem.js This patch corrects in instance where JavaScript is creating an input tag with type "number." To test, apply the patch and make sure the AcqCreateItem system preference is set to "when receiving." - Go to Acquisitions -> Vendor -> Invoices -> Invoice -> Go to receipt page -> Receive. - On the page for receiving items you should see an add item form. - Click the "Add multiple items" button at the bottom of the form. - Test the "Number of items to add" field. It should not accept any input except numbers. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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' Created attachment 120878 [details] [review] Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter Run prove t/Koha_Template_Plugin_Koha.t 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' 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 ;)" Created attachment 120881 [details] [review] Bug 28327: (QA follow-up) Add POD (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 (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 (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 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' Created attachment 126683 [details] [review] Bug 28327: Add unit tests for Koha::Template::Plugin::Koha::CSVDelimiter Run prove t/Koha_Template_Plugin_Koha.t 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' Created attachment 126685 [details] [review] Bug 28327: (QA follow-up) Add POD Fixed a typo in "CSVdelimiter special case for tabulation in perl" and rebase on current master 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> 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> 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> Created attachment 126967 [details] [review] Bug 28327: (QA follow-up) Add POD Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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... 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. 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 Created attachment 131996 [details] [review] Bug 28327: Add unit tests 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 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. (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? 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. |