Bugzilla – Attachment 87851 Details for
Bug 22521
Convert fines handling to use 'status' instead of two accounttypes
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 22521: Update fines handling to use accountline.status
Bug-22521-Update-fines-handling-to-use-accountline.patch (text/plain), 18.65 KB, created by
Martin Renvoize (ashimema)
on 2019-04-12 08:37:56 UTC
(
hide
)
Description:
Bug 22521: Update fines handling to use accountline.status
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2019-04-12 08:37:56 UTC
Size:
18.65 KB
patch
obsolete
>From 2e51efd05b8ec4d204c54ba248b27b9e257b98d2 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >Date: Fri, 29 Mar 2019 14:31:06 +0000 >Subject: [PATCH] Bug 22521: Update fines handling to use accountline.status > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >Signed-off-by: Josef Moravec <josef.moravec@gmail.com> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > C4/Circulation.pm | 7 ++--- > C4/Overdues.pm | 51 ++++++++++++++---------------------- > Koha/Account.pm | 7 ++--- > Koha/Account/Line.pm | 28 +++++++++++++------- > circ/branchoverdues.pl | 6 +++-- > misc/cronjobs/staticfines.pl | 4 +-- > opac/opac-user.pl | 2 +- > t/db_dependent/Circulation.t | 30 ++++++++++++--------- > t/db_dependent/Overdues.t | 2 +- > 9 files changed, 71 insertions(+), 66 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 765f621410..2196839426 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -2329,7 +2329,8 @@ sub _FixOverduesOnReturn { > { > borrowernumber => $borrowernumber, > itemnumber => $item, >- accounttype => 'FU' >+ accounttype => 'OVERDUE', >+ status => 'UNRETURNED' > } > )->next(); > return 0 unless $accountline; # no warning, there's just nothing to fix >@@ -2337,7 +2338,7 @@ sub _FixOverduesOnReturn { > if ($exemptfine) { > my $amountoutstanding = $accountline->amountoutstanding; > >- $accountline->accounttype('FFOR'); >+ $accountline->status('FORGIVEN'); > $accountline->amountoutstanding(0); > > Koha::Account::Offset->new( >@@ -2352,7 +2353,7 @@ sub _FixOverduesOnReturn { > &logaction("FINES", 'MODIFY',$borrowernumber,"Overdue forgiven: item $item"); > } > } else { >- $accountline->accounttype('F'); >+ $accountline->status('RETURNED'); > } > > return $accountline->store(); >diff --git a/C4/Overdues.pm b/C4/Overdues.pm >index a6ff257797..f031ff3999 100644 >--- a/C4/Overdues.pm >+++ b/C4/Overdues.pm >@@ -520,46 +520,33 @@ sub UpdateFine { > } > > my $dbh = C4::Context->dbh; >- # FIXME - What exactly is this query supposed to do? It looks up an >- # entry in accountlines that matches the given item and borrower >- # numbers, where the description contains $due, and where the >- # account type has one of several values, but what does this _mean_? >- # Does it look up existing fines for this item? >- # FIXME - What are these various account types? ("FU", "O", "F", "M") >- # "L" is LOST item >- # "A" is Account Management Fee >- # "N" is New Card >- # "M" is Sundry >- # "F" is Fine ?? >- # "FU" is Fine UPDATE?? >- # "Pay" is Payment >- # "REF" is Cash Refund >- my $sth = $dbh->prepare( >- "SELECT * FROM accountlines >- WHERE borrowernumber=? AND >- (( accounttype IN ('F','M') AND amountoutstanding<>0 ) OR >- accounttype = 'FU' )" >+ my $overdues = Koha::Account::Lines->search( >+ { >+ borrowernumber => $borrowernumber, >+ accounttype => [ 'OVERDUE', 'M' ], >+ amountoutstanding => { '<>' => 0 } >+ } > ); >- $sth->execute( $borrowernumber ); >- my $data; >+ >+ my $accountline; > my $total_amount_other = 0.00; > my $due_qr = qr/$due/; > # Cycle through the fines and > # - find line that relates to the requested $itemnum > # - accumulate fines for other items > # so we can update $itemnum fine taking in account fine caps >- while (my $rec = $sth->fetchrow_hashref) { >- if ( $rec->{issue_id} == $issue_id && $rec->{accounttype} eq 'FU' ) { >- if ($data) { >+ while (my $overdue = $overdues->next) { >+ if ( $overdue->issue_id == $issue_id && $overdue->status eq 'UNRETURNED' ) { >+ if ($accountline) { > $debug and warn "Not a unique accountlines record for issue_id $issue_id"; > #FIXME Should we still count this one in total_amount ?? > } > else { >- $data = $rec; >+ $accountline = $overdue; > next; > } > } >- $total_amount_other += $rec->{'amountoutstanding'}; >+ $total_amount_other += $overdue->amountoutstanding; > } > > if (my $maxfine = C4::Context->preference('MaxFine')) { >@@ -571,10 +558,9 @@ sub UpdateFine { > } > } > >- if ( $data ) { >- if ( $data->{'amount'} != $amount ) { >- my $accountline = >- Koha::Account::Lines->find( $data->{accountlines_id} ); >+ >+ if ( $accountline ) { >+ if ( $accountline->amount != $amount ) { > $accountline->adjust( > { > amount => $amount, >@@ -593,7 +579,7 @@ sub UpdateFine { > my $desc = "$title $due"; > > my $account = Koha::Account->new({ patron_id => $borrowernumber }); >- my $accountline = $account->add_debit( >+ $accountline = $account->add_debit( > { > amount => $amount, > description => $desc, >@@ -738,7 +724,8 @@ sub GetOverduesForBranch { > LEFT JOIN itemtypes ON itemtypes.itemtype = $itype_link > LEFT JOIN branches ON branches.branchcode = issues.branchcode > WHERE (accountlines.amountoutstanding != '0.000000') >- AND (accountlines.accounttype = 'FU' ) >+ AND (accountlines.accounttype = 'OVERDUE' ) >+ AND (accountlines.status = 'UNRETURNED' ) > AND (issues.branchcode = ? ) > AND (issues.date_due < NOW()) > "; >diff --git a/Koha/Account.pm b/Koha/Account.pm >index 4da0c1c5ad..ed127d1d4f 100644 >--- a/Koha/Account.pm >+++ b/Koha/Account.pm >@@ -425,7 +425,7 @@ my $debit_line = Koha::Account->new({ patron_id => $patron_id })->add_debit( > ); > > $debit_type can be any of: >- - fine >+ - overdue > - lost_item > - new_card > - account >@@ -495,6 +495,7 @@ sub add_debit { > itemnumber => $item_id, > issue_id => $issue_id, > branchcode => $library_id, >+ ( $type eq 'overdue' ? ( status => 'UNRETURNED' ) : ()), > } > )->store(); > >@@ -696,7 +697,7 @@ our $offset_type = { > 'processing' => 'Processing Fee', > 'lost_item' => 'Lost Item', > 'rent' => 'Rental Fee', >- 'fine' => 'Fine', >+ 'overdue' => 'OVERDUE', > 'manual_debit' => 'Manual Debit', > 'hold_expired' => 'Hold Expired' > }; >@@ -719,7 +720,7 @@ our $account_type_credit = { > > our $account_type_debit = { > 'account' => 'A', >- 'fine' => 'FU', >+ 'overdue' => 'OVERDUE', > 'lost_item' => 'L', > 'new_card' => 'N', > 'sundry' => 'M', >diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm >index 6443b4d346..2845bf6f2f 100644 >--- a/Koha/Account/Line.pm >+++ b/Koha/Account/Line.pm >@@ -236,7 +236,7 @@ This method allows updating a debit or credit on a patron's account > ); > > $update_type can be any of: >- - fine_update >+ - overdue_update > > Authors Note: The intention here is that this method is only used > to adjust accountlines where the final amount is not yet known/fixed. >@@ -259,11 +259,21 @@ sub adjust { > ); > } > >- my $account_type = $self->accounttype; >- unless ( $Koha::Account::Line::allowed_update->{$update_type} eq $account_type ) { >+ my $account_type = $self->accounttype; >+ my $account_status = $self->status; >+ unless ( >+ ( >+ exists( >+ $Koha::Account::Line::allowed_update->{$update_type} >+ ->{$account_type} >+ ) >+ && ( $Koha::Account::Line::allowed_update->{$update_type} >+ ->{$account_type} eq $account_status ) >+ ) >+ ) >+ { > Koha::Exceptions::Account::UnrecognisedType->throw( >- error => 'Update type not allowed on this accounttype' >- ); >+ error => 'Update type not allowed on this accounttype' ); > } > > my $schema = Koha::Database->new->schema; >@@ -289,7 +299,7 @@ sub adjust { > description => 'Overpayment refund', > type => 'credit', > interface => $interface, >- ( $update_type eq 'fine_update' ? ( item_id => $self->itemnumber ) : ()), >+ ( $update_type eq 'overdue_update' ? ( item_id => $self->itemnumber ) : ()), > } > ); > $new_outstanding = 0; >@@ -300,7 +310,7 @@ sub adjust { > { > date => \'NOW()', > amount => $amount, >- amountoutstanding => $new_outstanding >+ amountoutstanding => $new_outstanding, > } > )->store(); > >@@ -329,7 +339,7 @@ sub adjust { > manager_id => undef, > } > ) >- ) if ( $update_type eq 'fine_update' ); >+ ) if ( $update_type eq 'overdue_update' ); > } > } > ); >@@ -381,7 +391,7 @@ sub _type { > > =cut > >-our $allowed_update = { 'fine_update' => 'FU', }; >+our $allowed_update = { 'overdue_update' => { 'OVERDUE' => 'UNRETURNED' } }; > > =head1 AUTHORS > >diff --git a/circ/branchoverdues.pl b/circ/branchoverdues.pl >index d4c20cbb18..7f6218525e 100755 >--- a/circ/branchoverdues.pl >+++ b/circ/branchoverdues.pl >@@ -31,8 +31,10 @@ use Data::Dumper; > > =head1 branchoverdues.pl > >- this module is a new interface, allow to the librarian to check all items on overdues (based on the acountlines type 'FU' ) >- this interface is filtered by branches (automatically), and by location (optional) .... >+This view is used to display all overdue items to the librarian. >+ >+It is automatically filtered by branch and can optionally be filtered >+by item location. > > =cut > >diff --git a/misc/cronjobs/staticfines.pl b/misc/cronjobs/staticfines.pl >index 2a98fea6ec..774c3b0e56 100755 >--- a/misc/cronjobs/staticfines.pl >+++ b/misc/cronjobs/staticfines.pl >@@ -229,8 +229,8 @@ for ( my $i = 0 ; $i < scalar(@$data) ; $i++ ) { > > my $desc = "staticfine"; > my $query = "INSERT INTO accountlines >- (borrowernumber,itemnumber,date,amount,description,accounttype,amountoutstanding) >- VALUES (?,?,now(),?,?,'F',?)"; >+ (borrowernumber,itemnumber,date,amount,description,accounttype,status,amountoutstanding) >+ VALUES (?,?,now(),?,?,'OVERDUE','RETURNED',?)"; > my $sth2 = $dbh->prepare($query); > $bigdebug and warn "query: $query\nw/ args: $borrowernumber, $itemnumber, $amount, $desc, $amount\n"; > $sth2->execute( $borrowernumber, $itemnumber, $amount, $desc, $amount ); >diff --git a/opac/opac-user.pl b/opac/opac-user.pl >index 1fde24b538..7ed262f6a2 100755 >--- a/opac/opac-user.pl >+++ b/opac/opac-user.pl >@@ -196,7 +196,7 @@ if ( $pending_checkouts->count ) { # Useless test > { > borrowernumber => $patron->borrowernumber, > amountoutstanding => { '>' => 0 }, >- accounttype => [ 'F', 'FU', 'L' ], >+ accounttype => [ 'F', 'L' ], > itemnumber => $issue->{itemnumber} > }, > ); >diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t >index 1413d0816b..ed7d122dd3 100755 >--- a/t/db_dependent/Circulation.t >+++ b/t/db_dependent/Circulation.t >@@ -507,8 +507,8 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); > > my $fines = Koha::Account::Lines->search( { borrowernumber => $renewing_borrower->{borrowernumber}, itemnumber => $item_7->itemnumber } ); > is( $fines->count, 2 ); >- is( $fines->next->accounttype, 'F', 'Fine on renewed item is closed out properly' ); >- is( $fines->next->accounttype, 'F', 'Fine on renewed item is closed out properly' ); >+ isnt( $fines->next->status, 'UNRETURNED', 'Fine on renewed item is closed out properly' ); >+ isnt( $fines->next->status, 'UNRETURNED', 'Fine on renewed item is closed out properly' ); > $fines->delete(); > > >@@ -694,7 +694,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); > item_id => $item_to_auto_renew->{itemnumber}, > description => "Some fines" > } >- )->accounttype('F')->store; >+ )->status('RETURNED')->store; > ( $renewokay, $error ) = > CanBookBeRenewed( $renewing_borrowernumber, $item_to_auto_renew->{itemnumber} ); > is( $renewokay, 0, 'Do not renew, renewal is automatic' ); >@@ -708,7 +708,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); > item_id => $item_to_auto_renew->{itemnumber}, > description => "Some fines" > } >- )->accounttype('F')->store; >+ )->status('RETURNED')->store; > ( $renewokay, $error ) = > CanBookBeRenewed( $renewing_borrowernumber, $item_to_auto_renew->{itemnumber} ); > is( $renewokay, 0, 'Do not renew, renewal is automatic' ); >@@ -722,7 +722,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); > item_id => $item_to_auto_renew->{itemnumber}, > description => "Some fines" > } >- )->accounttype('F')->store; >+ )->status('RETURNED')->store; > ( $renewokay, $error ) = > CanBookBeRenewed( $renewing_borrowernumber, $item_to_auto_renew->{itemnumber} ); > is( $renewokay, 0, 'Do not renew, renewal is automatic' ); >@@ -856,7 +856,8 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); > ); > > my $line = Koha::Account::Lines->search({ borrowernumber => $renewing_borrower->{borrowernumber} })->next(); >- is( $line->accounttype, 'FU', 'Account line type is FU' ); >+ is( $line->accounttype, 'OVERDUE', 'Account line type is OVERDUE' ); >+ is( $line->status, 'UNRETURNED', 'Account line status is UNRETURNED' ); > is( $line->amountoutstanding, '15.000000', 'Account line amount outstanding is 15.00' ); > is( $line->amount, '15.000000', 'Account line amount is 15.00' ); > is( $line->issue_id, $issue->id, 'Account line issue id matches' ); >@@ -871,7 +872,8 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); > LostItem( $item_1->itemnumber, 'test', 1 ); > > $line = Koha::Account::Lines->find($line->id); >- is( $line->accounttype, 'F', 'Account type correctly changed from FU to F' ); >+ is( $line->accounttype, 'OVERDUE', 'Account type remains as OVERDUE' ); >+ isnt( $line->status, 'UNRETURNED', 'Account status correctly changed from UNRETURNED to RETURNED' ); > > my $item = Koha::Items->find($item_1->itemnumber); > ok( !$item->onloan(), "Lost item marked as returned has false onloan value" ); >@@ -1988,7 +1990,7 @@ subtest 'AddReturn | is_overdue' => sub { > # specify dropbox date 5 days later => overdue, or... not > AddIssue( $patron->unblessed, $item->{barcode}, $ten_days_ago ); # date due was 10d ago > AddReturn( $item->{barcode}, $library->{branchcode}, $five_days_ago ); >- is( int($patron->account->balance()), 0, 'AddReturn: pass return_date => no overdue in dropbox mode' ); # FIXME? This is weird, the FU fine is created ( _CalculateAndUpdateFine > C4::Overdues::UpdateFine ) then remove later (in _FixOverduesOnReturn). Looks like it is a feature >+ is( int($patron->account->balance()), 0, 'AddReturn: pass return_date => no overdue in dropbox mode' ); # FIXME? This is weird, the OVERDUE fine is created ( _CalculateAndUpdateFine > C4::Overdues::UpdateFine ) then remove later (in _FixOverduesOnReturn). Looks like it is a feature > Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber })->delete; > }; > >@@ -2370,7 +2372,7 @@ subtest '_FixAccountForLostAndReturned' => sub { > > is( $account->balance, $manual_debit_amount - $payment_amount, 'Balance is PF - payment (CR)' ); > >- my $manual_debit = Koha::Account::Lines->search({ borrowernumber => $patron->id, accounttype => 'FU' })->next; >+ my $manual_debit = Koha::Account::Lines->search({ borrowernumber => $patron->id, accounttype => 'OVERDUE', status => 'UNRETURNED' })->next; > is( $manual_debit->amountoutstanding + 0, $manual_debit_amount - $payment_amount, 'reconcile_balance was called' ); > }; > }; >@@ -2397,7 +2399,8 @@ subtest '_FixOverduesOnReturn' => sub { > my $accountline = Koha::Account::Line->new( > { > borrowernumber => $patron->{borrowernumber}, >- accounttype => 'FU', >+ accounttype => 'OVERDUE', >+ status => 'UNRETURNED', > itemnumber => $item->itemnumber, > amount => 99.00, > amountoutstanding => 99.00, >@@ -2410,13 +2413,14 @@ subtest '_FixOverduesOnReturn' => sub { > $accountline->_result()->discard_changes(); > > is( $accountline->amountoutstanding, '99.000000', 'Fine has the same amount outstanding as previously' ); >- is( $accountline->accounttype, 'F', 'Open fine ( account type FU ) has been closed out ( account type F )'); >+ is( $accountline->status, 'RETURNED', 'Open fine ( account type OVERDUE ) has been closed out ( status RETURNED )'); > > > ## Run again, with exemptfine enabled > $accountline->set( > { >- accounttype => 'FU', >+ accounttype => 'OVERDUE', >+ status => 'UNRETURNED', > amountoutstanding => 99.00, > } > )->store(); >@@ -2427,7 +2431,7 @@ subtest '_FixOverduesOnReturn' => sub { > my $offset = Koha::Account::Offsets->search({ debit_id => $accountline->id, type => 'Forgiven' })->next(); > > is( $accountline->amountoutstanding + 0, 0, 'Fine has been reduced to 0' ); >- is( $accountline->accounttype, 'FFOR', 'Open fine ( account type FU ) has been set to fine forgiven ( account type FFOR )'); >+ is( $accountline->status, 'FORGIVEN', 'Open fine ( account type OVERDUE ) has been set to fine forgiven ( status FORGIVEN )'); > is( ref $offset, "Koha::Account::Offset", "Found matching offset for fine reduction via forgiveness" ); > is( $offset->amount, '-99.000000', "Amount of offset is correct" ); > }; >diff --git a/t/db_dependent/Overdues.t b/t/db_dependent/Overdues.t >index 4d6f18939c..85c3502c6a 100644 >--- a/t/db_dependent/Overdues.t >+++ b/t/db_dependent/Overdues.t >@@ -251,7 +251,7 @@ subtest 'UpdateFine tests' => sub { > is( $fine2->amount, '30.000000', "Second fine increased after partial payment of first" ); > > # Fix fine 1, create third fine >- $fine->accounttype('F')->store; >+ $fine->status('RETURNED')->store; > UpdateFine( > { > issue_id => $checkout1->issue_id, >-- >2.20.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 22521
:
86808
|
86890
|
86891
|
86892
|
86893
|
86894
|
86895
|
86896
|
86897
|
86898
|
86899
|
86900
|
86901
|
86902
|
86918
|
86919
|
86920
|
86921
|
87063
|
87083
|
87084
|
87085
|
87086
|
87087
|
87097
|
87098
|
87099
|
87100
|
87101
|
87209
|
87210
|
87211
|
87212
|
87213
|
87214
|
87215
|
87216
|
87217
|
87218
|
87219
|
87220
|
87619
|
87620
|
87621
|
87622
|
87623
|
87624
|
87678
|
87679
|
87680
|
87681
|
87682
|
87683
|
87787
|
87788
|
87789
|
87790
|
87791
|
87792
|
87808
|
87809
|
87810
|
87811
|
87812
|
87813
|
87814
|
87850
|
87851
|
87852
|
87853
|
87854
|
87855
|
87856
|
87857
|
87858
|
87859
|
87860
|
87861
|
87862
|
87863
|
87864
|
87865
|
88064
|
88065
|
88066
|
88067
|
88068
|
88069
|
88070
|
88071
|
88149
|
88150
|
88151
|
88152
|
88153
|
88154
|
88155
|
88156
|
88286