I found this as a result of looking briefly at bug 11240. When the parameter was changed to a hash reference in bug 10380, I believe tools/inventory.pl was missed. It currently allowed for a non-hash to be passed, but assumes that is just the date. Any other parameters would be ignored. I am unsure of the correct course of action, whether the parameter needs to be passed, or whether the system preference is the preferred result. I'm looking further into this. I figure this is minor, since most people probably don't change the dateformat system preference to non-iso, and don't run tools/inventory.pl frequently.
Hi Mark, I am not sure I understand from your description how this bug is visible in tools, could you add a test plan?
tools/inventory.pl passes two parameters a DateTime and 'iso'. But output_pref in Koha/DateUtils.pm only pays attention to the first parameter for non-hash parameters. This means the results would be affected by the I18N/L10N preferences dateformat system preference. 1) Should it be affected by the system preference like that? OR 2) Should it always be iso regardless of the system preference? NOTE: 'it' being the output of the clicking submit on the tools inventory page, as far as I can tell. If (1), then 'iso' should be chopped from tools/inventory.pl. If (2), then the second parameter should be checked in Koha::DateUtils. As an aside, I think I found another bug while trying to create a sample test plan. Could someone else confirm that going to the inventory tool page and just clicking submit without actually uploading any barcodes generates a 500 error?
Create a small barcode file from books in a given branch. Set system preference of dateformat to mm/dd/yyyy. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column. Set system preference of dateformat to yyyy-mm-dd. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays nothing in "Unseen since" column. Set system preference of dateformat to dd/mm/yyyy. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit KABOOM! I would expect to see dates all the time. I would also expect that it is formatted either (a) according to the system preference, or (b) YYYY-MM-DD. But generally, the second parameter which is supposed to be setting 'iso' is ignored completely, and this weirdness ensues.
Created attachment 23179 [details] [review] Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed Took all the output_pref function calls and converted them to the newer hash format parameter.
Created attachment 23180 [details] [review] Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed Took all the output_pref function calls and converted them to the newer hash format parameter. Added special case for dd-mm-yyyy calls, so dt_from_string works correctly.
Created attachment 23876 [details] [review] Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed Took all the output_pref function calls and converted them to the newer hash format parameter. Added special case for dd-mm-yyyy calls, so dt_from_string works correctly. TEST PLAN --------- Create a small barcode file from books in a given branch. Set I18N/L10N system preference of dateformat to mm/dd/yyyy. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column. Set I18N/L10N system preference of dateformat to yyyy-mm-dd. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column. Set I18N/L10N system preference of dateformat to dd/mm/yyyy. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit KABOOM! Apply patch Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column.
Created attachment 25679 [details] [review] [SIGNED-OFF] Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed Created a SQL report select barcode from items; my set was small. Ran it and downloaded Then ran test plan before and after patch, seeing desired results.
Comment on attachment 25679 [details] [review] [SIGNED-OFF] Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed Review of attachment 25679 [details] [review]: ----------------------------------------------------------------- ::: tools/inventory.pl @@ +150,5 @@ > my @errorloop; > if ( $uploadbarcodes && length($uploadbarcodes) > 0 ) { > my $dbh = C4::Context->dbh; > + my $date; > + if (C4::Context->preference('dateformat') eq 'dd/mm/yyyy') { This test is done in dt_from_string. You should not check the dateformat pref here.
Created attachment 26437 [details] [review] Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed Took all the output_pref function calls and converted them to the newer hash format parameter. This then gave blank output. Looking at the template file, I decided that the html filter should be after the KohaDates filter. This corrected the display of the date for the dd/mm/yyyy case. TEST PLAN --------- Create a small barcode file from books in a given branch. Set I18N/L10N system preference of dateformat to mm/dd/yyyy. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column. Set I18N/L10N system preference of dateformat to yyyy-mm-dd. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column. Set I18N/L10N system preference of dateformat to dd/mm/yyyy. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit KABOOM! Apply patch Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column.
Created attachment 27081 [details] [review] Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed Took all the output_pref function calls and converted them to the newer hash format parameter. This then gave blank output. Looking at the template file, I decided that the html filter should be after the KohaDates filter. This corrected the display of the date for the dd/mm/yyyy case. TEST PLAN --------- Create a small barcode file from books in a given branch. Set I18N/L10N system preference of dateformat to mm/dd/yyyy. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column. Set I18N/L10N system preference of dateformat to yyyy-mm-dd. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column. Set I18N/L10N system preference of dateformat to dd/mm/yyyy. Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit KABOOM! Apply patch Go to inventory/stocktaking tool. Browse for the barcode file. Select the branch (i.e. not All libraries) to match. Click submit Table displays dates in "Unseen since" column. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Before the patch, I had this error when using dateformat=ddmmyy: Template process failed: undef error - The 'day' parameter ("2014") to DateTime::new did not pass the 'an integer which is a possible valid day of month' callback at /usr/local/share/perl5/Params/Validate/PP.pm line 633. This patch corrects the problem.
Why the last past does not change the call to output_pref for the $cdls variable? It made sense!
Mark, please take a look at Jonathan's question in comment 11. As Marcel signed off on the existing patch and I can find no wrong with it, I am going to pass.
Created attachment 27317 [details] [review] 0001-PASSED-QA-Bug-11253-tools-inventory.pl-output_pref-i.patch
Pushed to master. Thanks, Mark!
(In reply to Jonathan Druart from comment #11) > Why the last past does not change the call to output_pref for the $cdls > variable? It made sense! Ah, I was looking at the line that caused the explosion. :) There are two lines, much later in the code, which could be tweaked. However, their disrespect for the iso parameter seems irrelevant, as dd/mm/yyyy didn't blow up (clicking submit button in the barcode section after choosing a file). I suspect that $_->{datelastseen} is in SQL format (YYYY-MM-DD) for the $cdls, and the first one's disrespect still generates a valid date for $dls. I think there is merit for a follow up if one wants truly nice code (i.e. not working by accident).
(In reply to M. Tompsett from comment #15) > I think there is merit for a follow up if one wants truly nice code (i.e. > not working by accident). To answer the question you posed in IRC, please open a new bug.
Pushed to 3.14.x, will be in 3.14.07