Bug 27131

Summary: Move code from circ/pendingreserves.pl to modules
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: black23, dcook, fridolin.somers, joonas.kylmala, josef.moravec, m.de.rooy, nugged
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Bug Depends on: 24488    
Bug Blocks: 3142, 27718    
Attachments: Bug 27131: Add get_items_that_can_fill
Bug 27131: Add tests for get_items_that_can_fill
Bug 27131: Add tests for get_items_that_can_fill
Bug 27131: Add get_items_that_can_fill
Bug 27131: Add tests for get_items_that_can_fill
Bug 27131: Add get_items_that_can_fill
Bug 27131: Add tests for get_items_that_can_fill
Bug 27131: (QA follow-up) POD and comments
Bug 27131: Improve POD for the new method

Description Jonathan Druart 2020-12-02 08:20:39 UTC
We need the code covered by tests.
Comment 1 Jonathan Druart 2020-12-02 08:23:56 UTC
Created attachment 114085 [details] [review]
Bug 27131: Add get_items_that_can_fill
Comment 2 Jonathan Druart 2020-12-02 08:24:30 UTC
Joonas, is that what you had in mind? Or where you talking about more code?
Comment 3 Jonathan Druart 2020-12-02 08:25:18 UTC
This bug report has been opened after the discussion on bug 24488 starting at bug 24488 comment 127
Comment 4 Joonas Kylmälä 2020-12-02 08:47:29 UTC
(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
Comment 5 Joonas Kylmälä 2020-12-02 08:59:19 UTC
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 .. }
Comment 6 Jonathan Druart 2021-01-05 08:33:20 UTC
Created attachment 114851 [details] [review]
Bug 27131: Add tests for get_items_that_can_fill
Comment 7 Jonathan Druart 2021-01-05 08:35:22 UTC
(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.
Comment 8 Jonathan Druart 2021-01-05 08:38:50 UTC
Created attachment 114852 [details] [review]
Bug 27131: Add tests for get_items_that_can_fill
Comment 9 Joonas Kylmälä 2021-01-05 09:19:09 UTC
(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.
Comment 10 Jonathan Druart 2021-01-05 10:03:17 UTC
(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.
Comment 11 Joonas Kylmälä 2021-01-05 14:22:12 UTC
(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.
Comment 12 Andrii Nugged 2021-02-18 01:56:12 UTC
Created attachment 116969 [details] [review]
Bug 27131: Add get_items_that_can_fill

Signed-off-by: Andrew Nugged <nugged@gmail.com>
Comment 13 Andrii Nugged 2021-02-18 01:56:16 UTC
Created attachment 116970 [details] [review]
Bug 27131: Add tests for get_items_that_can_fill

Signed-off-by: Andrew Nugged <nugged@gmail.com>
Comment 14 Marcel de Rooy 2021-02-18 07:17:23 UTC
QAing
Comment 15 Marcel de Rooy 2021-02-18 07:49:27 UTC
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>
Comment 16 Marcel de Rooy 2021-02-18 07:49:30 UTC
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>
Comment 17 Marcel de Rooy 2021-02-18 07:49:34 UTC
Created attachment 116973 [details] [review]
Bug 27131: (QA follow-up) POD and comments

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2021-02-18 07:51:17 UTC
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.
Comment 19 Jonathan Druart 2021-02-18 09:07:28 UTC
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.
Comment 20 Marcel de Rooy 2021-02-18 09:18:15 UTC
(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.
Comment 21 Jonathan Druart 2021-02-18 11:03:11 UTC
Created attachment 116977 [details] [review]
Bug 27131: Improve POD for the new method
Comment 22 Jonathan Druart 2021-02-18 11:05:56 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 23 Fridolin Somers 2021-02-22 10:13:22 UTC
Depends on Bug 24488 not in 20.11.x
Comment 24 Jonathan Druart 2021-06-22 13:30:53 UTC
Aren't we missing damaged (and AllowHoldsOnDamagedItems) here?
Comment 25 Joonas Kylmälä 2021-06-22 13:40:17 UTC
(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.
Comment 26 Jonathan Druart 2021-06-22 13:59:56 UTC
(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.