Bug 8137 - Checkout limit for all libraries
Summary: Checkout limit for all libraries
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Thibaud Guillot (thibaud_g)
QA Contact:
URL:
Keywords: release-notes-needed
: 8371 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-05-22 16:39 UTC by Kyle M Hall
Modified: 2025-02-03 08:55 UTC (History)
41 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 8137 - Checkout limit by patron category for all libraries (2.67 KB, patch)
2012-05-22 17:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8137 - Checkout limit by patron category for all libraries (2.63 KB, patch)
2012-05-22 17:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8137 - Checkout limit by patron category for all libraries (3.15 KB, patch)
2012-07-23 12:06 UTC, Marc Véron
Details | Diff | Splinter Review
This patch add the ability to make a i"Default checkout limit by patron category" rule has priority. When a rule has priority, it overrides all others for the same categorycode. For "normal" rule, the behavior is the same than before (like a default rule (11.55 KB, patch)
2017-06-28 13:56 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8137 - Add Default checkout limit by patron category with priority. (11.61 KB, patch)
2017-06-28 14:00 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8137 - Add default issuing rule for all branches. (11.98 KB, patch)
2017-07-04 08:58 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8137 - Add default issuing rule for all branches. (10.46 KB, patch)
2017-07-07 08:32 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8137 - Add Default checkout limit by patron category with priority. (11.56 KB, patch)
2018-03-14 09:19 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8137 - Add default issuing rule for all branches. (10.90 KB, patch)
2018-03-14 09:19 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 8137: Add Default checkout limit by patron category with priority. (10.09 KB, patch)
2020-07-23 00:35 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (10.51 KB, patch)
2020-07-23 00:36 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: (follow-up) (24.73 KB, patch)
2020-07-23 00:36 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: (follow-up) (24.39 KB, patch)
2020-07-28 09:46 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: Add Default checkout limit by patron category with priority. (8.30 KB, patch)
2020-10-08 09:50 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (10.50 KB, patch)
2020-10-08 09:51 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: (follow-up) (23.92 KB, patch)
2020-10-08 09:51 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: (follow-up) (23.91 KB, patch)
2020-10-28 13:33 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: (follow-up) (6.99 KB, patch)
2020-11-19 08:12 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: (follow-up) Fix error on the rules cloning functionality (7.02 KB, patch)
2020-11-20 13:47 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 8137: Add Default checkout limit by patron category with priority. (8.37 KB, patch)
2020-12-18 17:01 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (10.64 KB, patch)
2020-12-18 17:01 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 8137: (follow-up) (24.03 KB, patch)
2020-12-18 17:01 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 8137: (follow-up) Fix error on the rules cloning functionality (7.10 KB, patch)
2020-12-18 17:01 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 8137: Add Default checkout limit by patron category with priority. (8.41 KB, patch)
2021-02-10 15:40 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (10.70 KB, patch)
2021-02-10 15:40 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 8137: (follow-up) (24.16 KB, patch)
2021-02-10 15:40 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 8137: (follow-up) Fix error on the rules cloning functionality (7.15 KB, patch)
2021-02-10 15:41 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 8137: (QA follow-up) Fix number of tests (723 bytes, patch)
2021-02-10 15:41 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 8137: Add Default checkout limit by patron category with priority. (16.55 KB, patch)
2023-01-19 15:17 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (28.17 KB, patch)
2023-01-19 15:17 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) (42.23 KB, patch)
2023-01-19 15:17 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) Fix error on the rules cloning functionality (7.13 KB, patch)
2023-01-19 15:17 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) Rebased on master (17.03 KB, patch)
2023-01-19 15:18 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add Default checkout limit by patron category with priority. (16.55 KB, patch)
2023-06-06 09:26 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (28.17 KB, patch)
2023-06-06 09:26 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) (42.23 KB, patch)
2023-06-06 09:26 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) Fix error on the rules cloning functionality (7.13 KB, patch)
2023-06-06 09:26 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) Rebased on master (16.94 KB, patch)
2023-06-06 09:26 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) Fix number of tests (710 bytes, patch)
2023-06-06 09:26 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (28.18 KB, patch)
2023-09-28 11:15 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (28.18 KB, patch)
2023-09-28 11:17 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add default issuing rule for all branches. (28.18 KB, patch)
2023-09-28 11:20 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) (42.13 KB, patch)
2023-09-28 11:20 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) Fix error on the rules cloning functionality (7.05 KB, patch)
2023-09-28 11:20 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Rebased on master (15.64 KB, patch)
2023-09-28 11:20 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) Fix number of tests (710 bytes, patch)
2023-09-28 11:21 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global' has_priority rules for checkouts (21.09 KB, patch)
2023-12-11 13:05 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global' has_priority rules for checkouts (20.60 KB, patch)
2024-02-23 09:18 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global' has_priority rules for checkouts (20.59 KB, patch)
2024-05-28 09:59 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global' has_priority rules for checkouts (20.74 KB, patch)
2024-06-20 09:31 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) fix search params on get_effective_rule (1.04 KB, patch)
2024-06-26 06:22 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global' has_priority rules for checkouts (21.62 KB, patch)
2024-07-17 14:43 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global' has_priority rules for checkouts (33.80 KB, patch)
2024-07-26 12:19 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global' has_priority rules for checkouts (34.42 KB, patch)
2024-07-26 13:17 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global' has_priority rules for checkouts (33.83 KB, patch)
2024-07-26 14:26 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global checkout limits' table (19.94 KB, patch)
2024-08-01 08:43 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global checkout limits' table (19.94 KB, patch)
2024-08-01 09:45 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global checkout limits' table (19.99 KB, patch)
2024-08-15 18:21 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8137: Add 'global checkout limits' table (20.06 KB, patch)
2024-10-29 09:34 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global checkout limits' table (20.07 KB, patch)
2024-10-29 09:59 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: Add 'global checkout limits' table (21.21 KB, patch)
2024-12-20 13:11 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) remove forbidden pattern and fix atomic file permission (1.41 KB, patch)
2024-12-20 13:11 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review
Bug 8137: (follow-up) fix behavior with edit and clear actions (10.34 KB, patch)
2025-02-03 08:55 UTC, Thibaud Guillot (thibaud_g)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-05-22 16:39:18 UTC
We have a patron category Temporary Adult WTMP. Under "Checkout limit by patron
category for all libraries" the limit is set to 5. This limit is not working. I
was able to check out 7 items to Robert Harper (24388200094358)in the WTMP
category

Here is what is going on. We have the system preference CircControl set to "Use the checkout and fines rules of *the library where the item is from*". we also have this limit set to 5. So what Koha is doing, is comparing the limit to the number of checkouts of items the borrower has *from the same home library as that item*.

If the item's home library is LibA, and the borrower has 4 items checked out whose home library is LibA, than the item will be checked out ( regardless of how many items are checked out ). If the borrower has 5 items checked out whose home library is LibA, Koha will indeed warn of too many checkouts.

This does not seem intuitive. Is there any way to globally limit the number if checkouts a borrower can have by patron category?
Comment 1 Kyle M Hall 2012-05-22 16:48:18 UTC
Just for reference, the relevant code is located in C4::Circulation::TooMany, right near the end of the subroutine.
Comment 2 Kyle M Hall 2012-05-22 17:18:04 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-05-22 17:19:34 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2012-07-23 12:06:52 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2012-08-03 14:54:22 UTC
QA Comment:

This patch is highly sensible, because it changes Circulation behaviour.
It replaces:
SELECT COUNT(*) FROM issues
  JOIN items USING (itemnumber)
  WHERE borrowernumber = ?
  AND issues.branchcode = ? (or  AND items.homebranch = ? or nothing, depending on sysprefs)

by a simple
SELECT COUNT(*) FROM issues
  WHERE borrowernumber = ?

Digging into the code, I see that the comparison is made against GetBranchBorrowerCircRule, which retrieve data from branch_borrower_circ_rules

Looking at admin/smart-rules.pl, I see that the description says:
=========
Checkout limit by patron category for <<LIBRARY NAME>>

For this library, you can specify the maximum number of loans that a patron of a given category can make, regardless of the item type.

If the total amount loanable for a given patron category is left blank, no limit applies, except possibly for a limit you define for a specific item type. 
=========

I feel it's unclear. What does "For this library" mean exactly ? is it "for items checked out from this library" "For patrons registered at this library" ?

The sub GetBranchBorrowerCircRule is called with $branch and $cat_borrower parameters
$branch is filled depending on sysprefs by _GetCircControlBranch, that return a value depending on CircControl syspref

the patch, if I understand it well, understand the smart-rule.pl description as: "For patrons registered to this library, you can specify the maimum number of loans, all branches included"
So it discard the behaviour: "You can specify the maximum number of loans that a patron can do from this library"

As a conclusion, I feel that this patch could unplease some libraries. Maybe more explanations are needed on smart-rules.pl.

Is there something I misunderstood ? (Am I clear in my explanations -because it's quite tricky- ?)
Comment 6 Jonathan Druart 2012-09-14 14:03:22 UTC
I change the status to in discussion
Comment 7 Koha Team University Lyon 3 2012-09-24 13:59:31 UTC
The Circcontrol had to take into account the table default_borrower_circ_rules.
This table is filled with values added in smart-rules.pl when selected libraries is "all libraries".

I'm not sure that we have to check circcontrol here cause the limit is true for all the libraries so you don't matter if it's the library of patrons or items.

In Circulation.pm, we can read :
"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.
This will first check for a specific branch and category match from branch_borrower_circ_rules. If no rule is found, it will then check default_branch_circ_rules (same branch, default category).  If no rule is found, it will then check default_borrower_circ_rules (default branch, same category), then failing that, default_circ_rules default branch, default category)."

