Bug 28057

Summary: Confusion of biblionumber and biblioitemnumber in request.pl
Product: Koha Reporter: Nick Clemens <nick>
Component: Hold requestsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: major    
Priority: P5 - low CC: andreas.jonsson, fridolin.somers, gmcharlt, jonathan.druart, joonas.kylmala, kyle, martin.renvoize, philippe.blouin, tomascohen, victor, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29660
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.03,20.11.09,20.05.15
Attachments: Bug 28057: Use the biblioitem's biblionumber for checking availability
Bug 28057: Use the biblioitem's biblionumber for checking availability
Bug 28057: (follow-up) Get the biblionumber column
Bug 28057: Use the biblioitem's biblionumber for checking availability
Bug 28057: (follow-up) Get the biblionumber column

Description Nick Clemens 2021-03-30 15:40:22 UTC
We have seen issues on sites where biblionumber and biblioitemnumbers are not the same - we hit an ISE for missing itemypes for items on record B when placing a hold on record A

biblionumber and biblioitemnumber are not guaranteed to be the same

I don't understand what is happening around this code, but I am certain this is not right:
447         $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblioitemnumber, patron => $    patron })
Comment 1 Nick Clemens 2021-04-14 18:25:07 UTC
Created attachment 119598 [details] [review]
Bug 28057: Use the biblioitem's biblionumber for checking availability

The loop here gets items from the record, plus analytic items. Because of this
we need to check more than 1 record - we decide to do this via biblioitems.

We need to preserve that, but when checking ItemsAnyAvailableAndNotRestricted we
cannot assume that the biblionumber and biblioitemnumber are the same (they should be
but this may not be the best of all possible worlds)

I simply switch the call here

To test:
1 - Apply patch
2 - Test placing holds on single bibs and multiple bibs
3 - Confirm it works as expected
Comment 2 Jonathan Druart 2021-04-15 06:40:23 UTC
*** Bug 26523 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2021-04-17 19:01:59 UTC
Sorry, but can you explain a bit more?

>The loop here gets items from the record, plus analytic items. Because of this
>we need to check more than 1 record - we decide to do this via biblioitems.

>We need to preserve that, but when checking ItemsAnyAvailableAndNotRestricted we
>cannot assume that the biblionumber and biblioitemnumber are the same (they >should be
>but this may not be the best of all possible worlds)

We have both biblioitemnumber and biblionumber on items table. biblioitems doesn't have any functionality at the moment, apart from it's 1:1 to biblio. 

So why do we need to preserve the use of biblioitemnumber?
Comment 4 Nick Clemens 2021-04-19 12:41:14 UTC
(In reply to Katrin Fischer from comment #3)
> Sorry, but can you explain a bit more?
> We have both biblioitemnumber and biblionumber on items table. biblioitems
> doesn't have any functionality at the moment, apart from it's 1:1 to biblio.
> So why do we need to preserve the use of biblioitemnumber?

We don't necessarily have to use biblioitem - but we do need to loop through multiple biblios in the case where we are dealing with analytics/host items

This bug is intended to simply fix the bug/confusion here, rather than refactor the whole script

My explanation was just to aid anyone looking, and to explain why I change the comment
Comment 5 Blou 2021-05-10 17:09:48 UTC
*** Bug 28310 has been marked as a duplicate of this bug. ***
Comment 6 Jonathan Druart 2021-06-23 12:51:50 UTC
Blou, can you signoff this one please?
Comment 7 Peter Vashchuk 2021-07-28 13:34:01 UTC
Created attachment 123250 [details] [review]
Bug 28057: Use the biblioitem's biblionumber for checking availability

The loop here gets items from the record, plus analytic items. Because of this
we need to check more than 1 record - we decide to do this via biblioitems.

We need to preserve that, but when checking ItemsAnyAvailableAndNotRestricted we
cannot assume that the biblionumber and biblioitemnumber are the same (they should be
but this may not be the best of all possible worlds)

I simply switch the call here

To test:
1 - Apply patch
2 - Test placing holds on single bibs and multiple bibs
3 - Confirm it works as expected

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 8 Joonas Kylmälä 2021-07-28 15:13:00 UTC
Even after this change I think we still calculate the availability wrong, in version f09e2ca27ee8 where the analytics item reserve support was added it seems to have really checked just based on the analytic record's reservability without considering whether a hold can be placed to the item also in the host record. The issue is that after the analytics code was merged the following incompatible code was introduced: 

>     } elsif ( $on_shelf_holds == 2 ) {

This is in IsAvailableForItemLevelRequest(). This code block checks in this case the host record's result for:

> ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron });

