[Bug 28057] New: Confusion of biblionumber and biblioitemnumber in request.pl
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Bug ID: 28057 Summary: Confusion of biblionumber and biblioitemnumber in request.pl Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com 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 }) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |joonas.kylmala@helsinki.fi, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com, tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 119598 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119598&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andreas.jonsson@kreablo.se --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- *** Bug 26523 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #3 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Blou <philippe.blouin@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |philippe.blouin@inlibro.com --- Comment #5 from Blou <philippe.blouin@inlibro.com> --- *** Bug 28310 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Blou, can you signoff this one please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119598|0 |1 is obsolete| | --- Comment #7 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 123250 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123250&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #8 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- 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?
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #10 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #11 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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: -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #12 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 123266 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123266&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123250|0 |1 is obsolete| | Attachment #123266|0 |1 is obsolete| | --- Comment #14 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 123278 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123278&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #15 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 123279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123279&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |joonas.kylmala@helsinki.fi |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #18 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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? :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- (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 :-) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #20 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.11.00 |21.11.00,21.05.03 released in| | --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.03 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|21.11.00,21.05.03 |21.11.00,21.05.03,20.11.09 released in| | --- Comment #22 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.09 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Pushed to oldstable |Pushed to oldoldstable Version(s)|21.11.00,21.05.03,20.11.09 |21.11.00,21.05.03,20.11.09, released in| |20.05.15 --- Comment #23 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Backported: Pushed to 20.05.x branch for 20.05.15 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 wainuiwitikapark@catalyst.net.nz changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz --- Comment #24 from wainuiwitikapark@catalyst.net.nz --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #25 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #26 from wainuiwitikapark@catalyst.net.nz --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #27 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #28 from wainuiwitikapark@catalyst.net.nz --- Not backported to 19.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 --- Comment #29 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28057 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29660 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org