From 7e2514ac36e14aceef48d60e57aa97239fcd61b5 Mon Sep 17 00:00:00 2001
From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Date: Fri, 14 Aug 2020 17:18:12 +0100
Subject: [PATCH] Bug 18501: (QA follow-up) Fix regressions highlighted by unit
 tests

The migration of unit tests in the preceeding patch highlighted a few
regressions which are dealt with here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
---
 Koha/Item.pm | 84 ++++++++++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/Koha/Item.pm b/Koha/Item.pm
index ff6a4d5690..78c23dbab0 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -121,38 +121,45 @@ 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);
-                }
+        # Retreive the item for comparison if we need to
+        my $pre_mod_item = $self->get_from_storage
+          if ( exists $updated_columns{itemlost}
+            or exists $updated_columns{withdrawn}
+            or exists $updated_columns{damaged} );
+
+        # 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,13 +176,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 $self->itemlost >= 1
-                and $updated_columns{itemlost} <= 0
-        ) {
-            $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('');
         }
 
@@ -786,16 +793,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;
@@ -879,7 +885,7 @@ sub _set_found_trigger {
 
     # Update the account status
     $accountline->status('FOUND');
-    $accountline->store;
+    $accountline->store();
 
     if ( defined $account and C4::Context->preference('AccountAutoReconcile') ) {
         $account->reconcile_balance;
-- 
2.39.5