Yes, $this_item is a great variable for a biblio.

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..

(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 :)

(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.

I am not sure you concern is valid, 1000 select into the circ rules table should be fast.

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>

(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.

(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.

(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..

Well, too bad performance wise. Push it without that follow-up..

Pushed to master for 17.11, thanks to everybody involved!