From c49d3df529d0285a9fa2e9ba6688feffeab49985 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 8 Jan 2024 17:34:09 +0000 Subject: [PATCH] Bug 35100: Revert change to request_transfer This patch reverts the change to request_transfer, opting to tackle the StockRotationAdvance requirement to stay in place in ModItemTransfer itself. We also add a FIXME to RotatingCollections.. I'll look to removing that on another bug to reduce the scope of this one. Signed-off-by: Martin Renvoize --- C4/Items.pm | 56 ++++++++++++++++++++++++++++++-------- C4/RotatingCollections.pm | 7 +++-- Koha/Item.pm | 2 +- t/db_dependent/Items.t | 30 +++++++++++++++++++- t/db_dependent/Koha/Item.t | 20 +------------- 5 files changed, 81 insertions(+), 34 deletions(-) diff --git a/C4/Items.pm b/C4/Items.pm index 174e7efa5c8..129a36f650c 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -54,6 +54,7 @@ use C4::Log qw( logaction ); use List::MoreUtils qw( any ); use DateTime::Format::MySQL; # debugging; so please don't remove this +use Try::Tiny qw( catch try ); use Koha::AuthorisedValues; use Koha::DateUtils qw( dt_from_string ); @@ -359,31 +360,64 @@ The last optional parameter allows for passing skip_record_index through to the sub ModItemTransfer { my ( $itemnumber, $frombranch, $tobranch, $trigger, $params ) = @_; - my $dbh = C4::Context->dbh; - my $item = Koha::Items->find( $itemnumber ); + my $dbh = C4::Context->dbh; + my $item = Koha::Items->find($itemnumber); # NOTE: This retains the existing hard coded behaviour by ignoring transfer limits # and always replacing any existing transfers. (In theory, calls to ModItemTransfer # will have been preceded by a check of branch transfer limits) my $to_library = Koha::Libraries->find($tobranch); - my $transfer = $item->request_transfer( - { - to => $to_library, - reason => $trigger, - ignore_limits => 1, - replace => 1 + my $transfer; + try { + $transfer = $item->request_transfer( + { + to => $to_library, + reason => $trigger, + ignore_limits => 1, + } + ); + } catch { + if ( $_->isa('Koha::Exceptions::Item::Transfer::InQueue') ) { + my $exception = $_; + my $found_transfer = $_->transfer; + + # If StockRotationAdvance, leave in place but ensure transit state is reset + if ( $found_transfer->reason eq 'StockrotationAdvance' ) { + $transfer = $item->request_transfer( + { + to => $to_library, + reason => $trigger, + ignore_limits => 1, + enqueue => 1 + } + ); + + # Ensure transit is reset + $found_transfer->datesent(undef)->store; + } else { + $transfer = $item->request_transfer( + { + to => $to_library, + reason => $trigger, + ignore_limits => 1, + replace => 1 + } + ); + } + } else { + $_->rethrow(); } - ); + }; # Immediately set the item to in transit if it is checked in if ( !$item->checkout ) { $item->holdingbranch($frombranch)->store( { log_action => 0, - skip_record_index => 1, # avoid indexing duplication, let ->transit handle it + skip_record_index => 1, # avoid indexing duplication, let ->transit handle it } ); - $transfer->transit({ skip_record_index => $params->{skip_record_index} }); + $transfer->transit( { skip_record_index => $params->{skip_record_index} } ); } return $transfer; diff --git a/C4/RotatingCollections.pm b/C4/RotatingCollections.pm index 18ba40a5c21..41d14d45aa4 100644 --- a/C4/RotatingCollections.pm +++ b/C4/RotatingCollections.pm @@ -487,8 +487,11 @@ sub TransferCollection { replace => 1 } ); # Replace transfer - # NOTE: If we just replaced a StockRotationAdvance, - # it will get enqueued afresh on the next cron run + # FIXME: If we just replaced a StockRotationAdvance, + # it will get enqueued afresh on the next cron run.. but + # that will also push the stage on too.. and what about if + # we were at the first stage.. then there won't be a datearrived + # to calculate against. See bug 35100 } } elsif ( $_->isa('Koha::Exceptions::Item::Transfer::Limit') ) { diff --git a/Koha/Item.pm b/Koha/Item.pm index ed548a93b2f..b325570910a 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -776,7 +776,7 @@ sub request_transfer { if ( $request && !$params->{enqueue} && !$params->{replace} ); $request->cancel( { reason => $params->{reason}, force => 1 } ) - if ( defined($request) && $params->{replace} && ( $request->reason ne 'StockrotationAdvance' ) ); + if ( defined($request) && $params->{replace} ); my $transfer = Koha::Item::Transfer->new( { diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index f6451c6f4e5..cfa0b74a41b 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -130,7 +130,7 @@ subtest 'General Add, Get and Del tests' => sub { }; subtest 'ModItemTransfer tests' => sub { - plan tests => 8; + plan tests => 14; $schema->storage->txn_begin; @@ -189,6 +189,34 @@ subtest 'ModItemTransfer tests' => sub { my $transfer3 = $transfers->next; is($transfer3->reason, 'Manual', "Reason set via ModItemTransfer"); + # Ensure StockrotationAdvance transfers are not cancelled + my $stock_transfer = $transfer3->reason('StockrotationAdvance')->store(); + is( + $stock_transfer->reason, 'StockrotationAdvance', + 'StockrotationAdvance transfer set' + ); + ok( + $stock_transfer->datesent, + 'StockrotationAdvance transfer is in transit' + ); + + ModItemTransfer( $item->itemnumber, $library1->{branchcode}, $library2->{branchcode}, 'Manual' ); + $transfers = Koha::Item::Transfers->search( + { itemnumber => $item->itemnumber, }, + { order_by => { '-desc' => 'branchtransfer_id' } } + ); + + my $replaced_transfer = $transfers->next; + is( + $replaced_transfer->reason, 'Manual', + 'StockrotationAdvance transfer "replaced" by manual transfer at top of queue' + ); + $stock_transfer->discard_changes; + is( $stock_transfer->datecancelled, undef, "StockrotationAdvance transfer not cancelled" ); + is( $stock_transfer->datesent, undef, "StockrotationAdvance transfer no longer in transit" ); + $transfers = $item->get_transfers; + is( $transfers->count, 2, "There are now 2 live transfers in the queue" ); + $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index 1b4c191956a..bdcd59f193d 100755 --- a/t/db_dependent/Koha/Item.t +++ b/t/db_dependent/Koha/Item.t @@ -846,7 +846,7 @@ subtest 'pickup_locations() tests' => sub { }; subtest 'request_transfer' => sub { - plan tests => 17; + plan tests => 13; $schema->storage->txn_begin; my $library1 = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -906,24 +906,6 @@ subtest 'request_transfer' => sub { is($transfers->count, 1, "There is only 1 live transfer in the queue"); $replaced_transfer->datearrived(dt_from_string)->store(); - # Replace StockrotationAdvance transfer - my $stock_transfer = $item->request_transfer( { to => $library1, reason => 'StockrotationAdvance' } ); - is( - ref($stock_transfer), 'Koha::Item::Transfer', - 'Koha::Item->request_transfer added StockrotationAdvance transfer' - ); - $replaced_transfer = $item->request_transfer( { to => $library2, reason => 'Manual', replace => 1 } ); - is( - ref($replaced_transfer), 'Koha::Item::Transfer', - 'Koha::Item->request_transfer allowed when replace is set' - ); - $stock_transfer->discard_changes; - is( $stock_transfer->datecancelled, undef, "StockrotationAdvance transfer left intact" ); - $transfers = $item->get_transfers; - is( $transfers->count, 2, "There are now 2 live transfers in the queue" ); - $replaced_transfer->datearrived(dt_from_string)->store(); - $stock_transfer->datearrived(dt_from_string)->store(); - # BranchTransferLimits t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1); t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); -- 2.46.0