The problem is in "if no rule is found". I think the level defined for all libraries must win on the level for each library.
Comment 8 Paul Poulain 2013-01-16 15:59:42 UTC
Kyle, ping !!!

You haven't answered my last-august question, and this patch seems lost in limbos. Could you answer and try to have it exiting from "in discussion" ?

thx

(In reply to comment #5)
> QA Comment:
> 
> This patch is highly sensible, because it changes Circulation behaviour.
> It replaces:
> SELECT COUNT(*) FROM issues
>   JOIN items USING (itemnumber)
>   WHERE borrowernumber = ?
>   AND issues.branchcode = ? (or  AND items.homebranch = ? or nothing,
> depending on sysprefs)
> 
> by a simple
> SELECT COUNT(*) FROM issues
>   WHERE borrowernumber = ?
> 
> Digging into the code, I see that the comparison is made against
> GetBranchBorrowerCircRule, which retrieve data from
> branch_borrower_circ_rules
> 
> Looking at admin/smart-rules.pl, I see that the description says:
> =========
> Checkout limit by patron category for <<LIBRARY NAME>>
> 
> For this library, you can specify the maximum number of loans that a patron
> of a given category can make, regardless of the item type.
> 
> If the total amount loanable for a given patron category is left blank, no
> limit applies, except possibly for a limit you define for a specific item
> type. 
> =========
> 
> I feel it's unclear. What does "For this library" mean exactly ? is it "for
> items checked out from this library" "For patrons registered at this
> library" ?
> 
> The sub GetBranchBorrowerCircRule is called with $branch and $cat_borrower
> parameters
> $branch is filled depending on sysprefs by _GetCircControlBranch, that
> return a value depending on CircControl syspref
> 
> the patch, if I understand it well, understand the smart-rule.pl description
> as: "For patrons registered to this library, you can specify the maimum
> number of loans, all branches included"
> So it discard the behaviour: "You can specify the maximum number of loans
> that a patron can do from this library"
> 
> As a conclusion, I feel that this patch could unplease some libraries. Maybe
> more explanations are needed on smart-rules.pl.
> 
> Is there something I misunderstood ? (Am I clear in my explanations -because
> it's quite tricky- ?)
Comment 9 Kyle M Hall (khall) 2013-01-16 18:18:26 UTC
Sorry! The answer is "For patrons registered at this library".

> You haven't answered my last-august question, and this patch seems lost in
> limbos. Could you answer and try to have it exiting from "in discussion" ?
Comment 10 Katrin Fischer 2014-10-09 22:12:16 UTC
Hi Kyle, I know this is quite an old patch, but as it is in a sensitive area, could we add a hint on the circulation rules page to make it more clear and have some regression tests please?
Comment 11 Kyle M Hall (khall) 2016-08-16 16:54:11 UTC
Comment on attachment 11079 [details] [review]
[SIGNED-OFF] Bug 8137 - Checkout limit by patron category for all libraries

This patch is so out of date this bug will require a new patch to be written. Obsoleting it and resetting the bug status.
Comment 12 Alex Arnaud 2017-06-28 13:56:32 UTC Comment hidden (obsolete)
Comment 13 Alex Arnaud 2017-06-28 14:00:02 UTC
Created attachment 64704 [details] [review]
Bug 8137 - Add Default checkout limit by patron category with priority.

This patch add the ability to make a 'Default checkout limit by patron category'
rule has priority. When a rule has priority, it overrides all others
for the same categorycode. For "normal" rule, the behavior is the same
than before (like a default rule if no one is specified at branch level).
Rule with priority can only be created on the circulation and fine rules
page for *all libraries*. The goal is to allow librarian to limit the
total number of checkouts by patron category for all libraries.

test plan:

  - Create a rule with 4 checkouts allowed in default checkout limit
    table
    for a patron category and make it has priority,
  - create more permissive rules for all other branches (at least 5
    checkouts allowed),
  - Checkout items of different branches for a patron with the same
    category,
  - Patron should be limited to 4 checkouts
Comment 14 Alex Arnaud 2017-06-28 14:01:26 UTC
Here's my proposal for this bug. Please read the description and test plan in commit message.
Comment 15 Alex Arnaud 2017-07-04 08:58:09 UTC Comment hidden (obsolete)
Comment 16 Laurence Rault 2017-07-06 10:20:47 UTC
tested on sandbox

I can't checkout a book

DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'me.has_priority' in 'field list' at /home/koha/src/Koha/Objects.pm line 191
Comment 17 Alex Arnaud 2017-07-07 08:32:55 UTC
Created attachment 64891 [details] [review]
Bug 8137 - Add default issuing rule for all branches.

This patch does the same that for 'Default checkout limit by patron
category'.

test plan:

  - Create an issuing rule a patron category and itemtype. make it
    has priority,
  - create more permissive issuing rules for all other branches,
  - check that the priority rule is taken into account for this
    category
    and itemtype.
Comment 18 Katrin Fischer 2017-08-08 13:07:56 UTC
Reading through this bug, I am slightly confused, but not sure if this is the right way to go about this.

1) You change the default OK to be category, itemtype, branch and has priority. Why is this necesssary? I think this means that it would be possible to have the same rule for the same branch, with the only difference being the 'has_priority' flag. But I don't feel like this should be possible from the interface as this would lead to conflicting rules.

2) As this will only apply to the max checkouts (I think?), I wonder if adding another column to the table is the right way to go. It's not clear from the interface, what the consequence of setting the checkbox will be.

3) I might not totally understand how this is all supposed to work - could you write up a more detailed testing plan with more examples?
Comment 19 Koha Team University Lyon 3 2018-02-26 12:28:46 UTC
Hello, 
Is somebody still working on this one ?
Sonia
Comment 20 Mark Tompsett 2018-02-26 13:14:30 UTC
Comment on attachment 64891 [details] [review]
Bug 8137 - Add default issuing rule for all branches.

