Bug 19298 - allow_onshelf_holds is not calculated correctly in opac-shelves
Summary: allow_onshelf_holds is not calculated correctly in opac-shelves
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 19301
  Show dependency treegraph
 
Reported: 2017-09-12 21:32 UTC by Jonathan Druart
Modified: 2017-10-25 15:19 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19298: Placing a hold from a list at the OPAC should respect issuing rules (2.24 KB, patch)
2017-09-13 18:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19298: Placing a hold from a list at the OPAC should respect issuing rules (2.61 KB, patch)
2017-09-27 02:35 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 19298: Placing a hold from a list at the OPAC should respect issuing rules (2.70 KB, patch)
2017-10-24 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19298: (QA follow-up) Remove GetBiblioData call (1.65 KB, patch)
2017-10-24 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-09-12 21:32:06 UTC
The call is:
  $this_item->{allow_onshelf_holds} = C4::Reserves::OnShelfHoldsAllowed($this_item, $patron);

But $this_item is not an item, but bibliographic record information.

C4::Reserves::_get_itype is used to know the correct "itemtype", but biblioitems.itemtype will always be picked as we do not have items information.
Comment 1 Jonathan Druart 2017-09-13 18:43:53 UTC
Created attachment 67116 [details] [review]
Bug 19298: Placing a hold from a list at the OPAC should respect issuing rules

The issuing rule retrieve to know if a hold can be placed on a record of
a list is not correct.

Test plan:
0/ With item-level_itypes = item level
1/ Define a item.itype=BK and biblioitems.itemtype=CF
2/ Create a default rule to allow on shelf holds
3/ Create a specific rule for CF with on shelf holds="If any
unavailable"
4/ Add this bibliographic record to a list and view the list
=> Without this patch you will not see "Place hold"
=> With this patch applied you will see the "Place hold" button,
respecting the correct issuing rule
Comment 2 Alex Buckley 2017-09-27 02:35:53 UTC
Created attachment 67387 [details] [review]
Bug 19298: Placing a hold from a list at the OPAC should respect issuing rules

The issuing rule retrieve to know if a hold can be placed on a record of
a list is not correct.

Test plan:
0/ With item-level_itypes = item level
1/ Define a item.itype=BK and biblioitems.itemtype=CF
2/ Create a default rule to allow on shelf holds
3/ Create a specific rule for CF with on shelf holds="If any
unavailable"
4/ Add this bibliographic record to a list and view the list
=> Without this patch you will not see "Place hold"
=> With this patch applied you will see the "Place hold" button,
respecting the correct issuing rule

Followed test plan, patches worked as described.

Note: Just to clarify the test plan slightly in step 4 where it says you will not see 'Place Hold' it means to
the left of the 'Save to another List' link below the item availability
in the opac-shelves.pl page. Not the 'Place hold' button in the grey
page header box.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 3 Marcel de Rooy 2017-10-20 09:59:31 UTC
Yes, $this_item is a great variable for a biblio.
Comment 4 Marcel de Rooy 2017-10-20 10:19:24 UTC
We will be doing this a lot of times when a biblio has more items:

my $issuing_rule = Koha::IssuingRules->get_effective_issuing_rule({ categorycode => $borrowercategory, itemtype => $itype, branchcode => $branchcode });

The categorycode is always the same. Branchcode is holding branch, often too. And probably we will not have a lot of different itemtypes for one biblio. So this could be more intelligent.
For longer lists with biblios having much items (serials), we are not improving performance.

