Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books
Summary: SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out B...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-20 12:49 UTC by Kyle M Hall
Modified: 2023-08-04 18:37 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books (3.08 KB, patch)
2012-02-20 13:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books (3.14 KB, patch)
2012-08-09 08:11 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books (3.15 KB, patch)
2013-03-11 17:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup (3.03 KB, patch)
2013-03-11 17:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup 2 (7.77 KB, patch)
2013-03-11 17:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup 2 (8.13 KB, patch)
2013-03-11 18:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books (3.23 KB, patch)
2013-05-17 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup (3.07 KB, patch)
2013-05-17 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup 2 (8.14 KB, patch)
2013-05-17 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup 2 (8.67 KB, patch)
2013-05-17 13:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup 2 (8.67 KB, patch)
2013-05-17 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup 2 (8.88 KB, patch)
2013-05-17 13:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books (3.21 KB, patch)
2013-07-31 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup (2.84 KB, patch)
2013-07-31 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - Unit Test (1.57 KB, patch)
2013-07-31 14:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books (3.55 KB, patch)
2015-09-16 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - QA Followup (2.72 KB, patch)
2015-09-16 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - Unit Test (2.02 KB, patch)
2015-09-16 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 [QA Followup] - Fix up unit test (2.67 KB, patch)
2015-10-19 11:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 [QA Followup] - Fix string name conflict, add screen message (1.22 KB, patch)
2015-10-22 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books (6.85 KB, patch)
2016-04-19 12:09 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-02-20 12:49:10 UTC
On our self checks, if you have over $10.00 in fines, you should not be able to
check out items. Currently patrons are not stopped from checking out items.
Comment 1 Kyle M Hall 2012-02-20 13:04:29 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-08-09 08:11:10 UTC Comment hidden (obsolete)
Comment 3 Mason James 2012-08-10 09:04:35 UTC
(In reply to comment #2)
> Created attachment 11475 [details] [review]
> Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to
> Check Out Books
> 
> Add checks in C4::ILS::Patron and C4::ILS::Transaction::Renew to make sure
> the patron
> is below the noissuescharge system pref for issues and OPACFineNoRenewals
> for renewals.
> 
> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

perl syntax error in C4/SIP/ILS/Transaction/Renew.pm
sorry, failing QA...


mason@xen1:~/git/head$ koha-qa.pl 
        * 2f47961 Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books
                C4/Overdues.pm
                C4/SIP/ILS/Patron.pm
                C4/SIP/ILS/Transaction/Renew.pm

- perlcritic-progressive tests... OK
- perl -c syntax tests... FAIL
        C4/SIP/ILS/Transaction/Renew.pm FAIL

- xt/tt_valid.t tests... OK
- xt/author/valid-template.t tests... OK
Comment 4 Mason James 2012-08-10 09:19:07 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Created attachment 11475 [details] [review]
> > Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to
> > Check Out Books
> > 
> > Add checks in C4::ILS::Patron and C4::ILS::Transaction::Renew to make sure
> > the patron
> > is below the noissuescharge system pref for issues and OPACFineNoRenewals
> > for renewals.
> > 
> > Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
> 
> perl syntax error in C4/SIP/ILS/Transaction/Renew.pm
> sorry, failing QA...
> 
> 

oops, it looks like perl -c fails *before* this patch too :(

ok, i'll have another look at it...
Comment 5 Colin Campbell 2012-08-13 14:54:40 UTC
Is this caused by the fact the code needs C4/SIP adding to the @INC as well as the standard path???
Comment 6 Chris Cormack 2012-08-14 01:07:32 UTC
(In reply to comment #5)
> Is this caused by the fact the code needs C4/SIP adding to the @INC as well
> as the standard path???

Yep that's it, looks like its a problem with your test suite mason.

% perl -c C4/SIP/ILS/Transaction/Renew.pm
Can't locate ILS.pm in @INC (@INC contains: /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at C4/SIP/ILS/Transaction/Renew.pm line 10.
BEGIN failed--compilation aborted at C4/SIP/ILS/Transaction/Renew.pm line 10.

perl -I C4/SIP -c C4/SIP/ILS/Transaction/Renew.pm 
Subroutine new redefined at C4/SIP/ILS/Transaction/Renew.pm line 22, <DATA> line 496.
Subroutine do_renew_for redefined at C4/SIP/ILS/Transaction/Renew.pm line 34, <DATA> line 496.
Subroutine do_renew redefined at C4/SIP/ILS/Transaction/Renew.pm line 57, <DATA> line 496.
C4/SIP/ILS/Transaction/Renew.pm syntax OK
Comment 7 Mason James 2012-09-14 15:31:47 UTC
> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

patch looks good, passing QA…

$ koha-qa.pl 

testing 1 commit(s) (applied to commit 0acebb8)
 * 0f3cfcd Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Be
      C4/Overdues.pm
      C4/SIP/ILS/Patron.pm
      C4/SIP/ILS/Transaction/Renew.pm

* C4/Overdues.pm                                                           OK
* C4/SIP/ILS/Patron.pm                                                     OK
* C4/SIP/ILS/Transaction/Renew.pm                                          OK
Comment 8 Paul Poulain 2012-09-17 16:57:18 UTC
QA comment:

shouldn't 
+=head2 GetTotalFines
+
+    my $total_fines_owed = GetTotalFines( $borrowernumber );
+
+    Returns the total amount owed by the given borrower.

be in C4/Accounts.pm more than in C4/Overdues.pm ?

I know there is calcfine and UpdateFine in C4/Overdues.pm, but I think it's a mistake !


If you agree with me and move GetTotalFines to C4/Accounts.pm, you're welcomed to factorize getcharges, getcredits, getrefunds, that are Getting things in accountlines table. Note this is not requested for earning QA, just a suggestion

You're also welcomed to name your sub GetFines, as GetSomethings() is already supposed to return a list of something, so the "Total" is useless
Comment 9 Kyle M Hall 2013-03-11 17:21:46 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-03-11 17:29:46 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-03-11 17:52:58 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2013-03-11 18:04:18 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-03-11 18:10:53 UTC
Paul, is QA Followup 2 what you were asking for?

I don't think GetTotalFines should be renamed GetFines as it does not return a list, but a scalar.

I've cleaned up the code in the subs you referenced. 

Also, if you want to file a bug for moving the relevant subs from C4::Overdues to C4::Accounts I'd be more than happy to get care of that as well.

One other thing, it appears that reports/stats.screen.pl and reports/stats.print.pl are dead code. Am I correct? If so, I'll file a bug to remove them.

(In reply to comment #8)
> QA comment:
> 
> shouldn't 
> +=head2 GetTotalFines
> +
> +    my $total_fines_owed = GetTotalFines( $borrowernumber );
> +
> +    Returns the total amount owed by the given borrower.
> 
> be in C4/Accounts.pm more than in C4/Overdues.pm ?
> 
> I know there is calcfine and UpdateFine in C4/Overdues.pm, but I think it's
> a mistake !
> 
> 
> If you agree with me and move GetTotalFines to C4/Accounts.pm, you're
> welcomed to factorize getcharges, getcredits, getrefunds, that are Getting
> things in accountlines table. Note this is not requested for earning QA,
> just a suggestion
> 
> You're also welcomed to name your sub GetFines, as GetSomethings() is
> already supposed to return a list of something, so the "Total" is useless
Comment 14 Jared Camins-Esakov 2013-03-15 23:16:42 UTC
I believe this patch is for master not 3.6. I would like another member of the QA team to take a look at this because I am not very familiar with the SIP code.
Comment 15 Katrin Fischer 2013-05-14 06:36:26 UTC
Kyle, could you please fix the merge conflict?

SIP is hard to test and I am still not familiar with it. Therefore I would like to call for a second sign off on this and given it passes the tests and QA script then it can pass QA.
Comment 16 Kyle M Hall 2013-05-17 13:11:23 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2013-05-17 13:11:34 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-05-17 13:11:44 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-05-17 13:13:38 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-05-17 13:14:50 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-05-17 13:15:14 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2013-06-16 06:42:26 UTC
Mason, do you maybe have time to take another look at this?
Comment 23 Katrin Fischer 2013-07-14 08:07:56 UTC
Hm, I know SIP is hard to test, but feeling a bit uncomfortable about the QA follow ups not having any sign off while they are adding a new sub to C4. 

I see there is one changed test, but I can't get it to work and there is also that: http://koha.1045719.n5.nabble.com/Test-suite-in-t-db-dependent-lib-KohaTest-td5760084.html

Switching to "Failed QA" for missing unit tests.
Comment 24 Katrin Fischer 2013-07-29 10:50:05 UTC
Ok, I can confirm this bug in 3.12.1+
Comment 25 Katrin Fischer 2013-07-29 10:52:58 UTC
Kyle, could you take a look at my comments? I would really like to see this moving forward.
Comment 26 Katrin Fischer 2013-07-29 12:39:44 UTC
Kyle, as I see it the second follow up is unrelated to this bug report and should probably go on a different bug?
Comment 27 Kyle M Hall 2013-07-31 14:14:30 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2013-07-31 14:14:44 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-07-31 14:27:16 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2013-09-08 12:12:07 UTC
Hi Kyle,

I am testing this by modifying Colin's test script and I think I found a problem. If I understand the 3M specification correctly the checkout should be processed if 'no block' is set to Y, but that doesn't happen - I still get the 'Patron Blocked' message. Could you take a look?

Request:11YY20130908    141050    203000                  AOMPL|AAadmin|ABsomething|ACadmin|ADadminAY4AZE6FC
Response:120NUN20130908    141050AOMPL|AAadmin|ABsomething|AJDas Sch�ne Konstanz am Bodensee und Rhein, die alte Stadt im deutschen S�den :|AH|AFPatron Blocked|BLY|AY4AZC9CA[0d][0a]
Comment 31 Katrin Fischer 2013-09-08 15:04:54 UTC
Not sure if this is a non-issue - but I think worth a question :)
Comment 32 Katrin Fischer 2013-09-22 17:36:30 UTC
Kyle, could you take a look? I am not sure if I tested correctly and would like to know if the flag is supported by Koha in general or not :)
Comment 33 Kyle M Hall 2014-05-09 14:25:39 UTC
(In reply to Katrin Fischer from comment #32)
> Kyle, could you take a look? I am not sure if I tested correctly and would
> like to know if the flag is supported by Koha in general or not :)

The flag is supported for checkouts, but *not* for renewals, including existing reasons that block renewals. Due to this, I would consider that a separate, independent bug, and I will file a bug report for it.
Comment 34 Kyle M Hall 2014-05-09 14:37:13 UTC
(In reply to Kyle M Hall from comment #33)
> (In reply to Katrin Fischer from comment #32)
> > Kyle, could you take a look? I am not sure if I tested correctly and would
> > like to know if the flag is supported by Koha in general or not :)
> 
> The flag is supported for checkouts, but *not* for renewals, including
> existing reasons that block renewals. Due to this, I would consider that a
> separate, independent bug, and I will file a bug report for it.

Further research makes me believe that the no_block flag is poorly supported. I think the reason your checkouts are failing even with the flag is due to the "no block" flag calling a non-existent method! This further re-enforces my conclusion that the "no block" flag is outside the scope of this bug.
Comment 35 Katrin Fischer 2014-09-26 14:56:36 UTC
Hi Kyle,

there is a merge conflict with another change on those lines - could you take a look please? It also seems related to fines (!$fine_blocked))
Comment 36 Kyle M Hall 2015-09-16 14:04:18 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2015-09-16 14:04:26 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2015-09-16 14:04:28 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2015-10-19 11:02:35 UTC
Comment on attachment 42597 [details] [review]
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books - Unit Test

Review of attachment 42597 [details] [review]:
-----------------------------------------------------------------

You are using 3 different ways to insert data in your tests: Test::Builder, Koha::Object->store and directly the INSERT statement.
It would make sense to use Test::Builder for all of them.

::: t/db_dependent/Accounts.t
@@ +291,4 @@
>      }
>  };
>  
> +subtest "makepartialpayment() tests" => sub {

Wrong c/p.
Comment 40 Jonathan Druart 2015-10-19 11:03:47 UTC
Comment on attachment 42595 [details] [review]
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books

Review of attachment 42595 [details] [review]:
-----------------------------------------------------------------

::: C4/SIP/ILS/Transaction/Renew.pm
@@ +47,5 @@
>      }
> +
> +    unless ( C4::Overdues::GetTotalFines($borrower->{borrowernumber}) < C4::Context->preference('OPACFineNoRenewals') ) {
> +        $renewokay = 0;
> +        $renewerror = 'too_many_fines'

I have not tested, but this certainly needs more changes.
Looking at the code, there is a 
  $renewerror=~s/too_many/Item has reached maximum renewals/;
few lines after.
Comment 41 Kyle M Hall 2015-10-19 11:15:53 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2015-10-22 10:12:47 UTC
And what about comment 40?
Comment 43 Kyle M Hall 2015-10-22 18:32:50 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2016-02-19 10:47:33 UTC
I could rebase the first three but then the fourth complained about useless sha1 info on Accounts.t
Please rebase and squash all patches. Or at least squash all QA follow-ups.
You could add the signoff from Chris (on the first one), but if you do, please add a line that the patch was amended after that.
Comment 45 Kyle M Hall 2016-04-19 12:09:11 UTC
Created attachment 50398 [details] [review]
Bug 7560 - SIP Self Checkout Ignoring Fines Thresholds for Not Being able to Check Out Books

Add checks in C4::ILS::Patron and C4::ILS::Transaction::Renew to make sure the patron
is below the noissuescharge system pref for issues and OPACFineNoRenewals for renewals.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Patch ammended post sign-off
Comment 46 Marcel de Rooy 2016-04-28 13:30:48 UTC
Will probably not finish this one today.
Doubts about:
+        charge_ok       => ( !$debarred && !$expired && !$fine_blocked && ( $fines_amount < C4::Context->preference('noissuescharge') ) ),
+        renew_ok        => ( !$debarred && !$expired && !$fine_blocked && ( $fines_amount < C4::Context->preference('OPACFineNoRenewals') ) ),

What if those prefs are just empty?
Comment 47 Marcel de Rooy 2016-04-28 17:59:29 UTC
This is not yet ready (see last comment):

Set noissuescharge to 0, I cannot checkout:
120NUN20160428    194310AORMA|AA2|AB123|AJMastering Perl /|AH|AFPatron Blocked|BLY|

Note: CanBookBeIssued does checks like $non_issue_charges > $amountlimit==noissuescharge
This is not exactly the same as: $fines_amount < C4::Context->preference('noissuescharge')
Similarly, opac-user checks the renewal amount like:
  defined($no_renewal_amt)  && $borr->{amountoutstanding} > $no_renewal_amt
Different than just $fines_amount < C4::Context->preference('OPACFineNoRenewals')

BTW Found this sql in Members.pm:
    $sth = $dbh->prepare("SELECT SUM(amountoutstanding) FROM accountlines WHERE borrowernumber = ?");
This is actually the same as GetTotalFines does. Are we using this more? Should we add a (simple) unit test for the new routine?

Failed QA
Comment 48 David Cook 2021-07-19 05:39:22 UTC
I think that this one may have morphed since it's early days?

At the moment, it seems that SIP blocks checkout if you have any fines. At least if you're using the system preference AllFinesNeedOverride.
Comment 49 David Cook 2021-07-19 05:59:07 UTC
(In reply to David Cook from comment #48)
> I think that this one may have morphed since it's early days?
> 
> At the moment, it seems that SIP blocks checkout if you have any fines. At
> least if you're using the system preference AllFinesNeedOverride.

Although if  you have AllFinesNeedOverride set to "Don't prevent", then you're OK...
Comment 50 Katrin Fischer 2023-08-04 18:37:24 UTC
Looking at the code, I am pretty sure this has been fixed since, see also bug 29755.

C4/SIP/ILS/Patron.pm:

sub _fee_limit {
    return C4::Context->preference('noissuescharge') || 5;
}

    # Get fines and add fines for guarantees (depends on preference NoIssuesChargeGuarantees)
    my $fines_amount = ($patron->account->balance > 0) ? $patron->account->non_issues_charges : 0;
    my $personal_fines_amount = $fines_amount;
    my $fee_limit = _fee_limit();
    my $noissueschargeguarantorswithguarantees = C4::Context->preference('NoIssuesChargeGuarantorsWithGuarantees');
    my $fines_msg = "";
    my $fine_blocked = 0;
    my $noissueschargeguarantees = C4::Context->preference('NoIssuesChargeGuarantees');
    if( $fines_amount > $fee_limit ){
        $fine_blocked = 1;
        $fines_msg .= " -- " . "Patron blocked by fines" if $fine_blocked;
    } elsif ( $noissueschargeguarantorswithguarantees ) {
        $fines_amount += $patron->relationships_debt({ include_guarantors => 1, only_this_guarantor => 0, include_this_patron => 0 });
        $fine_blocked ||= $fines_amount > $noissueschargeguarantorswithguarantees;
        $fines_msg .= " -- " . "Patron blocked by fines ($fines_amount) on related accounts" if $fine_blocked;
    } elsif ( $noissueschargeguarantees ) {
        if( $patron->guarantee_relationships->count ){
            $fines_amount += $patron->relationships_debt({ include_guarantors => 0, only_this_guarantor => 1, include_this_patron => 0 });
            $fine_blocked ||= $fines_amount > $noissueschargeguarantees;
            $fines_msg .= " -- " . "Patron blocked by fines ($fines_amount) on guaranteed accounts" if $fine_blocked;
        }
    }