Bugzilla – Attachment 64259 Details for
Bug 17680
C4::Circulation - Replace GetItemIssue with Koha::Checkouts
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug-17680-C4Circulation---Remove-GetItemIssue-comp.patch (text/plain), 14.97 KB, created by
Jonathan Druart
on 2017-06-13 14:20:04 UTC
(
hide
)
Description:
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2017-06-13 14:20:04 UTC
Size:
14.97 KB
patch
obsolete
>From 86a80f847b94668f17bcbfb65a1e3a44fdf52251 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Fri, 25 Nov 2016 13:53:03 +0100 >Subject: [PATCH] Bug 17680: C4::Circulation - Remove GetItemIssue, complex > calls >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >There are a few calls to GetItemIssue where it's not as easy to make >sure everything will be fine just replacing the calls with a >Koha::Issues->find >- In AddReturn the overdue flag is used (that's why this patch depends >on bug 17689) >- In CanBookBeRenewed, as well as the overdue flag the dates converted >to DateTime were used. It's now our job to convert them when we need >them. >- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we >need to update the variables in this subroutine. > >Note that, prior to this patch, AddReturn returned the GetItemIssue >hashref in the $iteminformation. Most of the time this variable is not >used, I have found only 1 place where it's used: circ/returns.pl >TODO: In this script we should call ->is_overdue instead of the >DateTime->compare calls > >Test plan: >All the circulation tests must pass (it's how I have caught the specific >cases). >Do some checkins/checkouts/renewal and focus on the due date > >Signed-off-by: Marc Véron <veron@veron.ch> >--- > C4/Circulation.pm | 83 +++++++++++++++++++++++++++---------------------------- > circ/returns.pl | 11 ++++---- > 2 files changed, 47 insertions(+), 47 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index ae7e6e5b81..7b36ec1237 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -1833,11 +1833,11 @@ sub AddReturn { > my $biblio = $item_level_itypes ? undef : GetBiblioData( $item->{ biblionumber } ); # don't get bib data unless we need it > my $itemtype = $item_level_itypes ? $item->{itype} : $biblio->{itemtype}; > >- my $issue = GetItemIssue($itemnumber); >- if ($issue and $issue->{borrowernumber}) { >- $borrower = C4::Members::GetMember( borrowernumber => $issue->{borrowernumber} ) >- or die "Data inconsistency: barcode $barcode (itemnumber:$itemnumber) claims to be issued to non-existent borrowernumber '$issue->{borrowernumber}'\n" >- . Dumper($issue) . "\n"; >+ my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ); >+ if ( $issue ) { >+ $borrower = C4::Members::GetMember( borrowernumber => $issue->borrowernumber ) >+ or die "Data inconsistency: barcode $barcode (itemnumber:$itemnumber) claims to be issued to non-existent borrowernumber '" . $issue->borrowernumber . "'\n" >+ . Dumper($issue->unblessed) . "\n"; > } else { > $messages->{'NotIssued'} = $barcode; > # even though item is not on loan, it may still be transferred; therefore, get current branch info >@@ -1919,7 +1919,8 @@ sub AddReturn { > my $today = DateTime->now( time_zone => C4::Context->tz() ); > > if ($doreturn) { >- my $datedue = $issue->{date_due}; >+ my $is_overdue; >+ die "The item is not issed and cannot be returned" unless $issue; # Just in case... > $borrower or warn "AddReturn without current borrower"; > my $circControlBranch; > if ($dropbox) { >@@ -1928,11 +1929,11 @@ sub AddReturn { > # FIXME: check issuedate > returndate, factoring in holidays > > $circControlBranch = _GetCircControlBranch($item,$borrower); >- $issue->{'overdue'} = DateTime->compare($issue->{'date_due'}, $dropboxdate ) == -1 ? 1 : 0; >+ $is_overdue = $issue->is_overdue( $dropboxdate ); > } > > if ($borrowernumber) { >- if ( ( C4::Context->preference('CalculateFinesOnReturn') && $issue->{'overdue'} ) || $return_date ) { >+ if ( ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue ) || $return_date ) { > _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower, return_date => $return_date } ); > } > >@@ -1954,7 +1955,7 @@ sub AddReturn { > > } > >- ModItem({ onloan => undef }, $issue->{'biblionumber'}, $item->{'itemnumber'}); >+ ModItem({ onloan => undef }, $item->{biblionumber}, $item->{'itemnumber'}); > } > > # the holdingbranch is updated if the document is returned to another location. >@@ -2014,18 +2015,18 @@ sub AddReturn { > if ($borrowernumber) { > my $fix = _FixOverduesOnReturn($borrowernumber, $item->{itemnumber}, $exemptfine, $dropbox); > defined($fix) or warn "_FixOverduesOnReturn($borrowernumber, $item->{itemnumber}...) failed!"; # zero is OK, check defined >- >- if ( $issue->{overdue} && $issue->{date_due} ) { >+ >+ if ( $issue and $issue->is_overdue ) { > # fix fine days > $today = $dropboxdate if $dropbox; >- my ($debardate,$reminder) = _debar_user_on_return( $borrower, $item, $issue->{date_due}, $today ); >+ my ($debardate,$reminder) = _debar_user_on_return( $borrower, $item, dt_from_string($issue->date_due), $today ); > if ($reminder){ > $messages->{'PrevDebarred'} = $debardate; > } else { > $messages->{'Debarred'} = $debardate if $debardate; > } > # there's no overdue on the item but borrower had been previously debarred >- } elsif ( $issue->{date_due} and $borrower->{'debarred'} ) { >+ } elsif ( $issue->date_due and $borrower->{'debarred'} ) { > if ( $borrower->{debarred} eq "9999-12-31") { > $messages->{'ForeverDebarred'} = $borrower->{'debarred'}; > } else { >@@ -2613,10 +2614,10 @@ sub CanBookBeRenewed { > my $renews = 1; > > my $item = GetItem($itemnumber) or return ( 0, 'no_item' ); >- my $itemissue = GetItemIssue($itemnumber) or return ( 0, 'no_checkout' ); >- return ( 0, 'onsite_checkout' ) if $itemissue->{onsite_checkout}; >+ my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return ( 0, 'no_checkout' ); >+ return ( 0, 'onsite_checkout' ) if $issue->onsite_checkout; > >- $borrowernumber ||= $itemissue->{borrowernumber}; >+ $borrowernumber ||= $issue->borrowernumber; > my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber ) > or return; > >@@ -2698,7 +2699,7 @@ sub CanBookBeRenewed { > ); > > return ( 0, "too_many" ) >- if not $issuing_rule or $issuing_rule->renewalsallowed <= $itemissue->{renewals}; >+ if not $issuing_rule or $issuing_rule->renewalsallowed <= $issue->renewals; > > my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing'); > my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing'); >@@ -2708,16 +2709,16 @@ sub CanBookBeRenewed { > > if ( $restricted and $restrictionblockrenewing ) { > return ( 0, 'restriction'); >- } elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) { >+ } elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($issue->is_overdue and $overduesblockrenewing eq 'blockitem') ) { > return ( 0, 'overdue'); > } > >- if ( $itemissue->{auto_renew} ) { >+ if ( $issue->auto_renew ) { > if ( defined $issuing_rule->no_auto_renewal_after > and $issuing_rule->no_auto_renewal_after ne "" ) { > # Get issue_date and add no_auto_renewal_after > # If this is greater than today, it's too late for renewal. >- my $maximum_renewal_date = dt_from_string($itemissue->{issuedate}); >+ my $maximum_renewal_date = dt_from_string($issue->issuedate, 'sql'); > $maximum_renewal_date->add( > $issuing_rule->lengthunit => $issuing_rule->no_auto_renewal_after > ); >@@ -2748,9 +2749,7 @@ sub CanBookBeRenewed { > { > > # Calculate soonest renewal by subtracting 'No renewal before' from due date >- my $soonestrenewal = >- $itemissue->{date_due}->clone() >- ->subtract( >+ my $soonestrenewal = dt_from_string( $issue->date_due, 'sql' )->subtract( > $issuing_rule->lengthunit => $issuing_rule->norenewalbefore ); > > # Depending on syspref reset the exact time, only check the date >@@ -2762,20 +2761,20 @@ sub CanBookBeRenewed { > > if ( $soonestrenewal > DateTime->now( time_zone => C4::Context->tz() ) ) > { >- return ( 0, "auto_too_soon" ) if $itemissue->{auto_renew}; >+ return ( 0, "auto_too_soon" ) if $issue->auto_renew; > return ( 0, "too_soon" ); > } >- elsif ( $itemissue->{auto_renew} ) { >+ elsif ( $issue->auto_renew ) { > return ( 0, "auto_renew" ); > } > } > > # Fallback for automatic renewals: > # If norenewalbefore is undef, don't renew before due date. >- if ( $itemissue->{auto_renew} ) { >+ if ( $issue->auto_renew ) { > my $now = dt_from_string; > return ( 0, "auto_renew" ) >- if $now >= $itemissue->{date_due}; >+ if $now >= dt_from_string( $issue->date_due, 'sql' ); > return ( 0, "auto_too_soon" ); > } > >@@ -2819,11 +2818,11 @@ sub AddRenewal { > my $dbh = C4::Context->dbh; > > # Find the issues record for this book >- my $issuedata = GetItemIssue($itemnumber); >+ my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ); > >- return unless ( $issuedata ); >+ return unless $issue; > >- $borrowernumber ||= $issuedata->{borrowernumber}; >+ $borrowernumber ||= $issue->borrowernumber; > > if ( defined $datedue && ref $datedue ne 'DateTime' ) { > carp 'Invalid date passed to AddRenewal.'; >@@ -2832,8 +2831,8 @@ sub AddRenewal { > > my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber ) or return; > >- if ( C4::Context->preference('CalculateFinesOnReturn') && $issuedata->{overdue} ) { >- _CalculateAndUpdateFine( { issue => $issuedata, item => $item, borrower => $borrower } ); >+ if ( C4::Context->preference('CalculateFinesOnReturn') && $issue->is_overdue ) { >+ _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower } ); > } > _FixOverduesOnReturn( $borrowernumber, $itemnumber ); > >@@ -2845,14 +2844,14 @@ sub AddRenewal { > my $itemtype = (C4::Context->preference('item-level_itypes')) ? $biblio->{'itype'} : $biblio->{'itemtype'}; > > $datedue = (C4::Context->preference('RenewalPeriodBase') eq 'date_due') ? >- dt_from_string( $issuedata->{date_due} ) : >+ dt_from_string( $issue->date_due, 'sql' ) : > DateTime->now( time_zone => C4::Context->tz()); > $datedue = CalcDateDue($datedue, $itemtype, _GetCircControlBranch($item, $borrower), $borrower, 'is a renewal'); > } > > # Update the issues record to have the new due date, and a new count > # of how many times it has been renewed. >- my $renews = $issuedata->{'renewals'} + 1; >+ my $renews = $issue->renewals + 1; > my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ? > WHERE borrowernumber=? > AND itemnumber=?" >@@ -4015,7 +4014,7 @@ sub _CalculateAndUpdateFine { > unless ($item) { carp "No item passed in!" && return; } > unless ($issue) { carp "No issue passed in!" && return; } > >- my $datedue = $issue->{date_due}; >+ my $datedue = dt_from_string( $issue->date_due ); > > # we only need to calculate and change the fines if we want to do that on return > # Should be on for hourly loans >@@ -4023,7 +4022,7 @@ sub _CalculateAndUpdateFine { > my $control_branchcode = > ( $control eq 'ItemHomeLibrary' ) ? $item->{homebranch} > : ( $control eq 'PatronLibrary' ) ? $borrower->{branchcode} >- : $issue->{branchcode}; >+ : $issue->branchcode; > > my $date_returned = $return_date ? dt_from_string($return_date) : dt_from_string(); > >@@ -4035,9 +4034,9 @@ sub _CalculateAndUpdateFine { > if ( C4::Context->preference('finesMode') eq 'production' ) { > if ( $amount > 0 ) { > C4::Overdues::UpdateFine({ >- issue_id => $issue->{issue_id}, >- itemnumber => $issue->{itemnumber}, >- borrowernumber => $issue->{borrowernumber}, >+ issue_id => $issue->issue_id, >+ itemnumber => $issue->itemnumber, >+ borrowernumber => $issue->borrowernumber, > amount => $amount, > type => $type, > due => output_pref($datedue), >@@ -4049,9 +4048,9 @@ sub _CalculateAndUpdateFine { > # so in this case, we need to drop those fines to 0 > > C4::Overdues::UpdateFine({ >- issue_id => $issue->{issue_id}, >- itemnumber => $issue->{itemnumber}, >- borrowernumber => $issue->{borrowernumber}, >+ issue_id => $issue->issue_id, >+ itemnumber => $issue->itemnumber, >+ borrowernumber => $issue->borrowernumber, > amount => 0, > type => $type, > due => output_pref($datedue), >diff --git a/circ/returns.pl b/circ/returns.pl >index d1b95e8d82..e5805dbf2a 100755 >--- a/circ/returns.pl >+++ b/circ/returns.pl >@@ -187,7 +187,7 @@ if ( $query->param('reserve_id') ) { > my $borrower; > my $returned = 0; > my $messages; >-my $issueinformation; >+my $issue; > my $itemnumber; > my $barcode = $query->param('barcode'); > my $exemptfine = $query->param('exemptfine'); >@@ -309,21 +309,22 @@ if ($barcode) { > ); > > # do the return >- ( $returned, $messages, $issueinformation, $borrower ) = >+ ( $returned, $messages, $issue, $borrower ) = > AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode, $return_date_override, $dropboxdate ); > > if ($returned) { > my $time_now = DateTime->now( time_zone => C4::Context->tz )->truncate( to => 'minute'); >- my $duedate = $issueinformation->{date_due}->strftime('%Y-%m-%d %H:%M'); >+ my $date_due_dt = dt_from_string( $issue->date_due, 'sql' ); >+ my $duedate = $date_due_dt->strftime('%Y-%m-%d %H:%M'); > $returneditems{0} = $barcode; > $riborrowernumber{0} = $borrower->{'borrowernumber'}; > $riduedate{0} = $duedate; > $input{borrowernumber} = $borrower->{'borrowernumber'}; > $input{duedate} = $duedate; > unless ( $dropboxmode ) { >- $input{return_overdue} = 1 if (DateTime->compare($issueinformation->{date_due}, DateTime->now()) == -1); >+ $input{return_overdue} = 1 if (DateTime->compare($date_due_dt, DateTime->now()) == -1); > } else { >- $input{return_overdue} = 1 if (DateTime->compare($issueinformation->{date_due}, $dropboxdate) == -1); >+ $input{return_overdue} = 1 if (DateTime->compare($date_due_dt, $dropboxdate) == -1); > } > push( @inputloop, \%input ); > >-- >2.11.0
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=64259">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 17680
:
57787
|
57788
|
57789
|
57847
|
58431
|
58432
|
58433
|
58501
|
58918
|
58919
|
58920
|
61761
|
61762
|
61763
|
62034
|
62086
|
62102
|
62103
|
62104
|
64257
|
64258
|
64259
|
64284
|
64337
|
64805
|
64806
|
64807
|
64833
|
64866
|
64867
|
64868
|
64869
|
64870
|
64871
|
64925
|
64926
|
64927
|
64928
|
64929
|
64930
|
64931
|
65302
|
65310