Description
Kyle M Hall
2012-05-22 16:39:18 UTC
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 |