From a2f208c25d8382f0e1c177333cf1881b074ad0dd 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 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Followed test plan, works as expected Signed-off-by: Marc Véron --- 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 | 8 +++-- offline_circ/process_koc.pl | 13 +++---- opac/opac-reserve.pl | 7 ++-- reserve/request.pl | 7 ++-- t/db_dependent/Circulation.t | 5 +-- t/db_dependent/Circulation/SwitchOnSiteCheckouts.t | 7 ++-- t/db_dependent/Circulation/issue.t | 8 +---- 11 files changed, 60 insertions(+), 59 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index a439021ba3..6988a0b48d 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -307,7 +307,7 @@ sub transferbook { my $messages; my $dotransfer = 1; my $itemnumber = GetItemnumberFromBarcode( $barcode ); - my $issue = GetItemIssue($itemnumber); + my $issue = Koha::Checkouts->find({ itemnumber => $itemnumber }); my $biblio = GetBiblioFromItemNumber($itemnumber); # bad barcode.. @@ -349,9 +349,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..... @@ -674,7 +674,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; @@ -832,9 +832,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 ) { @@ -941,13 +941,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 { @@ -968,10 +968,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} ); @@ -1304,13 +1304,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'}, @@ -1322,8 +1322,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} ); @@ -3037,9 +3036,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; @@ -3058,8 +3057,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' @@ -3097,9 +3095,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; @@ -3120,7 +3118,7 @@ sub GetLatestAutoRenewDate { my $maximum_renewal_date; if ( $issuing_rule->no_auto_renewal_after ) { - $maximum_renewal_date = dt_from_string($itemissue->{issuedate}); + $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 287ac0287b..0f4da11c23 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -34,8 +34,10 @@ use CGI qw ( -utf8 ); use DateTime; use C4::Auth; use C4::Members::Attributes qw(GetBorrowerAttributes); +use Koha::DateUtils; use Koha::Biblios; +use Koha::Checkouts; use Koha::Libraries; =head1 NAME @@ -581,12 +583,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 06497994db..d1ffec393c 100644 --- a/C4/SIP/ILS/Item.pm +++ b/C4/SIP/ILS/Item.pm @@ -23,7 +23,7 @@ use C4::Members; use C4::Reserves; use Koha::Database; use Koha::Biblios; - +use Koha::Checkouts; =encoding UTF-8 @@ -88,11 +88,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 $biblio = Koha::Biblios->find( $item->{biblionumber } ); my $holds = $biblio->current_holds->unblessed; diff --git a/C4/SIP/ILS/Transaction.pm b/C4/SIP/ILS/Transaction.pm index c751125907..011aa980b3 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 73a505387e..e7e8746c93 100755 --- a/offline_circ/enqueue_koc.pl +++ b/offline_circ/enqueue_koc.pl @@ -33,6 +33,8 @@ use C4::Items; use C4::Members; use C4::Stats; use Koha::UploadedFiles; +use Koha::Checkouts; +use Koha::Upload; use Date::Calc qw( Add_Delta_Days Date_to_Days ); @@ -192,7 +194,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 dbe8c402b8..7567dd3148 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::UploadedFiles; use Koha::Account; +use Koha::Checkouts; use Koha::Patrons; use Date::Calc qw( Add_Delta_Days Date_to_Days ); @@ -250,11 +251,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."; @@ -398,7 +399,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 7de7b978b9..c0fb2a96ba 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -36,6 +36,7 @@ use Koha::AuthorisedValues; use Koha::DateUtils; use Koha::Items; use Koha::ItemTypes; +use Koha::Checkouts; use Koha::Libraries; use Koha::Patrons; use Date::Calc qw/Today Date_to_Days/; @@ -465,9 +466,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 db35e165fa..791a2bdb48 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::Items; use Koha::ItemTypes; @@ -383,9 +384,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 9d0f77c005..7bbb25bf93 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -34,6 +34,7 @@ use C4::Overdues qw(UpdateFine CalcFine); use Koha::DateUtils; use Koha::Database; use Koha::IssuingRules; +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); @@ -1396,7 +1397,7 @@ subtest 'MultipleReserves' => sub { my $issue = AddIssue( $renewing_borrower, $barcode1); my $datedue = dt_from_string( $issue->date_due() ); is (defined $issue->date_due(), 1, "item 1 checked out"); - my $borrowing_borrowernumber = GetItemIssue($itemnumber1)->{borrowernumber}; + my $borrowing_borrowernumber = Koha::Checkouts->find({ itemnumber => $itemnumber1 })->borrowernumber; my %reserving_borrower_data1 = ( firstname => 'Katrin', diff --git a/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t b/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t index 7b77e9c04a..48c2a60eef 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; @@ -106,10 +107,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 22f4d0a32b..312f6a9afd 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.11.0