Review of attachment 64891 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Koha/IssuingRules.t
@@ +50,5 @@
> +        ok(Koha::IssuingRule->new({
> +            branchcode => $branchcode,
> +            categorycode => $categorycode,
> +            itemtype => $itemtype,
> +        })->store, "Given I added an issuing rule branchcode => $branchcode,'

The wording probably should avoid personal pronouns like "I added". "Issuing rule for ... added as expected." is probably better.
Comment 21 Mark Tompsett 2018-02-26 13:15:24 UTC
Comment on attachment 64704 [details] [review]
Bug 8137 - Add Default checkout limit by patron category with priority.

Review of attachment 64704 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
@@ +548,5 @@
>             no limit applies, except possibly for a limit you define for a specific item type.
>          </p>
> +        <p>
> +            <b>Has priority: </b>If checked, the rule will override those for all branches. Else
> +            it behaves like a default one: used if no rule existe for the coresponding branch.

exists, corresponding. -- typos.

@@ +559,4 @@
>                      <th>Patron category</th>
>                      <th>Total current checkouts allowed</th>
>                      <th>Total current on-site checkouts allowed</th>
> +                    [% UNLESS humanbranch %]<th>Has priority</th>[% END %]

humanbranch?! I haven't read the code, but this seems strange to read.

::: t/db_dependent/Circulation/Branch.t
@@ +201,5 @@
> +    (categorycode, maxissueqty, maxonsiteissueqty, has_priority)
> +    VALUES( ?, ?, ?, ? )
> +|;
> +$dbh->do($query, {}, $samplecat->{categorycode}, 3, 3, 0);
> +

Why is this change so far away from the other?
Comment 22 Alex Arnaud 2018-03-01 13:49:05 UTC
(In reply to M. Tompsett from comment #20)
> Why is this change so far away from the other?
Which other?
Comment 23 Victor Grousset/tuxayo 2018-03-12 11:04:40 UTC
Conflicts:

~/d/koha ❯❯❯ git bz apply 8137
Bug 8137 - Checkout limit by patron category for all libraries

64704 - Bug 8137 - Add Default checkout limit by patron category with priority.
64891 - Bug 8137 - Add default issuing rule for all branches.

Apply? [(y)es, (n)o, (i)nteractive] y
Application de  Bug 8137 - Add Default checkout limit by patron category with priority.
.git/rebase-apply/patch:20: trailing whitespace.
                $count_query .= " JOIN  biblioitems USING (biblionumber) 
warning: 1 ligne a ajouté des erreurs d'espace.
Utilisation de l'information de l'index pour reconstruire un arbre de base...
M	C4/Circulation.pm
M	admin/smart-rules.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
M	t/db_dependent/Circulation/Branch.t
.git/rebase-apply/patch:20: trailing whitespace.
                $count_query .= " JOIN  biblioitems USING (biblionumber) 
warning: 1 ligne a ajouté des erreurs d'espace.
Retour à un patch de la base et fusion à 3 points...
Fusion automatique de t/db_dependent/Circulation/Branch.t
Fusion automatique de koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Fusion automatique de admin/smart-rules.pl
CONFLIT (contenu) : Conflit de fusion dans admin/smart-rules.pl
Fusion automatique de C4/Circulation.pm
error: Échec d'intégration des modifications.
le patch a échoué à 0001 Bug 8137 - Add Default checkout limit by patron category with priority.
La copie du patch qui a échoué se trouve dans : .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-8137---Add-Default-checkout-limit-by-patron-ca-NJzNl6.patch
Comment 24 Alex Arnaud 2018-03-14 09:19:06 UTC
Created attachment 72831 [details] [review]
Bug 8137 - Add Default checkout limit by patron category with priority.

This patch add the ability to make a 'Default checkout limit by patron category'
rule has priority. When a rule has priority, it overrides all others
for the same categorycode. For "normal" rule, the behavior is the same
than before (like a default rule if no one is specified at branch level).
Rule with priority can only be created on the circulation and fine rules
page for *all libraries*. The goal is to allow librarian to limit the
total number of checkouts by patron category for all libraries.

test plan:

  - Create a rule with 4 checkouts allowed in default checkout limit
    table
    for a patron category and make it has priority,
  - create more permissive rules for all other branches (at least 5
    checkouts allowed),
  - Checkout items of different branches for a patron with the same
    category,
  - Patron should be limited to 4 checkouts

https://bugs.koha-community.org/show_bug.cgi?id=8137

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 25 Alex Arnaud 2018-03-14 09:19:19 UTC
Created attachment 72832 [details] [review]
Bug 8137 - Add default issuing rule for all branches.

This patch does the same that for 'Default checkout limit by patron
category'.

test plan:

  - Create an issuing rule a patron category and itemtype. make it
    has priority,
  - create more permissive issuing rules for all other branches,
  - check that the priority rule is taken into account for this
    category
    and itemtype.

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 26 Lucie Gay 2018-03-14 11:28:30 UTC
the priority rule is applied except for the loan period, it's the one define in the more permissive rule which is taken into account.
Comment 27 Fridolin Somers 2018-05-09 13:51:36 UTC
Needs a patch to update DBIx Schema no ?
Comment 28 Alex Arnaud 2018-06-19 12:19:29 UTC
(In reply to Lucie Gay from comment #26)
> the priority rule is applied except for the loan period, it's the one define
> in the more permissive rule which is taken into account.

Hello Lucie,

What do you mean by "except for the loan period" ?
Comment 29 Lucie Gay 2018-11-13 12:04:27 UTC
If I remember correctly, it was because the number of loans was correct but not the loan period, it was not the one defined in the priority rule.
But it was a long time ago and maybe someone should try again
Comment 30 Martin Renvoize (ashimema) 2019-03-12 11:02:32 UTC
Are you still hoping to move this one forward Alex.. trying to ascertain the current state.
Comment 31 Alex Arnaud 2019-04-15 07:27:10 UTC
(In reply to Martin Renvoize from comment #30)
> Are you still hoping to move this one forward Alex.. trying to ascertain the
> current state.

Hello Martin,

I can't reproduce the issue (and don't know how to reproduce it) Lucie is talking about. I would like some others points of view here.
Comment 32 Victor Grousset/tuxayo 2019-09-20 13:25:01 UTC
Conflicts mainly with bug 12365 which added a notes column. There are some column offsets in the code that are a bit tricky to fix.
Comment 33 Victor Grousset/tuxayo 2019-09-20 13:58:18 UTC
By the way, shouldn't these offsets depend whether the column has_priority is present?
Comment 34 Kyle M Hall (khall) 2020-02-05 11:40:16 UTC
Alex, it looks like this patch set will need to be updated to move the new rule into circulation_rules. Are you planning on doing that?
Comment 35 Kyle M Hall (khall) 2020-03-11 11:09:31 UTC
Alex, are you planning on rebasing this patch?
Comment 36 Arthur Suzuki 2020-07-21 08:34:54 UTC
Hi Kyle,
I'll try the rebase.
Arthur
Comment 37 Kyle M Hall (khall) 2020-07-21 13:22:21 UTC
Thanks Arthur!

(In reply to Arthur Suzuki from comment #36)
> Hi Kyle,
> I'll try the rebase.
> Arthur
Comment 38 Arthur Suzuki 2020-07-23 00:35:49 UTC
Created attachment 107207 [details] [review]
Bug 8137: Add Default checkout limit by patron category with priority.

This patch add the ability to make a 'Default checkout limit by patron category'
rule has priority. When a rule has priority, it overrides all others
for the same categorycode. For "normal" rule, the behavior is the same
than before (like a default rule if no one is specified at branch level).
Rule with priority can only be created on the circulation and fine rules
page for *all libraries*. The goal is to allow librarian to limit the
total number of checkouts by patron category for all libraries.

test plan:

  - Create a rule with 4 checkouts allowed in default checkout limit
    table
    for a patron category and make it has priority,
  - create more permissive rules for all other branches (at least 5
    checkouts allowed),
  - Checkout items of different branches for a patron with the same
    category,
  - Patron should be limited to 4 checkouts

https://bugs.koha-community.org/show_bug.cgi?id=8137

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Rebased: 2020-07-23, by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 39 Arthur Suzuki 2020-07-23 00:36:08 UTC
Created attachment 107208 [details] [review]
Bug 8137: Add default issuing rule for all branches.

This patch does the same that for 'Default checkout limit by patron
category'.

test plan:

  - Create an issuing rule a patron category and itemtype. make it
    has priority,
  - create more permissive issuing rules for all other branches,
  - check that the priority rule is taken into account for this
    category
    and itemtype.

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Rebased: 2020-07-34, by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 40 Arthur Suzuki 2020-07-23 00:36:13 UTC
Created attachment 107209 [details] [review]
Bug 8137: (follow-up)
Comment 41 Arthur Suzuki 2020-07-23 00:38:45 UTC
Oh well, I did my best but i'm not so sure it does work as expected.
Hope it will though!
Comment 42 Sally 2020-07-27 14:23:14 UTC
Sorry, patch doesn't apply.  

{UNKNOWN}: DBIx::Class::ResultSource::column_info(): No such column has_priority at /kohadevbox/koha/Koha/Schema/Result/CirculationRule.pm line 72
Compilation failed in require at /usr/share/perl5/Class/C3/Componentised.pm line 150. at /usr/share/perl5/Class/C3/Componentised.pm line 155
Compilation failed in require at /kohadevbox/koha/Koha/Database.pm line 50.
Comment 43 Arthur Suzuki 2020-07-28 09:23:10 UTC
Hi Sally,
There is a modification of the database.
Maybe your error would be solved by running this script?
./installer/data/mysql/updatedatabase.pl

Kr,
Arthur
Comment 44 Arthur Suzuki 2020-07-28 09:46:07 UTC
Created attachment 107459 [details] [review]
Bug 8137: (follow-up)
Comment 45 Arthur Suzuki 2020-07-28 09:47:33 UTC
There was an error on the atomic update.
This is solved now.
Kr,
Arthur
Comment 46 Andrew Fuerste-Henry 2020-08-04 14:25:37 UTC
(In reply to Lucie Gay from comment #26)
> the priority rule is applied except for the loan period, it's the one define
> in the more permissive rule which is taken into account.

This is still valid. To recreate:
- create an All Libraries rule for a specific patron category / item type that allows 1 checkout for 5 days. Mark this rule as Has Priority
- create a library specific rule for same patron category / item type that allows 2 checkouts for 1 day
- perform a checkout that matches the patron category / item type your wrote rules for. Confirm it checks out for 1 day (the loan period for the library-specific rule)
- attempt a second checkout that matches the patron category / item type your wrote rules for. Confirm the checkout is blocked because it exceeds the checkout limit of the All Libraries rule
Comment 47 Jonathan Druart 2020-08-04 15:23:41 UTC
Just noting: I think some code is going to conflict with bug 21946 (PQA) and bug 26132 (which I'd like to push along with 21946)
Comment 48 Arthur Suzuki 2020-10-08 08:33:19 UTC
Hi Joubu,
The 2 bugs you mention has been pushed to master :)
I'm trying to rebase on top of them.
Arthur
Comment 49 Arthur Suzuki 2020-10-08 09:00:37 UTC
rebase ok, I'm working on the remarks made by andrew
Comment 50 Arthur Suzuki 2020-10-08 09:33:05 UTC
Hi Andrew,
I can't reproduce the issue you mentionned using Koha-testing-docker.
Anyone willing to test is very welcome.
Will be uploading new versions of the patches in a minute.
Kr,
Arthur
Comment 51 Arthur Suzuki 2020-10-08 09:50:44 UTC
Created attachment 111368 [details] [review]
Bug 8137: Add Default checkout limit by patron category with priority.

This patch add the ability to make a 'Default checkout limit by patron category'
rule has priority. When a rule has priority, it overrides all others
for the same categorycode. For "normal" rule, the behavior is the same
than before (like a default rule if no one is specified at branch level).
Rule with priority can only be created on the circulation and fine rules
page for *all libraries*. The goal is to allow librarian to limit the
total number of checkouts by patron category for all libraries.

test plan:

  - Create a rule with 4 checkouts allowed in default checkout limit
    table
    for a patron category and make it has priority,
  - create more permissive rules for all other branches (at least 5
    checkouts allowed),
  - Checkout items of different branches for a patron with the same
    category,
  - Patron should be limited to 4 checkouts

https://bugs.koha-community.org/show_bug.cgi?id=8137

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Rebased: 2020-07-23, by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 52 Arthur Suzuki 2020-10-08 09:51:04 UTC
Created attachment 111369 [details] [review]
Bug 8137: Add default issuing rule for all branches.

This patch does the same that for 'Default checkout limit by patron
category'.

test plan:

  - Create an issuing rule a patron category and itemtype. make it
    has priority,
  - create more permissive issuing rules for all other branches,
  - check that the priority rule is taken into account for this
    category
    and itemtype.

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Rebased: 2020-07-34, by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 53 Arthur Suzuki 2020-10-08 09:51:13 UTC
Created attachment 111370 [details] [review]
Bug 8137: (follow-up)
Comment 54 Arthur Suzuki 2020-10-08 10:01:09 UTC
Can't see any signoff marks on this patch, actual state is probably "needs signoff"...
raising attention, "please" with a cherry on top :)
Arthur
Comment 55 Lisette Scheer 2020-10-23 23:49:13 UTC
Applying: Bug 8137: Add Default checkout limit by patron category with priority.
Applying: Bug 8137: Add default issuing rule for all branches.
error: sha1 information is lacking or useless (admin/smart-rules.pl).
error: could not build fake ancestor
Patch failed at 0001 Bug 8137: Add default issuing rule for all branches.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-8137-Add-default-issuing-rule-for-all-branches-zg277r.patch
Comment 56 Arthur Suzuki 2020-10-28 13:33:39 UTC
Created attachment 112640 [details] [review]
Bug 8137: (follow-up)
Comment 57 Arthur Suzuki 2020-10-28 13:36:28 UTC
Hi Lisette,
I've rebased the last patch which seemed to create some issues while trying to apply the patch.
Can you try to apply the patch again after refreshing master? (now working for me).
Kind regards,
Arthur for BibLibre's Support
Comment 58 Andrew Fuerste-Henry 2020-11-03 17:49:36 UTC
I applied your patches, updated the database, and restarted services.
I then edited my All/All rule for all libraries and saved it as Has Priority.
Then I copied my All/All rule for all libraries to one of my branches.
After doing that, I am unable to save changes to any of my circ rules. I cannot save edits my branch rules or my all libraries rules. When I click Save, the values revert back to their original values and I get this error in the plack log:

[2020/11/03 17:19:17] [WARN] DBD::mysql::st execute failed: Unknown column 'me.has_priority' in 'field list' [for Statement "SELECT `me`.`id`, `me`.`branchcode`, `me`.`categorycode`, `me`.`itemtype`, `me`.`has_priority`, `me`.`rule_name`, `me`.`rule_value` FROM `circulation_rules` `me` WHERE ( ( `me`.`branchcode` IS NULL AND `me`.`rule_name` = ? ) )" with ParamValues: 0='refund'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
Comment 59 Arthur Suzuki 2020-11-17 16:51:32 UTC
Hi Andrew,
Thanks for the extended testing.
I've reproduced the issue, will work on this and send a new version of the patch asap
Comment 60 Arthur Suzuki 2020-11-18 13:18:27 UTC
almost there, still a few tests failing in t/db_dependant/Circulation.t
Expect a followup patch tomorrow.
Comment 61 Arthur Suzuki 2020-11-19 08:12:46 UTC
Created attachment 113797 [details] [review]
Bug 8137: (follow-up)
Comment 62 Jonathan Druart 2020-11-19 09:10:32 UTC
(In reply to Arthur Suzuki from comment #61)
> Created attachment 113797 [details] [review] [review]
> Bug 8137: (follow-up)

"(follow-up)" is not a valid commit message. Please have a look at the wiki page - https://wiki.koha-community.org/wiki/Commit_messages
Comment 63 Arthur Suzuki 2020-11-20 13:47:29 UTC
Created attachment 113882 [details] [review]
Bug 8137: (follow-up) Fix error on the rules cloning functionality
Comment 64 Andrew Fuerste-Henry 2020-12-18 17:01:22 UTC
Created attachment 114531 [details] [review]
Bug 8137: Add Default checkout limit by patron category with priority.

This patch add the ability to make a 'Default checkout limit by patron category'
rule has priority. When a rule has priority, it overrides all others
for the same categorycode. For "normal" rule, the behavior is the same
than before (like a default rule if no one is specified at branch level).
Rule with priority can only be created on the circulation and fine rules
page for *all libraries*. The goal is to allow librarian to limit the
total number of checkouts by patron category for all libraries.

test plan:

  - Create a rule with 4 checkouts allowed in default checkout limit
    table
    for a patron category and make it has priority,
  - create more permissive rules for all other branches (at least 5
    checkouts allowed),
  - Checkout items of different branches for a patron with the same
    category,
  - Patron should be limited to 4 checkouts

https://bugs.koha-community.org/show_bug.cgi?id=8137

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Rebased: 2020-07-23, by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 65 Andrew Fuerste-Henry 2020-12-18 17:01:28 UTC
Created attachment 114532 [details] [review]
Bug 8137: Add default issuing rule for all branches.

This patch does the same that for 'Default checkout limit by patron
category'.

test plan:

  - Create an issuing rule a patron category and itemtype. make it
    has priority,
  - create more permissive issuing rules for all other branches,
  - check that the priority rule is taken into account for this
    category
    and itemtype.

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Rebased: 2020-07-34, by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 66 Andrew Fuerste-Henry 2020-12-18 17:01:33 UTC
Created attachment 114533 [details] [review]
Bug 8137: (follow-up)

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 67 Andrew Fuerste-Henry 2020-12-18 17:01:38 UTC
Created attachment 114534 [details] [review]
Bug 8137: (follow-up) Fix error on the rules cloning functionality

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 68 Martha Fuerst 2021-02-10 14:42:25 UTC
Being a squeaky wheel so this gets some attention from QA.

Thanks!

-Marti Fuerst
Comment 69 Kyle M Hall (khall) 2021-02-10 15:40:35 UTC
Created attachment 116676 [details] [review]
Bug 8137: Add Default checkout limit by patron category with priority.

This patch add the ability to make a 'Default checkout limit by patron category'
rule has priority. When a rule has priority, it overrides all others
for the same categorycode. For "normal" rule, the behavior is the same
than before (like a default rule if no one is specified at branch level).
Rule with priority can only be created on the circulation and fine rules
page for *all libraries*. The goal is to allow librarian to limit the
total number of checkouts by patron category for all libraries.

test plan:

  - Create a rule with 4 checkouts allowed in default checkout limit
    table
    for a patron category and make it has priority,
  - create more permissive rules for all other branches (at least 5
    checkouts allowed),
  - Checkout items of different branches for a patron with the same
    category,
  - Patron should be limited to 4 checkouts

https://bugs.koha-community.org/show_bug.cgi?id=8137

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Rebased: 2020-07-23, by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Kyle M Hall (khall) 2021-02-10 15:40:51 UTC
Created attachment 116677 [details] [review]
Bug 8137: Add default issuing rule for all branches.

This patch does the same that for 'Default checkout limit by patron
category'.

test plan:

  - Create an issuing rule a patron category and itemtype. make it
    has priority,
  - create more permissive issuing rules for all other branches,
  - check that the priority rule is taken into account for this
    category
    and itemtype.

Rebased: 2018-03-14, by: Alex Arnaud <alex.arnaud@biblibre.com>
Rebased: 2020-07-34, by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Kyle M Hall (khall) 2021-02-10 15:40:56 UTC
Created attachment 116678 [details] [review]
Bug 8137: (follow-up)

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Kyle M Hall (khall) 2021-02-10 15:41:01 UTC
Created attachment 116679 [details] [review]
Bug 8137: (follow-up) Fix error on the rules cloning functionality

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Kyle M Hall (khall) 2021-02-10 15:41:06 UTC
Created attachment 116680 [details] [review]
Bug 8137: (QA follow-up) Fix number of tests
Comment 74 Jonathan Druart 2021-02-12 12:02:27 UTC
1.
+sub priority {
+    my ($self) = @_;
+
+    return $self->has_priority;
+}

It does not seem very useful, especially when it's only used once, from test.

2.
+ALTER TABLE circulation_rules ADD KEY (branchcode , categorycode, itemtype, has_priority);

Why?
Note that it's missing from kohastructure.sql

3. There is a shift when you edit the line, the "unlimited" (stupid) trick

The followin line (smart-rules.tt):
1169                         } else if ( i == 5 || i == 6 || i == 26 || i == 27 || i == 28 ) {

4. I think we need a hint on the UI (tooltip on the "Has priority" header?) to explain what this column is about.

5. Reading the code I don't understand why the first query in get_effective_rule is not passing the item type.

211     $search_params->{rule_name} = $rule_name;
212     $search_params->{categorycode} = defined $categorycode ? [ $categorycode, undef ] : undef;
213     $search_params->{has_priority} = 1;

What will happen if
Cat 1 | BK | has priority | max_holds = 5
Cat 1 | VID | has priority | max_holds = 2

It feels like you are going to get a random value.
Comment 75 Jonathan Druart 2021-02-12 12:03:35 UTC
The bug report title and commit message must be updated to describe what we are doing here. It's not only impacting "checkouts", but all rules!
Comment 76 Jonathan Druart 2021-02-12 12:05:41 UTC
And, last question (hopefully), why don't we have the "has priority" only for "Default checkout, hold policy by patron category", if it is what we really wanted to fix?
Comment 77 Kyle M Hall (khall) 2021-03-23 14:48:49 UTC
Alex, what do you think of 75 and comment 76?

(In reply to Jonathan Druart from comment #75)
> The bug report title and commit message must be updated to describe what we
> are doing here. It's not only impacting "checkouts", but all rules!
Comment 78 Joonas Kylmälä 2021-05-21 09:36:52 UTC
I have an alternative solution (the solution b) proposed in bug 27834 for this problem. I think we should go with it because it fixes also the bug in the syspref description being incorrect and don't have to add new parameter to the rules at all.
Comment 79 Thibaud Guillot (thibaud_g) 2023-01-19 15:17:26 UTC Comment hidden (obsolete)
Comment 80 Thibaud Guillot (thibaud_g) 2023-01-19 15:17:36 UTC Comment hidden (obsolete)
Comment 81 Thibaud Guillot (thibaud_g) 2023-01-19 15:17:44 UTC Comment hidden (obsolete)
Comment 82 Thibaud Guillot (thibaud_g) 2023-01-19 15:17:54 UTC Comment hidden (obsolete)
Comment 83 Thibaud Guillot (thibaud_g) 2023-01-19 15:18:04 UTC Comment hidden (obsolete)
Comment 84 Andrii Nugged 2023-01-25 06:28:26 UTC
on which "master"/hash should this apply without errors?
I can't apply for the current master (fc650b3e871e1d558ca1ff9f0dcb2f1fa0ad5db2). (Or did I miss some obvious thing :shrug:?)
Comment 85 Thibaud Guillot (thibaud_g) 2023-06-06 09:26:07 UTC Comment hidden (obsolete)
Comment 86 Thibaud Guillot (thibaud_g) 2023-06-06 09:26:13 UTC Comment hidden (obsolete)
Comment 87 Thibaud Guillot (thibaud_g) 2023-06-06 09:26:19 UTC Comment hidden (obsolete)
Comment 88 Thibaud Guillot (thibaud_g) 2023-06-06 09:26:25 UTC Comment hidden (obsolete)
Comment 89 Thibaud Guillot (thibaud_g) 2023-06-06 09:26:30 UTC Comment hidden (obsolete)
Comment 90 Thibaud Guillot (thibaud_g) 2023-06-06 09:26:36 UTC Comment hidden (obsolete)
Comment 91 Jonathan Druart 2023-06-06 10:08:05 UTC
Comments 74-76 and 78 are waiting for an answer.
Comment 92 Katrin Fischer 2023-07-29 13:44:50 UTC
*** Bug 8371 has been marked as a duplicate of this bug. ***
Comment 93 Teresa Rice 2023-08-17 20:18:21 UTC
We are experiencing this issue as well. We have a check out limit set to 40. Some of our card types are not permitted to check out that many items. However, if they check out items from our different locations they can exceed the check out limit without a system warning. We would like an admin feature where we can group all our locations as a system wide. 

Our ILS used to be TLC and this was an issue there as well until a few years ago they applied a setting the counted all locations as one and did not let a patron type that is only able to check out 10 items to exceed that rule. It was actually handy with item types. Because we had set it so that patrons could only check out 5 DVDs, but they could check out more than 5 if they were from different home locations. After the fix, the system would let the staff know if the check out limit was exceeded regardless of the home location of the item.
Comment 94 Thibaud Guillot (thibaud_g) 2023-09-28 11:15:37 UTC Comment hidden (obsolete)
Comment 95 Thibaud Guillot (thibaud_g) 2023-09-28 11:17:27 UTC Comment hidden (obsolete)
Comment 96 Thibaud Guillot (thibaud_g) 2023-09-28 11:20:08 UTC Comment hidden (obsolete)
Comment 97 Thibaud Guillot (thibaud_g) 2023-09-28 11:20:23 UTC Comment hidden (obsolete)
Comment 98 Thibaud Guillot (thibaud_g) 2023-09-28 11:20:36 UTC Comment hidden (obsolete)
Comment 99 Thibaud Guillot (thibaud_g) 2023-09-28 11:20:52 UTC Comment hidden (obsolete)
Comment 100 Thibaud Guillot (thibaud_g) 2023-09-28 11:21:09 UTC Comment hidden (obsolete)
Comment 101 Thibaud Guillot (thibaud_g) 2023-12-11 13:05:29 UTC Comment hidden (obsolete)
Comment 102 Thibaud Guillot (thibaud_g) 2023-12-11 13:06:37 UTC Comment hidden (obsolete)
Comment 103 Lisette Scheer 2024-02-22 18:55:13 UTC
Template process failed: undef error - The method Koha::CirculationRule->has_priority is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 964

Once I try selecting the priority box and saving, my smart-rules.pl completely breaks.
Comment 104 Thibaud Guillot (thibaud_g) 2024-02-23 09:18:28 UTC
Created attachment 162367 [details] [review]
Bug 8137: Add 'global' has_priority rules for checkouts

I tried to review the desired behavior through this bug.
The goal would be to be able to have a general 'override' rule.
Currently, if you set a max_checkouts_allowed of 5 (regardless of itemtype) in one library, for example, and do the same in another, it adds up to 10.

By giving checkout rules priority, you "overwrite" other local rules per branchcode.

Test plan:

1) Set for library "A"  a max_checkouts_allowed to 3 for any type of user
   and item type.
2) On main rules set the same rule to 2
3) Do a few checkouts with a user belonging to library A, normally you'll be able to do 3.
4) Apply this patch and run updatedatabase.pl and also rebuild your
   schema by updating dbix class files
5) See now the Has priority checkboxes on "Default checkout, hold and return policy" and "Default checkout, hold policy by patron category"
6) You can set here you new rule for "Total current checkouts allowed"
   to 2
7) Repeat step 3 and normally you won't be able to make the third

