Description
Andrew Fuerste-Henry
2020-11-20 17:32:44 UTC
Created attachment 113900 [details] [review] Bug 27068: Add hold group limits to Holds Queue builder My patch is incomplete. HoldsQueue needs to understand how to handle hold_fullfillment_policy for "holdgroup" and "patrongroup". It should be relying on $item->pickup_locations more heavily. Will take a look on monday with Agustin. Created attachment 114125 [details] [review] Bug 27068: Control hold group logic in HoldsQueue Hello Kyle, could you please checkout this patch? I've placed holds policy checking (including hold groups) inside a function called _checkHoldsPolicy, and call it whenever it's needed. Created attachment 114140 [details] [review] Bug 27068: Control hold group logic in HoldsQueue Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 114141 [details] [review] Bug 27068: Fix errors in _checkHoldPolicy Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 114142 [details] [review] Bug 27068: Fetch libraries once for speed Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> What would be the test plan? Here is a try: 1) In library groups add a root group and check it as hold group. 2) Add two libraries to the group 3) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group' 4) Place a hold from a patron whose home library is from the group 5) Go to hold queue of the record 6) ??? (In reply to Victor Grousset/tuxayo from comment #9) > What would be the test plan? > > Here is a try: > 1) In library groups add a root group and check it as hold group. > 2) Add two libraries to the group > 3) In circulation and fines rules, in 'Default checkout, hold and return > policy', in Hold policy change the value to 'From local hold group' > 4) Place a hold from a patron whose home library is from the group 5) Go to /cgi-bin/koha/circ/view_holdsqueue.pl 6) Select the holding branch of the item with a hold 7) observe no results 8) Apply Patch 9) Repeat 5-6 10) The item should come up on the holds queue results 11) Place a hold on an item where 1 record has 2 copies, 1 in the hold group, 1 not. 12) Run the HoldQueue for the library not in the group and make sure the hold isn't showing. 13) Turn on transportation cost matrix and set costs for the libraries within the group. 14) Place a hold for a patron where multiple copies are on the bib. 15) Check both branch's hold queue for the item, it should only show on the lower cost branch's list if both copies are available. Agustin or Kyle could probably tell us if there are any steps missing in there. These are steps I wrote based on experience with the problem at hand and looking at the diff for the patches. Lisette Thanks for completing the test plan :)
> 10) The item should come up on the holds queue results
Unfortunately it doesn't come up.
My patron's library is Centerville
The item's current and home library is Centerville
Centerville is in the group
This configuration should work right?
(In reply to Victor Grousset/tuxayo from comment #11) > Thanks for completing the test plan :) > > > 10) The item should come up on the holds queue results > > Unfortunately it doesn't come up. > > My patron's library is Centerville > The item's current and home library is Centerville > Centerville is in the group > > This configuration should work right? Did you run the build_holds_queue script? Hi Augustin, this one needs unit tests for the changes Created attachment 115987 [details] [review] Bug 27068: Fetch libraries once for speed Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 116022 [details] [review] Bug 27068: Control hold group logic in HoldsQueue Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 116023 [details] [review] Bug 27068: Fix errors in _checkHoldPolicy Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 116024 [details] [review] Bug 27068: Fetch libraries once for speed Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 116025 [details] [review] Bug 27068: Add unit tests Created attachment 116026 [details] [review] Bug 27068: Add unit tests Created attachment 116607 [details] [review] Bug 27068: Control hold group logic in HoldsQueue Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 116608 [details] [review] Bug 27068: Fix errors in _checkHoldPolicy Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 116609 [details] [review] Bug 27068: Fetch libraries once for speed Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 116610 [details] [review] Bug 27068: Add unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> No regressions found, working as expected for me. QA scripts passing and we've got unit tests and they pass. Passing QA Can we get some text for the release notes though pretty please ;) Created attachment 116814 [details] [review] Bug 27068: Don't 'cache' Koha::Libraries Created attachment 116815 [details] [review] Bug 27068: Perltidy _checkHoldPolicy The way we cache Koha::Libraries at package level is quite a poor design here. I understand that we are doing it because _checkHoldPolicy is called in a loop, but we should maybe then redesign something else. What do you think? (In reply to Jonathan Druart from comment #27) > The way we cache Koha::Libraries at package level is quite a poor design > here. > I understand that we are doing it because _checkHoldPolicy is called in a > loop, but we should maybe then redesign something else. > > What do you think? Undoing it is fine with me. This runs as a cronjob so speed isn't of the utmost concern. If we really wanted to, we could use Koha::Cache. Created attachment 116816 [details] [review] Bug 27068: Don't 'cache' Koha::Libraries Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 116817 [details] [review] Bug 27068: Perltidy _checkHoldPolicy Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.03 Pushed to 20.05.x for 20.05.09 Well drat. I've for test failures after backporting this to 20.05: root@kohadevbox:koha(rmain2005)$ prove t/db_dependent/HoldsQueue.t t/db_dependent/HoldsQueue.t .. 3/55 The method Koha::Item->exclude_from_local_holds_priority is not covered by tests! Trace begun at /kohadevbox/koha/Koha/Object.pm line 780 Koha::Object::AUTOLOAD('Koha::Item=HASH(0x56534a141a70)') called at /kohadevbox/koha/C4/HoldsQueue.pm line 457 C4::HoldsQueue::MapItemsToHoldRequests('ARRAY(0x56534a155a30)', 'ARRAY(0x56534a11abd8)', 'ARRAY(0x56534a141008)', undef) called at /kohadevbox/koha/C4/HoldsQueue.pm line 212 C4::HoldsQueue::CreateQueue at t/db_dependent/HoldsQueue.t line 388 # Looks like your test exited with 11 just after 26. t/db_dependent/HoldsQueue.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 29/55 subtests Test Summary Report ------------------- t/db_dependent/HoldsQueue.t (Wstat: 2816 Tests: 26 Failed: 0) Non-zero exit status: 11 Parse errors: Bad plan. You planned 55 tests but ran 26. Files=1, Tests=26, 3 wallclock secs ( 0.02 usr 0.01 sys + 1.76 cusr 0.36 csys = 2.15 CPU) Result: FAIL root@kohadevbox:koha(rmain2005)$ Missing dependencies for 19.11.x, it's not affected, no backport. Created attachment 117215 [details] [review] [20.05.x] Bug 27068: (follow-up) Remove local hold priority exclusion-related code for 20.05.x The local hold priority exclusions feature is only present in 20.11, so the use case handling shouldn't be included in the backport. This code removes the couple lines in which this is handled (trivial) and tested (trivial, as it is disabled by default on the tests, we just remove the places in which the original patch set the non-existent attributes). To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => FAIL: Tests fail! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> |