(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

(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...

Is this caused by the fact the code needs C4/SIP adding to the @INC as well as the standard path???

(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

> 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

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

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

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.

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.

Mason, do you maybe have time to take another look at this?

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.

Ok, I can confirm this bug in 3.12.1+

Kyle, could you take a look at my comments? I would really like to see this moving forward.

Kyle, as I see it the second follow up is unrelated to this bug report and should probably go on a different bug?

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]

Not sure if this is a non-issue - but I think worth a question :)

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 :)

(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.

(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.

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 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 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.

And what about comment 40?

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.

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

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?

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

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.

(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...

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;
        }
    }