Sponsored-by: BibLibre
Comment 105 Thibaud Guillot (thibaud_g) 2024-02-23 09:21:07 UTC
(In reply to Lisette Scheer from comment #103)
> Template process failed: undef error - The method
> Koha::CirculationRule->has_priority is not covered by tests!
> 
> Trace begun at /kohadevbox/koha/Koha/Object.pm line 964
> 
> Once I try selecting the priority box and saving, my smart-rules.pl
> completely breaks.

Hi Lisette,

Thanks for testing and feedback :) 

So did you forget to update your schema ? I apply another time and its work great for me.

I rebased on master and I took the opportunity to indent some lines.
Comment 106 Lisette Scheer 2024-03-07 23:29:21 UTC
Applying: Bug 8137: Add 'global' has_priority rules for checkouts
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
M	admin/smart-rules.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
.git/rebase-apply/patch:315: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging admin/smart-rules.pl
Auto-merging C4/Circulation.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 8137: Add 'global' has_priority rules for checkouts
Comment 107 Lisette Scheer 2024-05-21 21:57:03 UTC
I went to test again and patch no longer applies.
Comment 108 Thibaud Guillot (thibaud_g) 2024-05-28 09:59:13 UTC
Created attachment 167208 [details] [review]
Bug 8137: Add 'global' has_priority rules for checkouts

