From 0d34450a7b8dca5bada5001d4044f6c58eb8bae8 Mon Sep 17 00:00:00 2001 From: Lucas Gass Date: Mon, 6 Oct 2025 16:49:22 +0000 Subject: [PATCH] Bug 26993: (follow-up) Return Koha::Patron resultset from last_returned_by_all --- Koha/Item.pm | 8 +++--- .../prog/en/modules/catalogue/moredetail.tt | 2 +- .../Circulation/StoreLastBorrower.t | 25 +++++++++++-------- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/Koha/Item.pm b/Koha/Item.pm index 5824d526da7..36946689d02 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -938,7 +938,7 @@ sub last_returned_by_all { my $max_stored_borrowers = C4::Context->preference('StoreLastBorrower') || 0; - my @borrowers = $self->_result->last_returned_by( + my $last_returned_rs = $self->_result->last_returned_by( {}, { order_by => [ { '-desc' => 'created_on' }, { '-desc' => 'id' } ], @@ -946,9 +946,9 @@ sub last_returned_by_all { } ); - my @stored_borrowers = map { Koha::Patron->_new_from_dbic( $_->borrowernumber ) } @borrowers; + my $borrowers_rs = $last_returned_rs->search_related('borrowernumber'); - return \@stored_borrowers; + return Koha::Patrons->_new_from_dbic($borrowers_rs); } =head3 last_returned_by @@ -977,7 +977,7 @@ sub last_returned_by { # If StoreLastBorrower is 0 or disabled, bail without storing anything. Also delete any remaining rows from the table. if ( $max_stored_borrowers == 0 ) { - $self->_result->last_returned_by->delete_all; + $self->last_returned_by_all->delete; return $self; } diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt index 655f0244a8b..c63c768963a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt @@ -496,7 +496,7 @@ [% IF Koha.Preference('StoreLastBorrower') && Koha.Preference('StoreLastBorrower') > 0 %] [% SET all_borrowers = ITEM_DAT.object.last_returned_by_all %] - [% IF all_borrowers.size > 0 %] + [% IF all_borrowers.count > 0 %]
  • Last returned by: [% FOREACH patron IN all_borrowers %] diff --git a/t/db_dependent/Circulation/StoreLastBorrower.t b/t/db_dependent/Circulation/StoreLastBorrower.t index 48648716e2b..f395e24d1bd 100755 --- a/t/db_dependent/Circulation/StoreLastBorrower.t +++ b/t/db_dependent/Circulation/StoreLastBorrower.t @@ -153,7 +153,7 @@ subtest 'Test StoreLastBorrower with multiple borrowers' => sub { # Test last_returned_by_all with no borrowers t::lib::Mocks::mock_preference( 'StoreLastBorrower', '3' ); my $borrowers = $item->last_returned_by_all(); - is( scalar(@$borrowers), 0, 'last_returned_by_all returns empty array when no borrowers stored' ); + is( $borrowers->count, 0, 'last_returned_by_all returns empty set when no borrowers stored' ); # Add 3 borrowers for testing, checkout/check in my @patrons; @@ -184,12 +184,13 @@ subtest 'Test StoreLastBorrower with multiple borrowers' => sub { # Test last_returned_by_all returns all borrowers $borrowers = $item->last_returned_by_all(); - is( scalar(@$borrowers), 3, 'Correctly returns 3 borrowers' ); + is( $borrowers->count, 3, 'Correctly returns 3 borrowers' ); # Test ordering - is( $borrowers->[0]->borrowernumber, $patrons[2]->{borrowernumber}, 'Most recent borrower first' ); - is( $borrowers->[1]->borrowernumber, $patrons[1]->{borrowernumber}, 'Second most recent borrower second' ); - is( $borrowers->[2]->borrowernumber, $patrons[0]->{borrowernumber}, 'Oldest borrower last' ); + my @borrowers_array = $borrowers->as_list; + is( $borrowers_array[0]->borrowernumber, $patrons[2]->{borrowernumber}, 'Most recent borrower first' ); + is( $borrowers_array[1]->borrowernumber, $patrons[1]->{borrowernumber}, 'Second most recent borrower second' ); + is( $borrowers_array[2]->borrowernumber, $patrons[0]->{borrowernumber}, 'Oldest borrower last' ); # Add 2 more borrowers/check out/check in for my $i ( 4 .. 5 ) { @@ -218,10 +219,11 @@ subtest 'Test StoreLastBorrower with multiple borrowers' => sub { $item = $item->get_from_storage; $borrowers = $item->last_returned_by_all(); is( - scalar(@$borrowers), 3, + $borrowers->count, 3, 'We only retain 3 borrowers when the sys pref is set to 3, even though there are 5 checkouts/checkins' ); - is( $borrowers->[0]->borrowernumber, $patrons[4]->{borrowernumber}, 'Most recent borrower after cleanup' ); + @borrowers_array = $borrowers->as_list; + is( $borrowers_array[0]->borrowernumber, $patrons[4]->{borrowernumber}, 'Most recent borrower after cleanup' ); # Reduce StoreLastBorrower to 2 t::lib::Mocks::mock_preference( 'StoreLastBorrower', '2' ); @@ -250,9 +252,10 @@ subtest 'Test StoreLastBorrower with multiple borrowers' => sub { $item = $item->get_from_storage; $borrowers = $item->last_returned_by_all(); - is( scalar(@$borrowers), 2, 'StoreLastBorrower was reduced to 2, we should now only keep 2 in the table' ); + is( $borrowers->count, 2, 'StoreLastBorrower was reduced to 2, we should now only keep 2 in the table' ); + @borrowers_array = $borrowers->as_list; is( - $borrowers->[0]->borrowernumber, $yet_another_patron->{borrowernumber}, + $borrowers_array[0]->borrowernumber, $yet_another_patron->{borrowernumber}, 'Most recent borrower after limit reduction' ); @@ -284,7 +287,7 @@ subtest 'Test StoreLastBorrower with multiple borrowers' => sub { $item = $item->get_from_storage; $borrowers = $item->last_returned_by_all(); - is( scalar(@$borrowers), 0, 'last_returned_by_all respects preference value 0' ); + is( $borrowers->count, 0, 'last_returned_by_all respects preference value 0' ); my $cleanup_patron = $builder->build( { @@ -310,7 +313,7 @@ subtest 'Test StoreLastBorrower with multiple borrowers' => sub { $item = $item->get_from_storage; $borrowers = $item->last_returned_by_all(); - is( scalar(@$borrowers), 0, 'All entries cleared when preference is 0' ); + is( $borrowers->count, 0, 'All entries cleared when preference is 0' ); is( $item->last_returned_by, undef, 'last_returned_by returns undef when no records' ); }; -- 2.39.5