From d64a206a25d6685be981975a8883ed5073adf7ef Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 5 Jan 2024 16:08:44 +0000 Subject: [PATCH] Bug 35100: Various fixes 1) Don't automagically always set a transfer to in transit on checkin.. wait for the user to actually confirm that's the case 2) New transfers triggered by a hold should take precidence, so hide transfers for any other reason from display 3) Update get_transfer and get_transfers to ensure ordering isn't lost when prefetch is used and add tests for this Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall Signed-off-by: Martin Renvoize --- C4/Circulation.pm | 12 +- C4/Items.pm | 2 +- Koha/Item.pm | 15 +- Koha/Schema/Result/Item.pm | 3 +- circ/returns.pl | 12 +- .../prog/en/modules/circ/returns.tt | 5 +- t/db_dependent/Circulation.t | 35 ++- t/db_dependent/Koha/Item.t | 251 +++++++++++++++++- 8 files changed, 303 insertions(+), 32 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 373a614568..bd8ecfc58f 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2374,10 +2374,9 @@ sub AddReturn { else { $messages->{'TransferTrigger'} = $transfer->reason; if ( $transfer->frombranch eq $branch ) { - $transfer->transit; - $messages->{'WasTransfered'} = $transfer->tobranch; - } - else { + $messages->{'TransferTo'} = $transfer->tobranch; + $messages->{'WasTransfered'} = $transfer->id; + } else { $messages->{'WrongTransfer'} = $transfer->tobranch; $messages->{'WrongTransferItem'} = $item->itemnumber; } @@ -2531,8 +2530,9 @@ sub AddReturn { (C4::Context->preference("UseBranchTransferLimits") and ! IsBranchTransferAllowed($branch, $returnbranch, $item->$BranchTransferLimitsType ) )) { - ModItemTransfer($item->itemnumber, $branch, $returnbranch, $transfer_trigger, { skip_record_index => 1 }); - $messages->{'WasTransfered'} = $returnbranch; + my $transfer = ModItemTransfer($item->itemnumber, $branch, $returnbranch, $transfer_trigger, { skip_record_index => 1 }); + $messages->{'TransferTo'} = $returnbranch; + $messages->{'WasTransfered'} = $transfer->id; $messages->{'TransferTrigger'} = $transfer_trigger; } else { $messages->{'NeedsTransfer'} = $returnbranch; diff --git a/C4/Items.pm b/C4/Items.pm index 3041d8f13e..174e7efa5c 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -386,7 +386,7 @@ sub ModItemTransfer { $transfer->transit({ skip_record_index => $params->{skip_record_index} }); } - return; + return $transfer; } =head2 ModDateLastSeen diff --git a/Koha/Item.pm b/Koha/Item.pm index 48a8f2ce1b..ed548a93b2 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -810,9 +810,13 @@ we still expect the item to end up at a final location eventually. sub get_transfer { my ($self) = @_; + my $transfers = $self->_result->current_branchtransfers->search( + undef, + { 'order_by' => [ { '-desc' => 'datesent' }, { '-asc' => 'daterequested' } ] } + ); - my $transfer = $self->_result->current_branchtransfers->next; - return Koha::Item::Transfer->_new_from_dbic($transfer) if $transfer; + my $transfer = $transfers->next; + return Koha::Item::Transfer->_new_from_dbic($transfer) if $transfer; } =head3 transfer @@ -856,9 +860,12 @@ we still expect the item to end up at a final location eventually. sub get_transfers { my ($self) = @_; - my $transfer_rs = $self->_result->current_branchtransfers; + my $transfers_rs = $self->_result->current_branchtransfers->search( + undef, + { 'order_by' => [ { '-desc' => 'datesent' }, { '-asc' => 'daterequested' } ] } + ); - return Koha::Item::Transfers->_new_from_dbic($transfer_rs); + return Koha::Item::Transfers->_new_from_dbic($transfers_rs); } =head3 last_returned_by diff --git a/Koha/Schema/Result/Item.pm b/Koha/Schema/Result/Item.pm index 444d3a5097..92fe4d1088 100644 --- a/Koha/Schema/Result/Item.pm +++ b/Koha/Schema/Result/Item.pm @@ -1057,8 +1057,7 @@ __PACKAGE__->has_many( "$args->{foreign_alias}.datearrived" => undef, "$args->{foreign_alias}.datecancelled" => undef, }; - }, - { order_by => [ { -desc => 'datesent' }, { -asc => 'daterequested' } ] } + } ); # Relationship with bundled items diff --git a/circ/returns.pl b/circ/returns.pl index 72e004a795..b57e64a920 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -540,10 +540,11 @@ my $recalled = 0; if ( $messages->{'WasTransfered'} ) { $template->param( - found => 1, - transfer => $messages->{'WasTransfered'}, - trigger => $messages->{'TransferTrigger'}, - itemnumber => $itemnumber, + found => 1, + transfer => $messages->{'WasTransfered'}, + transferto => $messages->{'TransferTo'}, + trigger => $messages->{'TransferTrigger'}, + itemnumber => $itemnumber, ); } @@ -720,6 +721,9 @@ foreach my $code ( keys %$messages ) { elsif ( $code eq 'WasTransfered' ) { ; # FIXME... anything to do here? } + elsif ( $code eq 'TransferTo' ) { + ; # Handled above, along with WasTransfered + } elsif ( $code eq 'withdrawn' ) { $err{withdrawn} = 1; } diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt index b4cbc64bb9..6c132ef2b8 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ -122,7 +122,7 @@ [% END # /IF hold_auto_filled %] - [% IF ( trigger ) %] + [% IF ( trigger && !transfertodo ) %]

