Attachment 114593 Details for Bug 27259 – Bug 27259: Add HomeOrHoldingBranch checks where it was missing from

[patch] Bug 27259: Add HomeOrHoldingBranch checks where it was missing from

Bug-27259-Add-HomeOrHoldingBranch-checks-where-it-.patch (text/plain), 8.66 KB, created by Joonas Kylmälä on 2020-12-22 10:51:09 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Joonas Kylmälä

Created: 2020-12-22 10:51:09 UTC

Size: 8.66 KB

patch

obsolete

>From 4b782ca51d83aef1889760ce9284480f4f7d28b3 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= <joonas.kylmala@helsinki.fi>
>Date: Thu, 17 Dec 2020 17:35:07 +0200
>Subject: [PATCH] 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
>---
> C4/Circulation.pm            | 13 ++++++++-----
> C4/Overdues.pm               |  4 ++--
> misc/cronjobs/fines.pl       |  3 ++-
> misc/cronjobs/staticfines.pl |  5 +++--
> 4 files changed, 15 insertions(+), 10 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 602bc54f6b..604d09b0d0 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -457,9 +457,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' => $maxissueqty_rule->branchcode },
>-                    { prefetch          => 'item' } );
>+                    { "item.$branch_type"   => $maxissueqty_rule->branchcode },
>+                    { prefetch              => 'item' } );
>             }
>         } else {
>             $checkouts = $patron->checkouts; # if rule is not branch specific then count all loans by patron
>@@ -545,9 +546,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;
>@@ -4341,8 +4343,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 d484c67b0f..a421192b28 100644
>--- a/C4/Overdues.pm
>+++ b/C4/Overdues.pm
>@@ -96,14 +96,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
>+   SELECT issues.*, items.itype as itemtype, items.homebranch, items.barcode, items.itemlost, items.replacementprice, 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
>+   SELECT issues.*, biblioitems.itemtype, items.itype, items.homebranch, items.barcode, items.itemlost, replacementprice, 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 9f61c0f598..e584ea365b 100755
>--- a/misc/cronjobs/fines.pl
>+++ b/misc/cronjobs/fines.pl
>@@ -98,6 +98,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('delimiter') || "\t";
> 
>@@ -128,7 +129,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 1e7d287702..cd70ad8ddd 100755
>--- a/misc/cronjobs/staticfines.pl
>+++ b/misc/cronjobs/staticfines.pl
>@@ -104,7 +104,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 {
>@@ -113,6 +113,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";
>@@ -166,7 +167,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.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 27259: 114591 | 114592 | 114593 | 133700 | 133701 | 133702 | 140388 | 140389 | 140390 | 140391 | 141021 | 141022 | 141023 | 143013 | 143014 | 143015