I tried to review the desired behavior through this bug.
The goal would be to be able to have a general 'override' rule.
Currently, if you set a max_checkouts_allowed of 5 (regardless of itemtype) in one library, for example, and do the same in another, it adds up to 10.

By giving checkout rules priority, you "overwrite" other local rules per branchcode.

Test plan:

1) Set for library "A"  a max_checkouts_allowed to 3 for any type of user
   and item type.
2) On main rules set the same rule to 2
3) Do a few checkouts with a user belonging to library A, normally you'll be able to do 3.
4) Apply this patch and run updatedatabase.pl and also rebuild your
   schema by updating dbix class files
5) See now the Has priority checkboxes on "Default checkout, hold and return policy" and "Default checkout, hold policy by patron category"
6) You can set here you new rule for "Total current checkouts allowed"
   to 2
7) Repeat step 3 and normally you won't be able to make the third

Sponsored-by: BibLibre
Comment 109 Brendan Lawlor 2024-05-28 13:51:27 UTC
(In reply to Thibaud Guillot from comment #108)
> 4) Apply this patch and run updatedatabase.pl and also rebuild your
>    schema by updating dbix class files

I tried testing this patch but had trouble with this part.

Testing in ktd, I applied the patch and ran these commands

updatedatabase
DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/Bug_8137-add-column-has_priority.pl  [13:35:41]: Bug  8137 - Add column has_priority
        Added column 'has_priority

dbic
... lots of output...
Dumping manual schema for Koha::Schema to directory ./ ...
Schema dump completed

Then restart_all

But when I tried to check the has priority box and save I got this error:

DBIx::Class::Row::store_column(): No such column 'has_priority' on Koha::Schema::Result::CirculationRule at /kohadevbox/koha/Koha/Object.pm line 81
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

I tried clearing my browser cache but got the same error. Am I missing a step?

I'd be happy to test this again because it's an important bug fix for our libraries.
Comment 110 David Nind 2024-05-29 04:42:40 UTC
I've tested as well, and get the same error.

