https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 114085 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114085&action=edit Bug 27131: Add get_items_that_can_fill -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- This bug report has been opened after the discussion on bug 24488 starting at bug 24488 comment 127 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #4 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 Michal Denar <black23@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |black23@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #9 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #11 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #13 from Andrew Nugged <nugged@gmail.com> --- Created attachment 116970 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116970&action=edit Bug 27131: Add tests for get_items_that_can_fill Signed-off-by: Andrew Nugged <nugged@gmail.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116970|0 |1 is obsolete| | --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 116972 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116972&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #18 from 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |RESOLVED --- Comment #23 from Fridolin Somers <fridolin.somers@biblibre.com> --- Depends on Bug 24488 not in 20.11.x -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #24 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Aren't we missing damaged (and AllowHoldsOnDamagedItems) here? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27131 --- Comment #26 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.