From 514cbb809a18d210680b605439d2c0b07bef3271 Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
Date: Fri, 11 Mar 2022 16:46:45 -0300
Subject: [PATCH] Bug 19532: (QA follow-up) Simplify resultset accessors

This patch makes the different ->recalls accessors implemented on this
bug be more standard. This means:
- They don't do special things like default sorting or stripping out
  special parameters. That's all left to the caller and the methods are
  clean: they just return the related objects
- Useful filtering methods for Koha::Recalls resultsets are added. The
  only used one (in the end) was ->filter_by_current. It seems like a
  better approach, because it gives devs more control on how they want
  to chain things, and there's a single place in which to maintain the
  criteria of what is 'current' or 'finished'. This clearly makes the
  'old' column obsolete IMHO, at least in the use cases I found. This is
  covered by tests as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
---
 C4/Circulation.pm             |  2 +-
 C4/Overdues.pm                |  2 +-
 C4/Reserves.pm                |  2 +-
 Koha/Biblio.pm                | 18 ++++---------
 Koha/Item.pm                  | 15 ++++++-----
 Koha/Patron.pm                | 16 +++--------
 Koha/Recalls.pm               | 45 +++++++++++++++++++++++++++++++
 circ/circulation.pl           |  2 +-
 members/moremember.pl         |  2 +-
 opac/opac-recall.pl           |  2 +-
 t/db_dependent/Koha/Biblio.t  | 10 +++----
 t/db_dependent/Koha/Patron.t  | 10 ++++---
 t/db_dependent/Koha/Recalls.t | 51 ++++++++++++++++++++++++++++++++++-
 13 files changed, 130 insertions(+), 47 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 03b5580d69..dea25b6915 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1125,7 +1125,7 @@ sub CanBookBeIssued {
     # Only bother doing this if UseRecalls is enabled and the item is recallable
     # Don't look at recalls that are in transit
     if ( C4::Context->preference('UseRecalls') and $item_object->can_be_waiting_recall ) {
-        my @recalls = $biblio->recalls->as_list;
+        my @recalls = $biblio->recalls({},{ order_by => { -asc => 'recalldate' } })->filter_by_current->as_list;
 
         foreach my $r ( @recalls ) {
             if ( $r->itemnumber and
diff --git a/C4/Overdues.pm b/C4/Overdues.pm
index 7d8877b303..09d6724ff3 100644
--- a/C4/Overdues.pm
+++ b/C4/Overdues.pm
@@ -260,7 +260,7 @@ sub CalcFine {
 
         # check if item has been recalled. recall should have been marked Overdue by cronjob, so only look at overdue recalls
         # only charge using recall_overdue_fine if there is an item-level recall for this particular item, OR a biblio-level recall
-        my @recalls = Koha::Recalls->search({ biblionumber => $item->{biblionumber}, old => undef, status => 'overdue' })->as_list;
+        my @recalls = Koha::Recalls->search({ biblionumber => $item->{biblionumber}, status => 'overdue' })->as_list;
         my $bib_level_recall = 0;
         $bib_level_recall = 1 if scalar @recalls > 0;
         foreach my $recall ( @recalls ) {
diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 9634a2bd4e..54af13d7f0 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -423,7 +423,7 @@ sub CanItemBeReserved {
 
     # check if a recall exists on this item from this borrower
     return { status => 'recall' }
-      if Koha::Recalls->search({ borrowernumber => $patron->borrowernumber, itemnumber => $item->itemnumber, old => undef })->count;
+      if $patron->recalls->filter_by_current->search({ itemnumber => $item->itemnumber })->count;
 
     my $controlbranch = C4::Context->preference('ReservesControlBranch');
 
diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm
index 42984a04e3..e2832c880d 100644
--- a/Koha/Biblio.pm
+++ b/Koha/Biblio.pm
@@ -1166,21 +1166,13 @@ sub get_marc_host {
 
     my @recalls = $biblio->recalls;
 
-Return all active recalls attached to this biblio, sorted by oldest first
+Return recalls linked to this biblio
 
 =cut
 
 sub recalls {
-    my ( $self, $params ) = @_;
-
-    my $args = {
-        biblionumber => $self->biblionumber,
-        old => undef,
-    };
-    $args->{ borrowernumber } = $params->{ borrowernumber } if $params->{ borrowernumber };
-
-    my $recalls_rs = $self->_result->recalls->search( $args, { order_by => { -asc => 'recalldate' } });
-    return Koha::Recalls->_new_from_dbic( $recalls_rs );
+    my ( $self ) = @_;
+    return Koha::Recalls->_new_from_dbic( scalar $self->_result->recalls );
 }
 
 =head3 can_be_recalled
@@ -1254,10 +1246,10 @@ sub can_be_recalled {
 
     if ( $patron ) {
         # check borrower has not reached open recalls allowed limit
-        return 0 if ( $patron->recalls->count >= $recalls_allowed );
+        return 0 if ( $patron->recalls->filter_by_current->count >= $recalls_allowed );
 
         # check borrower has not reached open recalls allowed per record limit
-        return 0 if ( $patron->recalls({ biblionumber => $self->biblionumber })->count >= $recalls_per_record );
+        return 0 if ( $patron->recalls->filter_by_current->search({ biblionumber => $self->biblionumber })->count >= $recalls_per_record );
 
         # check if any of the items under this biblio are already checked out by this borrower
         return 0 if ( Koha::Checkouts->search({ itemnumber => [ @all_itemnumbers ], borrowernumber => $patron->borrowernumber })->count > 0 );
diff --git a/Koha/Item.pm b/Koha/Item.pm
index 2b5ab5c044..80dcdf0d31 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -1517,13 +1517,13 @@ sub can_be_recalled {
 
     if ( $patron ) {
         # check borrower has not reached open recalls allowed limit
-        return 0 if ( $patron->recalls->count >= $rule->{recalls_allowed} );
+        return 0 if ( $patron->recalls->filter_by_current->count >= $rule->{recalls_allowed} );
 
         # check borrower has not reach open recalls allowed per record limit
-        return 0 if ( $patron->recalls({ biblionumber => $self->biblionumber })->count >= $rule->{recalls_per_record} );
+        return 0 if ( $patron->recalls->filter_by_current->search({ biblionumber => $self->biblionumber })->count >= $rule->{recalls_per_record} );
 
         # check if this patron has already recalled this item
-        return 0 if ( Koha::Recalls->search({ itemnumber => $self->itemnumber, borrowernumber => $patron->borrowernumber, old => undef })->count > 0 );
+        return 0 if ( Koha::Recalls->search({ itemnumber => $self->itemnumber, borrowernumber => $patron->borrowernumber })->filter_by_current->count > 0 );
 
         # check if this patron has already checked out this item
         return 0 if ( Koha::Checkouts->search({ itemnumber => $self->itemnumber, borrowernumber => $patron->borrowernumber })->count > 0 );
@@ -1608,9 +1608,12 @@ Get the most relevant recall for this item.
 sub check_recalls {
     my ( $self ) = @_;
 
-    my @recalls =
-      Koha::Recalls->search( { biblionumber => $self->biblionumber, itemnumber => [ $self->itemnumber, undef ], status => [ 'requested', 'overdue', 'waiting', 'in_transit' ] },
-        { order_by => { -asc => 'recalldate' } } )->as_list;
+    my @recalls = Koha::Recalls->search(
+        {   biblionumber => $self->biblionumber,
+            itemnumber   => [ $self->itemnumber, undef ]
+        },
+        { order_by => { -asc => 'recalldate' } }
+    )->filter_by_current->as_list;
 
     my $recall;
     # iterate through relevant recalls to find the best one.
diff --git a/Koha/Patron.pm b/Koha/Patron.pm
index 5f9a38f953..82760f200f 100644
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -2058,24 +2058,14 @@ sub safe_to_delete {
 
     my $recalls = $patron->recalls;
 
-    my $recalls = $patron->recalls({ biblionumber => $biblionumber });
-
-Return the patron's active recalls - total, or on a specific biblio
+Return the patron's recalls.
 
 =cut
 
 sub recalls {
-    my ( $self, $params ) = @_;
-
-    my $biblionumber = $params->{biblionumber};
-
-    my $recalls_rs = Koha::Recalls->search({ borrowernumber => $self->borrowernumber, old => undef });
-
-    if ( $biblionumber ) {
-        $recalls_rs = Koha::Recalls->search({ borrowernumber => $self->borrowernumber, old => undef, biblionumber => $biblionumber });
-    }
+    my ( $self ) = @_;
 
-    return $recalls_rs;
+    return Koha::Recalls->search({ borrowernumber => $self->borrowernumber });
 }
 
 =head2 Internal methods
diff --git a/Koha/Recalls.pm b/Koha/Recalls.pm
index b0777c3901..204680ca1b 100644
--- a/Koha/Recalls.pm
+++ b/Koha/Recalls.pm
@@ -35,6 +35,51 @@ Koha::Recalls - Koha Recalls Object set class
 
 =head2 Class methods
 
+=head3 filter_by_current
+
+    my $current_recalls = $recalls->filter_by_current;
+
+Returns a new resultset, filtering out finished recalls.
+
+=cut
+
+sub filter_by_current {
+    my ($self) = @_;
+
+    return $self->search(
+        {
+            status => [
+                'in_transit',
+                'overdue',
+                'requested',
+                'waiting',
+            ]
+        }
+    );
+}
+
+=head3 filter_by_finished
+
+    my $finished_recalls = $recalls->filter_by_finished;
+
+Returns a new resultset, filtering out current recalls.
+
+=cut
+
+sub filter_by_finished {
+    my ($self) = @_;
+
+    return $self->search(
+        {
+            status => [
+                'cancelled',
+                'expired',
+                'fulfilled',
+            ]
+        }
+    );
+}
+
 =head3 add_recall
 
     my ( $recall, $due_interval, $due_date ) = Koha::Recalls->add_recall({
diff --git a/circ/circulation.pl b/circ/circulation.pl
index 7c7f92344b..36f29d1b39 100755
--- a/circ/circulation.pl
+++ b/circ/circulation.pl
@@ -446,7 +446,7 @@ if ($patron) {
     $template->param(
         holds_count  => $holds->count(),
         WaitingHolds => $waiting_holds,
-        recalls => $patron->recalls,
+        recalls => $patron->recalls->filter_by_current->search({},->search({},{ order_by => { -asc => 'recalldate' } })),
         specific_patron => 1,
     );
 }
diff --git a/members/moremember.pl b/members/moremember.pl
index 2d3a39bc1c..af76827091 100755
--- a/members/moremember.pl
+++ b/members/moremember.pl
@@ -273,7 +273,7 @@ $template->param(
     files => Koha::Patron::Files->new( borrowernumber => $borrowernumber ) ->GetFilesInfo(),
     #debarments                => scalar GetDebarments({ borrowernumber => $borrowernumber }),
     has_modifications         => $has_modifications,
-    recalls         => $patron->recalls,
+    recalls         => $patron->recalls({},{ order_by => { -asc => 'recalldate' } })->filter_by_current,
     specific_patron => 1,
 );
 
diff --git a/opac/opac-recall.pl b/opac/opac-recall.pl
index 0e071dea78..8cea5efb79 100755
--- a/opac/opac-recall.pl
+++ b/opac/opac-recall.pl
@@ -46,7 +46,7 @@ if ( C4::Context->preference('UseRecalls') ) {
     my $items = Koha::Items->search({ biblionumber => $biblionumber })->as_list;
 
     # check if already recalled
-    my $recalled = $biblio->recalls({ borrowernumber => $borrowernumber })->count;
+    my $recalled = $biblio->recalls->filter_by_current->search({ borrowernumber => $borrowernumber })->count;
     if ( defined $recalled and $recalled > 0 ) {
         my $recalls_per_record = Koha::CirculationRules->get_effective_rule({
             categorycode => $patron->categorycode,
diff --git a/t/db_dependent/Koha/Biblio.t b/t/db_dependent/Koha/Biblio.t
index 3751fdca3e..0a7ed4d43f 100755
--- a/t/db_dependent/Koha/Biblio.t
+++ b/t/db_dependent/Koha/Biblio.t
@@ -885,7 +885,7 @@ subtest 'get_marc_authors() tests' => sub {
 
 subtest 'Recalls tests' => sub {
 
-    plan tests => 12;
+    plan tests => 13;
 
     $schema->storage->txn_begin;
 
@@ -929,14 +929,14 @@ subtest 'Recalls tests' => sub {
         }
     )->store;
 
-    my $recalls_count = $biblio->recalls->count;
-    is( $recalls_count, 3, 'Correctly get number of active recalls for biblio' );
+    my $recalls = $biblio->recalls;
+    is( $recalls->count, 3, 'Correctly get number of recalls for biblio' );
 
     $recall1->set_cancelled;
     $recall2->set_expired({ interface => 'COMMANDLINE' });
 
-    $recalls_count = $biblio->recalls->count;
-    is( $recalls_count, 1, 'Correctly get number of active recalls for biblio' );
+    is( $recalls->count, 3, 'Correctly get number of recalls for biblio' );
+    is( $recalls->filter_by_current->count, 1, 'Correctly get number of active recalls for biblio' );
 
     t::lib::Mocks::mock_preference('UseRecalls', 0);
     is( $biblio->can_be_recalled({ patron => $patron1 }), 0, "Can't recall with UseRecalls disabled" );
diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t
index a5f56167cc..a1aff0035e 100755
--- a/t/db_dependent/Koha/Patron.t
+++ b/t/db_dependent/Koha/Patron.t
@@ -1053,7 +1053,10 @@ subtest 'messages' => sub {
 
 subtest 'recalls() tests' => sub {
 
-    plan tests => 2;
+    plan tests => 3;
+
+    $schema->storage->txn_begin;
+
     my $patron = $builder->build_object( { class => 'Koha::Patrons' } );
     my $biblio1 = $builder->build_object({ class => 'Koha::Biblios' });
     my $item1 = $builder->build_object({ class => 'Koha::Items' }, { value => { biblionumber => $biblio1->biblionumber } });
@@ -1098,8 +1101,9 @@ subtest 'recalls() tests' => sub {
     )->store;
     $recall->set_cancelled;
 
-    is( $patron->recalls->count, 3, "Correctly gets this patron's active recalls" );
-    is( $patron->recalls({ biblionumber => $biblio1->biblionumber })->count, 2, "Correctly gets this patron's active recalls on a specific biblio" );
+    is( $patron->recalls->count,                                                                          4, "Correctly gets this patron's recalls" );
+    is( $patron->recalls->filter_by_current->count,                                                       3, "Correctly gets this patron's active recalls" );
+    is( $patron->recalls->filter_by_current->search( { biblionumber => $biblio1->biblionumber } )->count, 2, "Correctly gets this patron's active recalls on a specific biblio" );
 
     $schema->storage->txn_rollback;
 };
diff --git a/t/db_dependent/Koha/Recalls.t b/t/db_dependent/Koha/Recalls.t
index 4b0ac8f048..610a84a4e2 100755
--- a/t/db_dependent/Koha/Recalls.t
+++ b/t/db_dependent/Koha/Recalls.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 19;
+use Test::More tests => 20;
 use t::lib::TestBuilder;
 use t::lib::Mocks;
 
@@ -173,3 +173,52 @@ $recall = Koha::Recalls->find( $recall->recall_id );
 ok( $recall->fulfilled, "Recall fulfilled with move_recall" );
 
 $schema->storage->txn_rollback();
+
+subtest 'filter_by_current() and filter_by_finished() tests' => sub {
+
+    plan tests => 10;
+
+    $schema->storage->txn_begin;
+
+    my $in_transit = $builder->build_object( { class => 'Koha::Recalls', value => { status => 'in_transit' } } );
+    my $overdue    = $builder->build_object( { class => 'Koha::Recalls', value => { status => 'overdue' } } );
+    my $requested  = $builder->build_object( { class => 'Koha::Recalls', value => { status => 'requested' } } );
+    my $waiting    = $builder->build_object( { class => 'Koha::Recalls', value => { status => 'waiting' } } );
+    my $cancelled  = $builder->build_object( { class => 'Koha::Recalls', value => { status => 'cancelled' } } );
+    my $expired    = $builder->build_object( { class => 'Koha::Recalls', value => { status => 'expired' } } );
+    my $fulfilled  = $builder->build_object( { class => 'Koha::Recalls', value => { status => 'fulfilled' } } );
+
+    my $recalls = Koha::Recalls->search(
+        {
+            recall_id => [
+                $in_transit->id,
+                $overdue->id,
+                $requested->id,
+                $waiting->id,
+                $cancelled->id,
+                $expired->id,
+                $fulfilled->id,
+            ]
+        },
+        { order_by => [ 'recall_id' ] }
+    );
+
+    is( $recalls->count, 7, 'Resultset count is correct' );
+
+    my $current_recalls = $recalls->filter_by_current;
+    is( $current_recalls->count, 4, 'Current recalls count correct' );
+
+    is( $current_recalls->next->status, 'in_transit', 'Resultset correctly includes in_transit recall');
+    is( $current_recalls->next->status, 'overdue', 'Resultset correctly includes overdue recall');
+    is( $current_recalls->next->status, 'requested', 'Resultset correctly includes requested recall');
+    is( $current_recalls->next->status, 'waiting', 'Resultset correctly includes waiting recall');
+
+    my $finished_recalls = $recalls->filter_by_finished;
+    is( $finished_recalls->count, 3, 'Finished recalls count correct' );
+
+    is( $finished_recalls->next->status, 'cancelled', 'Resultset correctly includes cancelled recall');
+    is( $finished_recalls->next->status, 'expired', 'Resultset correctly includes expired recall');
+    is( $finished_recalls->next->status, 'fulfilled', 'Resultset correctly includes fulfilled recall');
+
+    $schema->storage->txn_rollback;
+};
-- 
2.39.5