Description
Kyle M Hall (khall)
2017-10-25 18:15:51 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 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> 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> I figured a trivial string patch of mine isn't going to Fail QA, unless the wording is horrible. $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? 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 (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! 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> Created attachment 71066 [details] [review] Bug 19530 - Don't update the date arrived for closed transfers > > 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.
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> 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> 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> 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? (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 (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.. (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)? (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. 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 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 Pushed to master for 18.05, thanks to everybody involved! Awesome work all! Pushed to stable for 17.11.03 Pushed to 17.05.x for v17.05.10 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. *** Bug 9322 has been marked as a duplicate of this bug. *** |