When placing a request we call CanBookBeReserved and loop items, calling CanItemBeReserved until we ifnd one that can fill a hold. We then loop CanItemBeReserved to check which items can be held. In some cases it would be nice to get the return values for all items passed back and avoid needing to call again
Created attachment 135426 [details] [review] Bug 30860: Add return_all_items parameter to CanBookBeReserved
Created attachment 135427 [details] [review] Bug 30860: Unit test
Created attachment 135428 [details] [review] Bug 30860: Return all items from CanBookBeReserved on request.pl This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before
Created attachment 135429 [details] [review] Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl Repeat test above on OPAC
Created attachment 144480 [details] [review] Bug 30860: Add return_all_items parameter to CanBookBeReserved
Created attachment 144481 [details] [review] Bug 30860: Unit test
Created attachment 144482 [details] [review] Bug 30860: Return all items from CanBookBeReserved on request.pl This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before
Created attachment 144483 [details] [review] Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl Repeat test above on OPAC
Created attachment 144506 [details] [review] Bug 30860: Add return_all_items parameter to CanBookBeReserved Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 144507 [details] [review] Bug 30860: Unit test Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 144508 [details] [review] Bug 30860: Return all items from CanBookBeReserved on request.pl This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 144509 [details] [review] Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl Repeat test above on OPAC Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Whoops, I added a signoff via a sandbox, was about to change the status here, and realized I hadn't tested on the OPAC. My bib has 12 items. When trying to place a hold via the staff client, 2 items show as holdable. The other 10 are not holdable: 1 is damaged, 1 is not for loan, the other 8 don't match the patron's library. When trying to place a hold on the OPAC for the same title and patron, I'm told there are no items that can be placed on hold.
(In reply to Andrew Fuerste-Henry from comment #13) > Whoops, I added a signoff via a sandbox, was about to change the status > here, and realized I hadn't tested on the OPAC. > > My bib has 12 items. When trying to place a hold via the staff client, 2 > items show as holdable. The other 10 are not holdable: 1 is damaged, 1 is > not for loan, the other 8 don't match the patron's library. > > When trying to place a hold on the OPAC for the same title and patron, I'm > told there are no items that can be placed on hold. In this case I believe the correct response is no items available on either interface, and that is what I see when I try to recreate. Can you retest, and share a screen shot of the record if you recreate?
Created attachment 145577 [details] [review] Bug 30860: Add return_all_items parameter to CanBookBeReserved Current status: Needs Signoff
Created attachment 145578 [details] [review] Bug 30860: Unit test
Created attachment 145579 [details] [review] Bug 30860: Return all items from CanBookBeReserved on request.pl This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before
Created attachment 145580 [details] [review] Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl Repeat test above on OPAC
Created attachment 145581 [details] [review] Bug 30860: Add return_all_items parameter to CanBookBeReserved
Created attachment 145582 [details] [review] Bug 30860: Unit test Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 145583 [details] [review] Bug 30860: Return all items from CanBookBeReserved on request.pl This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 145584 [details] [review] Bug 30860: Get all items info for CanBookBeReserved on opac-reserve.pl Repeat test above on OPAC Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Can you please take a look? FAIL t/db_dependent/Reserves.t FAIL valid "my" variable $can_be_all masks earlier declaration in same scope
Created attachment 147695 [details] [review] Bug 30860: (follow-up) Remove repeated variable declaration
I get the idea, but the implementation is weird. Why don't you simply cache the items holdability info in L1?
(In reply to Jonathan Druart from comment #25) > I get the idea, but the implementation is weird. Why don't you simply cache > the items holdability info in L1? "Simply" => It's not as simple as what you wrote. The idea is simple, not the implementation. But maybe it's worth spending time on it?
(In reply to Jonathan Druart from comment #25) > I get the idea, but the implementation is weird. Why don't you simply cache > the items holdability info in L1? I didn't think it was weird, it seemed straightforward to me. Rather than put in cache, retrieve from cache we just call and get all the values. I think moving to L1 cache would be a great patch if you want to write it :-)
(In reply to Jonathan Druart from comment #26) > (In reply to Jonathan Druart from comment #25) > > I get the idea, but the implementation is weird. Why don't you simply cache > > the items holdability info in L1? > > "Simply" => It's not as simple as what you wrote. The idea is simple, not > the implementation. But maybe it's worth spending time on it? I was thinking about the daemon, but maybe we deal with that already (there is a flush in rebuild_zebra). So it may be pretty easy actually.
Created attachment 148562 [details] [review] Bug 30860: [alternative patch] Cache CanItemBeReserved return value Instead of hacking CanBookBeReserved this patch suggests to simply cache the return value of CanItemBeReserved that could be then returned *on demand* We don't want to introduce side-effects hard to catch from this simple change, so let's return the cache value only from the 2 scripts we are dealing with.
(In reply to Jonathan Druart from comment #29) > Created attachment 148562 [details] [review] [review] > Bug 30860: [alternative patch] Cache CanItemBeReserved return value > > Instead of hacking CanBookBeReserved this patch suggests to simply cache > the return value of CanItemBeReserved that could be then returned *on > demand* > We don't want to introduce side-effects hard to catch from this simple > change, so let's return the cache value only from the 2 scripts we are > dealing with. This works, and in initial testing seems to offer improvement. I have one hesitation Current: code calls CanBookBeReserved, which exits on first holdable item, then we call CanItemBeReserved for every item Worst case, last item is holdable - for 100 items we call CanItemBeReserved 200 times My patches: code calls CanBookBeReserved which in turn calls CanItemBeReserved for every item. Then we loop over the results for each item Worst case, we call CanItemBeReserved 100 times - we do this no matter what Alternate patches: code calls CanBookBeReserved which exits on first holdable item we then retrieve any alreayd calculated from cache, then we cache each result as we go forward. Worst case - first item is holdable - we retrieve a single value from cache, then cache every subsequent CanItemBeReserved call and never use it - though we still only call CanItemBeReserved 100 times max I don't know what the actual tendency for number of items holdable, but it feels like we can do a lot of caching that we don't necessarily need.
(In reply to Nick Clemens from comment #30) > I don't know what the actual tendency for number of items holdable, but it > feels like we can do a lot of caching that we don't necessarily need. We can consider L1 caching insignificant in term of performance (unless we are caching huge structures!).
Created attachment 148812 [details] [review] Bug 30860: Cache CanItemBeReserved return value This patch caches the return value of CanItemBeReserved that could be then returned *on demand* We don't want to introduce side-effects hard to catch from this simple change, so let's return the cache value only from the 2 scripts we are dealing with. This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 149124 [details] [review] Bug 30860: Cache CanItemBeReserved return value This patch caches the return value of CanItemBeReserved that could be then returned *on demand* We don't want to introduce side-effects hard to catch from this simple change, so let's return the cache value only from the 2 scripts we are dealing with. This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 149377 [details] [review] Bug 30860: Cache CanItemBeReserved return value This patch caches the return value of CanItemBeReserved that could be then returned *on demand* We don't want to introduce side-effects hard to catch from this simple change, so let's return the cache value only from the 2 scripts we are dealing with. This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Also ran prove t/db_dependent/Holds.t t/db_dependent/Circulation.t Circulation.t gave me some trouble in an unrelated test, but results between master and patched branch match.
Pushed to master for 23.11. Nice work everyone, thanks!
This is so ugly
(In reply to Jonathan Druart from comment #37) > This is so ugly Writing that about your own creation?
(In reply to Katrin Fischer from comment #38) > (In reply to Jonathan Druart from comment #37) > > This is so ugly > > Writing that about your own creation? I would not date otherwise!
Thanks for all the hard work! Pushed to 23.05.x for the next release
Enhancement. Not pushing to 22.11.x.