Bug 16534 - Error when checking out already checked out item (depending on AllowReturnToBranch)
Summary: Error when checking out already checked out item (depending on AllowReturnToB...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 13790 14978
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-17 08:56 UTC by Theodoros Theodoropoulos
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

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


Attachments
Bug 16534: Do not process the checkout if the checkin has failed (3.17 KB, patch)
2016-05-21 13:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16534: Add tests for CanBookBeIssued & AllowReturnToBranch (5.26 KB, patch)
2016-05-22 09:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16534: Make CanBookBeIssued test if the issue can be returned (3.97 KB, patch)
2016-05-22 09:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16534: Add tests for AddIssue (4.34 KB, patch)
2016-05-22 09:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16534: Block AddIssue from issuing if the return is not possible (1.23 KB, patch)
2016-05-22 09:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16534: Add tests for CanBookBeIssued & AllowReturnToBranch (5.40 KB, patch)
2016-05-23 05:38 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16534: Make CanBookBeIssued test if the issue can be returned (4.14 KB, patch)
2016-05-23 05:39 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16534: Add tests for AddIssue (4.47 KB, patch)
2016-05-23 05:40 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16534: Block AddIssue from issuing if the return is not possible (1.36 KB, patch)
2016-05-23 05:42 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16534: (followup) Correct tiny typo (1.06 KB, patch)
2016-05-23 05:52 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16534: Add tests for CanBookBeIssued & AllowReturnToBranch (5.45 KB, patch)
2016-06-17 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16534: Add tests for CanBookBeIssued & AllowReturnToBranch (5.45 KB, patch)
2016-06-17 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16534: Make CanBookBeIssued test if the issue can be returned (4.19 KB, patch)
2016-06-17 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16534: Add tests for AddIssue (4.53 KB, patch)
2016-06-17 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16534: Block AddIssue from issuing if the return is not possible (1.41 KB, patch)
2016-06-17 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16534: (followup) Correct tiny typo (1.11 KB, patch)
2016-06-17 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16534: (followup) - Tidy AddIssue (14.19 KB, patch)
2016-06-17 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Theodoros Theodoropoulos 2016-05-17 08:56:00 UTC
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!
Comment 1 Theodoros Theodoropoulos 2016-05-17 09:11:24 UTC
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).
Comment 2 Jonathan Druart 2016-05-17 12:58:14 UTC
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.
Comment 3 Theodoros Theodoropoulos 2016-05-17 13:23:54 UTC
@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.
Comment 4 Theodoros Theodoropoulos 2016-05-17 13:44:23 UTC
@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
Comment 5 Jonathan Druart 2016-05-17 13:55:51 UTC
(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.
Comment 6 Jonathan Druart 2016-05-17 13:56:26 UTC
Are you able to reproduce the error on a sandbox or using the master branch?
Comment 7 Theodoros Theodoropoulos 2016-05-17 15:29:36 UTC
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.
Comment 8 Theodoros Theodoropoulos 2016-05-17 16:21:23 UTC
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...
Comment 9 Theodoros Theodoropoulos 2016-05-17 16:34:02 UTC
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)
Comment 10 Fridolin Somers 2016-05-18 10:43:41 UTC
(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.
Comment 11 Theodoros Theodoropoulos 2016-05-18 11:38:25 UTC
> 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?]
Comment 12 Jonathan Druart 2016-05-21 13:18:56 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-05-21 13:20:05 UTC
This patch should do the trick, but I will need to write some tests to make it pushable.
Comment 14 Jonathan Druart 2016-05-22 09:20:29 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2016-05-22 09:20:34 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-05-22 09:20:37 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-05-22 09:20:41 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-05-22 09:21:48 UTC
(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
Comment 19 Jonathan Druart 2016-05-22 09:23:13 UTC
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.
Comment 20 Marc Véron 2016-05-23 05:36:34 UTC
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
Comment 21 Marc Véron 2016-05-23 05:38:41 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2016-05-23 05:39:58 UTC Comment hidden (obsolete)
Comment 23 Marc Véron 2016-05-23 05:40:59 UTC Comment hidden (obsolete)
Comment 24 Marc Véron 2016-05-23 05:42:13 UTC Comment hidden (obsolete)
Comment 25 Marc Véron 2016-05-23 05:52:29 UTC Comment hidden (obsolete)
Comment 26 Theodoros Theodoropoulos 2016-05-23 11:25:39 UTC
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!
Comment 27 Kyle M Hall 2016-06-17 16:30:44 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2016-06-17 16:31:05 UTC
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>
Comment 29 Kyle M Hall 2016-06-17 16:31:12 UTC
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>
Comment 30 Kyle M Hall 2016-06-17 16:31:15 UTC
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>
Comment 31 Kyle M Hall 2016-06-17 16:31:17 UTC
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>
Comment 32 Kyle M Hall 2016-06-17 16:31:20 UTC
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>
Comment 33 Kyle M Hall 2016-06-17 16:31:23 UTC
Created attachment 52493 [details] [review]
Bug 16534: (followup) - Tidy AddIssue

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Kyle M Hall 2016-06-24 11:45:33 UTC
Pushed to master for 16.11! Thanks Jonathan!
Comment 35 Frédéric Demians 2016-08-01 11:48:52 UTC
Pushed in 16.05. Will be in 16.05.02.
Comment 36 Julian Maurice 2016-08-16 12:18:14 UTC
Patches pushed to 3.22.x, will be in 3.22.10