gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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>
This commit is contained in:
Tomas Cohen Arazi 2022-03-11 16:46:45 -03:00 committed by Fridolin Somers
parent ea6eb301a6
commit 514cbb809a
13 changed files with 130 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
C4/Circulation.pm
View File

@ -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

2
C4/Overdues.pm
View File

@ -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 ) {

2
C4/Reserves.pm
View File

@ -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');

18
Koha/Biblio.pm
View File

@ -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 );

15
Koha/Item.pm
View File

@ -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.

16
Koha/Patron.pm
View File

@ -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 ( $self ) = @_;
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 });
}
return $recalls_rs;
return Koha::Recalls->search({ borrowernumber => $self->borrowernumber });
}
=head2 Internal methods

45
Koha/Recalls.pm
View File

@ -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({

2
circ/circulation.pl
View File

@ -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,
);
}

2
members/moremember.pl
View File

@ -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,
);

2
opac/opac-recall.pl
View File

@ -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,

10
t/db_dependent/Koha/Biblio.t
View File

@ -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" );

10
t/db_dependent/Koha/Patron.t
View File

@ -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;
};

51
t/db_dependent/Koha/Recalls.t
View File

@ -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;
};