From 62df4804a382074bf9ed208d110c2c92e6e1b22f Mon Sep 17 00:00:00 2001
From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Date: Fri, 18 Dec 2020 16:36:20 +0000
Subject: [PATCH] Bug 24446: Improve StockRotationItem->advance tests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This patch adds additional checks in the tests for item homebranch
updates and cyclical transfers caused by the StockRotationItem->advance
method.

It highlights a bug in the original logic whereby a cyclical rotation
would always result in a completed transfer but the physical item would
not have actually been moved between branches.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
---
 t/db_dependent/StockRotationItems.t | 67 +++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 17 deletions(-)

diff --git a/t/db_dependent/StockRotationItems.t b/t/db_dependent/StockRotationItems.t
index 49da2cc304..a1917d5f54 100755
--- a/t/db_dependent/StockRotationItems.t
+++ b/t/db_dependent/StockRotationItems.t
@@ -22,6 +22,7 @@ use Modern::Perl;
 use DateTime;
 use DateTime::Duration;
 use Koha::Database;
+use Koha::DateUtils;
 use Koha::Item::Transfer;
 use t::lib::TestBuilder;
 use Test::Warn;
@@ -196,7 +197,7 @@ subtest "Tests for needs_advancing." => sub {
         'itemnumber'  => $dbitem->itemnumber_id,
         'frombranch'  => $dbitem->stage->branchcode_id,
         'tobranch'    => $dbitem->stage->branchcode_id,
-        'datesent'    => DateTime->now,
+        'datesent'    => dt_from_string(),
         'datearrived' => undef,
         'reason'      => "StockrotationAdvance",
     })->store;
@@ -209,7 +210,7 @@ subtest "Tests for needs_advancing." => sub {
     $dbitem->fresh(0)->store;
 
     # Test item will not be advanced if it has not spent enough time.
-    $dbtransfer->datearrived(DateTime->now)->store;
+    $dbtransfer->datearrived(dt_from_string())->store;
     is($dbitem->needs_advancing, 0, "Not ready to advance: Not spent enough time.");
     # Test item will be advanced if it has not spent enough time, but is fresh.
     $dbitem->fresh(1)->store;
@@ -218,10 +219,10 @@ subtest "Tests for needs_advancing." => sub {
 
     # Test item will be advanced if it has spent enough time.
     $dbtransfer->datesent(      # Item was sent 100 days ago...
-        DateTime->now - DateTime::Duration->new( days => 100 )
+        dt_from_string() - DateTime::Duration->new( days => 100 )
     )->store;
     $dbtransfer->datearrived(   # And arrived 75 days ago.
-        DateTime->now - DateTime::Duration->new( days => 75 )
+        dt_from_string() - DateTime::Duration->new( days => 75 )
     )->store;
     is($dbitem->needs_advancing, 1, "Ready to be advanced.");
     $dbtransfer->delete;
@@ -231,7 +232,7 @@ subtest "Tests for needs_advancing." => sub {
 };
 
 subtest "Tests for advance." => sub {
-    plan tests => 15;
+    plan tests => 23;
     $schema->storage->txn_begin;
 
     my $sritem = $builder->build(
@@ -289,22 +290,54 @@ subtest "Tests for advance." => sub {
     $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
     ## Test results
     is($dbitem->stage->stage_id, $dbstage2->stage_id, "Stage updated.");
+    is(
+        $dbitem->itemnumber->homebranch,
+        $dbstage2->branchcode_id,
+        "Item homebranch updated"
+    );
     my $intransfer = $dbitem->itemnumber->get_transfer;
     is($intransfer->frombranch, $dbstage->branchcode_id, "Origin correct.");
     is($intransfer->tobranch, $dbstage2->branchcode_id, "Target Correct.");
 
+    # Arrive at new branch
+    $intransfer->datearrived(dt_from_string())->store;
+    $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store;
+
+    # Test a cyclical advance
+    ok($dbitem->advance, "Cyclical advancement done.");
+    ## Refetch dbitem
+    $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
+    ## Test results
+    is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage updated.");
+    is(
+        $dbitem->itemnumber->homebranch,
+        $dbstage->branchcode_id,
+        "Item homebranch updated"
+    );
+    $intransfer = $dbitem->itemnumber->get_transfer;
+    is($intransfer->frombranch, $dbstage2->branchcode_id, "Origin correct.");
+    is($intransfer->tobranch, $dbstage->branchcode_id, "Target correct.");
+
+    # Arrive at new branch
+    $intransfer->datearrived(dt_from_string())->store;
+    $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store;
+
     $dbstage->rota->cyclical(0)->store;         # Set Rota to non-cyclical.
 
+    # Advance again, to end of rota.
+    ok($dbitem->advance, "Non-cyclical advance to last stage.");
+
     # Arrive at new branch
-    $intransfer->datearrived(DateTime->now)->store;
+    $intransfer->datearrived(dt_from_string())->store;
     $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store;
-    $dbitem->itemnumber->homebranch($srstage->{branchcode_id})->store;
 
     # Advance again, Remove from rota.
     ok($dbitem->advance, "Non-cyclical advance.");
     ## Refetch dbitem
     $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id});
     is($dbitem, undef, "StockRotationItem has been removed.");
+    my $item = Koha::Items->find($sritem->{itemnumber_id});
+    is($item->homebranch, $srstage->{branchcode_id}, "Item homebranch remains");
 
     $schema->storage->txn_rollback;
 };
@@ -356,8 +389,8 @@ subtest "Tests for investigate (singular)." => sub {
         'itemnumber'  => $dbitem->itemnumber_id,
         'frombranch'  => $dbitem->itemnumber->homebranch,
         'tobranch'    => $dbitem->itemnumber->homebranch,
-        'datesent'    => DateTime->now,
-        'datearrived' => DateTime->now,
+        'datesent'    => dt_from_string(),
+        'datearrived' => dt_from_string(),
         'reason'      => "StockrotationAdvance",
     })->store;
     is($dbitem->investigate->{reason}, 'repatriation', "older item repatriates.");
@@ -377,8 +410,8 @@ subtest "Tests for investigate (singular)." => sub {
         'itemnumber'  => $dbitem->itemnumber_id,
         'frombranch'  => $dbitem->itemnumber->homebranch,
         'tobranch'    => $dbitem->stage->branchcode_id,
-        'datesent'    => DateTime->now,
-        'datearrived' => DateTime->now,
+        'datesent'    => dt_from_string(),
+        'datearrived' => dt_from_string(),
         'reason'      => "StockrotationAdvance",
     })->store;
     $dbitem->itemnumber->homebranch($dbitem->stage->branchcode_id)->store;
