Under very specific circumstances[1] when a librarian tries to check out an item that is ALREADY checked out to another patron, one of these two nasty things happen: - If you're in Koha>=3.20.06 Software error: DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '1204321' for key 'itemnumber' at /home/koha/src/C4/Circulation.pm line 1272 - if you're in Koha<3.20.06 A DUPLICATE entry is entered in issues table [1] according to my tests, for the issue to appear, the item must belong to (or must be originally check out to a patron of) a DIFFERENT branch than the one currently trying to check it out!
Steps to reproduce: - Check out item X in patron A (item belonging to branch B1) - Go to circulation, select patron B (from branch B2!=B1) - Try to check-out the same item X to patron B - You get a warning (Please confirm checkout Item X is checked out to A. Check in and check out?). Click Yes - You get either the Duplicate entry error / or the duplicate entry in issues table [update] Newer tests indicate that the only requirement to get the error is: ITEM X must belong to a different branch than patron A/B (regardless of which branch patron A and patron B belong to).
I do not reproduce the error with the following steps: - Check an item (home library is L1) out to Patron P1 (from library L2) - Check the same item out to patron P2 (from library L3) Note that I do not use a fresh DB created from 3.20.06, it could make a difference. Could you please confirm you do not get any errors in the Koha log file about the checkin? This error should not appear if the checkin has been correctly done.
@Jonathan: Unfortunately, I cannot reproduce anymore the error for Koha<3.20.06 because we have upgraded both our test/production servers to 3.20.10. All I know is that BEFORE the upgrade we had 3 cases where duplicate lines (with the same itemnumber) were found in issues table. This means an item was found to be checked out to two different borrowers (at the same time). (I have a screenshot to prove it for one of them). After discussing with the librarians, they explained that all they did was they have checked out an already-checked out item to another patron. They DIDN'T get any error and the item ended up to be checkout to BOTH patron. With 3.20.10 now, the new error that appears means that Circulation.pm TRIES to create a duplicate line in issues (but fails), so the item is NOT checked out to the new patron in the end (but it would have been if it wasn't for the new unique key in the DB). The correct behavior is the item to be checked in and then checked out to the new patron.
@Jonathan: Can you please run: SELECT itemnumber,COUNT(*) AS nb FROM issues GROUP BY itemnumber HAVING nb > 1; If you get any lines, then you DO have the problem. You simple don't get the error because you currently don't have the Unique Key constraint in the DB
(In reply to Theodoros Theodoropoulos from comment #4) > @Jonathan: > > Can you please run: > SELECT itemnumber,COUNT(*) AS nb FROM issues GROUP BY itemnumber HAVING nb > > 1; > > If you get any lines, then you DO have the problem. You simple don't get the > error because you currently don't have the Unique Key constraint in the DB I have the unique key constraint.
Are you able to reproduce the error on a sandbox or using the master branch?
I couldn't reproduce it in a testing environment running v3.22.06.005 But I reproduced it several times in our production/test systems running v3.20.10 Maybe(?) it's also related to a system preference that we have and/or patron/item specific value. (I also created MT18564, I'll give you login details for our test server and example patron/items for you to play with) Thank you for your time. Oh, and I forgot to mention that credits also go to Fridolin Somers for pointing out that we still had items in this status and identifying them to us. When we first encountered the issue, we fixed it from Koha GUI. Thinking that it was a one-of issue, we didn't pay much attention to it until it happened another 2 times in a short period of time.
I've tested changing sysprefs in my v3.22.06.005 sandbox and it seems that one of the following parameters is related to the error! AutomaticItemReturn: Don't UseBranchTransferLimits: Enforce library transfer limits based on Item type IndependentBranches: Don't prevent AllowReturnToBranch: Allow materials to be returned to only the library the item is from AutomaticItemReturn: Don't automatically transfer items to their home library when they are returned. @Jonathan, can you please set them in your test installation and verify if you get the duplicate key error? ...we're getting close...
The culprit /seems/ to be AllowReturnToBranch. (Setting it to "Any library" prevents you from getting the error. Setting it to "Allow materials to be returned to only the library the item is from" produces the error)
(In reply to Theodoros Theodoropoulos from comment #9) > The culprit /seems/ to be AllowReturnToBranch. > > (Setting it to "Any library" prevents you from getting the error. Setting it > to "Allow materials to be returned to only the library the item is from" > produces the error) So do you think in this case Koha should not allow to perform the checkin+checkout ? Since checkin needs a transfer.
> So do you think in this case Koha should not allow to perform the > checkin+checkout ? Since checkin needs a transfer. Well, this sounds like the correct way to go... In any case, the process should not try to chekout the item to another patron (and try to create the duplicate line in the issues table) because the checkin process (no matter what the reason) was not complete. [Hmm... Maybe add an extra step in the checkin process to check if the checkin process completed without errors and only then proceed with the checkout?]
Created attachment 51687 [details] [review] Bug 16534: Do not process the checkout if the checkin has failed
This patch should do the trick, but I will need to write some tests to make it pushable.
Created attachment 51697 [details] [review] Bug 16534: Add tests for CanBookBeIssued & AllowReturnToBranch
Created attachment 51698 [details] [review] Bug 16534: Make CanBookBeIssued test if the issue can be returned If an issue is already checked out, CanBookBeIssued must check if the issue can be checked in before processing the return. In such cases (depending of the AllowReturnToBranch pref), the issue should not be allowed. Prior to this patch, the checkin was not done and the checkout failed with "Duplicate entry '1204321' for key 'itemnumber'". Indeed since bug 14978, there is an uniq key on issues.itemnumber. Before bug 14978 the issue existed but was hidden (and some weird behaviors certainly happened!). To avoid Koha to crash, a check is added to CanBookBeIssued (call to CanBookBeReturned) and the librarian is not able to process the checkout. Test plan: - Set AllowReturnToBranch to anywhere - Check an item (homebranch Library 1, holding branch Library 1) out from Library 1 - Check the item out from Library 2 => Confirm the checkout (should work with and without this patch) - Set AllowReturnToBranch to holdinbranch ("only the library the item was checked out from"). - Check an item (homebranch Library 1, holding branch Library 1) out from Library 1 - Check the item out from Library 2 => Without this patch, Koha crashed => With this patch, you will be warned that the checkin is not possible. Try other combinations of the AllowReturnToBranch syspref
Created attachment 51699 [details] [review] Bug 16534: Add tests for AddIssue
Created attachment 51700 [details] [review] Bug 16534: Block AddIssue from issuing if the return is not possible To make sure the return can be done, AddIssue must not trust callers (they should have done their job, but we are not sure) and check that the issue can be returned before issuing to the patron. There is no test plan here, this should not be possible from the Koha interface. However, looking at the code, it may be possible using SIP.
(In reply to Jonathan Druart from comment #17) > Created attachment 51700 [details] [review] [review] > Bug 16534: Block AddIssue from issuing if the return is not possible > > To make sure the return can be done, AddIssue must not trust callers (they > should have done their job, but we are not sure) and check that the issue can > be returned before issuing to the patron. > > There is no test plan here, this should not be possible from the Koha > interface. > However, looking at the code, it may be possible using SIP. This commit message is awful and does not meant anything but I hope it will be understood anyway :D
I think this is a critical for versions with and without bug 13790 and bug 14978. Feel free to lower the severity if you think I am wrong.
Reproduced on current master. Testing... All patches applied. Error is gone. Test t/db_dependent/Circulation.t pass (with empty tables reserves, old_reserves, old_issues) Reset, apply patches for t/db_dependent/Circulation.t only Test t/db_dependent/Circulation.t do not pass (as expected) Going to sign-off
Created attachment 51706 [details] [review] Bug 16534: Add tests for CanBookBeIssued & AllowReturnToBranch Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 51707 [details] [review] Bug 16534: Make CanBookBeIssued test if the issue can be returned If an issue is already checked out, CanBookBeIssued must check if the issue can be checked in before processing the return. In such cases (depending of the AllowReturnToBranch pref), the issue should not be allowed. Prior to this patch, the checkin was not done and the checkout failed with "Duplicate entry '1204321' for key 'itemnumber'". Indeed since bug 14978, there is an uniq key on issues.itemnumber. Before bug 14978 the issue existed but was hidden (and some weird behaviors certainly happened!). To avoid Koha to crash, a check is added to CanBookBeIssued (call to CanBookBeReturned) and the librarian is not able to process the checkout. Test plan: - Set AllowReturnToBranch to anywhere - Check an item (homebranch Library 1, holding branch Library 1) out from Library 1 - Check the item out from Library 2 => Confirm the checkout (should work with and without this patch) - Set AllowReturnToBranch to holdinbranch ("only the library the item was checked out from"). - Check an item (homebranch Library 1, holding branch Library 1) out from Library 1 - Check the item out from Library 2 => Without this patch, Koha crashed => With this patch, you will be warned that the checkin is not possible. Try other combinations of the AllowReturnToBranch syspref Followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 51708 [details] [review] Bug 16534: Add tests for AddIssue Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 51709 [details] [review] Bug 16534: Block AddIssue from issuing if the return is not possible To make sure the return can be done, AddIssue must not trust callers (they should have done their job, but we are not sure) and check that the issue can be returned before issuing to the patron. There is no test plan here, this should not be possible from the Koha interface. However, looking at the code, it may be possible using SIP. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 51710 [details] [review] Bug 16534: (followup) Correct tiny typo This patch corrects "...must be return..." to "must be returned..."
Thank you all for your interest and almost instant response to the bug! It seemed critical (especially if one doesn't have patch for 14978), but it's normal that it wasn't spotted before as it appears only under certain circumstances. I have not yet tried the patch, but seeing the code and your description, I am certain that it will solve the problem that we faced!
Created attachment 52487 [details] [review] Bug 16534: Add tests for CanBookBeIssued & AllowReturnToBranch Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 52488 [details] [review] Bug 16534: Add tests for CanBookBeIssued & AllowReturnToBranch Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 52489 [details] [review] Bug 16534: Make CanBookBeIssued test if the issue can be returned If an issue is already checked out, CanBookBeIssued must check if the issue can be checked in before processing the return. In such cases (depending of the AllowReturnToBranch pref), the issue should not be allowed. Prior to this patch, the checkin was not done and the checkout failed with "Duplicate entry '1204321' for key 'itemnumber'". Indeed since bug 14978, there is an uniq key on issues.itemnumber. Before bug 14978 the issue existed but was hidden (and some weird behaviors certainly happened!). To avoid Koha to crash, a check is added to CanBookBeIssued (call to CanBookBeReturned) and the librarian is not able to process the checkout. Test plan: - Set AllowReturnToBranch to anywhere - Check an item (homebranch Library 1, holding branch Library 1) out from Library 1 - Check the item out from Library 2 => Confirm the checkout (should work with and without this patch) - Set AllowReturnToBranch to holdinbranch ("only the library the item was checked out from"). - Check an item (homebranch Library 1, holding branch Library 1) out from Library 1 - Check the item out from Library 2 => Without this patch, Koha crashed => With this patch, you will be warned that the checkin is not possible. Try other combinations of the AllowReturnToBranch syspref Followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 52490 [details] [review] Bug 16534: Add tests for AddIssue Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 52491 [details] [review] Bug 16534: Block AddIssue from issuing if the return is not possible To make sure the return can be done, AddIssue must not trust callers (they should have done their job, but we are not sure) and check that the issue can be returned before issuing to the patron. There is no test plan here, this should not be possible from the Koha interface. However, looking at the code, it may be possible using SIP. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 52492 [details] [review] Bug 16534: (followup) Correct tiny typo This patch corrects "...must be return..." to "must be returned..." Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 52493 [details] [review] Bug 16534: (followup) - Tidy AddIssue Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 16.11! Thanks Jonathan!
Pushed in 16.05. Will be in 16.05.02.
Patches pushed to 3.22.x, will be in 3.22.10