Bug 13601

Summary: Get rid of DateTime::Format::DateParse
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: colin.campbell, katrin.fischer, mtompset, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11120
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10995
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13813
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13933
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10423
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12847
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14794
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14016, 14194    
Attachments: Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse
Bug 13601: Make dt_from_string not using DateTime::Format::DateParse
Bug 13601: get rid of DateTime::Format::DateParse
Bug 13601: Add a fallback check for compability with existing code
Bug 13601: Fix special case in basket.pl
Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse
Bug 13601: Make dt_from_string not using DateTime::Format::DateParse
Bug 13601: get rid of DateTime::Format::DateParse
Bug 13601: Add a fallback check for compability with existing code
Bug 13601: Fix special case in basket.pl
[Signed-off] Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse
[Signed-off] Bug 13601: Make dt_from_string not using DateTime::Format::DateParse
[Signed-off] Bug 13601: get rid of DateTime::Format::DateParse
[Signed-off] Bug 13601: Add a fallback check for compability with existing code
Bug 13601: Fix special case in basket.pl
Bug 13601: Fix special case in basket.pl
[Signed-off] Bug 13601: Fix special case in basket.pl
Bug 13601: t/Letters.t needs to mock the dateformat pref
[Signed-off] Bug 13601: t/Letters.t needs to mock the dateformat pref
Bug 13601: The fallback regex should contain the time part
[Signed-off] Bug 13601: The fallback regex should contain the time part
[PASSED QA] Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse
[PASSED QA] Bug 13601: Make dt_from_string not using DateTime::Format::DateParse
[PASSED QA] Bug 13601: get rid of DateTime::Format::DateParse
[PASSED QA] Bug 13601: Add a fallback check for compability with existing code
[PASSED QA] Bug 13601: Fix special case in basket.pl
[PASSED QA] Bug 13601: t/Letters.t needs to mock the dateformat pref
[PASSED QA] Bug 13601: The fallback regex should contain the time part

