@@ -, +, @@
wait for the user to actually confirm that's the case
transfers for any other reason from display
when prefetch is used and add tests for this
---
C4/Circulation.pm | 12 +-
C4/Items.pm | 2 +-
Koha/Item.pm | 15 +-
Koha/Schema/Result/Item.pm | 3 +-
catalogue/detail.pl | 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 +++++++++++++++++-
t/db_dependent/Koha/Items.t | 78 +-----
10 files changed, 306 insertions(+), 110 deletions(-)
--- a/C4/Circulation.pm
+++ a/C4/Circulation.pm
@@ -2380,10 +2380,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;
}
@@ -2537,8 +2536,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;
--- a/C4/Items.pm
+++ a/C4/Items.pm
@@ -386,7 +386,7 @@ sub ModItemTransfer {
$transfer->transit({ skip_record_index => $params->{skip_record_index} });
}
- return;
+ return $transfer;
}
=head2 ModDateLastSeen
--- a/Koha/Item.pm
+++ a/Koha/Item.pm
@@ -776,9 +776,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 get_transfers
@@ -801,9 +805,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
--- a/Koha/Schema/Result/Item.pm
+++ a/Koha/Schema/Result/Item.pm
@@ -1024,8 +1024,7 @@ __PACKAGE__->has_many(
"$args->{foreign_alias}.datearrived" => undef,
"$args->{foreign_alias}.datecancelled" => undef,
};
- },
- { order_by => [ { -desc => 'datesent' }, { -asc => 'daterequested' } ] }
+ }
);
# Relationship with bundled items
--- a/catalogue/detail.pl
+++ a/catalogue/detail.pl
@@ -196,7 +196,8 @@ $params->{ itemlost } = 0 if $patron->category->hidelostitems && !$showallitems;
my $items_params = {
( $invalid_marc_record ? () : ( host_items => 1 ) ),
};
-my $items = $biblio->items($items_params)->search_ordered( $params, { prefetch => ['issue','current_branchtransfers'] } );
+my $items =
+ $biblio->items($items_params)->search_ordered( $params, { prefetch => [ 'issue', 'current_branchtransfers' ] } );
# flag indicating existence of at least one item linked via a host record
my $hostrecords = $biblio->host_items->count;
--- a/circ/returns.pl
+++ a/circ/returns.pl
@@ -520,10 +520,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,
);
}
@@ -697,6 +698,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;
}
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
+++ a/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
@@ -745,7 +745,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 %]
@@ -772,6 +772,7 @@
[% ELSE %]
+
[% END %]
--- a/t/db_dependent/Circulation.t
+++ a/t/db_dependent/Circulation.t
@@ -6093,25 +6093,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 {
--- a/t/db_dependent/Koha/Item.t
+++ a/t/db_dependent/Koha/Item.t
@@ -20,7 +20,7 @@
use Modern::Perl;
use utf8;
-use Test::More tests => 31;
+use Test::More tests => 32;
use Test::Exception;
use Test::MockModule;
@@ -1100,8 +1100,157 @@ subtest 'Tests for itemtype' => 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();
@@ -1196,6 +1345,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;
};
--- a/t/db_dependent/Koha/Items.t
+++ a/t/db_dependent/Koha/Items.t
@@ -19,7 +19,7 @@
use Modern::Perl;
-use Test::More tests => 16;
+use Test::More tests => 15;
use Test::MockModule;
use Test::Exception;
@@ -1382,82 +1382,6 @@ subtest 'store' => sub {
};
};
-subtest 'get_transfer' => sub {
- plan tests => 7;
-
- my $transfer = $new_item_1->get_transfer();
- is( $transfer, undef, 'Koha::Item->get_transfer should return undef if the item is not in transit' );
-
- my $library_to = $builder->build( { source => 'Branch' } );
-
- my $transfer_1 = $builder->build_object(
- {
- class => 'Koha::Item::Transfers',
- value => {
- itemnumber => $new_item_1->itemnumber,
- frombranch => $new_item_1->holdingbranch,
- tobranch => $library_to->{branchcode},
- reason => 'Manual',
- datesent => undef,
- datearrived => undef,
- datecancelled => undef,
- daterequested => \'NOW()'
- }
- }
- );
-
- $transfer = $new_item_1->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 => $new_item_1->itemnumber,
- frombranch => $new_item_1->holdingbranch,
- tobranch => $library_to->{branchcode},
- reason => 'Manual',
- datesent => undef,
- datearrived => undef,
- datecancelled => undef,
- daterequested => \'NOW()'
- }
- }
- );
-
- $transfer = $new_item_1->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 = $new_item_1->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 => $new_item_1->itemnumber,
- frombranch => $new_item_1->holdingbranch,
- tobranch => $library_to->{branchcode},
- reason => 'Manual',
- datesent => undef,
- datearrived => undef,
- datecancelled => undef,
- daterequested => \'NOW()'
- }
- }
- );
-
- $transfer_2->datearrived(\'NOW()')->store;
- $transfer = $new_item_1->get_transfer();
- is( $transfer->branchtransfer_id, $transfer_1->branchtransfer_id, 'Koha::Item->get_transfer returns the next queued transfer');
- is( $transfer->itemnumber, $new_item_1->itemnumber, 'Koha::Item->get_transfer returns the right items transfer' );
-
- $transfer_1->datecancelled(\'NOW()')->store;
- $transfer = $new_item_1->get_transfer();
- is( $transfer->branchtransfer_id, $transfer_3->branchtransfer_id, 'Koha::Item->get_transfer ignores cancelled transfers');
-};
-
subtest 'holds' => sub {
plan tests => 5;
--