From 8a63d78c4b12665020be3cbfe4a6b1aec930d811 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Joonas=20Kylm=C3=A4l=C3=A4?= Date: Thu, 17 Dec 2020 17:35:07 +0200 Subject: [PATCH] Bug 27259: Add HomeOrHoldingBranch checks where it was missing from MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 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 and notice the fines are now not generated Rebased-by: Joonas Kylmälä Signed-off-by: Petro Vashchuk Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi --- C4/Circulation.pm | 11 +++++++---- C4/Overdues.pm | 4 ++-- misc/cronjobs/fines.pl | 3 ++- misc/cronjobs/staticfines.pl | 5 +++-- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index d74305aebc..d8b70cc996 100644 --- 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 56a0010f19..8074411a9a 100644 --- 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 dab688b780..b141c55723 100755 --- 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 53b9e4bf5b..2ccb1fb283 100755 --- 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). -- 2.39.5