From 65c3af5f26ff1cae677aa56802855ffc5b6744bc Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi 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 --- 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.32.0