From 39a2763109ba2604b9fa738f302b3666760265c0 Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
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 <tomascohen@theke.io>
---
 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 dc329ed706..9282b38307 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 f3b83ec4d1..f0910bd959 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.39.5