Changing status to Failed QA.
Comment 111 Thibaud Guillot (thibaud_g) 2024-06-20 09:31:32 UTC
Created attachment 167923 [details] [review]
Bug 8137: Add 'global' has_priority rules for checkouts

I tried to review the desired behavior through this bug.
The goal would be to be able to have a general 'override' rule.
Currently, if you set a max_checkouts_allowed of 5 (regardless of itemtype) in one library, for example, and do the same in another, it adds up to 10.

By giving checkout rules priority, you "overwrite" other local rules per branchcode.

Test plan:

1) Set for library "A"  a max_checkouts_allowed to 3 for any type of user
   and item type.
2) On main rules set the same rule to 2
3) Do a few checkouts with a user belonging to library A, normally you'll be able to do 3.
4) Apply this patch and run updatedatabase.pl and also rebuild your
   schema by updating dbix class files
5) See now the Has priority checkboxes on "Default checkout, hold and return policy" and "Default checkout, hold policy by patron category"
6) You can set here you new rule for "Total current checkouts allowed"
   to 2
7) Repeat step 3 and normally you won't be able to make the third

Sponsored-by: BibLibre
Comment 112 Thibaud Guillot (thibaud_g) 2024-06-26 06:22:26 UTC
Created attachment 168119 [details] [review]
Bug 8137: (follow-up) fix search params on get_effective_rule
Comment 113 David Nind 2024-07-04 20:06:15 UTC
Thanks Thibaud!

I'm still getting this error for steps 5 and 6:

 DBIx::Class::Row::store_column(): No such column 'has_priority' on Koha::Schema::Result::CirculationRule at /kohadevbox/koha/Koha/Object.pm line 81
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

I thought that it may be because when running dbic, it ends with this:

Dumping manual schema for Koha::Schema to directory ./ ...
DBIx::Class::Schema::Loader::make_schema_at(): Checksum mismatch in './/Koha/Schema/Result/CourseItem.pm', the auto-generated part of the file has been modified outside of this loader.  Aborting.
If you want to overwrite these modifications, set the 'overwrite_modifications' loader option.

However, I get that as well running dbic without the patch. So maybe an issue with KTD?

I also tried running dbic --force - I no longer got the error at the end when running dbic, however the error for steps 5 and 6 still occurs. 


Testing notes so far (using KTD):

1. Clone the rules that apply to all libraries to Centerville.
2. Edit the Centerville All All rule to change both "Current checkouts allowed" and "Current on-site checkouts allowed" to 3.
3. For the "Standard rule for all libraries", change both "Current checkouts allowed" and "Current on-site checkouts allowed" to 2.
4. With the library set to Centerville, check out three items to Mary. Try to check out a fourth item - you get a message:
   Please confirm checkout
   Too many checked out. 3 checked out, only 3 are allowed.
   Maximum checkouts calculated from the circulation rule for
      Item type: All item types
      Patron category: All patron categories
      Library: Centerville
   Yes, check out (Y)
   No, don't check out (N)
4. Change the library to Midway and try to check out another item - you get this message:
   Please confirm checkout
   Too many checked out. 3 checked out, only 2 are allowed.
   Maximum checkouts calculated from the circulation rule for
      Item type: All item types
      Patron category: All patron categories
      Library: All libraries
   Yes, check out (Y)
   No, don't check out (N)
5. Apply the patch
6. Update the database: updatedatabase
7. Run dbic: dbic
8. Clear the cache: flush_memcached
9. Restart everything: restart_all
10. Clear your browser cache.
Comment 114 Thibaud Guillot (thibaud_g) 2024-07-17 14:43:23 UTC
Created attachment 169084 [details] [review]
Bug 8137: Add 'global' has_priority rules for checkouts

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) - Set for library "A" a max checkouts allowed to 3 for any
   categorycode on "Default checkout, hold and return policy"
   - Set for library "B" a max checkouts allowed to 2 for any
   categorycode on "Default checkout, hold and return policy"
2) Currently you can accumulate on each branch to get 5 checkouts in
   this test.
3) Apply this patch
4) Run updatedatabase script and rebuild schema, restart_all..
5) Now you have a "Priority checkouts rule" checkboxes on "Default
   checkouts[..]" table and the same by categorycode below (only on
"Standard rules for all librairies" section).
6) If checked on save, these rules will override global max checkouts
   allowed even if you try to check out from different librairy.
7) According to this test plan, if you set a "global" max checkouts to 2
   you can only perform 2 checkouts and not more if you try on another
lib. Try on the both tables (by defaults and by defaults/categorycode).

Sponsored-by: BibLibre
Comment 115 Thibaud Guillot (thibaud_g) 2024-07-26 12:19:46 UTC
Created attachment 169696 [details] [review]
Bug 8137: Add 'global' has_priority rules for checkouts

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) - Set for library "A" a max checkouts allowed to 3 for any
   categorycode on "Default checkout, hold and return policy"
   - Set for library "B" a max checkouts allowed to 2 for any
   categorycode on "Default checkout, hold and return policy"
2) Currently you can accumulate on each branch to get 5 checkouts in
   this test.
3) Apply this patch
4) Run updatedatabase script and rebuild schema, restart_all..
5) Now you have a "Priority checkouts rule" checkboxes on the main
   circulation rules table, "Default
   checkouts[..]" table and the same  by categorycode below (only on
"Standard rules for all librairies" select option).
6) If checked on save, these rules will override global max checkouts
   allowed even if you try to check out from different librairy.
7) According to this test plan, if you set a "global" max checkouts to 2
   you can only perform 2 checkouts and not more if you try on another
lib. Try it on main circulation rules table and 2 defaults...

Sponsored-by: BibLibre
Comment 116 Thibaud Guillot (thibaud_g) 2024-07-26 13:17:53 UTC
Created attachment 169700 [details] [review]
Bug 8137: Add 'global' has_priority rules for checkouts

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) - Set for library "A" a max checkouts allowed to 3 for any
   categorycode on "Default checkout, hold and return policy"
   - Set for library "B" a max checkouts allowed to 2 for any
   categorycode on "Default checkout, hold and return policy"
2) Currently you can accumulate on each branch to get 5 checkouts in
   this test.
3) Apply this patch
4) Run updatedatabase script and rebuild schema, restart_all..
5) Now you have a "Priority checkouts rule" checkboxes on the main
   circulation rules table, "Default
   checkouts[..]" table and the same  by categorycode below (only on
"Standard rules for all librairies" select option).
6) If checked on save, these rules will override global max checkouts
   allowed even if you try to check out from different librairy.
7) According to this test plan, if you set a "global" max checkouts to 2
   you can only perform 2 checkouts and not more if you try on another
lib. Try it on main circulation rules table and 2 defaults...

Sponsored-by: BibLibre
Comment 117 Thibaud Guillot (thibaud_g) 2024-07-26 14:26:42 UTC
Created attachment 169727 [details] [review]
Bug 8137: Add 'global' has_priority rules for checkouts

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) - Set for library "A" a max checkouts allowed to 3 for any
   categorycode on "Default checkout, hold and return policy"
   - Set for library "B" a max checkouts allowed to 2 for any
   categorycode on "Default checkout, hold and return policy"
2) Currently you can accumulate on each branch to get 5 checkouts in
   this test.
3) Apply this patch
4) Run updatedatabase script and rebuild schema, restart_all..
5) Now you have a "Priority checkouts rule" checkboxes on the main
   circulation rules table, "Default
   checkouts[..]" table and the same  by categorycode below (only on
"Standard rules for all librairies" select option).
6) If checked on save, these rules will override global max checkouts
   allowed even if you try to check out from different librairy.
7) According to this test plan, if you set a "global" max checkouts to 2
   you can only perform 2 checkouts and not more if you try on another
lib. Try it on main circulation rules table and 2 defaults...

Sponsored-by: BibLibre
Comment 118 Nick Clemens (kidclamp) 2024-07-26 16:37:36 UTC
I have reservations about adding a new dimension to the circulation rules matrix with the "has priority" option. I almost feel it should be its own section, only on the "Standard rules for all libraries" page - and call "Global checkout limits" if that is what the intended outcome is.

I won't necessarily block on the concept, but there are some key things I think need to be addressed if this is going to move forward:
1 - The priority rules should not be determined in "TooMany" - but in 'get_effective_rule'. The name of that routine makes it clear, it should be the routine with all the calculations, and should be the single way of determining the correct rule to apply
2 - 'Default checkout,hold and return policy' has three hold options that are not affected by 'has_priority' and this needs to be visually clear to the user
3 - The rules from this page must be shown on the individual libraries rules page - otherwise they can setup options, but not understand that they are being overridden by a special set of rules
Comment 119 Katrin Fischer 2024-07-29 15:29:25 UTC
I agree with Nick about it being a separate thing would be better and apply to more use cases.

I think the "all libraries" is a bit confusing as it seems to move into a mix of "fallback when nothing is set" and "global total over all libraries". Making sure we know which is what is important here.
Comment 120 Thibaud Guillot (thibaud_g) 2024-08-01 08:43:38 UTC
Created attachment 169927 [details] [review]
Bug 8137: Add 'global checkout limits' table

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) Apply this patch
2) Run updatedatabase script and rebuild schema, restart_all..
3) On circulation rules page you will find a new table "Global checkout
   limits".
4) If you enter values, they will overwrite the existing values,
   creating limits for all branches combined.
