Bug 19530 - Prevent multiple transfers from existing for one item
Summary: Prevent multiple transfers from existing for one item
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P1 - high normal (vote)
Assignee: Kyle M Hall
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-25 18:15 UTC by Kyle M Hall
Modified: 2018-03-08 13:56 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19530 - Prevent multiple transfers from existing for one item (2.38 KB, patch)
2017-10-25 18:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19530 - Prevent multiple transfers from existing for one item (2.43 KB, patch)
2017-10-26 01:30 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 19530: Added commentary for last test when prove -v (1.16 KB, patch)
2017-10-26 01:30 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 19530 - Added commentary for last test when prove -v (1.22 KB, patch)
2018-01-30 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19530 - Don't update the date arrived for closed transfers (1.06 KB, patch)
2018-01-30 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19530: Prevent multiple transfers from existing for one item (2.49 KB, patch)
2018-02-05 16:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19530: Added commentary for last test when prove -v (1.27 KB, patch)
2018-02-05 16:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19530: Don't update the date arrived for closed transfers (1.12 KB, patch)
2018-02-05 16:15 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-10-25 18:15:51 UTC
We have found multiple open branch transfers for a given item. While I'm not sure how it is happening at this point, it seems that it would make sense to cancel any existing branch transfers when initiating a new one.
Comment 1 Kyle M Hall 2017-10-25 18:25:02 UTC
Created attachment 68541 [details] [review]
Bug 19530 - Prevent multiple transfers from existing for one item

We have found multiple open branch transfers for a given item. While I'm not sure how it is happening at this point, it seems that it would make sense to cancel any existing branch transfers when initiating a new one.

Test Plan:
1) prove t/db_dependent/Circulation/transfers.t
Comment 2 M. Tompsett 2017-10-26 01:30:22 UTC
Created attachment 68573 [details] [review]
Bug 19530 - Prevent multiple transfers from existing for one item

We have found multiple open branch transfers for a given item. While I'm not sure how it is happening at this point, it seems that it would make sense to cancel any existing branch transfers when initiating a new one.

Test Plan:
1) prove t/db_dependent/Circulation/transfers.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 3 M. Tompsett 2017-10-26 01:30:26 UTC
Created attachment 68574 [details] [review]
Bug 19530: Added commentary for last test when prove -v

Before patch, prove -v had no comment for the last test.
After patch, it says something in line with second last test.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 4 M. Tompsett 2017-10-26 01:32:33 UTC
I figured a trivial string patch of mine isn't going to Fail QA, unless the wording is horrible.
Comment 5 Marcel de Rooy 2017-11-27 12:48:51 UTC
    $dbh->do("UPDATE branchtransfers SET datearrived = NOW(), comments = ? WHERE itemnumber = ?", undef, "Canceled, new transfer from $frombranch to $tobranch created", $itemnumber);

We should at least limit this update by adding datearrived IS NULL. No need to update here all arrived items.
In this case you might even wonder if you should not DELETE those records instead?
I am wondering if transferbook (the sub calling ModItemTransfer) should check if there is no open transfer in the table. It is possible to add multiple transfers by selecting different destinations. What do you think?
Comment 6 NancyK. 2018-01-24 19:36:22 UTC
We are getting more of these multiple in-transits happening. The item is never in any of the indicated locations.  A real scrambled mess.

