From f509e8d3095eeb3d18a1591fa1cf457967958c9b Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Thu, 9 Feb 2023 11:26:56 -0300 Subject: [PATCH] Bug 31051: (QA follow-up) Use a single query and avoid duplicated sums This patch makes the `get_savings` method use a single query to calculate the sum of the replacement prices. This way we save one query per item and just rely on the DB features. It has a side effect: we are not summing items twice. Added tests for the 'itenumber is null' pathological but common use case (specially in old_issues), as mentioned by Lucas. Handling for this is added (grep filtering out undefined ones) and also in the return, for the empty case, with // 0. To test: 1. Apply this patch 2. Run: qa -c 6 --run-tests => SUCCESS: All good 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi --- Koha/Patron.pm | 25 +++++++------------------ t/db_dependent/Koha/Patron.t | 10 +++++++++- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index dc329ed7063..9282b38307f 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -2437,27 +2437,16 @@ Use the replacement price of patron's old and current issues to calculate how mu =cut sub get_savings { - my ( $self ) = @_; + my ($self) = @_; - my $savings = 0; + my @itemnumbers = grep { defined $_ } ( $self->old_checkouts->get_column('itemnumber'), $self->checkouts->get_column('itemnumber') ); - # get old checkouts - my @old_checkouts = $self->old_checkouts->as_list; - foreach my $old_checkout ( @old_checkouts ) { - if ( $old_checkout->item ) { - $savings += $old_checkout->item->replacementprice; + return Koha::Items->search( + { itemnumber => { -in => \@itemnumbers } }, + { select => [ { sum => 'me.replacementprice' } ], + as => ['total_savings'] } - } - - # get current checkouts - my @checkouts = $self->checkouts->as_list; - foreach my $checkout ( @checkouts ) { - if ( $checkout->item ) { - $savings += $checkout->item->replacementprice; - } - } - - return $savings; + )->next->get_column('total_savings') // 0; } =head2 Internal methods diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index f3b83ec4d14..f0910bd9591 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -1352,7 +1352,8 @@ subtest 'notify_library_of_registration()' => sub { }; subtest 'get_savings tests' => sub { - plan tests => 2; + + plan tests => 4; $schema->storage->txn_begin; @@ -1365,6 +1366,13 @@ subtest 'get_savings tests' => sub { my $item1 = $builder->build_object({ class => 'Koha::Items' }, { value => { biblionumber => $biblio1->biblionumber, replacementprice => '5.00', holdingbranch => $library->branchcode, homebranch => $library->branchcode } }); my $item2 = $builder->build_object({ class => 'Koha::Items' }, { value => { biblionumber => $biblio1->biblionumber, replacementprice => '5.00', holdingbranch => $library->branchcode, homebranch => $library->branchcode } }); + is( $patron->get_savings, 0, 'No checkouts, no savings' ); + + # Add an old checkout with deleted itemnumber + $builder->build_object({ class => 'Koha::Old::Checkouts', value => { itemnumber => undef, borrowernumber => $patron->id } }); + + is( $patron->get_savings, 0, 'No checkouts with itemnumber, no savings' ); + AddIssue( $patron->unblessed, $item1->barcode ); AddIssue( $patron->unblessed, $item2->barcode ); -- 2.34.1