From 9cf10e36be743832d556f1502f02525710a39e76 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 25 Nov 2016 11:12:20 +0100 Subject: [PATCH] Bug 17678: C4::Circulation - Replace GetIssues with Koha::Issues Content-Type: text/plain; charset=utf-8 The C4::Circulation::GetIssues subroutine is only called once and can be replaced with a call to Koha::Isues->search with a join on items. Test plan: - Apply first patch and make sure the tests pass - Apply second patch and make sure the tests still pass Signed-off-by: Josef Moravec Signed-off-by: Marcel de Rooy --- C4/Circulation.pm | 83 +++------------------- .../{GetIssues.t => GetPendingOnSiteCheckouts.t} | 23 +----- 2 files changed, 11 insertions(+), 95 deletions(-) rename t/db_dependent/Circulation/{GetIssues.t => GetPendingOnSiteCheckouts.t} (69%) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index b505031..cf0c3cf 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1064,14 +1064,18 @@ sub CanBookBeIssued { require C4::Serials; my $is_a_subscription = C4::Serials::CountSubscriptionFromBiblionumber($biblionumber); unless ($is_a_subscription) { - my $issues = GetIssues( { - borrowernumber => $borrower->{borrowernumber}, - biblionumber => $biblionumber, - } ); - my @issues = $issues ? @$issues : (); + my $issues = Koha::Issues->search( + { + borrowernumber => $borrower->{borrowernumber}, + biblionumber => $biblionumber, + }, + { + join => 'item', + } + ); # if we get here, we don't already have a loan on this item, # so if there are any loans on this bib, ask for confirmation - if (scalar @issues > 0) { + if ( $issues->count ) { $needsconfirmation{BIBLIO_ALREADY_ISSUED} = 1; } } @@ -2506,73 +2510,6 @@ sub GetOpenIssue { } -=head2 GetIssues - - $issues = GetIssues({}); # return all issues! - $issues = GetIssues({ borrowernumber => $borrowernumber, biblionumber => $biblionumber }); - -Returns all pending issues that match given criteria. -Returns a arrayref or undef if an error occurs. - -Allowed criteria are: - -=over 2 - -=item * borrowernumber - -=item * biblionumber - -=item * itemnumber - -=back - -=cut - -sub GetIssues { - my ($criteria) = @_; - - # Build filters - my @filters; - my @allowed = qw(borrowernumber biblionumber itemnumber); - foreach (@allowed) { - if (defined $criteria->{$_}) { - push @filters, { - field => $_, - value => $criteria->{$_}, - }; - } - } - - # Do we need to join other tables ? - my %join; - if (defined $criteria->{biblionumber}) { - $join{items} = 1; - } - - # Build SQL query - my $where = ''; - if (@filters) { - $where = "WHERE " . join(' AND ', map { "$_->{field} = ?" } @filters); - } - my $query = q{ - SELECT issues.* - FROM issues - }; - if (defined $join{items}) { - $query .= q{ - LEFT JOIN items ON (issues.itemnumber = items.itemnumber) - }; - } - $query .= $where; - - # Execute SQL query - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare($query); - my $rv = $sth->execute(map { $_->{value} } @filters); - - return $rv ? $sth->fetchall_arrayref({}) : undef; -} - =head2 GetItemIssues $issues = &GetItemIssues($itemnumber, $history); diff --git a/t/db_dependent/Circulation/GetIssues.t b/t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t similarity index 69% rename from t/db_dependent/Circulation/GetIssues.t rename to t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t index f271414..30aadcb 100644 --- a/t/db_dependent/Circulation/GetIssues.t +++ b/t/db_dependent/Circulation/GetPendingOnSiteCheckouts.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 10; +use Test::More tests => 2; use Test::MockModule; use t::lib::TestBuilder; @@ -72,27 +72,6 @@ AddIssue($borrower, '0101'); AddIssue($borrower, '0203'); # Begin tests... -my $issues; -$issues = C4::Circulation::GetIssues({biblionumber => $biblionumber1}); -is(scalar @$issues, 1, "Biblio $biblionumber1 has 1 item issued"); -is($issues->[0]->{itemnumber}, $itemnumber1, "First item of biblio $biblionumber1 is issued"); - -$issues = C4::Circulation::GetIssues({biblionumber => $biblionumber2}); -is(scalar @$issues, 1, "Biblio $biblionumber2 has 1 item issued"); -is($issues->[0]->{itemnumber}, $itemnumber3, "First item of biblio $biblionumber2 is issued"); - -$issues = C4::Circulation::GetIssues({borrowernumber => $borrowernumber}); -is(scalar @$issues, 2, "Borrower $borrowernumber checked out 2 items"); - -$issues = C4::Circulation::GetIssues({borrowernumber => $borrowernumber, biblionumber => $biblionumber1}); -is(scalar @$issues, 1, "One of those is an item from biblio $biblionumber1"); - -$issues = C4::Circulation::GetIssues({borrowernumber => $borrowernumber, biblionumber => $biblionumber2}); -is(scalar @$issues, 1, "The other is an item from biblio $biblionumber2"); - -$issues = C4::Circulation::GetIssues({itemnumber => $itemnumber2}); -is(scalar @$issues, 0, "No one has issued the second item of biblio $biblionumber2"); - my $onsite_checkouts = GetPendingOnSiteCheckouts; is( scalar @$onsite_checkouts, 0, "No pending on-site checkouts" ); -- 2.1.4