Bug 17512: Improve handling dates in C4::Items

This is a follow-up on the internal server error on 0000-00-00 in the items
column onloan. This patch deals with preventing to have such dates at all
in the date fields of items.

It is accomplished by:
[1] Adding a (private) subroutine _mod_item_dates. It takes an item hash
    and replaces date values if needed.
[2] AddItem and ModItem call _koha_new_item resp. koha_modify_item. In these
    routines a call to the new _mod_item_dates is inserted.
[3] Although the routine is actually private, I have added some unit tests
    to Items.t.

Test plan:
[1] Add a new item. Fill a correct date in dateaccessioned and an invalid
    date in Price effective from (=replacementpricedate).
[2] Verify that dateaccessioned is saved correctly and replacementpricedate
    is still null (does not contain 0000-00-00).
[3] Edit the item again. Fill some text in dateaccessioned and put a correct
    date in replacementpricedate. Verify the results.
[4] Run t/db_dependent/Items.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit 137cbd8d09)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

C4/Items.pm

@@ -2071,6 +2071,7 @@ sub _koha_new_item {
     my $dbh=C4::Context->dbh;  
     my $error;
     $item->{permanent_location} //= $item->{location};
+    _mod_item_dates( $item );
     my $query =
            "INSERT INTO items SET
             biblionumber        = ?,
@@ -2334,6 +2335,7 @@ sub _koha_modify_item {
 
     my $query = "UPDATE items SET ";
     my @bind;
+    _mod_item_dates( $item );
     for my $key ( keys %$item ) {
         next if ( $key eq 'itemnumber' );
         $query.="$key=?,";
@@ -2351,6 +2353,34 @@ sub _koha_modify_item {
     return ($item->{'itemnumber'},$error);
 }
 
+sub _mod_item_dates { # date formatting for date fields in item hash
+    my ( $item ) = @_;
+    return if !$item || ref($item) ne 'HASH';
+
+    my @keys = grep
+        { $_ =~ /^onloan$|^date|date$|datetime$/ }
+        keys %$item;
+    # Incl. dateaccessioned,replacementpricedate,datelastborrowed,datelastseen
+    # NOTE: We do not (yet) have items fields ending with datetime
+    # Fields with _on$ have been handled already
+
+    foreach my $key ( @keys ) {
+        next if !defined $item->{$key}; # skip undefs
+        my $dt = eval { dt_from_string( $item->{$key} ) };
+            # eval: dt_from_string will die on us if we pass illegal dates
+
+        my $newstr;
+        if( defined $dt  && ref($dt) eq 'DateTime' ) {
+            if( $key =~ /datetime/ ) {
+                $newstr = DateTime::Format::MySQL->format_datetime($dt);
+            } else {
+                $newstr = DateTime::Format::MySQL->format_date($dt);
+            }
+        }
+        $item->{$key} = $newstr; # might be undef to clear garbage
+    }
+}
+
 =head2 _koha_delete_item
 
   _koha_delete_item( $itemnum );

t/db_dependent/Items.t

@@ -26,7 +26,7 @@ use Koha::Library;
 use t::lib::Mocks;
 use t::lib::TestBuilder;
 
-use Test::More tests => 10;
+use Test::More tests => 11;
 
 use Test::Warn;
 
@@ -732,6 +732,56 @@ subtest 'C4::Items::_build_default_values_for_mod_marc' => sub {
     $schema->storage->txn_rollback;
 };
 
+subtest '_mod_item_dates' => sub {
+    plan tests => 11;
+
+    is( C4::Items::_mod_item_dates(), undef, 'Call without parameters' );
+    is( C4::Items::_mod_item_dates(1), undef, 'Call without hashref' );
+
+    my $orgitem;
+    my $item = {
+        itemcallnumber  => 'V II 149 1963',
+        barcode         => '109304',
+    };
+    $orgitem = { %$item };
+    C4::Items::_mod_item_dates($item);
+    is_deeply( $item, $orgitem, 'No dates passed to _mod_item_dates' );
+
+    # add two correct dates
+    t::lib::Mocks::mock_preference('dateformat', 'us');
+    $item->{dateaccessioned} = '01/31/2016';
+    $item->{onloan} =  $item->{dateaccessioned};
+    $orgitem = { %$item };
+    C4::Items::_mod_item_dates($item);
+    is( $item->{dateaccessioned}, '2016-01-31', 'dateaccessioned is fine' );
+    is( $item->{onloan}, '2016-01-31', 'onloan is fine too' );
+
+
+    # add some invalid dates
+    $item->{notexistingcolumndate} = '13/1/2015'; # wrong format
+    $item->{anotherdate} = 'tralala'; # even worse
+    $item->{myzerodate} = '0000-00-00'; # wrong too
+    C4::Items::_mod_item_dates($item);
+    is( $item->{notexistingcolumndate}, undef, 'Invalid date became NULL' );
+    is( $item->{anotherdate}, undef, 'Second invalid date became NULL too' );
+    is( $item->{myzerodate}, undef, '0000-00-00 became NULL too' );
+
+    # check if itemlost_on was not touched
+    $item->{itemlost_on} = '12345678';
+    $item->{withdrawn_on} = '12/31/2015 23:59:00';
+    $orgitem = { %$item };
+    C4::Items::_mod_item_dates($item);
+    is_deeply( $item, $orgitem, 'Colums with _on are not touched' );
+
+    t::lib::Mocks::mock_preference('dateformat', 'metric');
+    $item->{dateaccessioned} = '01/31/2016'; #wrong
+    $item->{yetanotherdatetime} = '20/01/2016 13:58:00'; #okay
+    C4::Items::_mod_item_dates($item);
+    is( $item->{dateaccessioned}, undef, 'dateaccessioned wrong format' );
+    is( $item->{yetanotherdatetime}, '2016-01-20 13:58:00',
+        'yetanotherdatetime is ok' );
+};
+
 # Helper method to set up a Biblio.
 sub get_biblio {
     my ( $frameworkcode ) = @_;