Description
Jesse Weaver
2016-01-07 22:31:05 UTC
We are very interested in this bug. Nothing has happened on this for over a year. Would someone give me a status update on this? Thanks, Rhonda K. Created attachment 64707 [details] [review] Bug 15524 - Set limit on maximum possible holds per patron by category It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category). This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to the circ rules editor, note the new max holds rules by patron category in the "Checkout limit by patron category". ( Should we rename this section? ) 4) Create find a patron that is allowed to place a hold, count the number of holds that patron has. Lets make that number 'X'. 5) Set the new max holds rule to X for "All libraries" 6) Note the patron can no longer place another hold 7) Set the new max holds rule to X + 1 for the patron's home library 8) Note the patron can again place another hold 9) Set the new max holds rule to X for the patron's home library 10) Note the patron can no longer place another hold Created attachment 64708 [details] [review] Bug 15524 - Update Schema 5) Set the new max holds rule to X for "All libraries" Do you mean in the first array ? I don't find any other, but if it is this is not really intuitive Sorry I changed them again after my last test to check icons where displayed (only alternate test can be displayed on the sandbox). However with all system preferences set to default, I can't see the icons. I can't test the Marc21 outside this sandbox, so if it don't work I can't really sign off your patch (In reply to Baptiste from comment #4) > 5) Set the new max holds rule to X for "All libraries" > > Do you mean in the first array ? I don't find any other, but if it is this > is not really intuitive I'm not sure what you mean, it's the last text box in this screenshot: https://monosnap.com/file/K2WEsjcCfRE5LF7TKO9xoYLbavdwjJ In my test I set the hold limit for DVDs to 10, and the "Maximum total holds allowed (count)" for patron type "Juvenile" to 5. I placed holds for a patron with that patron category until the number exceeded 5 and then exceeded 10. Neither hold limit appears to be working. (In reply to Owen Leonard from comment #7) > In my test I set the hold limit for DVDs to 10, and the "Maximum total holds > allowed (count)" for patron type "Juvenile" to 5. > > I placed holds for a patron with that patron category until the number > exceeded 5 and then exceeded 10. Neither hold limit appears to be working. Just tested and everything is working for me! It's especially odd that the existing rule isn't working since I didn't touch that code. Hi Kyle Can you please merge your patches with master branch as the patches attached to this bug report cannot be applied successfully: Using index info to reconstruct a base tree... M C4/Reserves.pm M installer/data/mysql/kohastructure.sql M koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt M t/db_dependent/Holds.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Holds.t CONFLICT (content): Merge conflict in t/db_dependent/Holds.t Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt Auto-merging installer/data/mysql/kohastructure.sql Auto-merging C4/Reserves.pm Failed to merge in the changes. Patch failed at 0001 Bug 15524 - Set limit on maximum possible holds per patron by category The copy of the patch that failed is found in: /home/vagrant/kohaclone/.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-15524---Set-limit-on-maximum-possible-holds-pe-TzOohy.patch Created attachment 67180 [details] [review] Bug 15524 - Set limit on maximum possible holds per patron by category It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category). This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to the circ rules editor, note the new max holds rules by patron category in the "Checkout limit by patron category". ( Should we rename this section? ) 4) Create find a patron that is allowed to place a hold, count the number of holds that patron has. Lets make that number 'X'. 5) Set the new max holds rule to X for "All libraries" 6) Note the patron can no longer place another hold 7) Set the new max holds rule to X + 1 for the patron's home library 8) Note the patron can again place another hold 9) Set the new max holds rule to X for the patron's home library 10) Note the patron can no longer place another hold Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 67181 [details] [review] Bug 15524 - Update Schema Signed-off-by: Josef Moravec <josef.moravec@gmail.com> I just had to fix small conflict in Holds.t (number of tests), so now this is rebased on master and signed off ;) Created attachment 67531 [details] [review] Bug 15524: (follow-up) Rename database update This ensures that this update is applied in the correct order relative to its dependent bugs. Just a rename to a DB update so this bug works better with its dependencies; leaving as signed off. QA: Looking here now QA Comment Interesting feature! Limited testing did not reveal problems. Functionality issue: In Administration, smart-rules, the section "Default checkout limit by patron category" contains the item "Maximum total holds allowed (count)". Not sure if the label should contain total and count btw. But the section should be renamed. This does not pertain any longer to checkout limits only. Furthermore I would expect to have max_holds also under Default checkout, hold and return policy. It seems consistent to add it also without a patron category restriction. Same as current checkouts etc. Hard to move it to FQA for that reason. At least I want to have some additional opinions. Setting to In Discussion. Please ask for feedback via IRC or ML. (In reply to Marcel de Rooy from comment #15) > QA Comment > Interesting feature! Limited testing did not reveal problems. > > Functionality issue: > In Administration, smart-rules, the section "Default checkout limit by > patron category" contains the item "Maximum total holds allowed (count)". I've submitted a patch to improve the section name. > Not sure if the label should contain total and count btw. Removed > But the section should be renamed. This does not pertain any longer to > checkout limits only. Agreed. > Furthermore I would expect to have max_holds also under Default checkout, > hold and return policy. It seems consistent to add it also without a patron > category restriction. That behavior is controlled by the maxreserves system preference. We should move that out of a syspref and into the circ rules. I'd be happy to do that on another bug but it's definitely out of the scope of this bug. Created attachment 70389 [details] [review] Bug 15524 - Set limit on maximum possible holds per patron by category It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category). This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to the circ rules editor, note the new max holds rules by patron category in the "Checkout limit by patron category". ( Should we rename this section? ) 4) Create find a patron that is allowed to place a hold, count the number of holds that patron has. Lets make that number 'X'. 5) Set the new max holds rule to X for "All libraries" 6) Note the patron can no longer place another hold 7) Set the new max holds rule to X + 1 for the patron's home library 8) Note the patron can again place another hold 9) Set the new max holds rule to X for the patron's home library 10) Note the patron can no longer place another hold Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 70390 [details] [review] Bug 15524 - Update Schema Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 70391 [details] [review] Bug 15524: (follow-up) Rename database update This ensures that this update is applied in the correct order relative to its dependent bugs. Created attachment 70392 [details] [review] Bug 15524 [QA Followup] - Change language used (In reply to Marcel de Rooy from comment #15) > QA Comment > Interesting feature! Limited testing did not reveal problems. > > Functionality issue: > In Administration, smart-rules, the section "Default checkout limit by > patron category" contains the item "Maximum total holds allowed (count)". I've submitted a patch to improve the section name. > Not sure if the label should contain total and count btw. Removed > But the section should be renamed. This does not pertain any longer to > checkout limits only. Agreed. > Furthermore I would expect to have max_holds also under Default checkout, > hold and return policy. It seems consistent to add it also without a patron > category restriction. That behavior is controlled by the maxreserves system preference. We should move that out of a syspref and into the circ rules. I'd be happy to do that on another bug but it's definitely out of the scope of this bug. QA: Looking here again FAIL installer/data/mysql/atomicupdate/max_holds.perl FAIL git manipulation The file has been added and deleted in the same patchset Created attachment 71704 [details] [review] Bug 15524 - Set limit on maximum possible holds per patron by category It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category). This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to the circ rules editor, note the new max holds rules by patron category in the "Checkout limit by patron category". ( Should we rename this section? ) 4) Create find a patron that is allowed to place a hold, count the number of holds that patron has. Lets make that number 'X'. 5) Set the new max holds rule to X for "All libraries" 6) Note the patron can no longer place another hold 7) Set the new max holds rule to X + 1 for the patron's home library 8) Note the patron can again place another hold 9) Set the new max holds rule to X for the patron's home library 10) Note the patron can no longer place another hold Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 71705 [details] [review] Bug 15524 - Update Schema Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 71706 [details] [review] Bug 15524 [QA Followup] - Change language used Comment on attachment 70391 [details] [review] Bug 15524: (follow-up) Rename database update No longer needed. (In reply to Marcel de Rooy from comment #23) > FAIL installer/data/mysql/atomicupdate/max_holds.perl > FAIL git manipulation > The file has been added and deleted in the same patchset Fixed Do we need a specific coding guideline to exclude new DBIx constructions like + my $rule = $schema->resultset('BranchBorrowerCircRule')->find( + { + branchcode => $borrower->{branchcode}, + categorycode => $borrower->{categorycode}, + } + ); while we could do the same by creating Koha objects ? We do not have this rule now. Created attachment 71737 [details] [review] Bug 15524 - Set limit on maximum possible holds per patron by category It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category). This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to the circ rules editor, note the new max holds rules by patron category in the "Checkout limit by patron category". ( Should we rename this section? ) 4) Create find a patron that is allowed to place a hold, count the number of holds that patron has. Lets make that number 'X'. 5) Set the new max holds rule to X for "All libraries" 6) Note the patron can no longer place another hold 7) Set the new max holds rule to X + 1 for the patron's home library 8) Note the patron can again place another hold 9) Set the new max holds rule to X for the patron's home library 10) Note the patron can no longer place another hold Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 71738 [details] [review] Bug 15524 - Update Schema Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 71739 [details] [review] Bug 15524 [QA Followup] - Change language used Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 71740 [details] [review] Bug 15524: (QA follow-up) Fix meaningless test The last test claims to allow a hold when branch=5 and patron=5, but look at the preceding statements: $rule_branch->max_holds(5); $rule_branch->update(); $rule_branch->max_holds(5); $rule_branch->insert(); The last insert will not be done, since the record is already present. A create should have triggered an error on the primary key. Obviously, we should use $rule_all. Test plan: Run the test again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 71741 [details] [review] Bug 15524: (QA follow-up) Pass max_holds at the right position in update When updating max_holds should be before the categorycode in the update query. Things that can be found by testing :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 3 remarks while passing QA (apart from 2 follow-ups): [1] With reference to above comments: >> Furthermore I would expect to have max_holds also under Default checkout, >> hold and return policy. It seems consistent to add it also without a patron >> category restriction. > That behavior is controlled by the maxreserves system preference. We should > move that out of a syspref and into the circ rules. I'd be happy to do that > on another bug but it's definitely out of the scope of this bug. Yes, sir. Please remove maxreserves on a new report. Note that you already added code for that by putting max_holds in default_circ_rules. At this moment it is actually not used by the interface. [2] See my earlier comment about introducing new DBIx constructions. No blocker for now, but imo we should add a coding guideline. [3] While staff shows too many reserves to me, opac does not. Simply no items available. This can be considered as outside the scope, since it should have been there already. But fixing that on yet another report would be so nice.. Thanks (In reply to Marcel de Rooy from comment #35) > [1] With reference to above comments: > >> Furthermore I would expect to have max_holds also under Default checkout, > >> hold and return policy. It seems consistent to add it also without a patron > >> category restriction. > > > That behavior is controlled by the maxreserves system preference. We should > > move that out of a syspref and into the circ rules. I'd be happy to do that > > on another bug but it's definitely out of the scope of this bug. > > Yes, sir. Please remove maxreserves on a new report. Note that you already > added code for that by putting max_holds in default_circ_rules. At this > moment it is actually not used by the interface. > > [2] See my earlier comment about introducing new DBIx constructions. No > blocker for now, but imo we should add a coding guideline. About 1 & 2: And I think that is what is wrong here. We should have default_circ_rules.max_holds on the interface, then call GetBranchBorrowerCircRule from CanItemBeReserved It sounds like a trivial change and so I think it should be added by this patch set. (In reply to Jonathan Druart from comment #36) > And I think that is what is wrong here. We should have > default_circ_rules.max_holds on the interface, then call > GetBranchBorrowerCircRule from CanItemBeReserved > It sounds like a trivial change and so I think it should be added by this > patch set. I've been trying to understand your request and I just cannot figure it out. Considering it would be a trivial change, would you be able to submit the patch? Kyle, Go to "Default checkout, hold and return policy", you do not see "Total holds allowed", fill the other fields, save: default_circ_rules.holdallowed is filled. We could add it to the interface and make it configurable, do not you think? Also if we add "max_holds", maybe we should rename "holdallowed" then. After that is done, the code in CanItemBeReserved could be replaced with a call to GetBranchBorrowerCircRule. Without these changes we add more confusion to the code in my opinion. I will not have the time to provide such change before the release. Setting to in discussion to get other QAer opinions on these patches. Feel free to provide a patch if you think my requests make sense to you. (In reply to Jonathan Druart from comment #38) > Kyle, > > Go to "Default checkout, hold and return policy", you do not see "Total > holds allowed", fill the other fields, save: default_circ_rules.holdallowed > is filled. > We could add it to the interface and make it configurable, do not you think? holdallowed doesn't define how many holds may be placed, it defines what items a patron may place holds on. It's already revealed in the rules editor! > > Also if we add "max_holds", maybe we should rename "holdallowed" then. Again, not the same thing. > > After that is done, the code in CanItemBeReserved could be replaced with a > call to GetBranchBorrowerCircRule. > > Without these changes we add more confusion to the code in my opinion. > > I will not have the time to provide such change before the release. > > Setting to in discussion to get other QAer opinions on these patches. > > Feel free to provide a patch if you think my requests make sense to you. I think your comment is based on a fundamental misunderstanding of what holdallowed does. If I'm wrong let me know! We could definitely add max_holds to the default_circ_rules table, but that should be done on another bug as it is out of scope for this one. Back to this. (In reply to Kyle M Hall from comment #39) > (In reply to Jonathan Druart from comment #38) > > Kyle, > > > > Go to "Default checkout, hold and return policy", you do not see "Total > > holds allowed", fill the other fields, save: default_circ_rules.holdallowed > > is filled. > > We could add it to the interface and make it configurable, do not you think? > > holdallowed doesn't define how many holds may be placed, it defines what > items a patron may place holds on. It's already revealed in the rules editor! > > > > > Also if we add "max_holds", maybe we should rename "holdallowed" then. > > Again, not the same thing. Well, I think you were right here. I do not understand what I wanted to tell. I guess I got holdallowed and max_holds mixed up. Sorry about that. > > After that is done, the code in CanItemBeReserved could be replaced with a > > call to GetBranchBorrowerCircRule. This is, I think, still a valid request/question. In CanItemBeReserved there are 2 Koha::CirculationRules->find I am wondering why we do not call Koha::CirculationRules->get_effective_rule instead. Moreover it seems that there is a mix between '*' and undef. The UI inserts '*' whereas these calls and the tests search for undef. If we use undef we will be able to add FKs (branchcode,categorycode,itemtype) which can be great. (In reply to Jonathan Druart from comment #40) > > > After that is done, the code in CanItemBeReserved could be replaced with a > > > call to GetBranchBorrowerCircRule. > > This is, I think, still a valid request/question. > In CanItemBeReserved there are 2 Koha::CirculationRules->find > I am wondering why we do not call Koha::CirculationRules->get_effective_rule > instead. > Moreover it seems that there is a mix between '*' and undef. The UI inserts > '*' whereas these calls and the tests search for undef. > If we use undef we will be able to add FKs > (branchcode,categorycode,itemtype) which can be great. And this is for bug 18887. Created attachment 77648 [details] [review] Bug 15524: (QA follow-up) Respect ReservesControlBranch Created attachment 77681 [details] [review] Bug 15524: (QA follow-up) Respect ReservesControlBranch Created attachment 77682 [details] [review] Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass limit to template Created attachment 78134 [details] [review] Bug 15524: Set limit on maximum possible holds per patron by category It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category). This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Go to the circ rules editor, note the new max holds rules by patron category in the "Checkout limit by patron category". ( Should we rename this section? ) 4) Create find a patron that is allowed to place a hold, count the number of holds that patron has. Lets make that number 'X'. 5) Set the new max holds rule to X for "All libraries" 6) Note the patron can no longer place another hold 7) Set the new max holds rule to X + 1 for the patron's home library 8) Note the patron can again place another hold 9) Set the new max holds rule to X for the patron's home library 10) Note the patron can no longer place another hold Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 78135 [details] [review] Bug 15524: (QA Followup) - Change language used Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 78136 [details] [review] Bug 15524: (QA follow-up) Fix meaningless test The last test claims to allow a hold when branch=5 and patron=5, but look at the preceding statements: $rule_branch->max_holds(5); $rule_branch->update(); $rule_branch->max_holds(5); $rule_branch->insert(); The last insert will not be done, since the record is already present. A create should have triggered an error on the primary key. Obviously, we should use $rule_all. Test plan: Run the test again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 78137 [details] [review] Bug 15524: (QA follow-up) Pass max_holds at the right position in update When updating max_holds should be before the categorycode in the update query. Things that can be found by testing :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 78138 [details] [review] Bug 15524: (QA follow-up) Respect ReservesControlBranch Created attachment 78139 [details] [review] Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass limit to template Created attachment 78140 [details] [review] Bug 15524: (RM follow-up) Fix calls and add filter Awesome work all! Pushed to master for 18.11 (In reply to Nick Clemens from comment #52) > Awesome work all! > > Pushed to master for 18.11 Pushed schema files as followup Enhancement, will not be backported to 18.05.x series. *** Bug 21415 has been marked as a duplicate of this bug. *** |