Bug 18501: Fix QA issues
[srvgit] / Koha / Item.pm
index 74f09ab..84ba2da 100644 (file)
@@ -121,38 +121,46 @@ sub store {
 
     } else { # ModItem
 
-        { # Update *_on  fields if needed
-          # Why not for AddItem as well?
-            my @fields = qw( itemlost withdrawn damaged );
-
-            # Only retrieve the item if we need to set an "on" date field
-            if ( $self->itemlost || $self->withdrawn || $self->damaged ) {
-                my $pre_mod_item = $self->get_from_storage;
-                for my $field (@fields) {
-                    if (    $self->$field
-                        and not $pre_mod_item->$field )
-                    {
-                        my $field_on = "${field}_on";
-                        $self->$field_on(
-                          DateTime::Format::MySQL->format_datetime( dt_from_string() )
-                        );
-                    }
-                }
-            }
+        my %updated_columns = $self->_result->get_dirty_columns;
+        return $self->SUPER::store unless %updated_columns;
 
-            # If the field is defined but empty, we are removing and,
-            # and thus need to clear out the 'on' field as well
-            for my $field (@fields) {
-                if ( defined( $self->$field ) && !$self->$field ) {
-                    my $field_on = "${field}_on";
-                    $self->$field_on(undef);
-                }
+        # Retrieve the item for comparison if we need to
+        my $pre_mod_item = (
+                 exists $updated_columns{itemlost}
+              or exists $updated_columns{withdrawn}
+              or exists $updated_columns{damaged}
+        ) ? $self->get_from_storage : undef;
+
+        # Update *_on  fields if needed
+        # FIXME: Why not for AddItem as well?
+        my @fields = qw( itemlost withdrawn damaged );
+        for my $field (@fields) {
+
+            # If the field is defined but empty or 0, we are
+            # removing/unsetting and thus need to clear out
+            # the 'on' field
+            if (   exists $updated_columns{$field}
+                && defined( $self->$field )
+                && !$self->$field )
+            {
+                my $field_on = "${field}_on";
+                $self->$field_on(undef);
+            }
+            # If the field has changed otherwise, we much update
+            # the 'on' field
+            elsif (exists $updated_columns{$field}
+                && $updated_columns{$field}
+                && !$pre_mod_item->$field )
+            {
+                my $field_on = "${field}_on";
+                $self->$field_on(
+                    DateTime::Format::MySQL->format_datetime(
+                        dt_from_string()
+                    )
+                );
             }
         }
 
-        my %updated_columns = $self->_result->get_dirty_columns;
-        return $self->SUPER::store unless %updated_columns;
-
         if (   exists $updated_columns{itemcallnumber}
             or exists $updated_columns{cn_source} )
         {
@@ -169,11 +177,13 @@ sub store {
             $self->permanent_location( $self->location );
         }
 
-        # If item was lost, it has now been found, reverse any list item charges if necessary.
-        if ( exists $updated_columns{itemlost}
-                and $self->itemlost != $updated_columns{itemlost}
-                and $updated_columns{itemlost} >= 1 ) {
-            $self->_set_found_trigger;
+        # If item was lost and has now been found,
+        # reverse any list item charges if necessary.
+        if (    exists $updated_columns{itemlost}
+            and $updated_columns{itemlost} <= 0
+            and $pre_mod_item->itemlost > 0 )
+        {
+            $self->_set_found_trigger($pre_mod_item);
             $self->paidfor('');
         }
 
@@ -776,7 +786,7 @@ sub renewal_branchcode {
     $self->_set_found_trigger
 
 Finds the most recent lost item charge for this item and refunds the patron
-appropriatly, taking into account any payments or writeoffs already applied
+appropriately, taking into account any payments or writeoffs already applied
 against the charge.
 
 Internal function, not exported, called only by Koha::Item->store.
@@ -784,16 +794,15 @@ Internal function, not exported, called only by Koha::Item->store.
 =cut
 
 sub _set_found_trigger {
-    my ( $self, $params ) = @_;
+    my ( $self, $pre_mod_item ) = @_;
 
     ## If item was lost, it has now been found, reverse any list item charges if necessary.
-    my $refund = 1;
     my $no_refund_after_days =
       C4::Context->preference('NoRefundOnLostReturnedItemsAge');
     if ($no_refund_after_days) {
         my $today = dt_from_string();
         my $lost_age_in_days =
-          dt_from_string( $self->itemlost_on )->delta_days($today)
+          dt_from_string( $pre_mod_item->itemlost_on )->delta_days($today)
           ->in_units('days');
 
         return $self unless $lost_age_in_days < $no_refund_after_days;
@@ -829,7 +838,7 @@ sub _set_found_trigger {
     my $patron = Koha::Patrons->find( $accountline->borrowernumber );
     return $self
       unless $patron;  # Patron has been deleted, nobody to credit the return to
-                       # FIXME Should not we notify this somehwere
+                       # FIXME Should not we notify this somewhere
 
     my $account = $patron->account;
 
@@ -862,11 +871,12 @@ sub _set_found_trigger {
         $credit = $account->add_credit(
             {
                 amount      => $credit_total,
-                description => 'Item found ' . $item_id,
+                description => 'Item found ' . $self->itemnumber,
                 type        => 'LOST_FOUND',
                 interface   => C4::Context->interface,
                 library_id  => $branchcode,
-                item_id     => $itemnumber
+                item_id     => $self->itemnumber,
+                issue_id    => $accountline->issue_id
             }
         );
 
@@ -875,9 +885,8 @@ sub _set_found_trigger {
     }
 
     # Update the account status
-    $accountline->discard_changes->status('FOUND')
-      ; # FIXME JD Why discard_changes? $accountline has not been modified since last fetch
-    $accountline->store;
+    $accountline->status('FOUND');
+    $accountline->store();
 
     if ( defined $account and C4::Context->preference('AccountAutoReconcile') ) {
         $account->reconcile_balance;