Reason for transfer

@@ -754,7 +754,7 @@

- Please return this item to [% IF transfer %][% Branches.GetName( transfer ) | html %][% ELSE %][% Branches.GetName( returnbranch ) | html %][% END %] + Please return this item to [% IF transferto %][% Branches.GetName( transferto ) | html %][% ELSE %][% Branches.GetName( returnbranch ) | html %][% END %]

@@ -781,6 +781,7 @@ [% ELSE %] + [% END %] diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 01699751df..009c968c44 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -6654,25 +6654,38 @@ subtest 'Tests for transfer not in transit' => sub { plan tests => 2; - # These tests are to ensure a 'pending' transfer, generated by # stock rotation, will be advanced when checked in - my $item = $builder->build_sample_item(); - my $transfer = $builder->build_object({ class => 'Koha::Item::Transfers', value => { - itemnumber => $item->id, - reason => 'StockrotationRepatriation', - datesent => undef, - frombranch => $item->homebranch, - }}); + my $item = $builder->build_sample_item(); + my $transfer = $builder->build_object( + { + class => 'Koha::Item::Transfers', + value => { + itemnumber => $item->id, + reason => 'StockrotationRepatriation', + datesent => undef, + frombranch => $item->homebranch, + } + } + ); my @return = AddReturn( $item->barcode, $item->homebranch, 0, undef ); is_deeply( \@return, - [ 0, { WasTransfered => $transfer->tobranch, TransferTrigger => 'StockrotationRepatriation', NotIssued => $item->barcode }, undef, {} ], "Item is reported to have been transferred"); + [ + 0, + { + TransferTo => $transfer->tobranch, WasTransfered => $transfer->id, + TransferTrigger => 'StockrotationRepatriation', NotIssued => $item->barcode + }, + undef, + {} + ], + "Item is reported to have been transferred" + ); $transfer->discard_changes; - ok( $transfer->datesent, 'The datesent field is populated, i.e. transfer is initiated'); - + is( $transfer->datesent, undef, 'The datesent field remains unpopulated, i.e. transfer is not yet initiated' ); }; subtest 'Tests for RecordLocalUseOnReturn' => sub { diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index a53f40ccb9..1b4c191956 100755 --- a/t/db_dependent/Koha/Item.t +++ b/t/db_dependent/Koha/Item.t @@ -20,7 +20,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 36; +use Test::More tests => 35; use Test::Exception; use Test::MockModule; @@ -1309,8 +1309,157 @@ subtest 'Tests for itemtype|item_type' => sub { $schema->storage->txn_rollback; }; +subtest 'get_transfer' => sub { + plan tests => 8; + $schema->storage->txn_begin; + + my $item = $builder->build_sample_item(); + + my $transfer = $item->get_transfer(); + is( $transfer, undef, 'Koha::Item->get_transfer should return undef if the item has no queued transfers' ); + + my $library_to = $builder->build( { source => 'Branch' } ); + + my $transfer_1 = $builder->build_object( + { + class => 'Koha::Item::Transfers', + value => { + itemnumber => $item->itemnumber, + frombranch => $item->holdingbranch, + tobranch => $library_to->{branchcode}, + reason => 'Manual', + datesent => undef, + datearrived => undef, + datecancelled => undef, + daterequested => \'NOW()' + } + } + ); + + $transfer = $item->get_transfer(); + is( + ref($transfer), 'Koha::Item::Transfer', + 'Koha::Item->get_transfer should return a Koha::Item::Transfer object' + ); + + my $transfer_2 = $builder->build_object( + { + class => 'Koha::Item::Transfers', + value => { + itemnumber => $item->itemnumber, + frombranch => $item->holdingbranch, + tobranch => $library_to->{branchcode}, + reason => 'Manual', + datesent => undef, + datearrived => undef, + datecancelled => undef, + daterequested => \'NOW()' + } + } + ); + + $transfer = $item->get_transfer(); + is( + $transfer->branchtransfer_id, $transfer_1->branchtransfer_id, + 'Koha::Item->get_transfer returns the oldest transfer request' + ); + + $transfer_2->datesent( \'NOW()' )->store; + $transfer = $item->get_transfer(); + is( + $transfer->branchtransfer_id, $transfer_2->branchtransfer_id, + 'Koha::Item->get_transfer returns the in_transit transfer' + ); + + my $transfer_3 = $builder->build_object( + { + class => 'Koha::Item::Transfers', + value => { + itemnumber => $item->itemnumber, + frombranch => $item->holdingbranch, + tobranch => $library_to->{branchcode}, + reason => 'Manual', + datesent => undef, + datearrived => undef, + datecancelled => undef, + daterequested => \'NOW()' + } + } + ); + + $transfer_2->datearrived( \'NOW()' )->store; + $transfer = $item->get_transfer(); + is( + $transfer->branchtransfer_id, $transfer_1->branchtransfer_id, + 'Koha::Item->get_transfer returns the next queued transfer' + ); + is( $transfer->itemnumber, $item->itemnumber, 'Koha::Item->get_transfer returns the right items transfer' ); + + $transfer_1->datecancelled( \'NOW()' )->store; + $transfer = $item->get_transfer(); + is( + $transfer->branchtransfer_id, $transfer_3->branchtransfer_id, + 'Koha::Item->get_transfer ignores cancelled transfers' + ); + + subtest "Ensure prefetches don't affect which transfer if returned" => sub { + plan tests => 5; + + # Reset test data + $transfer_1->datecancelled(undef)->store; + $transfer_2->set( { datesent => undef, datearrived => undef } )->store; + + my $prefetched_item = Koha::Items->search( + { 'me.itemnumber' => $item->itemnumber }, + { prefetch => ['current_branchtransfers'] } + )->next; + + $transfer = $prefetched_item->get_transfer(); + is( + $transfer->branchtransfer_id, $transfer_1->branchtransfer_id, + 'Koha::Item->get_transfer returns the oldest transfer request' + ); + + $transfer_2->datesent( \'NOW()' )->store; + $prefetched_item = Koha::Items->search( + { 'me.itemnumber' => $item->itemnumber }, + { prefetch => ['current_branchtransfers'] } + )->next; + + $transfer = $prefetched_item->get_transfer(); + is( + $transfer->branchtransfer_id, $transfer_2->branchtransfer_id, + 'Koha::Item->get_transfer returns the in_transit transfer' + ); + + $transfer_2->datearrived( \'NOW()' )->store; + $prefetched_item = Koha::Items->search( + { 'me.itemnumber' => $item->itemnumber }, + { prefetch => ['current_branchtransfers'] } + )->next; + $transfer = $prefetched_item->get_transfer(); + is( + $transfer->branchtransfer_id, $transfer_1->branchtransfer_id, + 'Koha::Item->get_transfer returns the next queued transfer' + ); + is( $transfer->itemnumber, $item->itemnumber, 'Koha::Item->get_transfer returns the right items transfer' ); + + $transfer_1->datecancelled( \'NOW()' )->store; + $prefetched_item = Koha::Items->search( + { 'me.itemnumber' => $item->itemnumber }, + { prefetch => ['current_branchtransfers'] } + )->next; + $transfer = $prefetched_item->get_transfer(); + is( + $transfer->branchtransfer_id, $transfer_3->branchtransfer_id, + 'Koha::Item->get_transfer ignores cancelled transfers' + ); + }; + $schema->storage->txn_rollback; +}; + subtest 'get_transfers' => sub { - plan tests => 16; + plan tests => 17; $schema->storage->txn_begin; my $item = $builder->build_sample_item(); @@ -1405,6 +1554,104 @@ subtest 'get_transfers' => sub { $result_1 = $transfers->next; is( $result_1->branchtransfer_id, $transfer_3->branchtransfer_id, 'Koha::Item->get_transfers returns the only transfer that remains'); + subtest "Ensure prefetches don't affect the return order" => sub { + plan tests => 13; + + # Reset test data + $transfer_2->datesent(undef)->store; + $transfer_2->datearrived(undef)->store; + $transfer_1->datecancelled(undef)->store; + + my $prefetched_item = Koha::Items->search( + { 'me.itemnumber' => $item->itemnumber }, + { prefetch => ['current_branchtransfers'] } + )->next; + + $transfers = $prefetched_item->get_transfers(); + is( + $transfers->count, 3, + 'When there are multiple open transfer requests, the Koha::Item::Transfers object contains them all' + ); + $result_1 = $transfers->next; + $result_2 = $transfers->next; + $result_3 = $transfers->next; + is( + $result_1->branchtransfer_id, $transfer_1->branchtransfer_id, + 'Koha::Item->get_transfers returns the oldest transfer request first' + ); + is( + $result_2->branchtransfer_id, $transfer_2->branchtransfer_id, + 'Koha::Item->get_transfers returns the newer transfer request second' + ); + is( + $result_3->branchtransfer_id, $transfer_3->branchtransfer_id, + 'Koha::Item->get_transfers returns the newest transfer request last' + ); + + $transfer_2->datesent( \'NOW()' )->store; + $prefetched_item = Koha::Items->search( + { 'me.itemnumber' => $item->itemnumber }, + { prefetch => ['current_branchtransfers'] } + )->next; + $transfers = $prefetched_item->get_transfers(); + is( + $transfers->count, 3, + 'When one transfer is set to in_transit, the Koha::Item::Transfers object still contains them all' + ); + $result_1 = $transfers->next; + $result_2 = $transfers->next; + $result_3 = $transfers->next; + is( + $result_1->branchtransfer_id, $transfer_2->branchtransfer_id, + 'Koha::Item->get_transfers returns the active transfer request first' + ); + is( + $result_2->branchtransfer_id, $transfer_1->branchtransfer_id, + 'Koha::Item->get_transfers returns the other transfers oldest to newest' + ); + is( + $result_3->branchtransfer_id, $transfer_3->branchtransfer_id, + 'Koha::Item->get_transfers returns the other transfers oldest to newest' + ); + + $transfer_2->datearrived( \'NOW()' )->store; + $prefetched_item = Koha::Items->search( + { 'me.itemnumber' => $item->itemnumber }, + { prefetch => ['current_branchtransfers'] } + )->next; + $transfers = $prefetched_item->get_transfers(); + is( + $transfers->count, 2, + 'Once a transfer is received, it no longer appears in the list from ->get_transfers()' + ); + $result_1 = $transfers->next; + $result_2 = $transfers->next; + is( + $result_1->branchtransfer_id, $transfer_1->branchtransfer_id, + 'Koha::Item->get_transfers returns the other transfers oldest to newest' + ); + is( + $result_2->branchtransfer_id, $transfer_3->branchtransfer_id, + 'Koha::Item->get_transfers returns the other transfers oldest to newest' + ); + + $transfer_1->datecancelled( \'NOW()' )->store; + $prefetched_item = Koha::Items->search( + { 'me.itemnumber' => $item->itemnumber }, + { prefetch => ['current_branchtransfers'] } + )->next; + $transfers = $prefetched_item->get_transfers(); + is( + $transfers->count, 1, + 'Once a transfer is cancelled, it no longer appears in the list from ->get_transfers()' + ); + $result_1 = $transfers->next; + is( + $result_1->branchtransfer_id, $transfer_3->branchtransfer_id, + 'Koha::Item->get_transfers returns the only transfer that remains' + ); + }; + $schema->storage->txn_rollback; };