Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed
Summary: tools/inventory.pl output_pref ignores 'iso' parameter passed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-14 19:55 UTC by Mark Tompsett
Modified: 2015-06-04 23:30 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:


Attachments
Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed (1.79 KB, patch)
2013-11-26 19:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed (2.13 KB, patch)
2013-11-26 20:41 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed (3.05 KB, patch)
2013-12-31 01:03 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed (3.11 KB, patch)
2014-02-26 18:05 UTC, wajasu
Details | Diff | Splinter Review
Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed (2.81 KB, patch)
2014-03-18 15:48 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11253 - tools/inventory.pl output_pref ignores 'iso' parameter passed (3.22 KB, patch)
2014-04-14 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
0001-PASSED-QA-Bug-11253-tools-inventory.pl-output_pref-i.patch (3.40 KB, patch)
2014-04-20 08:24 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2013-11-14 19:55:04 UTC
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.
Comment 1 Katrin Fischer 2013-11-24 20:38:49 UTC
Hi Mark,
I am not sure I understand from your description how this bug is visible in tools, could you add a test plan?
Comment 2 Mark Tompsett 2013-11-25 10:43:00 UTC
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?
Comment 3 Mark Tompsett 2013-11-25 20:50:10 UTC
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.
Comment 4 Mark Tompsett 2013-11-26 19:57:13 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2013-11-26 20:41:27 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2013-12-31 01:03:06 UTC Comment hidden (obsolete)
Comment 7 wajasu 2014-02-26 18:05:52 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-03-18 14:43:32 UTC
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.
Comment 9 Mark Tompsett 2014-03-18 15:48:17 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2014-04-14 10:06:42 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-04-14 10:20:35 UTC
Why the last past does not change the call to output_pref for the $cdls variable? It made sense!
Comment 12 Katrin Fischer 2014-04-20 08:23:38 UTC
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.
Comment 13 Katrin Fischer 2014-04-20 08:24:07 UTC
Created attachment 27317 [details] [review]
0001-PASSED-QA-Bug-11253-tools-inventory.pl-output_pref-i.patch
Comment 14 Galen Charlton 2014-04-20 15:07:48 UTC
Pushed to master.  Thanks, Mark!
Comment 15 Mark Tompsett 2014-04-21 01:11:06 UTC
(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).
Comment 16 Galen Charlton 2014-04-21 05:59:56 UTC
(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.
Comment 17 Fridolin Somers 2014-05-27 13:59:37 UTC
Pushed to 3.14.x, will be in 3.14.07