From 2ac337b4e74182d8d4b0ddbaefcd98e041911ae7 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 18 Dec 2020 16:36:51 +0000 Subject: [PATCH] Bug 24446: (follow-up) Handle cases of pre-existing transfers MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This is a long standing bug in stockrotation (and transfers in general) where by if a transfer is in progress and another transfer is requested then the original transfer is inexplicitly cancelled. This patch updates the stockrotation code to handle queued transfers, either adding a StockrotationAdvance transfer to the queue when an in progress transfer was triggered to fulfil a Reserve, or otherwise cancelling the transfer as stockrotation should take precidence. To test 1/ Run t/db_dependent/StockRotationsItems.t. 2/ Signoff Signed-off-by: Kathleen Milne Signed-off-by: Joonas Kylmälä Signed-off-by: Jonathan Druart --- Koha/Item.pm | 30 ++- Koha/StockRotationItem.pm | 71 +++++-- t/db_dependent/StockRotationItems.t | 299 +++++++++++++++++++++------- 3 files changed, 301 insertions(+), 99 deletions(-) diff --git a/Koha/Item.pm b/Koha/Item.pm index a66d3dc056..d894fb517d 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -405,17 +405,25 @@ sub holds { =head3 request_transfer my $transfer = $item->request_transfer( - { to => $to_library, reason => $reason, ignore_limits => 0 } ); + { + to => $to_library, + reason => $reason, + [ ignore_limits => 0, enqueue => 1, replace => 1 ] + } + ); Add a transfer request for this item to the given branch for the given reason. An exception will be thrown if the BranchTransferLimits would prevent the requested transfer, unless 'ignore_limits' is passed to override the limits. -Note: At this time, only one active transfer (i.e pending arrival date) may exist -at a time for any given item. An exception will be thrown should you attempt to -add a request when a transfer has already been queued, whether it is in transit -or just at the request stage. +An exception will be thrown if an active transfer (i.e pending arrival date) is found; +The caller should catch such cases and retry the transfer request as appropriate passing +an appropriate override. + +Overrides +* enqueue - Used to queue up the transfer when the existing transfer is found to be in transit. +* replace - Used to replace the existing transfer request with your own. =cut @@ -431,14 +439,17 @@ sub request_transfer { } } - my $request; - Koha::Exceptions::Item::Transfer::Found->throw( transfer => $request ) - if ( $request = $self->get_transfer ); - Koha::Exceptions::Item::Transfer::Limit->throw() unless ( $params->{ignore_limits} || $self->can_be_transferred( { to => $params->{to} } ) ); + my $request = $self->get_transfer; + Koha::Exceptions::Item::Transfer::Found->throw( transfer => $request ) + if ( $request && !$params->{enqueue} && !$params->{replace} ); + + $request->cancel( $params->{reason} ) + if ( defined($request) && $params->{replace} ); + my $transfer = Koha::Item::Transfer->new( { itemnumber => $self->itemnumber, @@ -449,6 +460,7 @@ sub request_transfer { comments => $params->{comment} } )->store(); + return $transfer; } diff --git a/Koha/StockRotationItem.pm b/Koha/StockRotationItem.pm index 4b9325790a..944746e5ec 100644 --- a/Koha/StockRotationItem.pm +++ b/Koha/StockRotationItem.pm @@ -26,6 +26,7 @@ use Koha::DateUtils qw/dt_from_string/; use Koha::Item::Transfer; use Koha::Item; use Koha::StockRotationStage; +use Try::Tiny; use base qw(Koha::Object); @@ -143,27 +144,33 @@ sub needs_advancing { 1|0 = $sritem->repatriate -Put this item into branch transfer with 'StockrotationCorrection' comment, so +Put this item into branch transfer with 'StockrotationRepatriation' comment, so that it may return to it's stage.branch to continue its rota as normal. +Note: Stockrotation falls outside of the normal branch transfer limits and so we +pass 'ignore_limits' in the call to request_transfer. + =cut sub repatriate { my ( $self, $msg ) = @_; # Create the transfer. - my $transfer = $self->itemnumber->request_transfer( - { - to => $self->stage->branchcode, - reason => "StockrotationRepatriation", - comment => $msg - } - ); + my $transfer = try { + $self->itemnumber->request_transfer( + { + to => $self->stage->branchcode, + reason => "StockrotationRepatriation", + comment => $msg, + ignore_limits => 1 + } + ); + }; # Ensure the homebranch is still in sync with the rota stage $self->itemnumber->homebranch( $self->stage->branchcode_id )->store; - return $transfer; + return defined($transfer) ? 1 : 0; } =head3 advance @@ -180,6 +187,9 @@ StockRotationItem. If this item is 'indemand', and advance is invoked, we disable 'indemand' and advance the item as per usual. +Note: Stockrotation falls outside of the normal branch transfer limits and so we +pass 'ignore_limits' in the call to request_transfer. + =cut sub advance { @@ -221,13 +231,44 @@ sub advance { # Update stage and record transfer $self->stage_id( $new_stage->stage_id )->store; # Set new stage $item->homebranch( $new_stage->branchcode_id )->store; # Update homebranch - $transfer = $item->request_transfer( - { - to => $new_stage->branchcode, - reason => "StockrotationAdvance", - ignore_limits => 1 + $transfer = try { + $item->request_transfer( + { + to => $new_stage->branchcode, + reason => "StockrotationAdvance", + ignore_limits => 1 # Ignore transfer limits + } + ); # Add transfer + } + catch { + if ( $_->isa('Koha::Exceptions::Item::Transfer::Found') ) { + my $exception = $_; + my $found_transfer = $_->transfer; + if ( $found_transfer->in_transit + || $found_transfer->reason eq 'Reserve' ) + { + return $item->request_transfer( + { + to => $new_stage->branchcode, + reason => "StockrotationAdvance", + ignore_limits => 1, + enqueue => 1 + } + ); # Queue transfer + } else { + return $item->request_transfer( + { + to => $new_stage->branchcode, + reason => "StockrotationAdvance", + ignore_limits => 1, + replace => 1 + } + ); # Replace transfer + } + } else { + $_->rethrow(); } - ); # Add transfer + }; $transfer->receive if $item->holdingbranch eq $new_stage->branchcode_id; # Already at branch diff --git a/t/db_dependent/StockRotationItems.t b/t/db_dependent/StockRotationItems.t index a1917d5f54..537c7f7eeb 100755 --- a/t/db_dependent/StockRotationItems.t +++ b/t/db_dependent/StockRotationItems.t @@ -24,8 +24,10 @@ use DateTime::Duration; use Koha::Database; use Koha::DateUtils; use Koha::Item::Transfer; -use t::lib::TestBuilder; + use Test::Warn; +use t::lib::TestBuilder; +use t::lib::Mocks; use Test::More tests => 8; @@ -136,26 +138,65 @@ subtest 'Tests for needs_repatriating' => sub { }; subtest "Tests for repatriate." => sub { - plan tests => 3; + plan tests => 9; $schema->storage->txn_begin; - my $sritem = $builder->build( + + my $sritem_1 = $builder->build_object( { - source => 'Stockrotationitem', - value => - { itemnumber_id => $builder->build_sample_item->itemnumber } + class => 'Koha::StockRotationItems', + value => { + itemnumber_id => $builder->build_sample_item->itemnumber + } } ); - my $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id}); - $dbitem->stage->position(1); - $dbitem->stage->duration(50); + my $item_id = $sritem_1->itemnumber->itemnumber; + my $srstage_1 = $sritem_1->stage; + $sritem_1->discard_changes; + $sritem_1->stage->position(1); + $sritem_1->stage->duration(50); my $branch = $builder->build({ source => 'Branch' }); - $dbitem->itemnumber->holdingbranch($branch->{branchcode}); + $sritem_1->itemnumber->holdingbranch($branch->{branchcode}); # Test a straight up repatriate - ok($dbitem->repatriate, "Repatriation done."); - my $intransfer = $dbitem->itemnumber->get_transfer; + ok($sritem_1->repatriate, "Repatriation done."); + my $intransfer = $sritem_1->itemnumber->get_transfer; is($intransfer->frombranch, $branch->{branchcode}, "Origin correct."); - is($intransfer->tobranch, $dbitem->stage->branchcode_id, "Target Correct."); + is($intransfer->tobranch, $sritem_1->stage->branchcode_id, "Target Correct."); + + # Reset + $intransfer->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($branch->{branchcode}); + + # Setup a conflicting manual transfer + my $item = Koha::Items->find($item_id); + $item->request_transfer({ to => $srstage_1->branchcode, reason => "Manual" }); + $intransfer = $item->get_transfer; + is (ref($intransfer), 'Koha::Item::Transfer', "Conflicting transfer added"); + is ($intransfer->reason, 'Manual', "Conflicting transfer reason is 'Manual'"); + + # Stockrotation should handle transfer clashes + is($sritem_1->repatriate, 0, "Repatriation skipped if transfer in progress."); + + # Reset + $intransfer->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($branch->{branchcode}); + + # Confirm that stockrotation ignores transfer limits + t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1); + t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); + my $limit = Koha::Item::Transfer::Limit->new( + { + fromBranch => $branch->{branchcode}, + toBranch => $srstage_1->branchcode_id, + itemtype => $sritem_1->itemnumber->effective_itemtype, + } + )->store; + + # Stockrotation should overrule transfer limits + ok($sritem_1->repatriate, "Repatriation done regardless of transfer limits."); + $intransfer = $sritem_1->itemnumber->get_transfer; + is($intransfer->frombranch, $branch->{branchcode}, "Origin correct."); + is($intransfer->tobranch, $sritem_1->stage->branchcode_id, "Target Correct."); $schema->storage->txn_rollback; }; @@ -232,112 +273,220 @@ subtest "Tests for needs_advancing." => sub { }; subtest "Tests for advance." => sub { - plan tests => 23; + plan tests => 44; $schema->storage->txn_begin; - my $sritem = $builder->build( + my $sritem_1 = $builder->build_object( { - source => 'Stockrotationitem', + class => 'Koha::StockRotationItems', value => { 'fresh' => 1, itemnumber_id => $builder->build_sample_item->itemnumber } } ); - my $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id}); - $dbitem->itemnumber->holdingbranch($dbitem->stage->branchcode_id); - my $dbstage = $dbitem->stage; - $dbstage->position(1)->duration(50)->store; # Configure stage. + $sritem_1->discard_changes; + $sritem_1->itemnumber->holdingbranch($sritem_1->stage->branchcode_id); + my $item_id = $sritem_1->itemnumber->itemnumber; + my $srstage_1 = $sritem_1->stage; + $srstage_1->position(1)->duration(50)->store; # Configure stage. # Configure item - $dbitem->itemnumber->holdingbranch($dbstage->branchcode_id)->store; - $dbitem->itemnumber->homebranch($dbstage->branchcode_id)->store; + $sritem_1->itemnumber->holdingbranch($srstage_1->branchcode_id)->store; + $sritem_1->itemnumber->homebranch($srstage_1->branchcode_id)->store; # Sanity check - is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage sanity check."); + is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Stage sanity check."); # Test if an item is fresh, always move to first stage. - is($dbitem->fresh, 1, "Fresh is correct."); - $dbitem->advance; - is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage is first stage after fresh advance."); - is($dbitem->fresh, 0, "Fresh reset after advance."); + is($sritem_1->fresh, 1, "Fresh is correct."); + $sritem_1->advance; + is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Stage is first stage after fresh advance."); + is($sritem_1->fresh, 0, "Fresh reset after advance."); # Test cases of single stage - $dbstage->rota->cyclical(1)->store; # Set Rota to cyclical. - ok($dbitem->advance, "Single stage cyclical advance done."); - ## Refetch dbitem - $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id}); - is($dbitem->stage->stage_id, $dbstage->stage_id, "Single stage cyclical stage OK."); + $srstage_1->rota->cyclical(1)->store; # Set Rota to cyclical. + ok($sritem_1->advance, "Single stage cyclical advance done."); + ## Refetch sritem_1 + $sritem_1->discard_changes; + is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Single stage cyclical stage OK."); # Test with indemand advance - $dbitem->indemand(1)->store; - ok($dbitem->advance, "Indemand item advance done."); - ## Refetch dbitem - $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id}); - is($dbitem->indemand, 0, "Indemand OK."); - is($dbitem->stage->stage_id, $dbstage->stage_id, "Indemand item advance stage OK."); + $sritem_1->indemand(1)->store; + ok($sritem_1->advance, "Indemand item advance done."); + ## Refetch sritem_1 + $sritem_1->discard_changes; + is($sritem_1->indemand, 0, "Indemand OK."); + is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Indemand item advance stage OK."); # Multi stages - my $srstage = $builder->build({ - source => 'Stockrotationstage', + my $srstage_2 = $builder->build_object({ + class => 'Koha::StockRotationStages', value => { duration => 50 } }); - my $dbstage2 = Koha::StockRotationStages->find($srstage->{stage_id}); - $dbstage2->move_to_group($dbitem->stage->rota_id); - $dbstage2->move_last; + $srstage_2->discard_changes; + $srstage_2->move_to_group($sritem_1->stage->rota_id); + $srstage_2->move_last; # Test a straight up advance - ok($dbitem->advance, "Advancement done."); - ## Refetch dbitem - $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id}); + ok($sritem_1->advance, "Advancement done."); + ## Refetch sritem_1 + $sritem_1->discard_changes; ## Test results - is($dbitem->stage->stage_id, $dbstage2->stage_id, "Stage updated."); + is($sritem_1->stage->stage_id, $srstage_2->stage_id, "Stage updated."); is( - $dbitem->itemnumber->homebranch, - $dbstage2->branchcode_id, + $sritem_1->itemnumber->homebranch, + $srstage_2->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."); + my $transfer_request = $sritem_1->itemnumber->get_transfer; + is($transfer_request->frombranch, $srstage_1->branchcode_id, "Origin correct."); + is($transfer_request->tobranch, $srstage_2->branchcode_id, "Target Correct."); + is($transfer_request->datesent, undef, "Transfer requested, but not sent."); # Arrive at new branch - $intransfer->datearrived(dt_from_string())->store; - $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store; + $transfer_request->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($srstage_2->branchcode_id)->store; # Test a cyclical advance - ok($dbitem->advance, "Cyclical advancement done."); - ## Refetch dbitem - $dbitem = Koha::StockRotationItems->find($sritem->{itemnumber_id}); + ok($sritem_1->advance, "Cyclical advancement done."); + ## Refetch sritem_1 + $sritem_1->discard_changes; ## Test results - is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage updated."); + is($sritem_1->stage->stage_id, $srstage_1->stage_id, "Stage updated."); is( - $dbitem->itemnumber->homebranch, - $dbstage->branchcode_id, + $sritem_1->itemnumber->homebranch, + $srstage_1->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."); + $transfer_request = $sritem_1->itemnumber->get_transfer; + is($transfer_request->frombranch, $srstage_2->branchcode_id, "Origin correct."); + is($transfer_request->tobranch, $srstage_1->branchcode_id, "Target correct."); # Arrive at new branch - $intransfer->datearrived(dt_from_string())->store; - $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store; + $transfer_request->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($srstage_1->branchcode_id)->store; + + # Confirm that stockrotation ignores transfer limits + t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1); + t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); + my $limit = Koha::Item::Transfer::Limit->new( + { + fromBranch => $srstage_1->branchcode_id, + toBranch => $srstage_2->branchcode_id, + itemtype => $sritem_1->itemnumber->effective_itemtype, + } + )->store; + + ok($sritem_1->advance, "Advancement overrules transfer limits."); + ## Refetch sritem_1 + $sritem_1->discard_changes; + ## Test results + is($sritem_1->stage->stage_id, $srstage_2->stage_id, "Stage updated ignoring transfer limits."); + is( + $sritem_1->itemnumber->homebranch, + $srstage_2->branchcode_id, + "Item homebranch updated ignoring transfer limits" + ); + $transfer_request = $sritem_1->itemnumber->get_transfer; + is($transfer_request->frombranch, $srstage_1->branchcode_id, "Origin correct ignoring transfer limits."); + is($transfer_request->tobranch, $srstage_2->branchcode_id, "Target correct ignoring transfer limits."); + + # Arrive at new branch + $transfer_request->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($srstage_2->branchcode_id)->store; + + # Setup a conflicting manual transfer + my $item = Koha::Items->find($item_id); + $item->request_transfer({ to => $srstage_1->branchcode, reason => "Manual" }); + $transfer_request = $item->get_transfer; + is (ref($transfer_request), 'Koha::Item::Transfer', "Conflicting transfer added"); + is ($transfer_request->reason, 'Manual', "Conflicting transfer reason is 'Manual'"); + + # Advance item whilst conflicting manual transfer exists + ok($sritem_1->advance, "Advancement done."); + ## Refetch sritem_1 + $sritem_1->discard_changes; + + ## Refetch conflicted transfer + $transfer_request->discard_changes; + + # Conflicted transfer should have been cancelled + isnt($transfer_request->datecancelled, undef, "Conflicting manual transfer was cancelled"); + + # StockRotationAdvance transfer added + $transfer_request = $sritem_1->itemnumber->get_transfer; + is($transfer_request->reason, 'StockrotationAdvance', "StockrotationAdvance transfer added"); + is($transfer_request->frombranch, $srstage_2->branchcode_id, "Origin correct."); + is($transfer_request->tobranch, $srstage_1->branchcode_id, "Target correct."); + + # Arrive at new branch + $transfer_request->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($srstage_1->branchcode_id)->store; - $dbstage->rota->cyclical(0)->store; # Set Rota to non-cyclical. + # Setup a conflicting reserve transfer + $item->request_transfer({ to => $srstage_2->branchcode, reason => "Reserve" }); + $transfer_request = $item->get_transfer; + is (ref($transfer_request), 'Koha::Item::Transfer', "Conflicting transfer added"); + is ($transfer_request->reason, 'Reserve', "Conflicting transfer reason is 'Reserve'"); + + # Advance item whilst conflicting reserve transfer exists + ok($sritem_1->advance, "Advancement done."); + ## Refetch sritem_1 + $sritem_1->discard_changes; + + ## Refetch conflicted transfer + $transfer_request->discard_changes; + + # Conflicted transfer should not been cancelled + is($transfer_request->datecancelled, undef, "Conflicting reserve transfer was not cancelled"); + + # StockRotationAdvance transfer added + my $transfer_requests = Koha::Item::Transfers->search( + { + itemnumber => $sritem_1->itemnumber->itemnumber, + datearrived => undef, + datecancelled => undef + } + ); + is($transfer_requests->count, '2', "StockrotationAdvance transfer queued"); + + # Arrive at new branch + $transfer_request->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($srstage_2->branchcode_id)->store; + + # StockRotationAdvance transfer added + $transfer_request = $sritem_1->itemnumber->get_transfer; + is($transfer_request->reason, 'StockrotationAdvance', "StockrotationAdvance transfer remains after reserve is met"); + is($transfer_request->frombranch, $srstage_1->branchcode_id, "Origin correct."); + is($transfer_request->tobranch, $srstage_2->branchcode_id, "Target correct."); + + # Arrive at new branch + $transfer_request->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($srstage_2->branchcode_id)->store; + + $srstage_1->rota->cyclical(0)->store; # Set Rota to non-cyclical. + + my $srstage_3 = $builder->build_object({ + class => 'Koha::StockRotationStages', + value => { duration => 50 } + }); + $srstage_3->discard_changes; + $srstage_3->move_to_group($sritem_1->stage->rota_id); + $srstage_3->move_last; # Advance again, to end of rota. - ok($dbitem->advance, "Non-cyclical advance to last stage."); + ok($sritem_1->advance, "Non-cyclical advance to last stage."); # Arrive at new branch - $intransfer->datearrived(dt_from_string())->store; - $dbitem->itemnumber->holdingbranch($srstage->{branchcode_id})->store; + $transfer_request->datearrived(dt_from_string())->store; + $sritem_1->itemnumber->holdingbranch($srstage_3->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"); + ok($sritem_1->advance, "Non-cyclical advance."); + ## Refetch sritem_1 + $sritem_1 = Koha::StockRotationItems->find({ itemnumber_id => $item_id }); + is($sritem_1, undef, "StockRotationItem has been removed."); + $item = Koha::Items->find($item_id); + is($item->homebranch, $srstage_3->branchcode_id, "Item homebranch remains"); $schema->storage->txn_rollback; }; -- 2.39.5