From fb6d367732eac7cbe15ff8735b6f959109272d54 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 8abc9c2..c3ebfe9 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::Issues->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..... @@ -670,7 +670,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::Issues->find( { itemnumber => $item->{itemnumber} } ); my $biblioitem = GetBiblioItemData($item->{biblioitemnumber}); $item->{'itemtype'}=$item->{'itype'}; my $dbh = C4::Context->dbh; @@ -826,9 +826,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 ) { @@ -936,13 +936,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 { @@ -963,10 +963,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} ); @@ -1292,13 +1292,13 @@ sub AddIssue { my $branch = _GetCircControlBranch( $item, $borrower ); # get actual issuing if there is one - my $actualissue = GetItemIssue( $item->{itemnumber} ); + my $actualissue = Koha::Issues->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'}, @@ -1310,8 +1310,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} ); @@ -3040,9 +3039,9 @@ sub GetSoonestRenewDate { my $dbh = C4::Context->dbh; my $item = GetItem($itemnumber) or return; - my $itemissue = GetItemIssue($itemnumber) or return; + my $itemissue = Koha::Issues->find( { itemnumber => $itemnumber } ) or return; - $borrowernumber ||= $itemissue->{borrowernumber}; + $borrowernumber ||= $itemissue->borrowernumber; my $borrower = C4::Members::GetMemberDetails($borrowernumber) or return; @@ -3061,8 +3060,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' @@ -3100,9 +3098,9 @@ sub GetLatestAutoRenewDate { my $dbh = C4::Context->dbh; my $item = GetItem($itemnumber) or return; - my $itemissue = GetItemIssue($itemnumber) or return; + my $itemissue = Koha::Issues->find( { itemnumber => $itemnumber } ) or return; - $borrowernumber ||= $itemissue->{borrowernumber}; + $borrowernumber ||= $itemissue->borrowernumber; my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber ) or return; @@ -3118,7 +3116,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 7c5e5f3..d527574 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; @@ -569,12 +571,12 @@ sub RenewLoan { my @renewal = CanBookBeRenewed( $borrowernumber, $itemnumber ); if ( $renewal[0] ) { AddRenewal( $borrowernumber, $itemnumber ); } - my $issue = GetItemIssue($itemnumber); + my $issue = Koha::Issues->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..a8b32a6 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::Issues; =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::Issues->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..1d2e3ce 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::Issues; 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::Issues->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..546243b 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::Issues; 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::Issues->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..55a8798 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::Issues; 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::Issues->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::Issues->find( { itemnumber => $item->{itemnumber} } ); + return unless $issue; + return $issue->borrowernumber; } diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index d02e96c..1ea42cd 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::Issues; use Koha::Libraries; use Koha::Patrons; use Date::Calc qw/Today Date_to_Days/; @@ -438,9 +439,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::Issues->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 bc55fdf..681e052 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::Issues; use Koha::Holds; use Koha::Libraries; @@ -376,9 +377,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::Issues->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 f29dcd0..4a96295 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::Issues; my $schema = Koha::Database->schema; $schema->storage->txn_begin; @@ -312,7 +313,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::Issues->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..0525744 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::Issues; 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::Issues->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 c1a157d..56d0b8e 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 => 32; +use Test::More tests => 31; use DateTime::Duration; use t::lib::Mocks; @@ -45,7 +45,6 @@ can_ok( AddReturn GetBiblioIssues GetIssuingCharges - GetItemIssue GetOpenIssue GetRenewCount GetUpcomingDueIssues @@ -218,11 +217,6 @@ like( #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.9.3