From 5e3fa5455209a3178cffe874e8dbe3c0c3e8d2ff Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 17 May 2021 17:14:16 +0100 Subject: [PATCH] Bug 28294: [ALT] Remove updateWrongTransfer This patch removes the last remaining use of updateWrongTransfer and the method itself. We replace it with a call to request_transfer that passes the 'replace' reason of 'WrongTransfer' through to Koha::Item::Transfer->cancel. Test plan 1/ git grep updateWrongTransfer to confirm there are no more uses of the method. 2/ Confirm an incorrect transfer still behaves as expected at circulation returns. --- C4/Circulation.pm | 31 ---------------- C4/Items.pm | 2 +- C4/RotatingCollections.pm | 2 +- Koha/Item.pm | 4 +-- Koha/StockRotationItem.pm | 2 +- circ/returns.pl | 7 ++-- t/db_dependent/Circulation.t | 44 ++--------------------- t/db_dependent/Koha/Item.t | 68 +++++++++++++++++++++--------------- 8 files changed, 51 insertions(+), 109 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 562cad72671..2ad06efb720 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -107,7 +107,6 @@ BEGIN { transferbook TooMany GetTransfersFromTo - updateWrongTransfer CalcDateDue CheckValidBarcode IsBranchTransferAllowed @@ -3806,36 +3805,6 @@ sub SendCirculationAlert { return; } -=head2 updateWrongTransfer - - $items = updateWrongTransfer($itemNumber,$borrowernumber,$waitingAtLibrary,$FromLibrary); - -This function validate the line of brachtransfer but with the wrong destination (mistake from a librarian ...), and create a new line in branchtransfer from the actual library to the original library of reservation - -=cut - -sub updateWrongTransfer { - my ( $itemNumber,$waitingAtLibrary,$FromLibrary ) = @_; - - # first step: cancel the original transfer - my $item = Koha::Items->find($itemNumber); - my $transfer = $item->get_transfer; - $transfer->set({ datecancelled => dt_from_string, cancellation_reason => 'WrongTransfer' })->store(); - - # second step: create a new transfer to the right location - my $new_transfer = $item->request_transfer( - { - to => $transfer->to_library, - reason => $transfer->reason, - comment => $transfer->comments, - ignore_limits => 1, - enqueue => 1 - } - ); - - return $new_transfer; -} - =head2 CalcDateDue $newdatedue = CalcDateDue($startdate,$itemtype,$branchcode,$borrower); diff --git a/C4/Items.pm b/C4/Items.pm index c640b63baa3..dd6c3ca64a5 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -371,7 +371,7 @@ sub ModItemTransfer { to => $to_library, reason => $trigger, ignore_limits => 1, - replace => 1 + replace => $trigger } ); diff --git a/C4/RotatingCollections.pm b/C4/RotatingCollections.pm index 18ba40a5c21..f7b9aa1d993 100644 --- a/C4/RotatingCollections.pm +++ b/C4/RotatingCollections.pm @@ -484,7 +484,7 @@ sub TransferCollection { to => $to_library, reason => "RotatingCollection", ignore_limits => 0, - replace => 1 + replace => "RotatingCollection" } ); # Replace transfer # NOTE: If we just replaced a StockRotationAdvance, diff --git a/Koha/Item.pm b/Koha/Item.pm index 22eaceac948..9ee84e7cd55 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -702,7 +702,7 @@ sub place_booking { { to => $to_library, reason => $reason, - [ ignore_limits => 0, enqueue => 1, replace => 1 ] + [ ignore_limits => 0, enqueue => 1, replace => 'reason' ] } ); @@ -741,7 +741,7 @@ sub request_transfer { Koha::Exceptions::Item::Transfer::InQueue->throw( transfer => $request ) if ( $request && !$params->{enqueue} && !$params->{replace} ); - $request->cancel( { reason => $params->{reason}, force => 1 } ) + $request->cancel( { reason => $params->{replace}, force => 1 } ) if ( defined($request) && $params->{replace} ); my $transfer = Koha::Item::Transfer->new( diff --git a/Koha/StockRotationItem.pm b/Koha/StockRotationItem.pm index 66578d83b01..48f8e4d5852 100644 --- a/Koha/StockRotationItem.pm +++ b/Koha/StockRotationItem.pm @@ -265,7 +265,7 @@ sub advance { to => $new_stage->branchcode, reason => "StockrotationAdvance", ignore_limits => 1, - replace => 1 + replace => "StockrotationAdvance" } ); # Replace transfer } diff --git a/circ/returns.pl b/circ/returns.pl index c0df0611390..8ebcd45f3c5 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -35,7 +35,7 @@ use CGI qw ( -utf8 ); use DateTime; use C4::Auth qw( get_template_and_user get_session haspermission ); -use C4::Circulation qw( barcodedecode GetBranchItemRule AddReturn updateWrongTransfer LostItem ); +use C4::Circulation qw( barcodedecode GetBranchItemRule AddReturn LostItem ); use C4::Context; use C4::Items qw( ModItemTransfer ); use C4::Members::Messaging; @@ -554,7 +554,10 @@ if ( $messages->{'WrongTransfer'} and not $messages->{'WasTransfered'}) { ); # Update the transfer to reflect the new item holdingbranch - my $new_transfer = updateWrongTransfer($messages->{'WrongTransferItem'},$messages->{'WrongTransfer'}, $userenv_branch); + my $item = Koha::Items->find($messages->{'WrongTransferItem'}); + my $old_transfer = $item->get_transfer; + my $new_transfer = $item->request_transfer( + { to => $old_transfer->tobranch, reason => $old_transfer->reason, replace => 'WrongTransfer' } ); $template->param( NewTransfer => $new_transfer->id ); diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 288c5650139..9e5395ce47a 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -18,7 +18,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 67; +use Test::More tests => 66; use Test::Exception; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -33,7 +33,7 @@ use t::lib::TestBuilder; use C4::Accounts; use C4::Calendar qw( new insert_single_holiday insert_week_day_holiday delete_holiday ); -use C4::Circulation qw( AddIssue AddReturn CanBookBeRenewed GetIssuingCharges AddRenewal GetSoonestRenewDate GetLatestAutoRenewDate LostItem GetUpcomingDueIssues CanBookBeIssued AddIssuingCharge MarkIssueReturned ProcessOfflinePayment transferbook updateWrongTransfer ); +use C4::Circulation qw( AddIssue AddReturn CanBookBeRenewed GetIssuingCharges AddRenewal GetSoonestRenewDate GetLatestAutoRenewDate LostItem GetUpcomingDueIssues CanBookBeIssued AddIssuingCharge MarkIssueReturned ProcessOfflinePayment transferbook ); use C4::Biblio; use C4::Items qw( ModItemTransfer ); use C4::Log; @@ -5843,46 +5843,6 @@ subtest "Item's onloan value should be set if checked out item is checked out to ok( $item->get_from_storage->onloan, "Item's onloan column is set after second checkout" ); }; -subtest "updateWrongTransfer tests" => sub { - plan tests => 5; - - my $library1 = $builder->build_object( { class => 'Koha::Libraries' } ); - my $library2 = $builder->build_object( { class => 'Koha::Libraries' } ); - my $library3 = $builder->build_object( { class => 'Koha::Libraries' } ); - my $item = $builder->build_sample_item( - { - homebranch => $library1->branchcode, - holdingbranch => $library2->branchcode, - datelastseen => undef - } - ); - - my $transfer = $builder->build_object( - { - class => 'Koha::Item::Transfers', - value => { - itemnumber => $item->itemnumber, - frombranch => $library2->branchcode, - tobranch => $library1->branchcode, - daterequested => dt_from_string, - datesent => dt_from_string, - datecancelled => undef, - datearrived => undef, - reason => 'Manual' - } - } - ); - is( ref($transfer), 'Koha::Item::Transfer', 'Mock transfer added' ); - - my $new_transfer = C4::Circulation::updateWrongTransfer($item->itemnumber, $library1->branchcode); - is(ref($new_transfer), 'Koha::Item::Transfer', "updateWrongTransfer returns a 'Koha::Item::Transfer' object"); - ok( !$new_transfer->in_transit, "New transfer is NOT created as in transit (or cancelled)"); - - my $original_transfer = $transfer->get_from_storage; - ok( defined($original_transfer->datecancelled), "Original transfer was cancelled"); - is( $original_transfer->cancellation_reason, 'WrongTransfer', "Original transfer cancellation reason is 'WrongTransfer'"); -}; - subtest "SendCirculationAlert" => sub { plan tests => 3; diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index 7a21db18f55..bbb49d073fb 100755 --- a/t/db_dependent/Koha/Item.t +++ b/t/db_dependent/Koha/Item.t @@ -757,7 +757,7 @@ subtest 'pickup_locations() tests' => sub { }; subtest 'request_transfer' => sub { - plan tests => 13; + plan tests => 14; $schema->storage->txn_begin; my $library1 = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -772,15 +772,16 @@ subtest 'request_transfer' => sub { # Mandatory fields tests throws_ok { $item->request_transfer( { to => $library1 } ) } 'Koha::Exceptions::MissingParameter', - 'Exception thrown if `reason` parameter is missing'; + 'Exception thrown if `reason` parameter is missing'; throws_ok { $item->request_transfer( { reason => 'Manual' } ) } 'Koha::Exceptions::MissingParameter', - 'Exception thrown if `to` parameter is missing'; + 'Exception thrown if `to` parameter is missing'; # Successful request - my $transfer = $item->request_transfer({ to => $library1, reason => 'Manual' }); - is( ref($transfer), 'Koha::Item::Transfer', + my $transfer = $item->request_transfer( { to => $library1, reason => 'Manual' } ); + is( + ref($transfer), 'Koha::Item::Transfer', 'Koha::Item->request_transfer should return a Koha::Item::Transfer object' ); my $original_transfer = $transfer->get_from_storage; @@ -788,50 +789,59 @@ subtest 'request_transfer' => sub { # Transfer already in progress throws_ok { $item->request_transfer( { to => $library2, reason => 'Manual' } ) } 'Koha::Exceptions::Item::Transfer::InQueue', - 'Exception thrown if transfer is already in progress'; + 'Exception thrown if transfer is already in progress'; my $exception = $@; - is( ref( $exception->transfer ), + is( + ref( $exception->transfer ), 'Koha::Item::Transfer', - 'The exception contains the found Koha::Item::Transfer' ); + 'The exception contains the found Koha::Item::Transfer' + ); # Queue transfer - my $queued_transfer = $item->request_transfer( - { to => $library2, reason => 'Manual', enqueue => 1 } ); - is( ref($queued_transfer), 'Koha::Item::Transfer', - 'Koha::Item->request_transfer allowed when enqueue is set' ); + my $queued_transfer = $item->request_transfer( { to => $library2, reason => 'Manual', enqueue => 1 } ); + is( + ref($queued_transfer), 'Koha::Item::Transfer', + 'Koha::Item->request_transfer allowed when enqueue is set' + ); my $transfers = $item->get_transfers; - is($transfers->count, 2, "There are now 2 live transfers in the queue"); + is( $transfers->count, 2, "There are now 2 live transfers in the queue" ); $transfer = $transfer->get_from_storage; - is_deeply($transfer->unblessed, $original_transfer->unblessed, "Original transfer unchanged"); + is_deeply( $transfer->unblessed, $original_transfer->unblessed, "Original transfer unchanged" ); $queued_transfer->datearrived(dt_from_string)->store(); # Replace transfer - my $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' ); + my $replaced_transfer = + $item->request_transfer( { to => $library2, reason => 'Manual', replace => 'WrongTransfer' } ); + is( + ref($replaced_transfer), 'Koha::Item::Transfer', + 'Koha::Item->request_transfer allowed when replace is set' + ); $original_transfer->discard_changes; - ok($original_transfer->datecancelled, "Original transfer cancelled"); + ok( $original_transfer->datecancelled, "Original transfer cancelled" ); + is( $original_transfer->cancellation_reason, "WrongTransfer", "Original cancellation_reason set correctly" ); $transfers = $item->get_transfers; - is($transfers->count, 1, "There is only 1 live transfer in the queue"); + is( $transfers->count, 1, "There is only 1 live transfer in the queue" ); $replaced_transfer->datearrived(dt_from_string)->store(); # BranchTransferLimits - t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1); - t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); - my $limit = Koha::Item::Transfer::Limit->new({ - fromBranch => $library2->branchcode, - toBranch => $library1->branchcode, - itemtype => $item->effective_itemtype, - })->store; + t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', 1 ); + t::lib::Mocks::mock_preference( 'BranchTransferLimitsType', 'itemtype' ); + my $limit = Koha::Item::Transfer::Limit->new( + { + fromBranch => $library2->branchcode, + toBranch => $library1->branchcode, + itemtype => $item->effective_itemtype, + } + )->store; throws_ok { $item->request_transfer( { to => $library1, reason => 'Manual' } ) } 'Koha::Exceptions::Item::Transfer::Limit', - 'Exception thrown if transfer is prevented by limits'; + 'Exception thrown if transfer is prevented by limits'; my $forced_transfer = $item->request_transfer( { to => $library1, reason => 'Manual', ignore_limits => 1 } ); - is( ref($forced_transfer), 'Koha::Item::Transfer', + is( + ref($forced_transfer), 'Koha::Item::Transfer', 'Koha::Item->request_transfer allowed when ignore_limits is set' ); -- 2.43.0