which as far as I understand differs from the original implementation in f09e2ca27ee8 (though back then there seemingly was no on shelf circ rules). The bug seems to have started occuring after the following commit:

Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (4e1d7a16896787)

It appears to not have taken into account that for analytic items' case we need to check the items for the biblio we are placing the hold for and not the host biblio's items.

Quite many things should be changed in our code to fix the situation. Nick, did you get a hold of what I just explained, do you see the problem?
Comment 9 Nick Clemens 2021-07-28 15:21:17 UTC
I think I follow some of the logic, this is all very complex.

I believe that this fix is still 'correct' though, i.e. we are fixing a separate issue that is definitely wrong. It leaves more work to do, but that should eb another bug.


(In reply to Joonas Kylmälä from comment #8)
> Even after this change I think we still calculate the availability wrong, in
> version f09e2ca27ee8 where the analytics item reserve support was added it
> seems to have really checked just based on the analytic record's
> reservability without considering whether a hold can be placed to the item
> also in the host record. The issue is that after the analytics code was
> merged the following incompatible code was introduced: 
> 
> >     } elsif ( $on_shelf_holds == 2 ) {
> 
> This is in IsAvailableForItemLevelRequest(). This code block checks in this
> case the host record's result for:
> 
> > ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron });
> 
> which as far as I understand differs from the original implementation in
> f09e2ca27ee8 (though back then there seemingly was no on shelf circ rules).
> The bug seems to have started occuring after the following commit:
> 
> Bug 15534 - Add the ability to prevent a patron from placing a hold on a
> record with available items (4e1d7a16896787)
> 
> It appears to not have taken into account that for analytic items' case we
> need to check the items for the biblio we are placing the hold for and not
> the host biblio's items.
> 
> Quite many things should be changed in our code to fix the situation. Nick,
> did you get a hold of what I just explained, do you see the problem?
Comment 10 Joonas Kylmälä 2021-07-28 15:54:14 UTC
This causes an regression for normal usage, to reproduce:

1. Set circ rule as follows: on shelf holds: If all unavailable
2. Add one (available) item to biblio
3. Notice that you get error message when trying to place a hold
4. Apply patch 
5. Refresh the hold page and notice the error message is gone!