@@ -404,8 +437,8 @@ subtest "Tests for investigate (singular)." => sub {
         'itemnumber'  => $dbitem->itemnumber_id,
         'frombranch'  => $dbitem->itemnumber->homebranch,
         'tobranch'    => $dbitem->stage->branchcode_id,
-        'datesent'    => DateTime->now - $sent_duration,
-        'datearrived' => DateTime->now - $arrived_duration,
+        'datesent'    => dt_from_string() - $sent_duration,
+        'datearrived' => dt_from_string() - $arrived_duration,
         'reason'      => "StockrotationAdvance",
     })->store;
     $dbitem->itemnumber->homebranch($dbitem->stage->branchcode_id)->store;
@@ -432,8 +465,8 @@ subtest "Tests for investigate (singular)." => sub {
         'itemnumber'  => $dbitem->itemnumber_id,
         'frombranch'  => $dbitem->itemnumber->homebranch,
         'tobranch'    => $dbitem->stage->branchcode_id,
-        'datesent'    => DateTime->now - $sent_duration,
-        'datearrived' => DateTime->now - $arrived_duration,
+        'datesent'    => dt_from_string() - $sent_duration,
+        'datearrived' => dt_from_string() - $arrived_duration,
         'reason'      => "StockrotationAdvance",
     })->store;
     $dbitem->itemnumber->homebranch($dbitem->stage->branchcode_id)->store;
@@ -460,8 +493,8 @@ subtest "Tests for investigate (singular)." => sub {
         'itemnumber'  => $dbitem->itemnumber_id,
         'frombranch'  => $dbitem->itemnumber->homebranch,
         'tobranch'    => $dbitem->stage->branchcode_id,
-        'datesent'    => DateTime->now - $sent_duration,
-        'datearrived' => DateTime->now - $arrived_duration,
+        'datesent'    => dt_from_string() - $sent_duration,
+        'datearrived' => dt_from_string() - $arrived_duration,
         'reason'      => "StockrotationAdvance",
     })->store;
     is($dbitem->investigate->{reason}, 'repatriation',
-- 
2.20.1