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 })
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
*** Bug 26523 has been marked as a duplicate of this bug. ***
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?
(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
*** Bug 28310 has been marked as a duplicate of this bug. ***
Blou, can you signoff this one please?
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>
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?
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?
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.
(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:
(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.
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
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>
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>
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?
(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
(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? :)
(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 :-)
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.03
Pushed to 20.11.x for 20.11.09
Backported: Pushed to 20.05.x branch for 20.05.15
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.
(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
(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
(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
Not backported to 19.11.x
(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.