Bug 27259: Add HomeOrHoldingBranch checks where it was missing from

The TooMany() function and fine calculation functions were incorrectly
hard coded to use homebranch for fetching the circulation rules. Those
ignored completely the syspref HomeOrHoldingBranch where the user
might have set it to holdingbranch and therefore the fines and whether
patron has too many checkouts (TooMany()) were counted using the
unintended branch's rules. This problem only arises in the cases where
there are branch specific circulation rules defined.

Test plan:
1. Make sure following tests pass:
   $ prove t/db_dependent/Circulation/_CalculateAndUpdateFine.t
   $ prove t/db_dependent/Circulation/TooMany.t

Test plan for fines.pl:
1. Add branch specific fine rules for branches A and B. A having a
   fine of 1 per day and B having a fine of 0 per day.
2. Set sysprefs:
   CircControl = the library the items is from
   finesMode = Calculate and charge
   HomeOrHoldingBranch = holdingbranch
3. Create an item with home and holding branch of A
4. Checkout the item with a due date in the past (the past due date can be
   specified by clicking "Checkout settings" in the checkout page) and
   make sure the branch you are checking from is B.
5. Run perl /usr/share/koha/bin/cronjobs/fines.pl
6. Notice that fines have popped up now to the patron incorrectly
7. Apply patch
8. Pay fines, Check-in the item and check it out again
9. Run perl /usr/share/koha/bin/cronjobs/fines.pl
10. Notice that fine is now 0. This means that the branch
    B (holdingbranch of the checked-out item) specific rule is used.

Test plan for staticfines.pl:
1. Add branch specific fine rules for branches A and B. A having a
   fine of 1 per day and B having a fine of 0 per day.
2. Set sysprefs:
   CircControl = the library the items is from
   finesMode = Calculate and charge
   HomeOrHoldingBranch = holdingbranch
3. Create an item with homebranch A and holding branch of A
4. Checkout the item with a due date in the past (the past due date can be
   specified by clicking "Checkout settings" in the checkout page) and
   make sure the branch you are checking from is B.
5. Run perl staticfines.pl --library A --library B --category <PATRONS_CATEGORYCODE>
   and notice that now there is inccorectly fines
6. Apply patch
7. Pay fines, Check-in the item and check it out again
8. Run perl staticfines.pl --library A --library B --category <PATRONS_CATEGORYCODE>
    and notice the fines are now not generated

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index d74305a..d8b70cc 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -478,8 +478,9 @@ sub TooMany {
             } elsif (C4::Context->preference('CircControl') eq 'PatronLibrary') {
                 $checkouts = $patron->checkouts; # if branch is the patron's home branch, then count all loans by patron
             } else {
+                my $branch_type = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch';
                 $checkouts = $patron->checkouts->search(
-                    { 'item.homebranch' => $maxissueqty_rule->branchcode } );
+                    { "item.$branch_type" => $maxissueqty_rule->branchcode } );
             }
         } else {
             $checkouts = $patron->checkouts; # if rule is not branch specific then count all loans by patron
@@ -574,9 +575,10 @@ sub TooMany {
         } elsif (C4::Context->preference('CircControl') eq 'PatronLibrary') {
             $checkouts = $patron->checkouts; # if branch is the patron's home branch, then count all loans by patron
         } else {
+            my $branch_type = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch';
             $checkouts = $patron->checkouts->search(
-                { 'item.homebranch' => $branch},
-                { prefetch          => 'item' } );
+                { "item.$branch_type" => $branch},
+                { prefetch            => 'item' } );
         }
 
         my $checkout_count = $checkouts->count;
