From 394d5656337f73040524e3d5bc62d5dbef275463 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 25 Nov 2016 12:02:01 +0000 Subject: [PATCH] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues --- C4/Circulation.pm | 40 ++++++++++------------ C4/ILSDI/Services.pm | 8 +++-- C4/SIP/ILS/Item.pm | 8 ++--- C4/SIP/ILS/Transaction.pm | 8 ++--- offline_circ/enqueue_koc.pl | 7 ++-- offline_circ/process_koc.pl | 13 +++---- opac/opac-reserve.pl | 7 ++-- reserve/request.pl | 7 ++-- t/db_dependent/Circulation.t | 3 +- t/db_dependent/Circulation/SwitchOnSiteCheckouts.t | 7 ++-- t/db_dependent/Circulation/issue.t | 8 +---- 11 files changed, 58 insertions(+), 58 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 80ae062..878ce97 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -308,7 +308,7 @@ sub transferbook { my $messages; my $dotransfer = 1; my $itemnumber = GetItemnumberFromBarcode( $barcode ); - my $issue = GetItemIssue($itemnumber); + my $issue = Koha::Issues->find({ itemnumber => $itemnumber }); my $biblio = GetBiblioFromItemNumber($itemnumber); # bad barcode.. @@ -350,9 +350,9 @@ sub transferbook { } # check if it is still issued to someone, return it... - if ($issue->{borrowernumber}) { + if ( $issue ) { AddReturn( $barcode, $fbr ); - $messages->{'WasReturned'} = $issue->{borrowernumber}; + $messages->{'WasReturned'} = $issue->borrowernumber; } # find reserves..... @@ -671,7 +671,7 @@ sub CanBookBeIssued { my $override_high_holds = $params->{override_high_holds} || 0; my $item = GetItem(GetItemnumberFromBarcode( $barcode )); - my $issue = GetItemIssue($item->{itemnumber}); + my $issue = Koha::Checkouts->find( { itemnumber => $item->{itemnumber} } ); my $biblioitem = GetBiblioItemData($item->{biblioitemnumber}); $item->{'itemtype'}=$item->{'itype'}; my $dbh = C4::Context->dbh; @@ -829,9 +829,9 @@ sub CanBookBeIssued { # my $switch_onsite_checkout = C4::Context->preference('SwitchOnSiteCheckouts') - and $issue->{onsite_checkout} and $issue - and $issue->{borrowernumber} == $borrower->{'borrowernumber'} ? 1 : 0; + and $issue->onsite_checkout + and $issue->borrowernumber == $borrower->{'borrowernumber'} ? 1 : 0; my $toomany = TooMany( $borrower, $item->{biblionumber}, $item, { onsite_checkout => $onsite_checkout, switch_onsite_checkout => $switch_onsite_checkout, } ); # if TooMany max_allowed returns 0 the user doesn't have permission to check out this book if ( $toomany ) { @@ -939,13 +939,13 @@ sub CanBookBeIssued { # # CHECK IF BOOK ALREADY ISSUED TO THIS BORROWER # - if ( $issue->{borrowernumber} && $issue->{borrowernumber} eq $borrower->{'borrowernumber'} ){ + if ( $issue && $issue->borrowernumber eq $borrower->{'borrowernumber'} ){ # Already issued to current borrower. # If it is an on-site checkout if it can be switched to a normal checkout # or ask whether the loan should be renewed - if ( $issue->{onsite_checkout} + if ( $issue->onsite_checkout and C4::Context->preference('SwitchOnSiteCheckouts') ) { $messages{ONSITE_CHECKOUT_WILL_BE_SWITCHED} = 1; } else { @@ -966,10 +966,10 @@ sub CanBookBeIssued { } } } - elsif ($issue->{borrowernumber}) { + elsif ( $issue ) { # issued to someone else - my $currborinfo = C4::Members::GetMember( borrowernumber => $issue->{borrowernumber} ); + my $currborinfo = C4::Members::GetMember( borrowernumber => $issue->borrowernumber ); my ( $can_be_returned, $message ) = CanBookBeReturned( $item, C4::Context->userenv->{branch} ); @@ -1299,13 +1299,13 @@ sub AddIssue { my $branch = _GetCircControlBranch( $item, $borrower ); # get actual issuing if there is one - my $actualissue = GetItemIssue( $item->{itemnumber} ); + my $actualissue = Koha::Checkouts->find( { itemnumber => $item->{itemnumber} } ); # get biblioinformation for this item my $biblio = GetBiblioFromItemNumber( $item->{itemnumber} ); # check if we just renew the issue. - if ( $actualissue->{borrowernumber} eq $borrower->{'borrowernumber'} + if ( $actualissue and $actualissue->borrowernumber eq $borrower->{'borrowernumber'} and not $switch_onsite_checkout ) { $datedue = AddRenewal( $borrower->{'borrowernumber'}, @@ -1317,8 +1317,7 @@ sub AddIssue { } else { # it's NOT a renewal - if ( $actualissue->{borrowernumber} - and not $switch_onsite_checkout ) { + if ( $actualissue and not $switch_onsite_checkout ) { # This book is currently on loan, but not to the person # who wants to borrow it now. mark it returned before issuing to the new borrower my ( $allowed, $message ) = CanBookBeReturned( $item, C4::Context->userenv->{branch} ); @@ -2984,9 +2983,9 @@ sub GetSoonestRenewDate { my $dbh = C4::Context->dbh; my $item = GetItem($itemnumber) or return; - my $itemissue = GetItemIssue($itemnumber) or return; + my $itemissue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return; - $borrowernumber ||= $itemissue->{borrowernumber}; + $borrowernumber ||= $itemissue->borrowernumber; my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber ) or return; @@ -3005,8 +3004,7 @@ sub GetSoonestRenewDate { and $issuing_rule->norenewalbefore ne "" ) { my $soonestrenewal = - $itemissue->{date_due}->clone() - ->subtract( + dt_from_string( $itemissue->date_due )->subtract( $issuing_rule->lengthunit => $issuing_rule->norenewalbefore ); if ( C4::Context->preference('NoRenewalBeforePrecision') eq 'date' @@ -3044,9 +3042,9 @@ sub GetLatestAutoRenewDate { my $dbh = C4::Context->dbh; my $item = GetItem($itemnumber) or return; - my $itemissue = GetItemIssue($itemnumber) or return; + my $itemissue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return; - $borrowernumber ||= $itemissue->{borrowernumber}; + $borrowernumber ||= $itemissue->borrowernumber; my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber ) or return; @@ -3062,7 +3060,7 @@ sub GetLatestAutoRenewDate { return if not $issuing_rule->no_auto_renewal_after or $issuing_rule->no_auto_renewal_after eq ''; - my $maximum_renewal_date = dt_from_string($itemissue->{issuedate}); + my $maximum_renewal_date = dt_from_string($itemissue->issuedate); $maximum_renewal_date->add( $issuing_rule->lengthunit => $issuing_rule->no_auto_renewal_after ); diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 23ec85a..edd6ffa 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -34,6 +34,8 @@ use CGI qw ( -utf8 ); use DateTime; use C4::Auth; use C4::Members::Attributes qw(GetBorrowerAttributes); +use Koha::DateUtils; +use Koha::Issues; use Koha::Libraries; @@ -579,12 +581,12 @@ sub RenewLoan { my @renewal = CanBookBeRenewed( $borrowernumber, $itemnumber ); if ( $renewal[0] ) { AddRenewal( $borrowernumber, $itemnumber ); } - my $issue = GetItemIssue($itemnumber); + my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return; # FIXME should be handled # Hashref building my $out; - $out->{'renewals'} = $issue->{'renewals'}; - $out->{date_due} = $issue->{date_due}->strftime('%Y-%m-%d %H:%S'); + $out->{'renewals'} = $issue->renewals; + $out->{date_due} = dt_from_string($issue->date_due)->strftime('%Y-%m-%d %H:%S'); $out->{'success'} = $renewal[0]; $out->{'error'} = $renewal[1]; diff --git a/C4/SIP/ILS/Item.pm b/C4/SIP/ILS/Item.pm index 578f382..b0fb8d3 100644 --- a/C4/SIP/ILS/Item.pm +++ b/C4/SIP/ILS/Item.pm @@ -22,7 +22,7 @@ use C4::Circulation; use C4::Members; use C4::Reserves; use Koha::Database; - +use Koha::Checkouts; =encoding UTF-8 @@ -93,11 +93,11 @@ sub new { $item->{sip_media_type} = $itemtype->sip_media_type() if $itemtype; # check if its on issue and if so get the borrower - my $issue = GetItemIssue($item->{'itemnumber'}); + my $issue = Koha::Checkouts->find( { itemnumber => $item->{itemnumber} } ); if ($issue) { - $item->{due_date} = $issue->{date_due}; + $item->{due_date} = dt_from_string( $issue->date_due, 'sql' )->truncate( to => 'minute' ); } - my $borrower = GetMember(borrowernumber=>$issue->{'borrowernumber'}); + my $borrower = $issue ? GetMember( borrowernumber => $issue->borrowernumber ) : {}; $item->{patron} = $borrower->{'cardnumber'}; my $reserves = GetReservesFromBiblionumber({ biblionumber => $item->{biblionumber} }); $item->{hold_queue} = [ sort priority_sort @$reserves ]; diff --git a/C4/SIP/ILS/Transaction.pm b/C4/SIP/ILS/Transaction.pm index c751125..011aa98 100644 --- a/C4/SIP/ILS/Transaction.pm +++ b/C4/SIP/ILS/Transaction.pm @@ -8,8 +8,8 @@ use Carp; use strict; use warnings; use C4::Context; -use C4::Circulation qw( GetItemIssue ); use Koha::DateUtils; +use Koha::Checkouts; my %fields = ( ok => 0, @@ -45,9 +45,9 @@ sub duedatefromissue { } # renew from AddIssue ?? else { # need to reread the issue to get due date - $iss = GetItemIssue($itemnum); - if ($iss && $iss->{date_due} ) { - $due_dt = dt_from_string( $iss->{date_due} ); + $iss = Koha::Checkouts->find( { itemnumber => $itemnum } ); + if ($iss && $iss->date_due ) { + $due_dt = dt_from_string( $iss->date_due, 'sql' ); } } return $due_dt; diff --git a/offline_circ/enqueue_koc.pl b/offline_circ/enqueue_koc.pl index a81aecb..4c62c9b 100755 --- a/offline_circ/enqueue_koc.pl +++ b/offline_circ/enqueue_koc.pl @@ -32,6 +32,7 @@ use C4::Circulation; use C4::Items; use C4::Members; use C4::Stats; +use Koha::Checkouts; use Koha::Upload; use Date::Calc qw( Add_Delta_Days Date_to_Days ); @@ -191,7 +192,7 @@ sub _get_borrowernumber_from_barcode { my $item = GetBiblioFromItemNumber( undef, $barcode ); return unless $item->{'itemnumber'}; - my $issue = C4::Circulation::GetItemIssue( $item->{'itemnumber'} ); - return unless $issue->{'borrowernumber'}; - return $issue->{'borrowernumber'}; + my $issue = Koha::Checkouts->find( { itemnumber => $item->{itemnumber} } ); + return unless $issue; + return $issue->borrowernumber; } diff --git a/offline_circ/process_koc.pl b/offline_circ/process_koc.pl index f959d6f..cdda82f 100755 --- a/offline_circ/process_koc.pl +++ b/offline_circ/process_koc.pl @@ -37,6 +37,7 @@ use C4::Stats; use C4::BackgroundJob; use Koha::Upload; use Koha::Account; +use Koha::Checkouts; use Koha::Patrons; use Date::Calc qw( Add_Delta_Days Date_to_Days ); @@ -249,11 +250,11 @@ sub kocIssueItem { my $branchcode = C4::Context->userenv->{branch}; my $borrower = GetMember( 'cardnumber'=>$circ->{ 'cardnumber' } ); my $item = GetBiblioFromItemNumber( undef, $circ->{ 'barcode' } ); - my $issue = GetItemIssue( $item->{'itemnumber'} ); + my $issue = Koha::Checkouts->find( { itemnumber => $item->{itemnumber} } ); - if ( $issue->{ 'date_due' } ) { ## Item is currently checked out to another person. + if ( $issue ) { ## Item is currently checked out to another person. #warn "Item Currently Issued."; - my $issue = GetOpenIssue( $item->{'itemnumber'} ); + my $issue = GetOpenIssue( $item->{'itemnumber'} ); # FIXME Hum? That does not make sense, if it's in the issue table, the issue is open (i.e. returndate is null) if ( $issue->{'borrowernumber'} eq $borrower->{'borrowernumber'} ) { ## Issued to this person already, renew it. #warn "Item issued to this member already, renewing."; @@ -397,7 +398,7 @@ sub _get_borrowernumber_from_barcode { my $item = GetBiblioFromItemNumber( undef, $barcode ); return unless $item->{'itemnumber'}; - my $issue = C4::Circulation::GetItemIssue( $item->{'itemnumber'} ); - return unless $issue->{'borrowernumber'}; - return $issue->{'borrowernumber'}; + my $issue = Koha::Checkouts->find( { itemnumber => $item->{itemnumber} } ); + return unless $issue; + return $issue->borrowernumber; } diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index f1fe2ff..7535004 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -33,6 +33,7 @@ use C4::Members; use C4::Overdues; use C4::Debug; use Koha::DateUtils; +use Koha::Checkouts; use Koha::Libraries; use Koha::Patrons; use Date::Calc qw/Today Date_to_Days/; @@ -441,9 +442,9 @@ foreach my $biblioNum (@biblionumbers) { # If the item is currently on loan, we display its return date and # change the background color. - my $issues= GetItemIssue($itemNum); - if ( $issues->{'date_due'} ) { - $itemLoopIter->{dateDue} = output_pref({ dt => dt_from_string($issues->{date_due}, 'sql'), as_due_date => 1 }); + my $issue = Koha::Checkouts->find( { itemnumber => $itemNum } ); + if ( $issue ) { + $itemLoopIter->{dateDue} = output_pref({ dt => dt_from_string($issue->date_due, 'sql'), as_due_date => 1 }); $itemLoopIter->{backgroundcolor} = 'onloan'; } diff --git a/reserve/request.pl b/reserve/request.pl index 3f4dba3..84005a4 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -43,6 +43,7 @@ use C4::Utils::DataTables::Members; use C4::Members; use C4::Search; # enabled_staff_search_views use Koha::DateUtils; +use Koha::Checkouts; use Koha::Holds; use Koha::Libraries; use Koha::Patrons; @@ -379,9 +380,9 @@ foreach my $biblionumber (@biblionumbers) { # if the item is currently on loan, we display its return date and # change the background color - my $issues= GetItemIssue($itemnumber); - if ( $issues->{'date_due'} ) { - $item->{date_due} = $issues->{date_due_sql}; + my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ); + if ( $issue ) { + $item->{date_due} = $issue->date_due; $item->{backgroundcolor} = 'onloan'; } diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index e3d4c8d..c2b4302 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -36,6 +36,7 @@ use C4::Reserves; use C4::Overdues qw(UpdateFine CalcFine); use Koha::DateUtils; use Koha::Database; +use Koha::Checkouts; use Koha::Subscriptions; my $schema = Koha::Database->schema; @@ -313,7 +314,7 @@ C4::Context->dbh->do("DELETE FROM accountlines"); is (defined $issue2, 1, "Item 2 checked out, due date: " . $issue2->date_due()); - my $borrowing_borrowernumber = GetItemIssue($itemnumber)->{borrowernumber}; + my $borrowing_borrowernumber = Koha::Checkouts->find( { itemnumber => $itemnumber } )->borrowernumber; is ($borrowing_borrowernumber, $renewing_borrowernumber, "Item checked out to $renewing_borrower->{firstname} $renewing_borrower->{surname}"); my ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrowernumber, $itemnumber, 1); diff --git a/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t b/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t index 024801e..792f732 100644 --- a/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t +++ b/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t @@ -26,6 +26,7 @@ use C4::Context; use Koha::DateUtils qw( dt_from_string ); use Koha::Database; +use Koha::Checkouts; use t::lib::TestBuilder; use t::lib::Mocks; @@ -100,10 +101,10 @@ t::lib::Mocks::mock_preference('SwitchOnSiteCheckouts', 1); is( $messages->{ONSITE_CHECKOUT_WILL_BE_SWITCHED}, 1, 'If SwitchOnSiteCheckouts, switch the on-site checkout' ); is( exists $impossible->{TOO_MANY}, '', 'If SwitchOnSiteCheckouts, switch the on-site checkout' ); C4::Circulation::AddIssue( $patron, $item->{barcode}, undef, undef, undef, undef, { switch_onsite_checkout => 1 } ); -my $issue = C4::Circulation::GetItemIssue( $item->{itemnumber} ); -is( $issue->{onsite_checkout}, 0, 'The issue should have been switched to a regular checkout' ); +my $issue = Koha::Checkouts->find( { itemnumber => $item->{itemnumber} } ); +is( $issue->onsite_checkout, 0, 'The issue should have been switched to a regular checkout' ); my $five_days_after = dt_from_string->add( days => 5 )->set( hour => 23, minute => 59, second => 0 ); -is( $issue->{date_due}, $five_days_after, 'The date_due should have been set depending on the circ rules when the on-site checkout has been switched' ); +is( dt_from_string($issue->date_due, 'sql'), $five_days_after, 'The date_due should have been set depending on the circ rules when the on-site checkout has been switched' ); # Specific case t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 1); diff --git a/t/db_dependent/Circulation/issue.t b/t/db_dependent/Circulation/issue.t index 22f4d0a..312f6a9 100644 --- a/t/db_dependent/Circulation/issue.t +++ b/t/db_dependent/Circulation/issue.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 33; +use Test::More tests => 32; use DateTime::Duration; use t::lib::Mocks; @@ -45,7 +45,6 @@ can_ok( AddReturn GetBiblioIssues GetIssuingCharges - GetItemIssue GetOpenIssue GetRenewCount GetUpcomingDueIssues @@ -234,11 +233,6 @@ ok( $stat, "Bug 17781 - 'Improper branchcode set during renewal' still fixed" ); #Test GetBiblioIssues is( GetBiblioIssues(), undef, "GetBiblio Issues without parameters" ); -#Test GetItemIssue -#FIXME : As the issues are not correctly added in the database, these tests don't work correctly -is(GetItemIssue,undef,"Without parameter GetItemIssue returns undef"); -#is(GetItemIssue($item_id1),{},"Item1's issues"); - #Test GetOpenIssue is( GetOpenIssue(), undef, "Without parameter GetOpenIssue returns undef" ); is( GetOpenIssue(-1), undef, -- 2.1.4