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?
Just for reference, the relevant code is located in C4::Circulation::TooMany, right near the end of the subroutine.
Created attachment 9705 [details] [review] Bug 8137 - Checkout limit by patron category for all libraries 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 a patron in the WTMP category. 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. I think circcontrol is being mis-used here. It should use the maxissueqty for the item's home library ( or fallback to the 'all libraries' rule ), but it should still count all issues from any library
Created attachment 9706 [details] [review] Bug 8137 - Checkout limit by patron category for all libraries 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 a patron in the WTMP category. 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. I think circcontrol is being mis-used here. It should use the maxissueqty for the item's home library ( or fallback to the 'all libraries' rule ), but it should still count all issues from any library
Created attachment 11079 [details] [review] [SIGNED-OFF] Bug 8137 - Checkout limit by patron category for all libraries 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 a patron in the WTMP category. 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. I think circcontrol is being mis-used here. It should use the maxissueqty for the item's home library ( or fallback to the 'all libraries' rule ), but it should still count all issues from any library Signed-off-by: Marc Veron <veron@veron.ch> How I teested: Set patrons category restriction to 2 checkouts. Set checkout restrictions for libraries to 10 checkouts. Without patch, I could checkout more items than allowed by patrons category restriction (but only if items from different libraries were involved). With patch, the patrons category restriction was respected (warning: 'Too many checked out. 2 checked out, only 2 are allowed.'). Patch behaves as described.expected.
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- ?)
I change the status to in discussion
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.
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- ?)
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" ?
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 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.
Created attachment 64703 [details] [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 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
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
Here's my proposal for this bug. Please read the description and test plan in commit message.
Created attachment 64791 [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.
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
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.
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?
Hello, Is somebody still working on this one ? Sonia
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 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?
(In reply to M. Tompsett from comment #20) > Why is this change so far away from the other? Which other?
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
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>
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>
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.
Needs a patch to update DBIx Schema no ?
(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" ?
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
Are you still hoping to move this one forward Alex.. trying to ascertain the current state.
(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.
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.
By the way, shouldn't these offsets depend whether the column has_priority is present?
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?
Alex, are you planning on rebasing this patch?
Hi Kyle, I'll try the rebase. Arthur
Thanks Arthur! (In reply to Arthur Suzuki from comment #36) > Hi Kyle, > I'll try the rebase. > Arthur
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>
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>
Created attachment 107209 [details] [review] Bug 8137: (follow-up)
Oh well, I did my best but i'm not so sure it does work as expected. Hope it will though!
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.
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
Created attachment 107459 [details] [review] Bug 8137: (follow-up)
There was an error on the atomic update. This is solved now. Kr, Arthur
(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
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)
Hi Joubu, The 2 bugs you mention has been pushed to master :) I'm trying to rebase on top of them. Arthur
rebase ok, I'm working on the remarks made by andrew
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
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>
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>
Created attachment 111370 [details] [review] Bug 8137: (follow-up)
Can't see any signoff marks on this patch, actual state is probably "needs signoff"... raising attention, "please" with a cherry on top :) Arthur
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
Created attachment 112640 [details] [review] Bug 8137: (follow-up)
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
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.
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
almost there, still a few tests failing in t/db_dependant/Circulation.t Expect a followup patch tomorrow.
Created attachment 113797 [details] [review] Bug 8137: (follow-up)
(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
Created attachment 113882 [details] [review] Bug 8137: (follow-up) Fix error on the rules cloning functionality
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>
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>
Created attachment 114533 [details] [review] Bug 8137: (follow-up) Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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>
Being a squeaky wheel so this gets some attention from QA. Thanks! -Marti Fuerst
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>
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>
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>
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>
Created attachment 116680 [details] [review] Bug 8137: (QA follow-up) Fix number of tests
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.
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!
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?
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!
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.
Created attachment 145476 [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>
Created attachment 145477 [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>
Created attachment 145478 [details] [review] Bug 8137: (follow-up) Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145479 [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>
Created attachment 145480 [details] [review] Bug 8137: (follow-up) Rebased on master Made some adjustments to make the patch work, I tested it and apprently it works as expected but I'm interested in your feedback otherwise
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:?)
Created attachment 152040 [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>
Created attachment 152041 [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>
Created attachment 152042 [details] [review] Bug 8137: (follow-up) Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 152043 [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>
Created attachment 152044 [details] [review] Bug 8137: (follow-up) Rebased on master Made some adjustments to make the patch work, I tested it and apprently it works as expected but I'm interested in your feedback otherwise
Created attachment 152045 [details] [review] Bug 8137: (follow-up) Fix number of tests
Comments 74-76 and 78 are waiting for an answer.
*** Bug 8371 has been marked as a duplicate of this bug. ***
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.
Created attachment 156316 [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>
Created attachment 156317 [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>
Created attachment 156318 [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>
Created attachment 156319 [details] [review] Bug 8137: (follow-up)
Created attachment 156320 [details] [review] Bug 8137: (follow-up) Fix error on the rules cloning functionality
Created attachment 156321 [details] [review] Bug 8137: Rebased on master
Created attachment 156322 [details] [review] Bug 8137: (follow-up) Fix number of tests
Created attachment 159712 [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
During my test, in some use cases I got a "brake foreign key error" without really knowing why, so this is a reworking of the bug in order to bring a different solution and get your feedback so we can move it forward.
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.
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
(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.
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
I went to test again and patch no longer applies.
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
(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.
I've tested as well, and get the same error. Changing status to Failed QA.
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
Created attachment 168119 [details] [review] Bug 8137: (follow-up) fix search params on get_effective_rule
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.
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
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
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
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
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
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.
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
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!
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
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>
Worked great!
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
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.
(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?
Not needed, Thibaud is on it
(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
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>
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>
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
Hi We would be interested by testing too. Thibauld do you plan to provide a fixed patch in next months ?
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>
Created attachment 175838 [details] [review] Bug 8137: (follow-up) remove forbidden pattern and fix atomic file permission
(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
(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.
Created attachment 177464 [details] [review] Bug 8137: (follow-up) fix behavior with edit and clear actions