NancyK
Comment 7 Kyle M Hall 2018-01-30 16:20:44 UTC
(In reply to Marcel de Rooy from comment #5)
>     $dbh->do("UPDATE branchtransfers SET datearrived = NOW(), comments = ?
> WHERE itemnumber = ?", undef, "Canceled, new transfer from $frombranch to
> $tobranch created", $itemnumber);
> 
> We should at least limit this update by adding datearrived IS NULL. No need
> to update here all arrived items.
> In this case you might even wonder if you should not DELETE those records
> instead?
> I am wondering if transferbook (the sub calling ModItemTransfer) should
> check if there is no open transfer in the table. It is possible to add
> multiple transfers by selecting different destinations. What do you think?

Good idea. I'll submit a followup for that!
Comment 8 Kyle M Hall 2018-01-30 16:23:25 UTC
Created attachment 71065 [details] [review]
Bug 19530 - Added commentary for last test when prove -v

Before patch, prove -v had no comment for the last test.
After patch, it says something in line with second last test.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2018-01-30 16:23:32 UTC
Created attachment 71066 [details] [review]
Bug 19530 - Don't update the date arrived for closed transfers
Comment 10 Kyle M Hall 2018-01-30 16:26:44 UTC
> > I am wondering if transferbook (the sub calling ModItemTransfer) should
> > check if there is no open transfer in the table. It is possible to add
> > multiple transfers by selecting different destinations. What do you think?

I'm not sure what you mean. How do you select multiple destination? Do you mean to give feedback that a previous transfer was closed? If so I think we should put that out of scope for this bug and create a new bug report to add user feedback.
Comment 11 Josef Moravec 2018-02-05 16:14:54 UTC
Created attachment 71235 [details] [review]
Bug 19530: Prevent multiple transfers from existing for one item

We have found multiple open branch transfers for a given item. While I'm not sure how it is happening at this point, it seems that it would make sense to cancel any existing branch transfers when initiating a new one.

Test Plan:
1) prove t/db_dependent/Circulation/transfers.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2018-02-05 16:14:58 UTC
Created attachment 71236 [details] [review]
Bug 19530: Added commentary for last test when prove -v

Before patch, prove -v had no comment for the last test.
After patch, it says something in line with second last test.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2018-02-05 16:15:01 UTC
Created attachment 71237 [details] [review]
Bug 19530: Don't update the date arrived for closed transfers

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Jonathan Druart 2018-02-08 19:51:40 UTC
Sounds like something is going wrong before that point and we are going to hide the problem.
Should not we add a debug message to the logs with some useful information?

Marcel, are you ok to push it?
Comment 15 Marcel de Rooy 2018-02-09 06:33:28 UTC
(In reply to Jonathan Druart from comment #14)
> Sounds like something is going wrong before that point and we are going to
> hide the problem.
> Should not we add a debug message to the logs with some useful information?
> 
> Marcel, are you ok to push it?

Will have another look today
Comment 16 Marcel de Rooy 2018-02-09 10:29:49 UTC
(In reply to Jonathan Druart from comment #14)
> Sounds like something is going wrong before that point and we are going to
> hide the problem.
> Should not we add a debug message to the logs with some useful information?
> 
> Marcel, are you ok to push it?

Yes, ok. But we could indeed add a warn, because we are combatting symptoms..
Comment 17 Jonathan Druart 2018-02-09 12:30:51 UTC
(In reply to Kyle M Hall from comment #0)
> We have found multiple open branch transfers for a given item. While I'm not
> sure how it is happening at this point, it seems that it would make sense to
> cancel any existing branch transfers when initiating a new one.

Kyle, can you give more details please? Which version? On only 1 installation or more? Using other developments (fork)?
Comment 18 Kyle M Hall 2018-02-09 15:55:35 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Kyle M Hall from comment #0)
> > We have found multiple open branch transfers for a given item. While I'm not
> > sure how it is happening at this point, it seems that it would make sense to
> > cancel any existing branch transfers when initiating a new one.
> 
> Kyle, can you give more details please? Which version? On only 1
> installation or more? Using other developments (fork)?

It's happened on a few partners. No changes to the code. We just were unable to figure out how the librarians were cause the issue. It's been happening for the 16.x and 17.x at the very least, possibly longer.
Comment 19 NancyK. 2018-02-09 16:02:21 UTC
We noticed this when we went to 17.05.
If an item on hold gets put in transit, but is thrown in the wrong bin and gets sent to the wrong library, when they check the item in, it stays in transit.  That's different behavior.  They are used to being able to just put it in transit again to the correct library. When that second in-transit happens its chaos.

NancyK
Washoe County Library System
Comment 20 NancyK. 2018-02-09 16:58:51 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2018-02-13 17:17:54 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 22 Nick Clemens 2018-02-26 11:40:55 UTC
Awesome work all! Pushed to stable for 17.11.03
Comment 23 Fridolin SOMERS 2018-03-08 09:40:52 UTC
Pushed to 17.05.x for v17.05.10
Comment 24 Victor Grousset/tuxayo 2018-03-08 13:56:54 UTC
Maybe the root cause will be found with bug 20344. As the scenarios without the patches of this bug lead to the creation of duplicates.