Bug 3142 - Standardize how OPAC and staff determine requestability
Summary: Standardize how OPAC and staff determine requestability
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 28605 28667 (view as bug list)
Depends on: 27131
Blocks: 29087 29374
  Show dependency treegraph
 
Reported: 2009-04-19 01:21 UTC by Galen Charlton
Modified: 2022-12-12 21:23 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.11


Attachments
Bug 3142: Add itemlost, withdrawn and onloan (997 bytes, patch)
2021-06-22 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Exclude damaged items (719 bytes, patch)
2021-06-22 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add itype (1.46 KB, patch)
2021-06-22 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Use filter_by_for_holds where holdability is calculated (8.58 KB, patch)
2021-06-22 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add itemlost, withdrawn and notforloan (969 bytes, patch)
2021-06-23 08:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Exclude damaged items (720 bytes, patch)
2021-06-23 08:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add itype (1.33 KB, patch)
2021-06-23 08:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Use filter_by_for_holds where holdability is calculated (8.62 KB, patch)
2021-06-23 08:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add itemlost, withdrawn and notforloan (1.01 KB, patch)
2021-10-05 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3142: Exclude damaged items (783 bytes, patch)
2021-10-05 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3142: Add itype (1.40 KB, patch)
2021-10-05 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3142: Use filter_by_for_holds where holdability is calculated (8.51 KB, patch)
2021-10-05 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3142: Add tests (2.74 KB, patch)
2021-10-05 12:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add test for the circ rule (1.65 KB, patch)
2021-10-05 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add note about imprecision of filter_by_for_hold (813 bytes, patch)
2021-10-05 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add itemlost, withdrawn and notforloan (1.10 KB, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: Exclude damaged items (881 bytes, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: Add itype (1.49 KB, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: Use filter_by_for_holds where holdability is calculated (8.61 KB, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: Add tests (2.83 KB, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: Add test for the circ rule (1.74 KB, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: Add note about imprecision of filter_by_for_hold (908 bytes, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: (QA follow-up) Cosmetic changes (1.55 KB, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: (QA follow-up) Include notforloan itemtypes (2.62 KB, patch)
2021-10-08 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 3142: Handle biblio level item type (1.82 KB, patch)
2021-10-08 10:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add tests for item-level_itypes=0 (1.25 KB, patch)
2021-10-08 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 3142: Add itemlost, withdrawn and notforloan (1.12 KB, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: Exclude damaged items (898 bytes, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: Add itype (1.51 KB, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: Use filter_by_for_holds where holdability is calculated (8.62 KB, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: Add tests (2.85 KB, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: Add test for the circ rule (1.76 KB, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: Add note about imprecision of filter_by_for_hold (925 bytes, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: (QA follow-up) Cosmetic changes (1.57 KB, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: (QA follow-up) Include notforloan itemtypes (2.64 KB, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: Handle biblio level item type (1.87 KB, patch)
2021-10-08 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 3142: Add tests for item-level_itypes=0 (1.31 KB, patch)
2021-10-08 14:32 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:06:58 UTC


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

Comment 1 delaye 2012-02-09 16:38:02 UTC
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
Comment 2 Marc Véron 2015-12-20 15:34:54 UTC
Still valid?
Comment 3 Jonathan Druart 2021-06-22 13:37:30 UTC
*** Bug 28605 has been marked as a duplicate of this bug. ***
Comment 4 Jonathan Druart 2021-06-22 13:57:05 UTC
Created attachment 122284 [details] [review]
Bug 3142: Add itemlost, withdrawn and onloan
Comment 5 Jonathan Druart 2021-06-22 13:57:09 UTC
Created attachment 122285 [details] [review]
Bug 3142: Exclude damaged items
Comment 6 Jonathan Druart 2021-06-22 13:57:14 UTC
Created attachment 122286 [details] [review]
Bug 3142: Add itype
Comment 7 Jonathan Druart 2021-06-22 13:57:18 UTC
Created attachment 122287 [details] [review]
Bug 3142: Use filter_by_for_holds where holdability is calculated
Comment 8 Jonathan Druart 2021-06-22 13:58:27 UTC
Patches for discussion.

This look like it's going into the right direction, are these patches correct?

(Tests are missing of course)
Comment 9 David Cook 2021-06-22 23:16:07 UTC
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.
Comment 10 Jonathan Druart 2021-06-23 08:28:35 UTC
Created attachment 122314 [details] [review]
Bug 3142: Add itemlost, withdrawn and notforloan
Comment 11 Jonathan Druart 2021-06-23 08:28:40 UTC
Created attachment 122315 [details] [review]
Bug 3142: Exclude damaged items
Comment 12 Jonathan Druart 2021-06-23 08:28:45 UTC
Created attachment 122316 [details] [review]
Bug 3142: Add itype
Comment 13 Jonathan Druart 2021-06-23 08:28:49 UTC
Created attachment 122317 [details] [review]
Bug 3142: Use filter_by_for_holds where holdability is calculated
Comment 14 Jonathan Druart 2021-06-23 08:29:32 UTC
(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
Comment 15 Jonathan Druart 2021-07-15 08:53:06 UTC
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)
Comment 16 Jonathan Druart 2021-09-20 10:35:19 UTC
*** Bug 28667 has been marked as a duplicate of this bug. ***
Comment 17 Martin Renvoize 2021-10-05 11:31:36 UTC
Created attachment 125727 [details] [review]
Bug 3142: Add itemlost, withdrawn and notforloan

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-10-05 11:31:42 UTC
Created attachment 125728 [details] [review]
Bug 3142: Exclude damaged items

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-10-05 11:31:48 UTC
Created attachment 125729 [details] [review]
Bug 3142: Add itype

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-10-05 11:31:53 UTC
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>
Comment 21 Martin Renvoize 2021-10-05 11:33:28 UTC
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.
Comment 22 Marcel de Rooy 2021-10-05 11:39:22 UTC
(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?
Comment 23 Marcel de Rooy 2021-10-05 11:41:01 UTC
+    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.
Comment 24 Jonathan Druart 2021-10-05 12:02:08 UTC
(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.
Comment 25 Jonathan Druart 2021-10-05 12:07:53 UTC
(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.
Comment 26 Jonathan Druart 2021-10-05 12:17:45 UTC
Created attachment 125732 [details] [review]
Bug 3142: Add tests

And fix a bug!
Comment 27 Marcel de Rooy 2021-10-05 12:21:26 UTC
(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
Comment 28 Marcel de Rooy 2021-10-05 12:22:45 UTC
Will QA this one soon.
Comment 29 Jonathan Druart 2021-10-05 12:33:14 UTC
Created attachment 125733 [details] [review]
Bug 3142: Add test for the circ rule
Comment 30 Jonathan Druart 2021-10-05 12:33:18 UTC
Created attachment 125734 [details] [review]
Bug 3142: Add note about imprecision of filter_by_for_hold
Comment 31 Jonathan Druart 2021-10-05 12:35:26 UTC
(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.
Comment 32 Marcel de Rooy 2021-10-08 07:19:47 UTC
QA second try
Comment 33 Marcel de Rooy 2021-10-08 09:26:00 UTC
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>
Comment 34 Marcel de Rooy 2021-10-08 09:26:04 UTC
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>
Comment 35 Marcel de Rooy 2021-10-08 09:26:08 UTC
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>
Comment 36 Marcel de Rooy 2021-10-08 09:26:12 UTC
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>
Comment 37 Marcel de Rooy 2021-10-08 09:26:16 UTC
Created attachment 125946 [details] [review]
Bug 3142: Add tests

And fix a bug!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2021-10-08 09:26:20 UTC
Created attachment 125947 [details] [review]
Bug 3142: Add test for the circ rule

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2021-10-08 09:26:24 UTC
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>
Comment 40 Marcel de Rooy 2021-10-08 09:26:28 UTC
Created attachment 125949 [details] [review]
Bug 3142: (QA follow-up) Cosmetic changes

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2021-10-08 09:26:32 UTC
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>
Comment 42 Marcel de Rooy 2021-10-08 09:27:07 UTC
 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.
Comment 43 Jonathan Druart 2021-10-08 09:58:24 UTC
(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 :-/
Comment 44 Jonathan Druart 2021-10-08 10:03:58 UTC
Created attachment 125951 [details] [review]
Bug 3142: Handle biblio level item type
Comment 45 Jonathan Druart 2021-10-08 10:04:13 UTC
(In reply to Jonathan Druart from comment #44)
> Created attachment 125951 [details] [review] [review]
> Bug 3142: Handle biblio level item type

Something like that?
Comment 46 Marcel de Rooy 2021-10-08 10:50:45 UTC
(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.
Comment 47 Marcel de Rooy 2021-10-08 10:55:05 UTC
Bug 29106 comment14
Comment 48 Jonathan Druart 2021-10-08 13:06:03 UTC
Created attachment 125956 [details] [review]
Bug 3142: Add tests for item-level_itypes=0
Comment 49 Jonathan Druart 2021-10-08 13:06:23 UTC
QA stamp needed on the last 2 patches.
Comment 50 Jonathan Druart 2021-10-08 13:07:34 UTC
(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.
Comment 51 Nick Clemens 2021-10-08 14:31:11 UTC
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>
Comment 52 Nick Clemens 2021-10-08 14:31:16 UTC
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>
Comment 53 Nick Clemens 2021-10-08 14:31:19 UTC
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>
Comment 54 Nick Clemens 2021-10-08 14:31:24 UTC
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>
Comment 55 Nick Clemens 2021-10-08 14:31:31 UTC
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>
Comment 56 Nick Clemens 2021-10-08 14:31:37 UTC
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>
Comment 57 Nick Clemens 2021-10-08 14:31:41 UTC
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>
Comment 58 Nick Clemens 2021-10-08 14:31:45 UTC
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>
Comment 59 Nick Clemens 2021-10-08 14:31:52 UTC
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>
Comment 60 Nick Clemens 2021-10-08 14:31:57 UTC
Created attachment 125976 [details] [review]
Bug 3142: Handle biblio level item type

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 61 Nick Clemens 2021-10-08 14:32:01 UTC
Created attachment 125977 [details] [review]
Bug 3142: Add tests for item-level_itypes=0

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 62 Jonathan Druart 2021-10-08 15:01:23 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 63 Martin Renvoize 2021-10-08 18:43:35 UTC
Great teamwork there's guys, so nice to see this go even further since I last looked.  Great effort
Comment 64 Marcel de Rooy 2021-10-11 13:07:31 UTC
(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 :)
Comment 65 Jonathan Druart 2021-10-11 13:15:58 UTC
(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!
Comment 66 Jonathan Druart 2021-10-27 12:37:22 UTC
+            $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
Comment 67 Jonathan Druart 2021-10-29 13:40:37 UTC
(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
Comment 68 Andrew Fuerste-Henry 2022-02-10 20:27:50 UTC
Pushed to 21.05.x for 21.05.11
Comment 69 Victor Grousset/tuxayo 2022-02-18 23:10:42 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.

(congrats on closing a 13 years old ticket!!!)