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 <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

C4/Circulation.pm

@@ -2374,10 +2374,9 @@ sub AddReturn {
             else {
                 $messages->{'TransferTrigger'} = $transfer->reason;
                 if ( $transfer->frombranch eq $branch ) {
-                    $transfer->transit;
+                    $messages->{'TransferTo'}    = $transfer->tobranch;
-                    $messages->{'WasTransfered'}   = $transfer->tobranch;
+                    $messages->{'WasTransfered'} = $transfer->id;
-                }
+                } else {
-                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 });
+            my $transfer = ModItemTransfer($item->itemnumber, $branch, $returnbranch, $transfer_trigger, { skip_record_index => 1 });
-            $messages->{'WasTransfered'} = $returnbranch;
+            $messages->{'TransferTo'} = $returnbranch;
+            $messages->{'WasTransfered'} = $transfer->id;
             $messages->{'TransferTrigger'} = $transfer_trigger;
         } else {
             $messages->{'NeedsTransfer'} = $returnbranch;

C4/Items.pm

@@ -386,7 +386,7 @@ sub ModItemTransfer {
         $transfer->transit({ skip_record_index => $params->{skip_record_index} });
     }
 
-    return;
+    return $transfer;
 }
 
 =head2 ModDateLastSeen

Koha/Item.pm

@@ -810,8 +810,12 @@ 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;
+    my $transfer = $transfers->next;
     return Koha::Item::Transfer->_new_from_dbic($transfer) if $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

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

circ/returns.pl

@@ -542,6 +542,7 @@ if ( $messages->{'WasTransfered'} ) {
     $template->param(
         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;
     }

koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt

@@ -122,7 +122,7 @@
                                 </div> <!-- /.hold-auto-filled -->
                             [% END # /IF hold_auto_filled %]
 
-                            [% IF ( trigger ) %]
+                            [% IF ( trigger && !transfertodo ) %]
                                  <div id="transfer-trigger" class="alert alert-info">
                                      <h3>Reason for transfer</h3>
                                      <p>
@@ -754,7 +754,7 @@
 
                                                 <div class="modal-header">
                                                     <h1 class="modal-title">
-                                                        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 %]
                                                     </h1>
                                                 </div>
                                                 <div class="modal-body">
@@ -781,6 +781,7 @@
                                                         <button type="button" name="dotransfer" class="btn btn-default print openWin" data-url="transfer-slip.pl?transferitem=[% itemnumber | uri %]&amp;&amp;branchcode=[% returnbranch | uri %]&amp;op=slip"><i class="fa fa-print"></i> Yes, print slip</button>
                                                         <button type="button" data-bs-dismiss="modal" class="btn btn-default deny" name="notransfer" value="No" accesskey="n"><i class="fa fa-times"></i> No (N)</button>
                                                     [% ELSE %]
+                                                        <input type="hidden" name="transit" value="[% transfer | uri %]"/>
                                                         <button type="button" data-bs-dismiss="modal" class="btn btn-default" accesskey="y"><i class="fa fa-check"></i> OK (Y)</button>
                                                         <button type="button" data-bs-dismiss="modal" class="btn btn-default print openWin" data-url="transfer-slip.pl?transferitem=[% itemnumber | uri %]&amp;branchcode=[% transfer | uri %]&amp;op=slip" accesskey="p"><i class="fa fa-print"></i> Print slip (P)</button>
                                                     [% END %]

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 => {
+    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 {

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;
 };