5) Test it by set a "current checkouts allowed" to 2 on "standard rules section"
   and set a "global" maxissueqty to 1.
   Normally 1 is the new limit for now.
6) You can also add a "current checkouts allowed" on other branches and
   see that you can't accumulate checkouts (you can also delete "global"
   values and see that if you have 3 on a branch and 2 on other, you can
   perform 5 checkouts in total.
7) Note that on specific branch circulation rules page the table cannot
   be modified, but just shown as a reminder. If main matrix is empty on
specific branch the table is hidden by default (cause if none values
it's referred to standard rules).

Sponsored-by: BibLibre
Comment 121 Thibaud Guillot (thibaud_g) 2024-08-01 08:52:11 UTC
Hello 

Thank you for your very interesting feedback.. Indeed, integrating it directly with get_effective_rule makes it possible to lighten the logic and not touch the TooMany function in the way I did.. 
So I decided to draw up a new table in the "standard rules" section. He's going to define maxissueqty and maxonsiteissueqty as "priority". 
These rules take over others less specific I think (but maybe I'm wrong) that there is no need to define patron_maxissueqty and patron_maxonsiteissueqty in this case.

The table is displayed as a reminder on the specific pages of each library but hidden if no rules from this library are defined in the main matrix, and displayed but not editable otherwise.

I have repeated this dev several times, normally I have not left too much trace of the previous patches but if there is an error of logic forgive me I have had my head upside down in the circulation rules lol.

I know that tests should be added but I already prefer to give you an overview of the rendering if we go on this way.

Thank you for your feedback to everyone!
Comment 122 Thibaud Guillot (thibaud_g) 2024-08-01 09:45:14 UTC
Created attachment 169928 [details] [review]
Bug 8137: Add 'global checkout limits' table

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) Apply this patch
2) Run updatedatabase script and rebuild schema, restart_all..
3) On circulation rules page you will find a new table "Global checkout
   limits".
4) If you enter values, they will overwrite the existing values,
   creating limits for all branches combined.
5) Test it by set a "current checkouts allowed" to 2 on "standard rules section"
   and set a "global" maxissueqty to 1.
   Normally 1 is the new limit for now.
6) You can also add a "current checkouts allowed" on other branches and
   see that you can't accumulate checkouts (you can also delete "global"
   values and see that if you have 3 on a branch and 2 on other, you can
   perform 5 checkouts in total.
7) Note that on specific branch circulation rules page the table cannot
   be modified, but just shown as a reminder. If main matrix is empty on
specific branch the table is hidden by default (cause if none values
it's referred to standard rules).

Sponsored-by: BibLibre
Comment 123 ByWater Sandboxes 2024-08-15 18:21:39 UTC
Created attachment 170411 [details] [review]
Bug 8137: Add 'global checkout limits' table

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) Apply this patch
2) Run updatedatabase script and rebuild schema, restart_all..
3) On circulation rules page you will find a new table "Global checkout
   limits".
4) If you enter values, they will overwrite the existing values,
   creating limits for all branches combined.
5) Test it by set a "current checkouts allowed" to 2 on "standard rules section"
   and set a "global" maxissueqty to 1.
   Normally 1 is the new limit for now.
6) You can also add a "current checkouts allowed" on other branches and
   see that you can't accumulate checkouts (you can also delete "global"
   values and see that if you have 3 on a branch and 2 on other, you can
   perform 5 checkouts in total.
7) Note that on specific branch circulation rules page the table cannot
   be modified, but just shown as a reminder. If main matrix is empty on
specific branch the table is hidden by default (cause if none values
it's referred to standard rules).

Sponsored-by: BibLibre
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 124 Lisette Scheer 2024-08-15 18:22:24 UTC
Worked great!
Comment 125 Fridolin Somers 2024-10-28 10:30:59 UTC
Ah small difference between atomic update and kohastructure.sql :

+        $dbh->do('ALTER TABLE circulation_rules ADD IF NOT EXISTS has_priority INT(1) DEFAULT 0');

+++ b/installer/data/mysql/kohastructure.sql
@@ -1863,6 +1863,7 @@ CREATE TABLE `circulation_rules` (
   `itemtype` varchar(10) DEFAULT NULL,
   `rule_name` varchar(32) NOT NULL,
   `rule_value` varchar(32) NOT NULL,
+  `has_priority` tinyint(4) DEFAULT NULL,

Its a boolean so looks like atomic update is correct and kohastructure.sql not
Comment 126 Fridolin Somers 2024-10-28 10:42:11 UTC
And Atomic should use :

if ( !column_exists( 'circulation_rules', 'has_priority' ) ) {

To tell "Added column circulation_rules.has_priority" otherwise say nothing.
Like many other column added.
Comment 127 Katrin Fischer 2024-10-28 10:44:15 UTC
(In reply to Fridolin Somers from comment #126)
> And Atomic should use :
> 
> if ( !column_exists( 'circulation_rules', 'has_priority' ) ) {
> 
> To tell "Added column circulation_rules.has_priority" otherwise say nothing.
> Like many other column added.

Failed QA?
Comment 128 Fridolin Somers 2024-10-28 11:19:32 UTC
Not needed, Thibaud is on it
Comment 129 Fridolin Somers 2024-10-28 15:21:00 UTC
(In reply to Fridolin Somers from comment #125)
> Ah small difference between atomic update and kohastructure.sql :
> 
> +        $dbh->do('ALTER TABLE circulation_rules ADD IF NOT EXISTS
> has_priority INT(1) DEFAULT 0');
> 
> +++ b/installer/data/mysql/kohastructure.sql
> @@ -1863,6 +1863,7 @@ CREATE TABLE `circulation_rules` (
>    `itemtype` varchar(10) DEFAULT NULL,
>    `rule_name` varchar(32) NOT NULL,
>    `rule_value` varchar(32) NOT NULL,
> +  `has_priority` tinyint(4) DEFAULT NULL,
> 
> Its a boolean so looks like atomic update is correct and kohastructure.sql
> not

Nope, kohastructure.sql is correct for DEFAULT NULL so :
tinyint(1) DEFAULT NULL
Comment 130 Thibaud Guillot (thibaud_g) 2024-10-29 09:34:45 UTC
Created attachment 173605 [details] [review]
Bug 8137: Add 'global checkout limits' table

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) Apply this patch
2) Run updatedatabase script and rebuild schema, restart_all..
3) On circulation rules page you will find a new table "Global checkout
   limits".
4) If you enter values, they will overwrite the existing values,
   creating limits for all branches combined.
5) Test it by set a "current checkouts allowed" to 2 on "standard rules section"
   and set a "global" maxissueqty to 1.
   Normally 1 is the new limit for now.
6) You can also add a "current checkouts allowed" on other branches and
   see that you can't accumulate checkouts (you can also delete "global"
   values and see that if you have 3 on a branch and 2 on other, you can
   perform 5 checkouts in total.
7) Note that on specific branch circulation rules page the table cannot
   be modified, but just shown as a reminder. If main matrix is empty on
specific branch the table is hidden by default (cause if none values
it's referred to standard rules).

Sponsored-by: BibLibre
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 131 Thibaud Guillot (thibaud_g) 2024-10-29 09:59:31 UTC
Created attachment 173608 [details] [review]
Bug 8137: Add 'global checkout limits' table

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) Apply this patch
2) Run updatedatabase script and rebuild schema, restart_all..
3) On circulation rules page you will find a new table "Global checkout
   limits".
4) If you enter values, they will overwrite the existing values,
   creating limits for all branches combined.
5) Test it by set a "current checkouts allowed" to 2 on "standard rules section"
   and set a "global" maxissueqty to 1.
   Normally 1 is the new limit for now.
6) You can also add a "current checkouts allowed" on other branches and
   see that you can't accumulate checkouts (you can also delete "global"
   values and see that if you have 3 on a branch and 2 on other, you can
   perform 5 checkouts in total.
7) Note that on specific branch circulation rules page the table cannot
   be modified, but just shown as a reminder. If main matrix is empty on