Description Jonathan Druart 2015-01-20 14:22:04 UTC
For a couple of reasons, dt_from_string should not use DateTime::Format::DateParse:
1/ It does not manage date < 1900 (certainly caused by l.47 of this module:
     $p{ year }      = $year ? $year + 1900 : DateTime->now->year;
2/ It considers 31/01/2015 as a valid us date, which is not.
Comment 1 Jonathan Druart 2015-01-20 15:12:20 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-01-20 15:12:24 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-01-20 15:12:32 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-01-20 15:12:45 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-01-20 15:14:20 UTC
Colin,
As you are the main author of Koha::DateUtils, I really would like to get your opinion on this patch set :)
Especially on the QA comment (second patch).
Comment 6 Colin Campbell 2015-01-20 16:21:40 UTC
(In reply to Jonathan Druart from comment #0)
> For a couple of reasons, dt_from_string should not use
> DateTime::Format::DateParse:
> 1/ It does not manage date < 1900 (certainly caused by l.47 of this module:
>      $p{ year }      = $year ? $year + 1900 : DateTime->now->year;
> 2/ It considers 31/01/2015 as a valid us date, which is not.

The reason dates before 1900 are not handled is in the doc for Date::Parse 

    Date::Parse uses Time::Local internally, so is limited to only parsing
    dates which result in valid values for Time::Local::timelocal. This
    generally means dates between 1901-12-17 00:00:00 GMT and 2038-01-16
    23:59:59 GMT

That wasnt a problem with DateUtils as it was just ensuring the old C4::Dates routines could be handled which were also unable to handle pre 1900 dates
Comment 7 Colin Campbell 2015-01-20 16:24:58 UTC
(In reply to Jonathan Druart from comment #2)
> Created attachment 35406 [details] [review] [review]
> Bug 13601: Make dt_from_string not using DateTime::Format::DateParse
> 
> For a couple of reasons, dt_from_string should not use
> DateTime::Format::DateParse:
> 1/ It does not manage date < 1900, certainly caused by l.47 of this
> module:
>  $p{ year }      = $year ? $year + 1900 : DateTime->now->year;
> 
> 2/ It considers 31/01/2015 as a valid us date, which is not.
> 
> Test plan:
> 1/ Verify that
>   prove t/DateUtils.t
> returns green
> 2/ Play with dates in Koha (yes I know, it's vague...)
> 3/ Try to find a regression with dates
> 4/ Create a date with year <= 1900 and confirm it works
> 
> QA comment:
> Why the sql format switch was:
> -                $date_string =~
> -s/(\d{4})(\d{2})(\d{2})\s+(\d{2})(\d{2})(\d{2})/$1-$2-$3T$4:$5:$6/;
> 
> From where a date like "yyyymmdd hhmmss" can come?

C4::Dates supported the format so DateUtils had to accept it.
Comment 8 Jonathan Druart 2015-01-21 11:36:03 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-01-21 11:37:12 UTC
> display date on 4 digits and month/day on 2 digits.

It was

display *year* on 4 digits and month/day on 2 digits.
Comment 10 Mark Tompsett 2015-03-07 15:49:20 UTC
There are conflicts in:
- t/DateUtils.t
- Koha/DateUtils.pm
- and something sha1 blob fatal in basket.pl
Comment 11 Jonathan Druart 2015-03-09 08:39:59 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-03-09 08:40:02 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-03-09 08:40:06 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-03-09 08:40:09 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-03-09 08:40:13 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-03-09 08:42:11 UTC
Rebase not trivial, please check there is no regression with bug 12669.
Comment 17 Marc Véron 2015-03-09 17:12:43 UTC
Patch did not apply:

Applying: Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse
Applying: Bug 13601: Make dt_from_string not using DateTime::Format::DateParse
Applying: Bug 13601: get rid of DateTime::Format::DateParse
Applying: Bug 13601: Add a fallback check for compability with existing code
Applying: Bug 13601: Fix special case in basket.pl
fatal: corrupt patch at line 55
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 13601: Fix special case in basket.pl
Comment 18 Marc Véron 2015-03-09 17:53:28 UTC Comment hidden (obsolete)
Comment 19 Marc Véron 2015-03-09 18:09:03 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2015-03-09 18:13:37 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2015-03-09 18:23:27 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2015-03-09 18:27:40 UTC
After signing off the first 4 patches I tried again with "Fix secial case in basket.pl", this one still does not apply with:
fatal: corrupt patch at line 55
Comment 23 Jonathan Druart 2015-03-10 08:17:38 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-03-10 08:20:44 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-03-10 08:21:49 UTC
(In reply to Marc Véron from comment #22)
> After signing off the first 4 patches I tried again with "Fix secial case in
> basket.pl", this one still does not apply with:
> fatal: corrupt patch at line 55

I c/p a diff in the commit message, git does not like that :)
Should be fixed now (I added a tab at the beginning of the lines).
Comment 26 Marc Véron 2015-03-10 09:14:34 UTC Comment hidden (obsolete)
Comment 27 Marc Véron 2015-03-10 09:16:05 UTC
Hi Jonathan,

Is it possible to extract the regexp part in patch "Make dt_from_string not using DateTime::Format::DateParse" to a separate sub (as replacement for the regexp function in C4::Dates)?

Or should it be done in Bug 13813?

Marc
Comment 28 Jonathan Druart 2015-03-10 09:19:46 UTC
(In reply to Marc Véron from comment #27)
> Hi Jonathan,
> 
> Is it possible to extract the regexp part in patch "Make dt_from_string not
> using DateTime::Format::DateParse" to a separate sub (as replacement for the
> regexp function in C4::Dates)?
> 
> Or should it be done in Bug 13813?
> 
> Marc

Hello Marc,
I would prefer not to add any other changes to this patch set.
Comment 29 Marc Véron 2015-03-10 09:39:04 UTC
Ok, I set to Signed-off and hope that it will be soon QAed and pushed to master :-)
Comment 30 Katrin Fischer 2015-03-15 22:18:06 UTC
This breaks t/Letters.t - tests pass OK on master:

perl t/Letters.t 
1..5
ok 1 - use C4::Letters;
ok 2 - letter name for "ISBN" letter is book
ok 3 - GetLetters returns the 2 inserted letters
Use of uninitialized value $date_format in string eq at /home/katrin/kohaclone/Koha/DateUtils.pm line 79.
Use of uninitialized value $date_format in string eq at /home/katrin/kohaclone/Koha/DateUtils.pm line 89.
Use of uninitialized value $date_format in string eq at /home/katrin/kohaclone/Koha/DateUtils.pm line 99.
Use of uninitialized value $date_format in string eq at /home/katrin/kohaclone/Koha/DateUtils.pm line 99.
Use of uninitialized value $date_format in concatenation (.) or string at /home/katrin/kohaclone/Koha/DateUtils.pm line 104.
Invalid dateformat parameter () at /home/katrin/kohaclone/Koha/DateUtils.pm line 104.
# Looks like you planned 5 tests but ran 3.
# Looks like your test exited with 255 just after 3.
Comment 31 Jonathan Druart 2015-03-16 13:55:28 UTC Comment hidden (obsolete)
Comment 32 Marc Véron 2015-03-16 14:38:14 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2015-03-16 22:14:46 UTC
I am having problems checking out items with a certain due time with this date.

For a test I am checking out an item, with the due date specified to:
19/03/2015 07:45
But the item checks out with:
19/03/2015 00:00

But... in the database it shows the correct due date, it's just not being displayed?
Comment 34 Jonathan Druart 2015-03-17 08:47:01 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2015-03-17 08:48:15 UTC
(In reply to Katrin Fischer from comment #33)
> I am having problems checking out items with a certain due time with this
> date.
> 
> For a test I am checking out an item, with the due date specified to:
> 19/03/2015 07:45
> But the item checks out with:
> 19/03/2015 00:00
> 
> But... in the database it shows the correct due date, it's just not being
> displayed?

Good catch Katrin!
All sql dates were impacted.
Comment 36 Marc Véron 2015-03-20 08:59:46 UTC Comment hidden (obsolete)
Comment 37 Katrin Fischer 2015-03-22 19:56:59 UTC
Created attachment 37087 [details] [review]
[PASSED QA] Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse

Test behaves as expected (Fail as expected  with first patch only, OK after 3rd patch)
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2015-03-22 19:57:02 UTC
Created attachment 37088 [details] [review]
[PASSED QA] Bug 13601: Make dt_from_string not using DateTime::Format::DateParse

For a couple of reasons, dt_from_string should not use
DateTime::Format::DateParse:
1/ It does not manage date < 1900, certainly caused by l.47 of this
module:
 $p{ year }      = $year ? $year + 1900 : DateTime->now->year;

2/ It considers 31/01/2015 as a valid us date, which is not.

Test plan:
1/ Verify that
  prove t/DateUtils.t
returns green
2/ Play with dates in Koha (yes I know, it's vague...)
3/ Try to find a regression with dates
4/ Create a date with year <= 1900 and confirm it works

QA comment:
Why the sql format switch was:
-                $date_string =~
-s/(\d{4})(\d{2})(\d{2})\s+(\d{2})(\d{2})(\d{2})/$1-$2-$3T$4:$5:$6/;

From where a date like "yyyymmdd hhmmss" can come?

Tested patches 1 - 3 together. Worked as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2015-03-22 19:57:06 UTC
Created attachment 37089 [details] [review]
[PASSED QA] Bug 13601: get rid of DateTime::Format::DateParse

This module is used in C4::Members::GetPendingIssues too, but we can use
dt_from_string.

Test plan:
1/ Verify that
    prove t/db_dependent/Members/GetPendingIssues.t
returns green
2/ On the patron pending issue list, verify that the issue and the due
dates are correctly displayed.

Tested together with other patches (except "Fix special cases). Worked as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 Katrin Fischer 2015-03-22 19:57:09 UTC
Created attachment 37090 [details] [review]
[PASSED QA] Bug 13601: Add a fallback check for compability with existing code

There are a lot of places where the date comes from the DB but the
dateformat parameter is not set to 'sql'.
dt_from_string needs to fallback with this format if the pref format
does not match.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 41 Katrin Fischer 2015-03-22 19:57:13 UTC
Created attachment 37091 [details] [review]
[PASSED QA] Bug 13601: Fix special case in basket.pl

There is a badly managed date in acqui/basket.pl:
if the date is 15/01/2015 (metric format), it will become
2015-1-15 after the following line:
  $estimateddeliverydate = "$year-$month-$day";
Add_Delta_Days is used at several place, and the ouput is forced to
display date on 4 digits and month/day on 2 digits.
This patch does the same thing for $estimateddeliverydate.

Note that I previously developed a patch to take this format into account (with missing 0)
in Koha::DateUtils::dt_from_string, but I don't think it's a good idea
to manage bad formated dates.
We will certainly find some issues after previous patches, but it will permit to catch
them!
IMO it's preferable than to keep them hidden.

The patch was:
    diff --git a/Koha/DateUtils.pm b/Koha/DateUtils.pm
    index 5fe2653..4434a67 100644
    --- a/Koha/DateUtils.pm
    +++ b/Koha/DateUtils.pm
    @@ -72,17 +72,17 @@ sub dt_from_string {
             my $fallback_re = qr|
                 (?<year>\d{4})
                 -
    -            (?<month>\d{2})
    +            (?<month>\d{1,2})
                 -
    -            (?<day>\d{2})
    +            (?<day>\d{1,2})
             |xms;

             if ( $date_format eq 'metric' ) {
                 # metric format is "dd/mm/yyyy[ hh:mm:ss]"
                 $regex = qr|
    -                (?<day>\d{2})
    +                (?<day>\d{1,2})
                     /
    -                (?<month>\d{2})
    +                (?<month>\d{1,2})
                     /
                     (?<year>\d{4})
                 |xms;
    @@ -90,9 +90,9 @@ sub dt_from_string {
             elsif ( $date_format eq 'us' ) {
                 # us format is "mm/dd/yyyy[ hh:mm:ss]"
                 $regex = qr|
    -                (?<month>\d{2})
    +                (?<month>\d{1,2})
                     /
    -                (?<day>\d{2})
    +                (?<day>\d{1,2})
                     /
                     (?<year>\d{4})
                 |xms;
    diff --git a/t/DateUtils.t b/t/DateUtils.t
    index 886e1d6..0877240 100755
    --- a/t/DateUtils.t
    +++ b/t/DateUtils.t
    @@ -189,3 +189,8 @@ is( output_pref( { dt => $dt } ), '31/01/2015 12:34', 'dt_from_string should mat
     # date before 1900
     $dt = dt_from_string('01/01/1900');
     is( output_pref( { dt => $dt, dateonly => 1 } ), '01/01/1900', 'dt_from_string should manage date < 1900' );
    +
    +# missing 0
    +$dt = dt_from_string('1/1/2015');
    +is( output_pref( { dt => $dt, dateonly => 1 } ), '01/01/2015', 'dt_from_string should generate a DT object even if 0 are missing' );

Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 42 Katrin Fischer 2015-03-22 19:57:16 UTC
Created attachment 37092 [details] [review]
[PASSED QA] Bug 13601: t/Letters.t needs to mock the dateformat pref

Test passes OK.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 43 Katrin Fischer 2015-03-22 19:57:21 UTC
Created attachment 37093 [details] [review]
[PASSED QA] Bug 13601: The fallback regex should contain the time part

On displaying a sql date, if the dateformat is not set to sql, the
output should contain the date.
In the previous version, the fallback regex (used for sql format) did not
included the time part (hh:mm:ss).

Checked out an item with due date an ddue time. Time displays as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 44 Tomás Cohen Arazi 2015-03-30 16:50:20 UTC
Patches pushed to master.

Good job Jonathan!