Bug 4402 - Date filter in parcels.tt requires db format dates
Summary: Date filter in parcels.tt requires db format dates
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-23 01:48 UTC by Colin Campbell
Modified: 2015-12-03 22:11 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 4402: Date filter on parcels.tt required db date format (2.67 KB, patch)
2015-01-06 15:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
[Signed-off] Bug 4402: Date filter on parcels.tt required db date format (2.87 KB, patch)
2015-01-06 16:33 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 4402: Date filter on parcels.tt required db date format (2.97 KB, patch)
2015-01-07 11:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4402: Date filter on parcels.tt required db date format (3.03 KB, patch)
2015-01-12 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4402: Date filter on parcels.tt required db date format (3.01 KB, patch)
2015-01-12 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4402: Date filter on parcels.tt required db date format (3.10 KB, patch)
2015-01-13 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 4402: Date filter on parcels.tt required db date format (3.19 KB, patch)
2015-03-03 11:24 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[PASSED QA] Bug 4402: Date filter on parcels.tt required db date format (3.23 KB, patch)
2015-03-05 10:34 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 Chris Cormack 2010-05-21 01:26:45 UTC


---- Reported by colin.campbell@ptfs-europe.com 2010-04-23 13:48:35 ----

Users cannot use the system format for dates in the filter specification. The date is passed direct to the select without being reformatted



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:26 UTC  ---

This bug was previously known as _bug_ 4402 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4402

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.

Comment 1 Owen Leonard 2013-01-09 14:16:10 UTC
This bug appears to still be valid.
Comment 2 Katrin Fischer 2014-03-15 07:57:47 UTC
This bug is still valid. The filters on the "Receive shipments" page of a vendor only work when you enter the date in the database format "2014-01-12". Also, no date widget is offered for selecting a date.
Comment 3 Katrin Fischer 2015-01-06 14:56:22 UTC
Still valid...
Comment 4 Katrin Fischer 2015-01-06 15:40:11 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2015-01-06 15:42:03 UTC
Not sure about the date handling - probably should have been using Koha::DateUtils? Feedback appreciated.
Comment 6 Marc Véron 2015-01-06 16:33:11 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-01-07 11:25:54 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-01-07 11:26:55 UTC
Katrin,
I resubmitted quite the same patch using the DateUtils module.
I Hope you are fine with it.
Comment 9 Katrin Fischer 2015-01-07 13:12:15 UTC
Hi Joubu,
trying to improve on my programming skills so hints are very welcome. I tried using the DateUtils, but it didn't work for me because what I had included a time at the end of the string. Have to take another look at the documentation.
Comment 10 Katrin Fischer 2015-01-12 06:24:10 UTC
Hm, the new patch does cause some weird side effects. When I access the bookseller parcels page via 'receive shipment' the old shipments/open invoices don't show up.

I think it's the same problem I've run into trying to avoid using C4::Dates, Koha::DateUtils date_to_string returns not only the date and it will also return "now" if no date is given:

2015-01-12T07:13:11 

