|
Description
Jonathan Druart
2020-12-02 08:20:39 UTC
Created attachment 114085 [details] [review] Bug 27131: Add get_items_that_can_fill Joonas, is that what you had in mind? Or where you talking about more code? This bug report has been opened after the discussion on bug 24488 starting at bug 24488 comment 127 (In reply to Jonathan Druart from comment #2) > Joonas, is that what you had in mind? Or where you talking about more code? Thanks! Something like this but I hope we could add more checks here for this to be useful for many other places in Koha, at least similar stuff is needed in <https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659#c6>. So ideally this would be the same check as when you check-in a book in staff interface and Koha check's whether the item can satisfy any of the holds in that biblio using this item. This includes checking also circulation rules. Currently the pendingreserves.pl has more bugs than just the incorrect patron information, it lists at least also wrong amount of items to pull and wrongs items / callnumbers to pull. To achieve a reusable solution across the whole Koha codebase we probably need to go on more granular level: Koha::Item::can_fill_hold Koha::Hold::get_items_that_can_fill Koha::Holds::get_items_that_can_fill To further elaborate on the idea when we have
Koha::Holds::get_items_that_can_fill
then we could have in pendingreserves.pl something like this
for biblio in biblios_with_holds:
holds = biblio->holds
items = holds->get_items_that_can_fill
callnums = _get_callnumbers(items)
itemtypes = _get_itypes(items)
first_hold = biblio->first_hold_to_fill
push @reservedata, { biblio->biblionumber, first_hold->hold_id, callnums, itemtypes .. }
Created attachment 114851 [details] [review] Bug 27131: Add tests for get_items_that_can_fill (In reply to Joonas Kylmälä from comment #5) > To further elaborate on the idea when we have > > Koha::Holds::get_items_that_can_fill > > then we could have in pendingreserves.pl something like this > > for biblio in biblios_with_holds: > holds = biblio->holds > items = holds->get_items_that_can_fill > callnums = _get_callnumbers(items) > itemtypes = _get_itypes(items) > first_hold = biblio->first_hold_to_fill > > push @reservedata, { biblio->biblionumber, first_hold->hold_id, callnums, > itemtypes .. } We need to limit the number of queries. Doing that will be linear and will depend on the number of biblios, I am not sure it's a good idea. I've attached some tests, I don't think we can do more here. Created attachment 114852 [details] [review] Bug 27131: Add tests for get_items_that_can_fill (In reply to Jonathan Druart from comment #7) > We need to limit the number of queries. Doing that will be linear and will > depend on the number of biblios, I am not sure it's a good idea. > > I've attached some tests, I don't think we can do more here. We show 10 biblios on the pendingreserves.pl page so I don't see any major performance issues coming in this case. I think the current patches sent here are however an improvement already and can be further refined later so we could take them in the current form. (In reply to Joonas Kylmälä from comment #9) > We show 10 biblios on the pendingreserves.pl page so I don't see any major > performance issues coming in this case. Are you sure about that? IIRC only 10 are shown by DataTables, but all are loaded. (In reply to Jonathan Druart from comment #10) > (In reply to Joonas Kylmälä from comment #9) > > We show 10 biblios on the pendingreserves.pl page so I don't see any major > > performance issues coming in this case. > > Are you sure about that? IIRC only 10 are shown by DataTables, but all are > loaded. It probably is how you say, but it is not an issue, just have to fix the bug. Created attachment 116969 [details] [review] Bug 27131: Add get_items_that_can_fill Signed-off-by: Andrew Nugged <nugged@gmail.com> Created attachment 116970 [details] [review] Bug 27131: Add tests for get_items_that_can_fill Signed-off-by: Andrew Nugged <nugged@gmail.com> QAing Created attachment 116971 [details] [review] Bug 27131: Add get_items_that_can_fill Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116972 [details] [review] Bug 27131: Add tests for get_items_that_can_fill Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116973 [details] [review] Bug 27131: (QA follow-up) POD and comments Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This change looks good to me, but I certainly expected to see more code moving from the pending holds script to a module when I qa'ed the previous one. Hi Marcel, thanks for QAing. Which code from the controller do you have in mind? I had a look carefully and I cannot think of some business code we could move to a module. There are 1. lot a search calls and 2. a loop with very specific code to set variables for the template. IMO none of them will profit from being moved. (In reply to Jonathan Druart from comment #19) > Hi Marcel, thanks for QAing. > > Which code from the controller do you have in mind? I had a look carefully > and I cannot think of some business code we could move to a module. > There are 1. lot a search calls and 2. a loop with very specific code to set > variables for the template. > IMO none of them will profit from being moved. I had this expectation in general, not took the time to give specific details. But you did. Maybe we can improve further later on. Created attachment 116977 [details] [review] Bug 27131: Improve POD for the new method Pushed to master for 21.05, thanks to everybody involved! Depends on Bug 24488 not in 20.11.x Aren't we missing damaged (and AllowHoldsOnDamagedItems) here? (In reply to Jonathan Druart from comment #24) > Aren't we missing damaged (and AllowHoldsOnDamagedItems) here? In get_items_that_can_fill? If so, there also a lot more things missing. As per my original suggestion we should not re-invent the reservability checking but use CanItemBeReserved, etc. (In reply to Joonas Kylmälä from comment #25) > (In reply to Jonathan Druart from comment #24) > > Aren't we missing damaged (and AllowHoldsOnDamagedItems) here? > > In get_items_that_can_fill? If so, there also a lot more things missing. As > per my original suggestion we should not re-invent the reservability > checking but use CanItemBeReserved, etc. I've attached some patches on bug 3142. |