And another thing:
We are using OnShelfHoldsAllowed on detail forms but NOT on search results with 20 biblios. Why should we use it on a list that may contain much more biblios?
I would rather set $this_item->{allow_onshelf_holds} to 1 and let opac-reserve handle the details, similar to opac-search. The template already contains itemsloo.norequests and also tests itemsloo.itemsissued..
Comment 5 Jonathan Druart 2017-10-20 16:24:20 UTC
(In reply to Marcel de Rooy from comment #4)
> We will be doing this a lot of times when a biblio has more items:
> 
> my $issuing_rule = Koha::IssuingRules->get_effective_issuing_rule({
> categorycode => $borrowercategory, itemtype => $itype, branchcode =>
> $branchcode });
> 
> The categorycode is always the same. Branchcode is holding branch, often
> too. And probably we will not have a lot of different itemtypes for one
> biblio. So this could be more intelligent.
> For longer lists with biblios having much items (serials), we are not
> improving performance.

The calculation was wrong, this patch suggests to fix it (calculate it as it is everywhere else).
The performance point is valid, but is outside the scope of this bug report. See as the related bug to understand where I am going (move to Koha namespace, then improve code from there. We can easily imagine a cache mechanism in Koha::IssuingRules). 

> And another thing:
> We are using OnShelfHoldsAllowed on detail forms but NOT on search results
> with 20 biblios. Why should we use it on a list that may contain much more
> biblios?
> I would rather set $this_item->{allow_onshelf_holds} to 1 and let
> opac-reserve handle the details, similar to opac-search. The template
> already contains itemsloo.norequests and also tests itemsloo.itemsissued..

I guess that could be considered as a regression by somebody else.

Again, this is a bug I found where I started moving code. I would like to improve the way it is calculated/displayed, etc (see bug 19297).
But this patch is the very first step :)
Comment 6 Marcel de Rooy 2017-10-23 06:57:10 UTC
(In reply to Jonathan Druart from comment #5)
> The calculation was wrong, this patch suggests to fix it (calculate it as it
> is everywhere else).
> The performance point is valid, but is outside the scope of this bug report.
> See as the related bug to understand where I am going (move to Koha
> namespace, then improve code from there. We can easily imagine a cache
> mechanism in Koha::IssuingRules). 

Only problem is release dates ;)
You add it to 17.11 and we backport it to a few versions. They all have a degraded performance now. And the new caching mechanism comes in say 18.05 and is not backported.
Comment 7 Jonathan Druart 2017-10-23 14:26:10 UTC
I am not sure you concern is valid, 1000 select into the circ rules table should be fast.
Comment 8 Marcel de Rooy 2017-10-24 12:52:19 UTC
Created attachment 68460 [details] [review]
Bug 19298: Placing a hold from a list at the OPAC should respect issuing rules

The issuing rule retrieve to know if a hold can be placed on a record of
a list is not correct.

Test plan:
0/ With item-level_itypes = item level
1/ Define a item.itype=BK and biblioitems.itemtype=CF
2/ Create a default rule to allow on shelf holds
3/ Create a specific rule for CF with on shelf holds="If any
unavailable"
4/ Add this bibliographic record to a list and view the list
=> Without this patch you will not see "Place hold"
=> With this patch applied you will see the "Place hold" button,
respecting the correct issuing rule

Followed test plan, patches worked as described.

Note: Just to clarify the test plan slightly in step 4 where it says you will not see 'Place Hold' it means to
the left of the 'Save to another List' link below the item availability
in the opac-shelves.pl page. Not the 'Place hold' button in the grey
page header box.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-10-24 12:52:23 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2017-10-24 12:53:36 UTC
(In reply to Jonathan Druart from comment #7)
> I am not sure you concern is valid, 1000 select into the circ rules table
> should be fast.

You also added a Koha::Biblios call and additional calls for each item.
I replaced the GetBiblioData call in a follow-up to gain something back.
Comment 11 Jonathan Druart 2017-10-24 14:50:37 UTC
(In reply to Marcel de Rooy from comment #9)
> Created attachment 68461 [details] [review] [review]
> Bug 19298: (QA follow-up) Remove GetBiblioData call
> 
> In order to do at least something for performance, we could replace
> the call to GetBiblioData since we are already fetching biblio data
> with Koha::Biblios.

GetBiblioData also returns biblioitems fields, and the template uses them.
At least publishercode, place, publicationyear, pages, notes, etc.
Comment 12 Marcel de Rooy 2017-10-24 14:57:45 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2017-10-24 14:58:03 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2017-10-24 14:58:33 UTC
(In reply to Marcel de Rooy from comment #13)
> Oops I see it now

Funny thing is that I didnt see anything particular when testing..
Comment 15 Marcel de Rooy 2017-10-24 14:59:19 UTC
Well, too bad performance wise. Push it without that follow-up..
Comment 16 Jonathan Druart 2017-10-25 15:19:24 UTC
Pushed to master for 17.11, thanks to everybody involved!