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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19298 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |19301 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19301 [Bug 19301] Move C4::Reserves::OnShelfHoldsAllowed to the code namespace -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19298 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #67116|0 |1 is obsolete| | --- Comment #2 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 67387 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=67387&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19298 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19298 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I am not sure you concern is valid, 1000 select into the circ rules table should be fast. -- You are receiving this mail because: You are watching all bug changes.

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

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19298 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #11)

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

my $query = " SELECT * , biblioitems.notes AS bnotes, itemtypes.notforloan as bi_notforloan, biblio.notes FROM biblio LEFT JOIN biblioitems ON biblio.biblionumber = biblioitems.biblionumber LEFT JOIN itemtypes ON biblioitems.itemtype = itemtypes.itemtype WHERE biblio.biblionumber = ?"; Please explain. See my commit message too. -- You are receiving this mail because: You are watching all bug changes.

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

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19298 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.