Attachment 103049 Details for Bug 25089 – Bug 25089: Remove on-site specific circulation rules

[patch] Bug 25089: Remove on-site specific circulation rules

Bug-25089-Remove-on-site-specific-circulation-rule.patch (text/plain), 37.80 KB, created by Lari Taskula on 2020-04-15 23:50:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Lari Taskula

Created: 2020-04-15 23:50:13 UTC

Size: 37.80 KB

patch

obsolete

>From 540b4284338046980f6c13fc437053b95a0318dc Mon Sep 17 00:00:00 2001
>From: Lari Taskula <lari.taskula@hypernova.fi>
>Date: Tue, 31 Mar 2020 16:25:59 +0000
>Subject: [PATCH] Bug 25089: Remove on-site specific circulation rules
>
>To apply:
>1. perl installer/data/mysql/updatedatabase.pl
>
>To test:
>1. Search for patron_maxonsiteissueqty rule
>
>   $ grep -rn 'patron_maxonsiteissueqty'
>
>2. Observe no results in Koha source files
>3. Search for maxonsiteissueqty rule
>
>   $ grep -rn 'maxonsiteissueqty'
>
>4. Observe no results (apart from .git)
>5. Check modification made to C4::Circulation::GetBranchBorrowerCircRule().
>   Make sure the new parameter is being used everywhere. You can search
>   the usage of this subroutine with similar grep commands as above.
>
>6. Run the following tests:
>
>prove t/db_dependent/Circulation/Branch.t
>prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t
>prove t/db_dependent/Circulation/TooMany.t
>
>Sponsored-by: The National Library of Finland
>---
> C4/Circulation.pm                             | 49 +++++++------------
> Koha/CirculationRules.pm                      |  6 ---
> admin/smart-rules.pl                          | 19 +------
> ...bug_24101_2_convert_circulation_rules.perl | 33 +++++++++++++
> installer/onboarding.pl                       |  2 -
> .../prog/en/modules/admin/smart-rules.tt      | 38 ++------------
> t/db_dependent/Circulation/Branch.t           | 47 ++++++++++++------
> .../Circulation/SwitchOnSiteCheckouts.t       | 26 +++++++++-
> t/db_dependent/Circulation/TooMany.t          | 36 +++++++++-----
> 9 files changed, 138 insertions(+), 118 deletions(-)
> create mode 100644 installer/data/mysql/atomicupdate/bug_24101_2_convert_circulation_rules.perl
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index efdc2b15a0..acc1f14786 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -385,6 +385,11 @@ sub TooMany {
> 	# Get which branchcode we need
>     $branch = _GetCircControlBranch($item_object->unblessed,$borrower);
>     my $type = $item_object->effective_itemtype;
>+    my $checkout_type = $onsite_checkout
>+        ? $switch_onsite_checkout
>+            ? $Koha::Checkouts::type->{checkout}
>+            : $Koha::Checkouts::type->{onsite_checkout}
>+        : $Koha::Checkouts::type->{checkout};
> 
>     # given branch, patron category, and item type, determine
>     # applicable issuing rule
>@@ -396,14 +401,6 @@ sub TooMany {
>             rule_name    => 'maxissueqty',
>         }
>     );
>-    my $maxonsiteissueqty_rule = Koha::CirculationRules->get_effective_rule(
>-        {
>-            categorycode => $cat_borrower,
>-            itemtype     => $type,
>-            branchcode   => $branch,
>-            rule_name    => 'maxonsiteissueqty',
>-        }
>-    );
> 
> 
>     # if a rule is found and has a loan limit set, count
>@@ -473,14 +470,13 @@ sub TooMany {
>         my ( $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $count_query, {}, @bind_params );
> 
>         my $max_checkouts_allowed = $maxissueqty_rule ? $maxissueqty_rule->rule_value : undef;
>-        my $max_onsite_checkouts_allowed = $maxonsiteissueqty_rule ? $maxonsiteissueqty_rule->rule_value : undef;
> 
>-        if ( $onsite_checkout and $max_onsite_checkouts_allowed ne '' ) {
>-            if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed )  {
>+        if ( $onsite_checkout and $max_checkouts_allowed ne '' ) {
>+            if ( $onsite_checkout_count >= $max_checkouts_allowed )  {
>                 return {
>                     reason => 'TOO_MANY_ONSITE_CHECKOUTS',
>                     count => $onsite_checkout_count,
>-                    max_allowed => $max_onsite_checkouts_allowed,
>+                    max_allowed => $max_checkouts_allowed,
>                 }
>             }
>         }
>@@ -505,7 +501,7 @@ sub TooMany {
>     }
> 
>     # Now count total loans against the limit for the branch
>-    my $branch_borrower_circ_rule = GetBranchBorrowerCircRule($branch, $cat_borrower);
>+    my $branch_borrower_circ_rule = GetBranchBorrowerCircRule($branch, $cat_borrower, $checkout_type );
>     if (defined($branch_borrower_circ_rule->{patron_maxissueqty}) and $branch_borrower_circ_rule->{patron_maxissueqty} ne '') {
>         my @bind_params = ();
>         my $branch_count_query = qq|
>@@ -527,14 +523,13 @@ sub TooMany {
>         }
>         my ( $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $branch_count_query, {}, @bind_params );
>         my $max_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxissueqty};
>-        my $max_onsite_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxonsiteissueqty};
> 
>-        if ( $onsite_checkout and $max_onsite_checkouts_allowed ne '' ) {
>-            if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed )  {
>+        if ( $onsite_checkout and $max_checkouts_allowed ne '' ) {
>+            if ( $onsite_checkout_count >= $max_checkouts_allowed )  {
>                 return {
>                     reason => 'TOO_MANY_ONSITE_CHECKOUTS',
>                     count => $onsite_checkout_count,
>-                    max_allowed => $max_onsite_checkouts_allowed,
>+                    max_allowed => $max_checkouts_allowed,
>                 }
>             }
>         }
>@@ -1601,7 +1596,7 @@ sub GetHardDueDate {
> 
> =head2 GetBranchBorrowerCircRule
> 
>-  my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode);
>+  my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode, $checkout_type);
> 
> Retrieves circulation rule attributes that apply to the given
> branch and patron category, regardless of item type.  
>@@ -1611,21 +1606,13 @@ patron_maxissueqty - maximum number of loans that a
> patron of the given category can have at the given
> branch.  If the value is undef, no limit.
> 
>-patron_maxonsiteissueqty - maximum of on-site checkouts that a
>-patron of the given category can have at the given
>-branch.  If the value is undef, no limit.
>-
>-This will check for different branch/category combinations in the following order:
>-branch and category
>-branch only
>-category only
>-default branch and category
>+The order in which rules are searched is defined in circulation
>+rules user interface.
> 
> If no rule has been found in the database, it will default to
> the buillt in rule:
> 
> patron_maxissueqty - undef
>-patron_maxonsiteissueqty - undef
> 
> C<$branchcode> and C<$categorycode> should contain the
> literal branch code and patron category code, respectively - no
>@@ -1634,21 +1621,21 @@ wildcards.
> =cut
> 
> sub GetBranchBorrowerCircRule {
>-    my ( $branchcode, $categorycode ) = @_;
>+    my ( $branchcode, $categorycode, $checkout_type ) = @_;
> 
>     # Initialize default values
>     my $rules = {
>         patron_maxissueqty       => undef,
>-        patron_maxonsiteissueqty => undef,
>     };
> 
>     # Search for rules!
>-    foreach my $rule_name (qw( patron_maxissueqty patron_maxonsiteissueqty )) {
>+    foreach my $rule_name (qw( patron_maxissueqty )) {
>         my $rule = Koha::CirculationRules->get_effective_rule(
>             {
>                 categorycode => $categorycode,
>                 itemtype     => undef,
>                 branchcode   => $branchcode,
>+                checkout_type => $checkout_type,
>                 rule_name    => $rule_name,
>             }
>         );
>diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm
>index 4b8d8d8675..c267930bda 100644
>--- a/Koha/CirculationRules.pm
>+++ b/Koha/CirculationRules.pm
>@@ -53,9 +53,6 @@ our $RULE_KINDS = {
>     patron_maxissueqty => {
>         scope => [ 'branchcode', 'categorycode', 'checkout_type' ],
>     },
>-    patron_maxonsiteissueqty => {
>-        scope => [ 'branchcode', 'categorycode' ],
>-    },
>     max_holds => {
>         scope => [ 'branchcode', 'categorycode' ],
>     },
>@@ -115,9 +112,6 @@ our $RULE_KINDS = {
>     maxissueqty => {
>         scope => [ 'branchcode', 'categorycode', 'itemtype', 'checkout_type' ],
>     },
>-    maxonsiteissueqty => {
>-        scope => [ 'branchcode', 'categorycode', 'itemtype' ],
>-    },
>     maxsuspensiondays => {
>         scope => [ 'branchcode', 'categorycode', 'itemtype', 'checkout_type' ],
>     },
>diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl
>index 6bea920dd9..941abdae73 100755
>--- a/admin/smart-rules.pl
>+++ b/admin/smart-rules.pl
>@@ -84,7 +84,6 @@ if ($op eq 'delete') {
>             itemtype     => $itemtype eq '*' ? undef : $itemtype,
>             rules        => {
>                 maxissueqty                      => undef,
>-                maxonsiteissueqty                => undef,
>                 rentaldiscount                   => undef,
>                 fine                             => undef,
>                 finedays                         => undef,
>@@ -127,7 +126,6 @@ elsif ($op eq 'delete-branch-cat') {
>                     rules        => {
>                         max_holds                      => undef,
>                         patron_maxissueqty             => undef,
>-                        patron_maxonsiteissueqty       => undef,
>                     }
>                 }
>             );
>@@ -150,7 +148,6 @@ elsif ($op eq 'delete-branch-cat') {
>                     rules        => {
>                         max_holds                => undef,
>                         patron_maxissueqty       => undef,
>-                        patron_maxonsiteissueqty => undef,
>                     }
>                 }
>             );
>@@ -162,7 +159,6 @@ elsif ($op eq 'delete-branch-cat') {
>                 categorycode => undef,
>                 rules        => {
>                     patron_maxissueqty       => undef,
>-                    patron_maxonsiteissueqty => undef,
>                 }
>             }
>         );
>@@ -185,7 +181,6 @@ elsif ($op eq 'delete-branch-cat') {
>                 rules        => {
>                     max_holds         => undef,
>                     patron_maxissueqty       => undef,
>-                    patron_maxonsiteissueqty => undef,
>                 }
>             }
>         );
>@@ -260,7 +255,6 @@ elsif ($op eq 'add') {
>     my $chargeperiod = $input->param('chargeperiod');
>     my $chargeperiod_charge_at = $input->param('chargeperiod_charge_at');
>     my $maxissueqty  = strip_non_numeric($input->param('maxissueqty'));
>-    my $maxonsiteissueqty  = strip_non_numeric($input->param('maxonsiteissueqty'));
>     my $renewalsallowed  = $input->param('renewalsallowed');
>     my $renewalperiod    = $input->param('renewalperiod');
>     my $norenewalbefore  = $input->param('norenewalbefore');
>@@ -288,11 +282,10 @@ elsif ($op eq 'add') {
>     my $overduefinescap = $input->param('overduefinescap') || '';
>     my $cap_fine_to_replacement_price = $input->param('cap_fine_to_replacement_price') eq 'on';
>     my $note = $input->param('note');
>-    $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $maxonsiteissueqty, $cap_fine_to_replacement_price";
>+    $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $cap_fine_to_replacement_price";
> 
>     my $rules = {
>         maxissueqty                   => $maxissueqty,
>-        maxonsiteissueqty             => $maxonsiteissueqty,
>         rentaldiscount                => $rentaldiscount,
>         fine                          => $fine,
>         finedays                      => $finedays,
>@@ -335,8 +328,6 @@ elsif ($op eq 'add') {
> elsif ($op eq "set-branch-defaults") {
>     my $categorycode  = $input->param('categorycode');
>     my $patron_maxissueqty   = strip_non_numeric($input->param('patron_maxissueqty'));
>-    my $patron_maxonsiteissueqty = $input->param('patron_maxonsiteissueqty');
>-    $patron_maxonsiteissueqty = strip_non_numeric($patron_maxonsiteissueqty);
>     my $holdallowed   = $input->param('holdallowed');
>     my $hold_fulfillment_policy = $input->param('hold_fulfillment_policy');
>     my $returnbranch  = $input->param('returnbranch');
>@@ -362,7 +353,6 @@ elsif ($op eq "set-branch-defaults") {
>                 branchcode   => undef,
>                 rules        => {
>                     patron_maxissueqty             => $patron_maxissueqty,
>-                    patron_maxonsiteissueqty       => $patron_maxonsiteissueqty,
>                 }
>             }
>         );
>@@ -384,7 +374,6 @@ elsif ($op eq "set-branch-defaults") {
>                 branchcode   => $branch,
>                 rules        => {
>                     patron_maxissueqty             => $patron_maxissueqty,
>-                    patron_maxonsiteissueqty       => $patron_maxonsiteissueqty,
>                 }
>             }
>         );
>@@ -401,8 +390,6 @@ elsif ($op eq "set-branch-defaults") {
> elsif ($op eq "add-branch-cat") {
>     my $categorycode  = $input->param('categorycode');
>     my $patron_maxissueqty = strip_non_numeric($input->param('patron_maxissueqty'));
>-    my $patron_maxonsiteissueqty = $input->param('patron_maxonsiteissueqty');
>-    $patron_maxonsiteissueqty = strip_non_numeric($patron_maxonsiteissueqty);
>     my $max_holds = $input->param('max_holds');
>     $max_holds =~ s/\s//g;
>     $max_holds = undef if $max_holds !~ /^\d+/;
>@@ -416,7 +403,6 @@ elsif ($op eq "add-branch-cat") {
>                     rules        => {
>                         max_holds         => $max_holds,
>                         patron_maxissueqty       => $patron_maxissueqty,
>-                        patron_maxonsiteissueqty => $patron_maxonsiteissueqty,
>                     }
>                 }
>             );
>@@ -428,7 +414,6 @@ elsif ($op eq "add-branch-cat") {
>                     rules        => {
>                         max_holds         => $max_holds,
>                         patron_maxissueqty       => $patron_maxissueqty,
>-                        patron_maxonsiteissueqty => $patron_maxonsiteissueqty,
>                     }
>                 }
>             );
>@@ -441,7 +426,6 @@ elsif ($op eq "add-branch-cat") {
>                 rules        => {
>                     max_holds         => $max_holds,
>                     patron_maxissueqty       => $patron_maxissueqty,
>-                    patron_maxonsiteissueqty => $patron_maxonsiteissueqty,
>                 }
>             }
>         );
>@@ -453,7 +437,6 @@ elsif ($op eq "add-branch-cat") {
>                 rules        => {
>                     max_holds         => $max_holds,
>                     patron_maxissueqty       => $patron_maxissueqty,
>-                    patron_maxonsiteissueqty => $patron_maxonsiteissueqty,
>                 }
>             }
>         );
>diff --git a/installer/data/mysql/atomicupdate/bug_24101_2_convert_circulation_rules.perl b/installer/data/mysql/atomicupdate/bug_24101_2_convert_circulation_rules.perl
>new file mode 100644
>index 0000000000..7b7df4922e
>--- /dev/null
>+++ b/installer/data/mysql/atomicupdate/bug_24101_2_convert_circulation_rules.perl
>@@ -0,0 +1,33 @@
>+$DBversion = 'XXX'; # will be replaced by the RM
>+if( CheckVersion( $DBversion ) ) {
>+
>+    # First, convert maxonsiteissueqty to maxissueqty
>+    my $update_sth = $dbh->prepare(q|
>+        UPDATE circulation_rules
>+        SET checkout_type = ?,
>+            rule_name = ?
>+        WHERE rule_name=?
>+    |);
>+    $update_sth->execute(
>+        'OS',                       # checkout_type => 'OS' (on-site checkout)
>+        'maxissueqty',
>+        'maxonsiteissueqty'
>+    );
>+
>+    # Then, convert patron_maxonsiteissueqty to patron_maxissueqty
>+    $update_sth = $dbh->prepare(q|
>+        UPDATE circulation_rules
>+        SET checkout_type = ?,
>+            rule_name = ?
>+        WHERE rule_name=?
>+    |);
>+    $update_sth->execute(
>+        'OS',                       # checkout_type => 'OS' (on-site checkout)
>+        'patron_maxissueqty',
>+        'patron_maxonsiteissueqty'
>+    );
>+
>+    # Always end with this (adjust the bug info)
>+    SetVersion( $DBversion );
>+    print "Upgrade to $DBversion done (Bug 24101 - Convert maxonsiteissueqty to maxissueqty )\n";
>+}
>diff --git a/installer/onboarding.pl b/installer/onboarding.pl
>index 547df29e0d..dab7980ee9 100755
>--- a/installer/onboarding.pl
>+++ b/installer/onboarding.pl
>@@ -273,7 +273,6 @@ if ( $step == 5 ) {
>                 holds_per_day                    => $holds_per_day,
>                 holds_per_record                 => $holds_per_record,
>                 maxissueqty                      => "",
>-                maxonsiteissueqty                => "",
>                 maxsuspensiondays                => "",
>                 no_auto_renewal_after            => "",
>                 no_auto_renewal_after_hard_limit => "",
>@@ -291,7 +290,6 @@ if ( $step == 5 ) {
>             categorycode => $categorycode,
>             rules        => {
>                 patron_maxissueqty       => "",
>-                patron_maxonsiteissueqty => "",
>                 max_holds                => "",
>             }
>         };
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
>index 5ae3bb94e1..86f3edee0d 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
>@@ -91,7 +91,6 @@
>                 <th>Actions</th>
>                 <th>Note</th>
>                 <th>Current checkouts allowed</th>
>-                <th>Current on-site checkouts allowed</th>
>                 <th>Loan period</th>
>                 <th>Unit</th>
>                 <th>Hard due date</th>
>@@ -128,7 +127,6 @@
>                         [% SET i = '' UNLESS i.defined %]
>                         [% SET note = all_rules.$c.$i.note %]
>                         [% SET maxissueqty = all_rules.$c.$i.maxissueqty %]
>-                        [% SET maxonsiteissueqty = all_rules.$c.$i.maxonsiteissueqty %]
>                         [% SET issuelength = all_rules.$c.$i.issuelength %]
>                         [% SET lengthunit = all_rules.$c.$i.lengthunit %]
>                         [% SET hardduedate = all_rules.$c.$i.hardduedate %]
>@@ -156,7 +154,7 @@
>                         [% SET article_requests = all_rules.$c.$i.article_requests %]
>                         [% SET rentaldiscount = all_rules.$c.$i.rentaldiscount %]
> 
>-                        [% SET show_rule = maxissueqty || maxonsiteissueqty || issuelength || lengthunit || hardduedate || hardduedatebefore || hardduedateexact || fine || chargeperiod || chargeperiod_charge_at || firstremind || overduefinescap || cap_fine_to_replacement_price || finedays || maxsuspensiondays || suspension_chargeperiod || renewalsallowed || renewalsallowed || norenewalbefore || auto_renew || no_auto_renewal_after || no_auto_renewal_after_hard_limit || reservesallowed || holds_per_day || holds_per_record || onshelfholds || opacitemholds || article_requests || article_requests %]
>+                        [% SET show_rule = maxissueqty || issuelength || lengthunit || hardduedate || hardduedatebefore || hardduedateexact || fine || chargeperiod || chargeperiod_charge_at || firstremind || overduefinescap || cap_fine_to_replacement_price || finedays || maxsuspensiondays || suspension_chargeperiod || renewalsallowed || renewalsallowed || norenewalbefore || auto_renew || no_auto_renewal_after || no_auto_renewal_after_hard_limit || reservesallowed || holds_per_day || holds_per_record || onshelfholds || opacitemholds || article_requests || article_requests %]
>                         [% IF show_rule %]
>                             [% SET row_count = row_count + 1 %]
>                             <tr row_countd="row_[% row_count | html %]">
>@@ -190,13 +188,6 @@
>                                             <span>Unlimited</span>
>                                         [% END %]
>                                     </td>
>-                                    <td>
>-                                        [% IF maxonsiteissueqty.defined && maxonsiteissueqty != ''  %]
>-                                            [% maxonsiteissueqty | html %]
>-                                        [% ELSE %]
>-                                            <span>Unlimited</span>
>-                                        [% END %]
>-                                    </td>
>                                     <td>[% issuelength | html %]</td>
>                                     <td>
>                                         [% IF ( lengthunit == 'days' ) %]
>@@ -333,7 +324,6 @@
>                     </td>
>                     <td><input type="text" name="note" id="note" size="15" value="" maxlength="100"></td>
>                     <td><input type="text" name="maxissueqty" id="maxissueqty" size="3" /></td>
>-                    <td><input type="text" name="maxonsiteissueqty" id="maxonsiteissueqty" size="3" /></td>
>                     <td><input type="text" name="issuelength" id="issuelength" size="3" /> </td>
>                     <td>
>                       <select name="lengthunit" id="lengthunit">
>@@ -417,7 +407,6 @@
>                       <th>&nbsp;</th>
>                       <th>Note</th>
>                       <th>Current checkouts allowed</th>
>-                      <th>Current on-site checkouts allowed</th>
>                       <th>Loan period</th>
>                       <th>Unit</th>
>                       <th>Hard due date</th>
>@@ -460,7 +449,6 @@
>                 <tr>
>                     <th>&nbsp;</th>
>                     <th>Total current checkouts allowed</th>
>-                    <th>Total current on-site checkouts allowed</th>
>                     <th>Maximum total holds allowed (count)</th>
>                     <th>Hold policy</th>
>                     <th>Hold pickup library match</th>
>@@ -473,10 +461,6 @@
>                         [% SET patron_maxissueqty = CirculationRules.Search( current_branch, undef, undef, 'patron_maxissueqty' ) %]
>                         <input type="text" name="patron_maxissueqty" size="3" value="[% patron_maxissueqty | html %]"/>
>                     </td>
>-                    <td>
>-                        [% SET patron_maxonsiteissueqty = CirculationRules.Search( current_branch, undef, undef, 'patron_maxonsiteissueqty' ) %]
>-                        <input type="text" name="patron_maxonsiteissueqty" size="3" value="[% patron_maxonsiteissueqty | html %]"/>
>-                    </td>
>                     <td>
>                         [% SET rule_value = CirculationRules.Search( current_branch, undef , undef, 'max_holds' ) %]
>                         <input name="max_holds" size="3" value="[% rule_value | html %]" />
>@@ -635,17 +619,15 @@
>                 <tr>
>                     <th>Patron category</th>
>                     <th>Total current checkouts allowed</th>
>-                    <th>Total current on-site checkouts allowed</th>
>                     <th>Total holds allowed</th>
>                     <th>&nbsp;</th>
>                 </tr>
>                 [% FOREACH c IN categorycodes %]
>                     [% NEXT UNLESS c %]
>                     [% SET patron_maxissueqty = CirculationRules.Search( branchcode, c, undef, 'patron_maxissueqty' ) %]
>-                    [% SET patron_maxonsiteissueqty = CirculationRules.Search( branchcode, c, undef, 'patron_maxonsiteissueqty' ) %]
>                     [% SET max_holds = CirculationRules.Search( branchcode, c, undef, 'max_holds' ) %]
> 
>-                    [% IF  ( patron_maxissueqty.defined && patron_maxissueqty != '' ) || ( patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' ) || ( max_holds.defined && max_holds != '' ) %]
>+                    [% IF  ( patron_maxissueqty.defined && patron_maxissueqty != '' ) || ( max_holds.defined && max_holds != '' ) %]
>                     <tr>
>                         <td>
>                             [% IF c == undef %]
>@@ -661,13 +643,6 @@
>                                 <span>Unlimited</span>
>                             [% END %]
>                         </td>
>-                        <td>
>-                            [% IF patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' %]
>-                                [% patron_maxonsiteissueqty | html %]
>-                            [% ELSE %]
>-                                <span>Unlimited</span>
>-                            [% END %]
>-                        </td>
>                         <td>
>                             [% IF max_holds.defined && max_holds != '' %]
>                                 [% max_holds | html %]
>@@ -690,8 +665,6 @@
>                         [% END %]
>                         </select>
>                     </td>
>-                    <td><input name="patron_maxissueqty" size="3" type="text" /></td>
>-                    <td><input name="patron_maxonsiteissueqty" size="3" type="text" /></td>
>                     <td><input name="max_holds" size="3" type="text" /></td>
>                     <td class="actions"><button type="submit" class="btn btn-default btn-xs"><i class="fa fa-plus"></i> Add</td>
>                 </tr>
>@@ -988,7 +961,7 @@
>                         // specific processing for the Note column
>                         var note = $(this).find("a[name='viewnote']").data("content");
>                         $(current_column).find("input[type='text']").val(note);
>-                    } else if ( i == 8 ) {
>+                    } else if ( i == 7 ) {
>                         // specific processing for the Hard due date column
>                         var select_value = $(this).find("input[type='hidden'][name='hardduedatecomparebackup']").val();
>                         var input_value = '';
>@@ -999,7 +972,7 @@
>                         }
>                         $(current_column).find("input[type='text']").val(input_value);
>                         $(current_column).find("select").val(select_value);
>-                    } else if ( i == 14 ) {
>+                    } else if ( i == 13 ) {
>                         // specific processing for cap_fine_to_replacement_price
>                         var cap_fine_to_replacement_price = $(this).find("input[type='checkbox']");
>                         $('#cap_fine_to_replacement_price').prop('checked', cap_fine_to_replacement_price.is(':checked') );
>@@ -1024,10 +997,9 @@
>                             // Remove potential previous input added
>                             $(current_column).find("input").remove();
>                             $(current_column).append("<input type='hidden' name='"+name+"' value='"+val+"' />");
>-                        } else if ( i == 4 || i == 5 || i == 24 || i == 25 || i == 26 ) {
>+                        } else if ( i == 4 || i == 23 || i == 24 || i == 25 ) {
>                             // If the value is not an integer for
>                             //     - "Current checkouts allowed"
>-                            //     - "Current on-site checkouts allowed"
>                             //     - "Holds allowed (total)"
>                             //     - "Holds allowed (daily)"
>                             //     - "Holds per record (count)"
>diff --git a/t/db_dependent/Circulation/Branch.t b/t/db_dependent/Circulation/Branch.t
>index 16e590fcc3..2a706780a3 100644
>--- a/t/db_dependent/Circulation/Branch.t
>+++ b/t/db_dependent/Circulation/Branch.t
>@@ -25,7 +25,7 @@ use Koha::CirculationRules;
> 
> use Koha::Patrons;
> 
>-use Test::More tests => 14;
>+use Test::More tests => 15;
> use t::lib::Mocks;
> use t::lib::TestBuilder;
> 
>@@ -143,29 +143,39 @@ my $borrower_id1 = Koha::Patron->new({
> 
> is_deeply(
>     GetBranchBorrowerCircRule(),
>-    { patron_maxissueqty => undef, patron_maxonsiteissueqty => undef },
>-"Without parameter, GetBranchBorrower returns undef (unilimited) for patron_maxissueqty and patron_maxonsiteissueqty if no rules defined"
>+    { patron_maxissueqty => undef },
>+"Without parameter, GetBranchBorrower returns undef (unilimited) for patron_maxissueqty if no rules defined"
> );
> 
> Koha::CirculationRules->set_rules(
>     {
>         branchcode   => $samplebranch1->{branchcode},
>         categorycode => $samplecat->{categorycode},
>+        checkout_type => undef,
>         rules        => {
>             patron_maxissueqty       => 5,
>-            patron_maxonsiteissueqty => 6,
>         }
>     }
> );
> 
>+Koha::CirculationRules->set_rules(
>+    {
>+        branchcode   => $samplebranch1->{branchcode},
>+        categorycode => $samplecat->{categorycode},
>+        checkout_type => $Koha::Checkouts::type->{onsite_checkout},
>+        rules        => {
>+            patron_maxissueqty       => 9001,
>+        }
>+    }
>+);
> 
> Koha::CirculationRules->set_rules(
>     {
>         branchcode   => $samplebranch2->{branchcode},
>         categorycode => undef,
>+        checkout_type => undef,
>         rules        => {
>             patron_maxissueqty       => 3,
>-            patron_maxonsiteissueqty => 2,
>         }
>     }
> );
>@@ -184,9 +194,9 @@ Koha::CirculationRules->set_rules(
>     {
>         branchcode   => undef,
>         categorycode => undef,
>+        checkout_type => undef,
>         rules        => {
>             patron_maxissueqty       => 4,
>-            patron_maxonsiteissueqty => 5,
>         }
>     }
> );
>@@ -235,26 +245,35 @@ Koha::CirculationRules->set_rules(
> #Test GetBranchBorrowerCircRule
> is_deeply(
>     GetBranchBorrowerCircRule(),
>-    { patron_maxissueqty => 4, patron_maxonsiteissueqty => 5 },
>-"Without parameter, GetBranchBorrower returns the patron_maxissueqty and patron_maxonsiteissueqty of default_circ_rules"
>+    { patron_maxissueqty => 4 },
>+"Without parameter, GetBranchBorrower returns the patron_maxissueqty of default_circ_rules"
> );
> is_deeply(
>     GetBranchBorrowerCircRule( $samplebranch2->{branchcode} ),
>-    { patron_maxissueqty => 3, patron_maxonsiteissueqty => 2 },
>-"Without only the branchcode specified, GetBranchBorrower returns the patron_maxissueqty and patron_maxonsiteissueqty corresponding"
>+    { patron_maxissueqty => 3 },
>+"Without only the branchcode specified, GetBranchBorrower returns the patron_maxissueqty corresponding"
> );
> is_deeply(
>     GetBranchBorrowerCircRule(
>         $samplebranch1->{branchcode},
>         $samplecat->{categorycode}
>     ),
>-    { patron_maxissueqty => 5, patron_maxonsiteissueqty => 6 },
>-    "GetBranchBorrower returns the patron_maxissueqty and patron_maxonsiteissueqty of the branch1 and the category1"
>+    { patron_maxissueqty => 5 },
>+    "GetBranchBorrower returns the patron_maxissueqty of the branch1 and the category1"
>+);
>+is_deeply(
>+    GetBranchBorrowerCircRule(
>+        $samplebranch1->{branchcode},
>+        $samplecat->{categorycode},
>+        $Koha::Checkouts::type->{onsite_checkout}
>+    ),
>+    { patron_maxissueqty => 9001 },
>+     "GetBranchBorrower returns the patron_maxissueqty of the branch1 and the category1 and checkout_type on-site"
> );
> is_deeply(
>     GetBranchBorrowerCircRule( -1, -1 ),
>-    { patron_maxissueqty => 4, patron_maxonsiteissueqty => 5 },
>-"GetBranchBorrower with wrong parameters returns the patron_maxissueqty and patron_maxonsiteissueqty of default_circ_rules"
>+    { patron_maxissueqty => 4 },
>+"GetBranchBorrower with wrong parameters returns the patron_maxissueqty of default_circ_rules"
> );
> 
> #Test GetBranchItemRule
>diff --git a/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t b/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t
>index 41e4eb8433..7989c5e5eb 100644
>--- a/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t
>+++ b/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t
>@@ -87,9 +87,9 @@ Koha::CirculationRules->set_rules(
>         branchcode   => $branch->{branchcode},
>         categorycode => undef,
>         itemtype     => undef,
>+        checkout_type => undef,
>         rules        => {
>             maxissueqty       => 2,
>-            maxonsiteissueqty => 1,
>             lengthunit        => 'days',
>             issuelength       => 5,
>             hardduedate        => undef,
>@@ -97,6 +97,17 @@ Koha::CirculationRules->set_rules(
>         }
>     }
> );
>+Koha::CirculationRules->set_rules(
>+    {
>+        branchcode   => $branch->{branchcode},
>+        categorycode => undef,
>+        itemtype     => undef,
>+        checkout_type => $Koha::Checkouts::type->{onsite_checkout},
>+        rules        => {
>+            maxissueqty       => 1,
>+        }
>+    }
>+);
> 
> t::lib::Mocks::mock_userenv({ patron => $patron });
> 
>@@ -159,9 +170,20 @@ Koha::CirculationRules->set_rules(
>         branchcode   => $branch->{branchcode},
>         categorycode => undef,
>         itemtype     => undef,
>+        checkout_type => undef,
>+        rules        => {
>+            maxissueqty       => 2,
>+        }
>+    }
>+);
>+Koha::CirculationRules->set_rules(
>+    {
>+        branchcode   => $branch->{branchcode},
>+        categorycode => undef,
>+        itemtype     => undef,
>+        checkout_type => $Koha::Checkouts::type->{onsite_checkout},
>         rules        => {
>             maxissueqty       => 2,
>-            maxonsiteissueqty => 1,
>         }
>     }
> );
>diff --git a/t/db_dependent/Circulation/TooMany.t b/t/db_dependent/Circulation/TooMany.t
>index 31ec01563b..46f1f55531 100644
>--- a/t/db_dependent/Circulation/TooMany.t
>+++ b/t/db_dependent/Circulation/TooMany.t
>@@ -108,9 +108,9 @@ subtest '1 Issuingrule exist 0 0: no issue allowed' => sub {
>             branchcode   => $branch->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => undef,
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 0,
>-                maxonsiteissueqty => 0,
>             }
>         },
>     );
>@@ -165,9 +165,20 @@ subtest '1 Issuingrule exist with onsiteissueqty=unlimited' => sub {
>             branchcode   => $branch->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => undef,
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => undef,
>+            }
>+        },
>+    );
>+    Koha::CirculationRules->set_rules(
>+        {
>+            branchcode   => $branch->{branchcode},
>+            categorycode => $category->{categorycode},
>+            itemtype     => undef,
>+            checkout_type => $Koha::Checkouts::type->{onsite_checkout},
>+            rules        => {
>+                maxissueqty       => undef,
>             }
>         },
>     );
>@@ -220,9 +231,9 @@ subtest '1 Issuingrule exist 1 1: issue is allowed' => sub {
>             branchcode   => $branch->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => undef,
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => 1,
>             }
>         }
>     );
>@@ -260,9 +271,9 @@ subtest '1 Issuingrule exist: 1 CO allowed, 1 OSCO allowed. Do a CO' => sub {
>             branchcode   => $branch->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => undef,
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => 1,
>             }
>         }
>     );
>@@ -316,9 +327,9 @@ subtest '1 Issuingrule exist: 1 CO allowed, 1 OSCO allowed, Do a OSCO' => sub {
>             branchcode   => $branch->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => undef,
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => 1,
>             }
>         }
>     );
>@@ -375,9 +386,9 @@ subtest '1 BranchBorrowerCircRule exist: 1 CO allowed, 1 OSCO allowed' => sub {
>             branchcode   => $branch->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => undef,
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => 1,
>             }
>         }
>     );
>@@ -427,9 +438,9 @@ subtest '1 BranchBorrowerCircRule exist: 1 CO allowed, 1 OSCO allowed' => sub {
>             branchcode   => $branch->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => undef,
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => 1,
>             }
>         }
>     );
>@@ -505,6 +516,7 @@ subtest 'General vs specific rules limit quantity correctly' => sub {
>             categorycode => '*',
>             itemtype     => $itemtype->{itemtype},
>             branchcode   => '*',
>+            checkout_type => '*',
>             rules        => {
>                 issuelength => 1,
>                 firstremind => 1,        # 1 day of grace
>@@ -519,10 +531,10 @@ subtest 'General vs specific rules limit quantity correctly' => sub {
>         {
>             branchcode   => '*',
>             categorycode => '*',
>+            checkout_type => '*',
>             itemtype     => $itemtype->{itemtype},
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => 1,
>             }
>         }
>     );
>@@ -573,9 +585,9 @@ subtest 'General vs specific rules limit quantity correctly' => sub {
>             branchcode   => $branch->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => $itemtype->{itemtype},
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => 1,
>             }
>         }
>     );
>@@ -669,9 +681,9 @@ subtest 'General vs specific rules limit quantity correctly' => sub {
>             branchcode   => $branch2->{branchcode},
>             categorycode => $category->{categorycode},
>             itemtype     => $itemtype->{itemtype},
>+            checkout_type => undef,
>             rules        => {
>                 maxissueqty       => 1,
>-                maxonsiteissueqty => 1,
>             }
>         }
>     );
>@@ -691,9 +703,9 @@ subtest 'empty string means unlimited' => sub {
>             branchcode   => '*',
>             categorycode => '*',
>             itemtype     => '*',
>+            checkout_type => '*',
>             rules        => {
>                 maxissueqty       => '',
>-                maxonsiteissueqty => '',
>             }
>         },
>     );
>@@ -706,7 +718,7 @@ subtest 'empty string means unlimited' => sub {
>     is(
>         C4::Circulation::TooMany( $patron, $item_object, { onsite_checkout => 1 } ),
>         undef,
>-        'maxonsiteissueqty="" should mean unlimited'
>+        'maxissueqty="" should mean unlimited'
>     );
> 
>     teardown();
>-- 
>2.17.1

Actions: View | Diff | Splinter Review

Attachments on bug 25089: 103046 | 103047 | 103048 | 103049 | 103050 | 103051 | 103052 | 103053 | 103054 | 103239 | 103240 | 103241 | 103242 | 103243 | 103244 | 103245 | 103246 | 103247 | 103248 | 103283 | 103284 | 103285 | 103286 | 103287 | 103288 | 103289 | 103290 | 103291 | 103292 | 103339 | 103340 | 103341 | 103342 | 103343 | 103344 | 103345 | 103346 | 103347 | 103348 | 103349 | 103351 | 103352 | 103353 | 103354 | 103355 | 103356 | 103357 | 103358 | 103359 | 103360 | 103361 | 103537 | 103538 | 103539 | 103540 | 103541 | 103542 | 103543 | 103544 | 103545 | 103546 | 103547 | 104274 | 104275 | 107523 | 107524 | 107525 | 107526 | 107527 | 107528 | 107529 | 107530 | 107531 | 107532 | 107533