From bb7b48de1b2c58380213dcd7dc5ae74ce7268a66 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 Signed-off-by: Tomas Cohen Arazi --- C4/Circulation.pm | 40 ++++++++++------------ C4/ILSDI/Services.pm | 8 +++-- C4/SIP/ILS/Item.pm | 7 ++-- 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(+), 58 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 1cfa69b..6725d4b 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} ); @@ -3045,9 +3044,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; @@ -3066,8 +3065,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' @@ -3105,9 +3103,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; @@ -3128,7 +3126,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 c1632d5..d4d8a96 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; use Koha::Patrons; @@ -587,12 +589,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 a6c735c..922c24c 100644 --- a/C4/SIP/ILS/Item.pm +++ b/C4/SIP/ILS/Item.pm @@ -24,6 +24,7 @@ use C4::Members; use C4::Reserves; use Koha::Database; use Koha::Biblios; +use Koha::Checkouts; use Koha::Patrons; =encoding UTF-8 @@ -89,11 +90,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 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 73a5053..e7e8746 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 dbe8c40..7567dd3 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 9ddbb26..7bb6e7a 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 db35e16..791a2bd 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 9d0f77c..7bbb25b 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 7b77e9c..48c2a60 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 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.7.4