specific branch the table is hidden by default (cause if none values
it's referred to standard rules).

Sponsored-by: BibLibre
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 132 Brendan Lawlor 2024-12-13 20:41:39 UTC
Thanks for working on this Thibaud! A global checkout limit is important for our consortium.

I am happy to test this again when this error is resolved.

The patch applies, but after running updatedatabase and dbic I get an error when running restart_all:

Starting Z39.50/SRU daemon for kohadev:"my" variable $self masks earlier declaration in same scope at /kohadevbox/koha/Koha/CirculationRules.pm line 324.
"my" variable $params masks earlier declaration in same scope at /kohadevbox/koha/Koha/CirculationRules.pm line 324.
"state" variable @_ masks earlier declaration in same scope at /kohadevbox/koha/Koha/CirculationRules.pm line 324.

Also the QA script complains on a few things:

 FAIL   Koha/CirculationRules.pm
   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   FAIL   valid
                "my" variable $self masks earlier declaration in same scope
                "my" variable $params masks earlier declaration in same scope
                "state" variable @_ masks earlier declaration in same scope
                "my" variable $rule_name masks earlier declaration in same scope
                "my" variable $categorycode masks earlier declaration in same scope
                "my" variable $itemtype masks earlier declaration in same scope
                "my" variable $branchcode masks earlier declaration in same scope
                "my" variable $rule masks earlier declaration in same scope
                "my" variable $self masks earlier declaration in same scope
                "my" variable $params masks earlier declaration in same scope
                "state" variable @_ masks earlier declaration in same scope
                "my" variable $categorycode masks earlier declaration in same scope
                "my" variable $itemtype masks earlier declaration in same scope
                "my" variable $branchcode masks earlier declaration in same scope
                "my" variable $has_priority masks earlier declaration in same scope
                "my" variable $self masks earlier declaration in same scope
                "my" variable $params masks earlier declaration in same scope
                "state" variable @_ masks earlier declaration in same scope
                "my" variable $branchcode masks earlier declaration in same scope
                "my" variable $categorycode masks earlier declaration in same scope
                "my" variable $itemtype masks earlier declaration in same scope
                "my" variable $rule_name masks earlier declaration in same scope
                "my" variable $has_priority masks earlier declaration in same scope
                "my" variable $rule masks earlier declaration in same scope
                "my" variable $memory_cache masks earlier declaration in same scope
                syntax error
                Can't use global @_ in "my"
                Global symbol "$params" requires explicit package name (did you forget to declare "my $params"?)
                syntax error
                Can't use global @_ in "my"
                syntax error
                Can't use global @_ in "my"
                syntax error
                Can't use global @_ in "my"
                syntax error
                /kohadevbox/koha/Koha/CirculationRules.pm has too many errors.
                Compilation failed in require

 WARN   Koha/Schema/Result/CirculationRule.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 41, now: 43)

 FAIL   Koha/Template/Plugin/CirculationRules.pm
   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   FAIL   valid
                "my" variable $self masks earlier declaration in same scope
                "my" variable $params masks earlier declaration in same scope
                ...

 FAIL   admin/smart-rules.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 137, now: 154)
   FAIL   valid
                "my" variable $self masks earlier declaration in same scope
                "my" variable $params masks earlier declaration in same scope
                "state" variable @_ masks earlier declaration in same scope
                ...

 FAIL   installer/data/mysql/atomicupdate/Bug_8137-add-column-has_priority.pl
   FAIL   file permissions
                File must have the exec flag

 FAIL   installer/data/mysql/kohastructure.sql
   FAIL   tinyint_has_boolean_flag
                The new column circulation_rules.has_priority is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/CirculationRule.pm), see the SQL12 coding guideline

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 1835)
                forbidden pattern: tab char (line 1837)
   FAIL   spelling
                 librairies ==> libraries
                 librairies ==> libraries
Comment 133 Mathieu Saby 2024-12-19 10:28:58 UTC
Hi
We would be interested by testing too. Thibauld do you plan to provide a fixed patch in next months ?
Comment 134 Thibaud Guillot (thibaud_g) 2024-12-20 13:11:31 UTC
Created attachment 175837 [details] [review]
Bug 8137: Add 'global checkout limits' table

The aim here is to avoid the accumulation of checkouts possibilities through the rules of the various branches, in order to have a global quota.

Test plan:

1) Apply this patch
2) Run updatedatabase script and rebuild schema, restart_all..
3) On circulation rules page you will find a new table "Global checkout
   limits".
4) If you enter values, they will overwrite the existing values,
   creating limits for all branches combined.
5) Test it by set a "current checkouts allowed" to 2 on "standard rules section"
   and set a "global" maxissueqty to 1.
   Normally 1 is the new limit for now.
6) You can also add a "current checkouts allowed" on other branches and
   see that you can't accumulate checkouts (you can also delete "global"
   values and see that if you have 3 on a branch and 2 on other, you can
   perform 5 checkouts in total.
7) Note that on specific branch circulation rules page the table cannot
   be modified, but just shown as a reminder. If main matrix is empty on
specific branch the table is hidden by default (cause if none values
it's referred to standard rules).

Sponsored-by: BibLibre
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 135 Thibaud Guillot (thibaud_g) 2024-12-20 13:11:37 UTC
Created attachment 175838 [details] [review]
Bug 8137: (follow-up) remove forbidden pattern and fix atomic file permission
Comment 136 Thibaud Guillot (thibaud_g) 2024-12-20 13:13:19 UTC
(In reply to Brendan Lawlor from comment #132)
> Thanks for working on this Thibaud! A global checkout limit is important for
> our consortium.
> 
> I am happy to test this again when this error is resolved.
> 
> The patch applies, but after running updatedatabase and dbic I get an error
> when running restart_all:
> 
> Starting Z39.50/SRU daemon for kohadev:"my" variable $self masks earlier
> declaration in same scope at /kohadevbox/koha/Koha/CirculationRules.pm line
> 324.
> "my" variable $params masks earlier declaration in same scope at
> /kohadevbox/koha/Koha/CirculationRules.pm line 324.
> "state" variable @_ masks earlier declaration in same scope at
> /kohadevbox/koha/Koha/CirculationRules.pm line 324.
> 
> Also the QA script complains on a few things:
> 
>  FAIL   Koha/CirculationRules.pm
>    FAIL   pod coverage
>                 POD coverage was greater before, try perl
> -MPod::Coverage=PackageName -e666
>    FAIL   valid
>                 "my" variable $self masks earlier declaration in same scope
>                 "my" variable $params masks earlier declaration in same scope
>                 "state" variable @_ masks earlier declaration in same scope
>                 "my" variable $rule_name masks earlier declaration in same
> scope
>                 "my" variable $categorycode masks earlier declaration in
> same scope
>                 "my" variable $itemtype masks earlier declaration in same
> scope
>                 "my" variable $branchcode masks earlier declaration in same
> scope
>                 "my" variable $rule masks earlier declaration in same scope
>                 "my" variable $self masks earlier declaration in same scope
>                 "my" variable $params masks earlier declaration in same scope
>                 "state" variable @_ masks earlier declaration in same scope
>                 "my" variable $categorycode masks earlier declaration in
> same scope
>                 "my" variable $itemtype masks earlier declaration in same
> scope
>                 "my" variable $branchcode masks earlier declaration in same
> scope
>                 "my" variable $has_priority masks earlier declaration in
> same scope
>                 "my" variable $self masks earlier declaration in same scope
>                 "my" variable $params masks earlier declaration in same scope
>                 "state" variable @_ masks earlier declaration in same scope
>                 "my" variable $branchcode masks earlier declaration in same
> scope
>                 "my" variable $categorycode masks earlier declaration in
> same scope
>                 "my" variable $itemtype masks earlier declaration in same
> scope
>                 "my" variable $rule_name masks earlier declaration in same
> scope
>                 "my" variable $has_priority masks earlier declaration in
> same scope
>                 "my" variable $rule masks earlier declaration in same scope
>                 "my" variable $memory_cache masks earlier declaration in
> same scope
>                 syntax error
>                 Can't use global @_ in "my"
>                 Global symbol "$params" requires explicit package name (did
> you forget to declare "my $params"?)
>                 syntax error
>                 Can't use global @_ in "my"
>                 syntax error
>                 Can't use global @_ in "my"
>                 syntax error
>                 Can't use global @_ in "my"
>                 syntax error
>                 /kohadevbox/koha/Koha/CirculationRules.pm has too many
> errors.
>                 Compilation failed in require
> 
>  WARN   Koha/Schema/Result/CirculationRule.pm
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 41, now: 43)
> 
>  FAIL   Koha/Template/Plugin/CirculationRules.pm
>    FAIL   pod coverage
>                 POD coverage was greater before, try perl
> -MPod::Coverage=PackageName -e666
>    FAIL   valid
>                 "my" variable $self masks earlier declaration in same scope
>                 "my" variable $params masks earlier declaration in same scope
>                 ...
> 
>  FAIL   admin/smart-rules.pl
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 137, now: 154)
>    FAIL   valid
>                 "my" variable $self masks earlier declaration in same scope
>                 "my" variable $params masks earlier declaration in same scope
>                 "state" variable @_ masks earlier declaration in same scope
>                 ...
> 
>  FAIL   installer/data/mysql/atomicupdate/Bug_8137-add-column-has_priority.pl
>    FAIL   file permissions
>                 File must have the exec flag
> 
>  FAIL   installer/data/mysql/kohastructure.sql
>    FAIL   tinyint_has_boolean_flag
>                 The new column circulation_rules.has_priority is using
> TINYINT(1) as type but is not defined as boolean in the schema file
> (Koha/Schema/Result/CirculationRule.pm), see the SQL12 coding guideline
> 
>  FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
>    FAIL   forbidden patterns
>                 forbidden pattern: tab char (line 1835)
>                 forbidden pattern: tab char (line 1837)
>    FAIL   spelling
>                  librairies ==> libraries
>                  librairies ==> libraries

Hello Brendan and thanks for your feedback, I miss some bracket :/ during the last rebase. It's fixed ! And I also add a follow up for QA fixes
Comment 137 Thibaud Guillot (thibaud_g) 2024-12-20 13:15:01 UTC
(In reply to Mathieu Saby from comment #133)
> Hi
> We would be interested by testing too. Thibauld do you plan to provide a
> fixed patch in next months ?

Hello Mathieu,

I'm glad to read about this patch. I look forward to reading your feedback.
Comment 138 Thibaud Guillot (thibaud_g) 2025-02-03 08:55:38 UTC
Created attachment 177464 [details] [review]
Bug 8137: (follow-up) fix behavior with edit and clear actions