This patch should not break the on shelf hold reservation rules for non-analytic record items the very least.
Comment 11 Joonas Kylmälä 2021-07-28 15:59:51 UTC
(In reply to Joonas Kylmälä from comment #10)
> This causes an regression for normal usage, to reproduce:
> 
> 1. Set circ rule as follows: on shelf holds: If all unavailable
> 2. Add one (available) item to biblio
> 3. Notice that you get error message when trying to place a hold
> 4. Apply patch 
> 5. Refresh the hold page and notice the error message is gone!
> 
> This patch should not break the on shelf hold reservation rules for
> non-analytic record items the very least.

The problem appears to be that the new variable is undefined. I added some debugging code:

+warn "Checking with: " . $biblioitem->{biblionumber} . "\n";

this produces the following log result:
[2021/07/28 15:58:11] [WARN] Checking with:
Comment 12 Joonas Kylmälä 2021-07-28 16:02:47 UTC
(In reply to Nick Clemens from comment #9)
> I believe that this fix is still 'correct' though, i.e. we are fixing a
> separate issue that is definitely wrong. It leaves more work to do, but that
> should eb another bug.

Well, we shouldn't be passing the biblionumber of the host record here at all but I don't think it makes a huge difference here whether we use biblioitemnumber or bibitem's biblionumber. So it is fine by me to merge this given the regression I just spotted is fixed.
Comment 13 Nick Clemens 2021-07-28 17:40:08 UTC
Created attachment 123266 [details] [review]
Bug 28057: (follow-up) Get the biblionumber column

When we fetch the biblioitems we use a select to limit the columns fetched,
we must include the biblionumber as well
Comment 14 Joonas Kylmälä 2021-07-29 09:31:45 UTC
Created attachment 123278 [details] [review]
Bug 28057: Use the biblioitem's biblionumber for checking availability

The loop here gets items from the record, plus analytic items. Because of this
we need to check more than 1 record - we decide to do this via biblioitems.

We need to preserve that, but when checking ItemsAnyAvailableAndNotRestricted we
cannot assume that the biblionumber and biblioitemnumber are the same (they should be
but this may not be the best of all possible worlds)

I simply switch the call here

To test:
1 - Apply patch
2 - Test placing holds on single bibs and multiple bibs
3 - Confirm it works as expected

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 15 Joonas Kylmälä 2021-07-29 09:31:49 UTC
Created attachment 123279 [details] [review]
Bug 28057: (follow-up) Get the biblionumber column

When we fetch the biblioitems we use a select to limit the columns fetched,
we must include the biblionumber as well

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 16 Jonathan Druart 2021-08-03 10:04:42 UTC
Why aren't we going further here?

IMO we should completely remove the code related to biblioitemnumber.

This comment:
405     ## Here we go backwards again to create hash of biblioitemnumber to itemnumbers
406     ## this is important when we have analytic items which may be on another record

It implies that we must use biblioitemnumber to be correct, but we actually should use biblionumber.

Is this correct?
Comment 17 Nick Clemens 2021-08-03 12:32:46 UTC
(In reply to Jonathan Druart from comment #16)
> Why aren't we going further here?
> 
> IMO we should completely remove the code related to biblioitemnumber.
> 
> This comment:
> 405     ## Here we go backwards again to create hash of biblioitemnumber to
> itemnumbers
> 406     ## this is important when we have analytic items which may be on
> another record
> 
> It implies that we must use biblioitemnumber to be correct, but we actually
> should use biblionumber.
> 
> Is this correct?

The comment is meant to describe what we are doing so that we can fix it in the future

I don't go further to keep this small for backporting, but I agree, we should rewrite much of this
Comment 18 Jonathan Druart 2021-08-03 12:48:33 UTC
(In reply to Nick Clemens from comment #17)
> I don't go further to keep this small for backporting, but I agree, we
> should rewrite much of this

I am willing to do it, can I count on you for test or QA? :)
Comment 19 Nick Clemens 2021-08-03 12:56:03 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Nick Clemens from comment #17)
> > I don't go further to keep this small for backporting, but I agree, we
> > should rewrite much of this
> 
> I am willing to do it, can I count on you for test or QA? :)

Yes, but another bug report please :-)
Comment 20 Jonathan Druart 2021-08-06 10:14:47 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 21 Kyle M Hall 2021-08-06 14:57:50 UTC
Pushed to 21.05.x for 21.05.03
Comment 22 Fridolin Somers 2021-08-11 19:57:19 UTC
Pushed to 20.11.x for 20.11.09
Comment 23 Victor Grousset/tuxayo 2021-08-16 20:59:24 UTC
Backported: Pushed to 20.05.x branch for 20.05.15
Comment 24 wainuiwitikapark 2021-08-17 01:37:49 UTC
Should this be backported to 19.11.x for 19.11.21?

Or if it contains string changes, backported to 19.11.22? As we are currently in a string freeze.
Comment 25 Nick Clemens 2021-08-23 15:47:08 UTC
(In reply to wainuiwitikapark from comment #24)
> Should this be backported to 19.11.x for 19.11.21?
> 
> Or if it contains string changes, backported to 19.11.22? As we are
> currently in a string freeze.

Not urgent, so 22 is fine if it applies
Comment 26 wainuiwitikapark 2021-09-07 01:53:42 UTC
(In reply to Nick Clemens from comment #25)
> (In reply to wainuiwitikapark from comment #24)
> > Should this be backported to 19.11.x for 19.11.21?
> > 
> > Or if it contains string changes, backported to 19.11.22? As we are
> > currently in a string freeze.
> 
> Not urgent, so 22 is fine if it applies

Hi Nick,

Unfortunately there are conflicts when applying these patches
Comment 27 Nick Clemens 2021-09-07 10:53:15 UTC
(In reply to wainuiwitikapark from comment #26)
> (In reply to Nick Clemens from comment #25)
> > (In reply to wainuiwitikapark from comment #24)
> > > Should this be backported to 19.11.x for 19.11.21?
> > > 
> > > Or if it contains string changes, backported to 19.11.22? As we are
> > > currently in a string freeze.
> > 
> > Not urgent, so 22 is fine if it applies
> 
> Hi Nick,
> 
> Unfortunately there are conflicts when applying these patches

It looks like this is not in 19.11 and can be skipped
Comment 28 wainuiwitikapark 2021-09-08 01:16:30 UTC
Not backported to 19.11.x
Comment 29 Jonathan Druart 2021-12-08 15:30:27 UTC
(In reply to Nick Clemens from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > Why aren't we going further here?
> > 
> > IMO we should completely remove the code related to biblioitemnumber.
> > 
> > This comment:
> > 405     ## Here we go backwards again to create hash of biblioitemnumber to
> > itemnumbers
> > 406     ## this is important when we have analytic items which may be on
> > another record
> > 
> > It implies that we must use biblioitemnumber to be correct, but we actually
> > should use biblionumber.
> > 
> > Is this correct?
> 
> The comment is meant to describe what we are doing so that we can fix it in
> the future
> 
> I don't go further to keep this small for backporting, but I agree, we
> should rewrite much of this

I have opened bug 29660.