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: Failed QA
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: 2017-01-04 10:03 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
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