---- Reported by gmcharlt@gmail.com 2009-04-19 13:21:12 ---- The OPAC and bib details and MARC display have varying code to determine whether an item should be displayed as available to place a hold request. These should be consolidated into a function: catalogue/detail.pl: $norequests = 0 unless ( ( $item->{'notforloan'} > 0 ) || ( $item->{'itemnotforloan'} > 0 ) ); catalogue/detailprint.pl: $norequests = 0 unless $itm->{'notforloan'}; catalogue/MARCdetail.pl: $norequests = 0 if $subf[$i][1] ==0 and $tagslib->{ $field->tag() }->{ $subf[$i][0] }->{kohafield} eq 'items.notforloan' ; NOTE: There is another problem here - this is grabbing item data directly from the MARC record when it should be using GetItems(). opac/opac-detail.pl: 126 $norequests = 0 127 if ( (not $itm->{'wthdrawn'} ) 128 && (not $itm->{'itemlost'} ) 129 && ($itm->{'itemnotforloan'}<0 || not $itm->{'itemnotforloan'} ) 130 && (not $itemtypes->{$itm->{'itype'}}->{notforloan} ) 131 && ($itm->{'itemnumber'} ) ); opac/opac-detailprint.pl: $norequests = 0 unless $itm->{'notforloan'}; Note that there can be legitimate differences between how the requestability is calculated between OPAC and staff. --- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:06 UTC --- This bug was previously known as _bug_ 3142 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3142 Actual time not defined. Setting to 0.0 CC member adrea@moablibrary.org does not have an account here
I have three branch: A, B and C Each branch uses a different rule for reservations: A: all branch B: no hold C: branch attachment I have a bib record that has three copies: 1 copy under A 1 copy under B A copy in C The three copies are borrowed. At the OPAC the borrowerss can hold the items A borrower located in B wishes to reserve the document. It activates the "hold the items" Since the interface professional koha knows how to interpret the prohibitions and so I can hold only copy A At the OPAC the borrower can hold the three copies -> bug
Still valid?
*** Bug 28605 has been marked as a duplicate of this bug. ***
Created attachment 122284 [details] [review] Bug 3142: Add itemlost, withdrawn and onloan
Created attachment 122285 [details] [review] Bug 3142: Exclude damaged items
Created attachment 122286 [details] [review] Bug 3142: Add itype
Created attachment 122287 [details] [review] Bug 3142: Use filter_by_for_holds where holdability is calculated
Patches for discussion. This look like it's going into the right direction, are these patches correct? (Tests are missing of course)
Comment on attachment 122284 [details] [review] Bug 3142: Add itemlost, withdrawn and onloan Review of attachment 122284 [details] [review]: ----------------------------------------------------------------- ::: Koha/Items.pm @@ +53,5 @@ > + itemlost => 0, > + withdrawn => 0, > + notforloan => { '<=' => 0 } > + , # items with negative or zero notforloan value are holdable > + onloan => undef, Why is onloan undef here? If I recall correctly, there are some rules/system preferences for determining holdability based on loan status. For instance, some settings only allow you to place holds if an item is already checked out. But then there is "allow on shelf holds" where "onloan" would be irrelevant. So it's probably a better idea to create the hashref earlier in the function and then changing the hashref based on rules/settings.
Created attachment 122314 [details] [review] Bug 3142: Add itemlost, withdrawn and notforloan
Created attachment 122315 [details] [review] Bug 3142: Exclude damaged items
Created attachment 122316 [details] [review] Bug 3142: Add itype
Created attachment 122317 [details] [review] Bug 3142: Use filter_by_for_holds where holdability is calculated
(In reply to David Cook from comment #9) > Comment on attachment 122284 [details] [review] [review] > Bug 3142: Add itemlost, withdrawn and onloan > > Review of attachment 122284 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Items.pm > @@ +53,5 @@ > > + itemlost => 0, > > + withdrawn => 0, > > + notforloan => { '<=' => 0 } > > + , # items with negative or zero notforloan value are holdable > > + onloan => undef, > > Why is onloan undef here? I removed it and kept it in Koha::Items->filter_by_for_hold
Test plan: Confirm that the holdability is calculated identicaly with and without the patch on the following view: - search result(staff and opac) - opac detail pages (normal, ISBD and MARC) - pending reserves (confirm the item list is the same)
*** Bug 28667 has been marked as a duplicate of this bug. ***
Created attachment 125727 [details] [review] Bug 3142: Add itemlost, withdrawn and notforloan Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125728 [details] [review] Bug 3142: Exclude damaged items Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125729 [details] [review] Bug 3142: Add itype Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125730 [details] [review] Bug 3142: Use filter_by_for_holds where holdability is calculated Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This works well and certainly clarifies the code.. it does need tests still though ;) Whilst I see David's point about on shelf holds, I think we can defer that piece to a next step in a follow-up bug. Signing off.
(In reply to Martin Renvoize from comment #21) > This works well and certainly clarifies the code.. it does need tests still > though ;) If it does, we cant get it further from here?
+ my @hold_not_allowed_itypes = Koha::CirculationRules->search( + { + rule_name => 'holdallowed', + branchcode => undef, + categorycode => undef, + rule_value => 'not_allowed', + } + )->get_column('itemtype'); I have my doubts here btw. This is too simple. You are looking at circ rules with exception for various combinations of category, itemtype etc. You cant just do a get_column here. This depends on context. Please clarify and add tests.
(In reply to Marcel de Rooy from comment #22) > (In reply to Martin Renvoize from comment #21) > > This works well and certainly clarifies the code.. it does need tests still > > though ;) > > If it does, we cant get it further from here? What do you suggest? I didn't get what suggested David in comment 9. The unique ambition of these patches is to reuse an existing method and avoid duplication of code.
(In reply to Marcel de Rooy from comment #23) > + my @hold_not_allowed_itypes = Koha::CirculationRules->search( > + { > + rule_name => 'holdallowed', > + branchcode => undef, > + categorycode => undef, > + rule_value => 'not_allowed', > + } > + )->get_column('itemtype'); > > I have my doubts here btw. > This is too simple. You are looking at circ rules with exception for various > combinations of category, itemtype etc. > You cant just do a get_column here. This depends on context. > Please clarify and add tests. Which context? The logged in branch? Here we are not in C4::Reserves, we don't want to offer all the complexity of CanItemBeReserved. As said previously I am not adding new code of functionality (apart for the additional damaged check). Tests are definitely missing, agreed.
Created attachment 125732 [details] [review] Bug 3142: Add tests And fix a bug!
(In reply to Jonathan Druart from comment #25) > (In reply to Marcel de Rooy from comment #23) > > + my @hold_not_allowed_itypes = Koha::CirculationRules->search( > > + { > > + rule_name => 'holdallowed', > > + branchcode => undef, > > + categorycode => undef, > > + rule_value => 'not_allowed', > > + } > > + )->get_column('itemtype'); > > > > I have my doubts here btw. > > This is too simple. You are looking at circ rules with exception for various > > combinations of category, itemtype etc. > > You cant just do a get_column here. This depends on context. > > Please clarify and add tests. > > Which context? The logged in branch? > Here we are not in C4::Reserves, we don't want to offer all the complexity > of CanItemBeReserved. As said previously I am not adding new code of > functionality (apart for the additional damaged check). Ok. If this is a reflection of what we did already, I cant justify to fail it :) Koha never fails! Back to SO
Will QA this one soon.
Created attachment 125733 [details] [review] Bug 3142: Add test for the circ rule
Created attachment 125734 [details] [review] Bug 3142: Add note about imprecision of filter_by_for_hold
(In reply to Marcel de Rooy from comment #27) > (In reply to Jonathan Druart from comment #25) > > (In reply to Marcel de Rooy from comment #23) > > > + my @hold_not_allowed_itypes = Koha::CirculationRules->search( > > > + { > > > + rule_name => 'holdallowed', > > > + branchcode => undef, > > > + categorycode => undef, > > > + rule_value => 'not_allowed', > > > + } > > > + )->get_column('itemtype'); > > > > > > I have my doubts here btw. > > > This is too simple. You are looking at circ rules with exception for various > > > combinations of category, itemtype etc. > > > You cant just do a get_column here. This depends on context. > > > Please clarify and add tests. > > > > Which context? The logged in branch? > > Here we are not in C4::Reserves, we don't want to offer all the complexity > > of CanItemBeReserved. As said previously I am not adding new code of > > functionality (apart for the additional damaged check). > > Ok. If this is a reflection of what we did already, I cant justify to fail > it :) Koha never fails! > Back to SO lol I am feeling your sarcasms, Marcel :) I will be happy to improve anything you think that may be useful. But keep in mind that the point here is only to centralize things.
QA second try
Created attachment 125942 [details] [review] Bug 3142: Add itemlost, withdrawn and notforloan Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125943 [details] [review] Bug 3142: Exclude damaged items Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125944 [details] [review] Bug 3142: Add itype Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125945 [details] [review] Bug 3142: Use filter_by_for_holds where holdability is calculated Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125946 [details] [review] Bug 3142: Add tests And fix a bug! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125947 [details] [review] Bug 3142: Add test for the circ rule Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125948 [details] [review] Bug 3142: Add note about imprecision of filter_by_for_hold Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125949 [details] [review] Bug 3142: (QA follow-up) Cosmetic changes Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125950 [details] [review] Bug 3142: (QA follow-up) Include notforloan itemtypes As was done in the code already. Note that we are ignoring effective itemtype now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
while ( my $item = $items->next ) { - $norequests = 0 - if $norequests - && !$item->withdrawn - && !$item->itemlost - && ($item->notforloan < 0 || not $item->notforloan ) - && !Koha::ItemTypes->find($item->effective_itemtype)->notforloan - && $item->itemnumber; - $allow_onshelf_holds = Koha::CirculationRules->get_onshelfholds_policy( { item => $item, patron => $patron } ) unless $allow_onshelf_holds; Could this loop be done smarter now? opac/opac-ISBDdetail.pl - && !Koha::ItemTypes->find($item->effective_itemtype)->notforloan opac/opac-MARCdetail.pl - && !Koha::ItemTypes->find($item->effective_itemtype)->notforloan opac-detail.pl - && !$itemtypes->{$itm->{'itype'}}->{notforloan} Added a follow-up that looks at notforloan but effective is still another story.
(In reply to Marcel de Rooy from comment #42) > while ( my $item = $items->next ) { > - $norequests = 0 > - if $norequests > - && !$item->withdrawn > - && !$item->itemlost > - && ($item->notforloan < 0 || not $item->notforloan ) > - && !Koha::ItemTypes->find($item->effective_itemtype)->notforloan > - && $item->itemnumber; > - > $allow_onshelf_holds = Koha::CirculationRules->get_onshelfholds_policy( > { item => $item, patron => $patron } ) > unless $allow_onshelf_holds; > > Could this loop be done smarter now? We could certainly have get_onshelfholds_policy take a Koha::Items iterator and do things there to avoid the while. > opac/opac-ISBDdetail.pl > - && !Koha::ItemTypes->find($item->effective_itemtype)->notforloan > opac/opac-MARCdetail.pl > - && !Koha::ItemTypes->find($item->effective_itemtype)->notforloan > opac-detail.pl > - && !$itemtypes->{$itm->{'itype'}}->{notforloan} > Added a follow-up that looks at notforloan but effective is still another > story. Good catch. Isn't it blocker? Looks like a regression :-/
Created attachment 125951 [details] [review] Bug 3142: Handle biblio level item type
(In reply to Jonathan Druart from comment #44) > Created attachment 125951 [details] [review] [review] > Bug 3142: Handle biblio level item type Something like that?
(In reply to Jonathan Druart from comment #45) > (In reply to Jonathan Druart from comment #44) > > Created attachment 125951 [details] [review] [review] [review] > > Bug 3142: Handle biblio level item type > > Something like that? Yeah, looks like it. But we should definitely get rid of this preference. The maintenance cost is much higher than the user benefit (where a large majority is not using it; 95% or more?) At upgrade time we could set items.itype correct for this minority and make sure that the itype at item creation defaults to 942c correctly. It was discussed already somewhere. We should do it in 22.05. Didnt test this change yet btw.
Bug 29106 comment14
Created attachment 125956 [details] [review] Bug 3142: Add tests for item-level_itypes=0
QA stamp needed on the last 2 patches.
(In reply to Marcel de Rooy from comment #46) > Yeah, looks like it. But we should definitely get rid of this preference. > It was discussed already somewhere. We should do it in 22.05. I totally support that and can work on that if people commits to provide fast tests and QA.
Created attachment 125967 [details] [review] Bug 3142: Add itemlost, withdrawn and notforloan Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125968 [details] [review] Bug 3142: Exclude damaged items Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125969 [details] [review] Bug 3142: Add itype Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125970 [details] [review] Bug 3142: Use filter_by_for_holds where holdability is calculated Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125971 [details] [review] Bug 3142: Add tests And fix a bug! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125972 [details] [review] Bug 3142: Add test for the circ rule Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125973 [details] [review] Bug 3142: Add note about imprecision of filter_by_for_hold Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125974 [details] [review] Bug 3142: (QA follow-up) Cosmetic changes Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125975 [details] [review] Bug 3142: (QA follow-up) Include notforloan itemtypes As was done in the code already. Note that we are ignoring effective itemtype now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125976 [details] [review] Bug 3142: Handle biblio level item type Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 125977 [details] [review] Bug 3142: Add tests for item-level_itypes=0 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 21.11, thanks to everybody involved!
Great teamwork there's guys, so nice to see this go even further since I last looked. Great effort
(In reply to Jonathan Druart from comment #50) > I totally support that and can work on that if people commits to provide > fast tests and QA. Ping me when you are ready :)
(In reply to Marcel de Rooy from comment #64) > (In reply to Jonathan Druart from comment #50) > > I totally support that and can work on that if people commits to provide > > fast tests and QA. > > Ping me when you are ready :) Added to the roadmap as a "22.05" topic!
+ $can_place_holds = $biblio_object->items->filter_by_for_hold()->count; This is exploding early in C4::Search::searchResults if the record is indexed but deleted from the DB. Can't call method "items" on an undefined value at /kohadevbox/koha/C4/Search.pm line 2000 C4::Search::searchResults('HASH(0x55dc9a4076c0)', 'kw,wrdl: d', 432, 20, 0, undef, 'ARRAY(0x55dc9a3757e8)', 'HASH(0x55dc99f94a50)') called at /kohadevbox/koha/opac/opac-search.pl line 613
(In reply to Jonathan Druart from comment #66) > + $can_place_holds = > $biblio_object->items->filter_by_for_hold()->count; > > This is exploding early in C4::Search::searchResults if the record is > indexed but deleted from the DB. > > > Can't call method "items" on an undefined value at > /kohadevbox/koha/C4/Search.pm line 2000 > C4::Search::searchResults('HASH(0x55dc9a4076c0)', 'kw,wrdl: d', 432, 20, 0, > undef, 'ARRAY(0x55dc9a3757e8)', 'HASH(0x55dc99f94a50)') called at > /kohadevbox/koha/opac/opac-search.pl line 613 See bug 29374
Pushed to 21.05.x for 21.05.11
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed. (congrats on closing a 13 years old ticket!!!)