And that won't work in the SQL.
Comment 11 Katrin Fischer 2015-01-12 06:30:19 UTC
I am still not sure how to reformat a date formatted to our dateformat pref and convert to iso format :(
Comment 12 Katrin Fischer 2015-01-12 06:37:35 UTC
... not using C4::Dates.
Comment 13 Jonathan Druart 2015-01-12 08:48:15 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-01-12 08:51:38 UTC
(In reply to Katrin Fischer from comment #10)
> Hm, the new patch does cause some weird side effects. When I access the
> bookseller parcels page via 'receive shipment' the old shipments/open
> invoices don't show up.
> 
> I think it's the same problem I've run into trying to avoid using C4::Dates,
> Koha::DateUtils date_to_string returns not only the date and it will also
> return "now" if no date is given:
> 
> 2015-01-12T07:13:11 
> 
> And that won't work in the SQL.

The date generated by dt_from_string(undef) is today, we don't want to generate a date if the date parameters are not set.
Comment 15 Jonathan Druart 2015-01-12 08:54:20 UTC
(In reply to Katrin Fischer from comment #11)
> I am still not sure how to reformat a date formatted to our dateformat pref
> and convert to iso format :(

my $dt = dt_from_string # Generate a DT for today (no date passed)
my $date_string_pref = output_pref( $dt ); # Generate the date string for the given DT, depending on the syspref.
my $date_string_iso = output_pref( $dt, 'iso' ); # Generate the date string for the given DT formatted to iso format.
Comment 16 Jonathan Druart 2015-01-12 09:18:28 UTC Comment hidden (obsolete)
Comment 17 Marc Véron 2015-01-13 10:56:38 UTC
I tested with dateformat dd/m/yyy
and entered manually:
From: 1/2/2014
To: 15/2/2014

The date picker came up for both fields. 
I canceled date picker both times and then hit "Filter".

Result:
-----
Software 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/lib/perl5/DateTime.pm line 199
	DateTime::new(undef, 'month', 2, 'day', 2014, 'year', 1915, 'time_zone', 'Europe/Zurich', ...) called at /usr/share/perl5/DateTime/Format/DateParse.pm line 58
	DateTime::Format::DateParse::parse_datetime('DateTime::Format::DateParse', '15/02/2014', 'Europe/Zurich') called at /usr/share/kohaclone/Koha/DateUtils.pm line 92
	Koha::DateUtils::dt_from_string('15/02/2014', 'iso') called at /usr/share/kohaclone/acqui/parcels.pl line 145
-----

If I do the same without patch, no error occurs.
Comment 18 Jonathan Druart 2015-01-13 11:38:02 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2015-01-13 11:40:24 UTC
(In reply to Marc Véron from comment #17)
> I tested with dateformat dd/m/yyy
> and entered manually:
> From: 1/2/2014
> To: 15/2/2014
> 
> The date picker came up for both fields. 
> I canceled date picker both times and then hit "Filter".

Sorry, the patch was stupid.
A DateTime object was sent to GetInvoices, instead of a date string in iso format.
Comment 20 Marc Véron 2015-01-14 13:45:02 UTC
Sorry, I can still provocate a crash bei entering an invalid date (e.g.invalid month) and skipping the date picker.

Example:

From: 01/12/2014  => Works OK

From: 01/16/2014 => Crash

Software 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/lib/perl5/DateTime.pm line 199
	DateTime::new(undef, 'day', 2014, 'year', 1916, 'time_zone', 'Europe/Zurich') called at /usr/share/perl5/DateTime/Format/DateParse.pm line 58
	DateTime::Format::DateParse::parse_datetime('DateTime::Format::DateParse', '16/01/2014', 'Europe/Zurich') called at /usr/share/kohaclone/Koha/DateUtils.pm line 92
	Koha::DateUtils::dt_from_string('01/16/2014') called at /usr/share/kohaclone/acqui/parcels.pl line 145
Comment 21 Katrin Fischer 2015-01-14 14:00:49 UTC
Hm, maybe we could catch the error and just give back a message that an invalid date was entered?
Comment 22 Jonathan Druart 2015-01-14 14:14:46 UTC
Not sure it's valid.
Koha does not catch this kind of error. You are not suppose to enter an invalid date :)
That's why we use a date picker.
Comment 23 Katrin Fischer 2015-01-14 14:20:56 UTC
I think maybe out of scope here, but we should have a general solution. The datepicker is not accessible, I had to unlock a few fields for a library because of that. But I also have reports from other libraries that complain about having to use the datepicker for dates and feel entering manually is much easier.
Comment 24 Jonathan Druart 2015-01-14 14:30:40 UTC
The input is not readonly, so they are able to enter a date.
I agree with what you said, it should be fixed.
But not here :)
Comment 25 Katrin Fischer 2015-01-14 14:48:09 UTC
I agree.
Comment 26 Marc Véron 2015-01-14 14:57:34 UTC
I agree with Jonathan that I am not supposed to give a wrong date. I did it accidentally, and other users will do that for sure :-)
Koha should handle it gracefully and not crash with a software error.

And I agree with Katrin that we should have a more general solution for fields that involve DatePicker. This would be an other bug. (Keep in mind Bug 11448 /Accessibility)

At other places the date is validated by JavaScript, e.g. on memberentry.pl. 
If I enter a wrong month for date of birth, I get a warning "Invalid month entered in field dateofbirth"

The related JavaScript function CheckDate() comes from /intranet-tmpl/prog/en/js/members.js  (= local solution)

For this bug the question is:

Keep DatePicker and have the chance for crashes if a user skips the DatePicker with a wrong date? If yes, I'll sign-off.

Or keep it the old way (no DatePicker, no crash) until we have a more general solution?
Comment 27 Katrin Fischer 2015-01-14 15:08:22 UTC
I think the datepicker is not the problem here - that's just the way data gets into the field. The date processing in the .pl is what throws the error. It didn't before, because there was no date processing :(
Comment 28 Marc Véron 2015-01-14 15:46:02 UTC
The crash occurs in Koha/DateUtils.pm

DateUtils.pm line 92, sub dt_from_string 

return DateTime::Format::DateParse->parse_datetime( $date_string,
            $tz->name() );

Description:
Passed a date string returns a DateTime object format and timezone default
to the system preferences. If the date string is empty DateTime->now is returned

I propose to have a new bug to change the function for testing the validity of the string and return DateTime->now as well if the date string is not valid.
Validation could be done e.g. with DateTime::Format::DateManip->parse_datetime ( $datestring ).
Comment 29 Jonathan Druart 2015-01-14 15:50:12 UTC
The easier thing to do would be to add a check on all inputs managed by datepicker to validate what the user filled.
Comment 30 Katrin Fischer 2015-03-03 08:40:12 UTC
Jonathan, I am not sure I understand what you propose in your last comment - should this go on a separate bug? I'd really like to see this fixed, but not sure how to move on.
Comment 31 Jonathan Druart 2015-03-03 10:33:21 UTC
(In reply to Katrin Fischer from comment #30)
> Jonathan, I am not sure I understand what you propose in your last comment -
> should this go on a separate bug? I'd really like to see this fixed, but not
> sure how to move on.

Yes, it is not in the scope of this bug.
I suggested to add a global JS rule to add a check on all input dates.
Comment 32 Nicole C. Engard 2015-03-03 11:24:16 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2015-03-05 10:34:23 UTC
Created attachment 36549 [details] [review]
[PASSED QA] Bug 4402: Date filter on parcels.tt required db date format

The date filters on the parcel page would only work with
dates formatted YYYY-MM-DD.

To test:
- Select a vendor, that already has a few invoices
- "Receive shipment" - you are on the parcels page
- Use the From and To filters on the left, notice there
  is now a date picker on those fields
- Verify the search works correctly for different date
  formats

Signed-off-by: Nicole <nicole@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Tomás Cohen Arazi 2015-03-15 12:12:46 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 35 Chris Cormack 2015-03-21 21:56:40 UTC
Template changes held for 3.18.6