@@ -4315,8 +4317,9 @@ sub _CalculateAndUpdateFine {
     # we only need to calculate and change the fines if we want to do that on return
     # Should be on for hourly loans
     my $control = C4::Context->preference('CircControl');
+    my $branch_type = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch';
     my $control_branchcode =
-        ( $control eq 'ItemHomeLibrary' ) ? $item->{homebranch}
+        ( $control eq 'ItemHomeLibrary' ) ? $item->{$branch_type}
       : ( $control eq 'PatronLibrary' )   ? $borrower->{branchcode}
       :                                     $issue->branchcode;
 

diff --git a/C4/Overdues.pm b/C4/Overdues.pm
index 56a0010..8074411 100644 (file)
--- a/C4/Overdues.pm
+++ b/C4/Overdues.pm
@@ -92,14 +92,14 @@ sub Getoverdues {
     my $statement;
     if ( C4::Context->preference('item-level_itypes') ) {
         $statement = "
-   SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber
+   SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, items.biblionumber, items.holdingbranch
      FROM issues 
 LEFT JOIN items       USING (itemnumber)
     WHERE date_due < NOW()
 ";
     } else {
         $statement = "
-   SELECT issues.*, biblioitems.itemtype, items.itype, items.homebranch, items.barcode, items.itemlost, replacementprice, items.biblionumber
+   SELECT issues.*, biblioitems.itemtype, items.itype, items.homebranch, items.barcode, items.itemlost, replacementprice, items.biblionumber, items.holdingbranch
      FROM issues 
 LEFT JOIN items       USING (itemnumber)
 LEFT JOIN biblioitems USING (biblioitemnumber)

diff --git a/misc/cronjobs/fines.pl b/misc/cronjobs/fines.pl
index dab688b..b141c55 100755 (executable)
--- a/misc/cronjobs/fines.pl
+++ b/misc/cronjobs/fines.pl
@@ -100,6 +100,7 @@ my @item_fields  = qw(itemnumber barcode date_due);
 my @other_fields = qw(days_overdue fine);
 my $libname      = C4::Context->preference('LibraryName');
 my $control      = C4::Context->preference('CircControl');
+my $branch_type  = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch';
 my $mode         = C4::Context->preference('finesMode');
 my $delim = "\t";    # ?  C4::Context->preference('CSVDelimiter') || "\t";
 
@@ -131,7 +132,7 @@ for my $overdue ( @{$overdues} ) {
     }
     my $patron = Koha::Patrons->find( $overdue->{borrowernumber} );
     my $branchcode =
-        ( $control eq 'ItemHomeLibrary' ) ? $overdue->{homebranch}
+        ( $control eq 'ItemHomeLibrary' ) ? $overdue->{$branch_type}
       : ( $control eq 'PatronLibrary' )   ? $patron->branchcode
       :                                     $overdue->{branchcode};
 

diff --git a/misc/cronjobs/staticfines.pl b/misc/cronjobs/staticfines.pl
index 53b9e4b..2ccb1fb 100755 (executable)
--- a/misc/cronjobs/staticfines.pl
+++ b/misc/cronjobs/staticfines.pl
@@ -95,7 +95,7 @@ foreach (@pcategories) {
 }
 
 use vars qw(@borrower_fields @item_fields @other_fields);
-use vars qw($fldir $libname $control $mode $delim $dbname $today $today_iso $today_days);
+use vars qw($fldir $libname $control $branch_type $mode $delim $dbname $today $today_iso $today_days);
 use vars qw($filename);
 
 CHECK {
@@ -104,6 +104,7 @@ CHECK {
     @other_fields    = qw(type days_overdue fine);
     $libname         = C4::Context->preference('LibraryName');
     $control         = C4::Context->preference('CircControl');
+    $branch_type     = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch';
     $mode            = C4::Context->preference('finesMode');
     $dbname          = C4::Context->config('database');
     $delim           = "\t";                                                                          # ?  C4::Context->preference('delimiter') || "\t";
@@ -157,7 +158,7 @@ for ( my $i = 0 ; $i < scalar(@$data) ; $i++ ) {
 
     my $branchcode =
         ( $useborrowerlibrary )           ? $patron->branchcode
-      : ( $control eq 'ItemHomeLibrary' ) ? $data->[$i]->{homebranch}
+      : ( $control eq 'ItemHomeLibrary' ) ? $data->[$i]->{$branch_type}
       : ( $control eq 'PatronLibrary' )   ? $patron->branchcode
       :                                     $data->[$i]->{branchcode};
     # In final case, CircControl must be PickupLibrary. (branchcode comes from issues table here).