Created attachment 12249 [details] [review] Bug 8559 - conflicting item statuses If you check out a waiting hold to a different patron it gives the item conflicting statuses. It will both be checked out for one patron, and also waiting for a different patron. This patch solves the problem by forcing the librarian to cancel the hold if they inist on checking the item out to a different patron. Created attachment 12250 [details] [review] Bug 8559 - Conflicting item statuses - Force cancel waiting hold If you check out a waiting hold to a different patron it gives the item conflicting statuses. It will both be checked out for one patron, and also waiting for a different patron. This patch solves the problem by forcing the librarian to cancel the hold if they inist on checking the item out to a different patron. Created attachment 12252 [details] [review] Bug 8559 - Conflicting item statuses - Make issuing a waiting item impossible. Note, these are two *alternative* patches to solve this problem. The first one solves it be automatically canceling any waiting reserves when force issuing and item. The latter patch simply disallows issuing the item. I'm personally a fan of "Make issuing a waiting item impossible". I imagine patrons will be upset if they find out an item that should have been waiting for them is not. If they need to force the issue, the librarian can modify the waiting hold first, without deleting it completely. There are two situations I can think of off the top of my head when a librarian would want to check out a waiting item to someone other than the person it is on hold for: 1. Someone else is picking up the item on behalf of that person. We get this all the time with husbands picking up books for their wives. If we want to check the item out to the person who is showing us their card, we have to override the hold and check it out to them. In this case we would want to choose to remove the hold upon checkout. 2. A book which had been made waiting for someone accidentally was put on the shelf and someone brings it up to check out. In this case we would check out the item (because it's our mistake, not the patron's) and keep the hold. In this case the proper thing would be to remove the waiting status but keep the hold at the top of the list. From IRC (http://stats.workbuffer.org/irclog/koha/2012-09-14#i_1077360): [15:09] <oleonard> khall: Sorry, but I think neither is correct. [15:09] <oleonard> The solution should be to make the item no longer waiting, but still on hold. [15:09] <oleonard> Is that not possible? [15:14] <khall> that is quite complicated [15:15] <khall> at this point, we can no longer know if the reserve was item or bib level [15:15] <khall> we could assume it is bib level and push it back on the queue as priority 1. [15:15] <khall> or assume it's item level. That would be safer [15:16] <khall> but either way, there is no way to guarantee the style of hold is preserved. [15:16] <oleonard> Safer but not as quick for the patron if it was originally biblio-level and there are multiple copies [15:17] <khall> plus, there is a likelihood that the person the hold has been waiting for has been notified by email or sms. Is there a solution where checking out in scenario 1 above is allowed but scenario 2 is not? Created attachment 12275 [details] [review] Bug 8559 - conflicting item statuses - Force cancel or revert If a librarian checks out a waiting hold to a different patron it gives the item conflicting statuses. The item will show as both checked out to the different patron, and waiting for the original patron. This patch fixes this by not allowing this situation to occurr. If a librarian attempts to issue an item that is waiting for a different patron, the system will force the librarian to choose to a) not issue the item b) issue the item, and cancel the waiting hold c) issue the item, and revert the waiting hold In this scenario, reverting the waiting hold means to push it back on the reserves queue as a hold with a priority of 1, which will push the priorities of any existing holds back by 1 as well. It will become an item level hold for the given item, as we cannot know if the hold was item-level or bib-level given the data we have about the hold. Created attachment 12276 [details] [review] Bug 8559 - conflicting item statuses - Force cancel or revert If a librarian checks out a waiting hold to a different patron it gives the item conflicting statuses. The item will show as both checked out to the different patron, and waiting for the original patron. This patch fixes this by not allowing this situation to occurr. If a librarian attempts to issue an item that is waiting for a different patron, the system will force the librarian to choose to a) not issue the item b) issue the item, and cancel the waiting hold c) issue the item, and revert the waiting hold In this scenario, reverting the waiting hold means to push it back on the reserves queue as a hold with a priority of 1, which will push the priorities of any existing holds back by 1 as well. It will become an item level hold for the given item, as we cannot know if the hold was item-level or bib-level given the data we have about the hold. I think this patch represents the best possible solution given the way we handle holds right now. I wish we didn't have to force the hold to be an item-level hold, but it's a limitation of how we store hold information. I would ask for two changes: 1. Make "revert" the default option. That is the less "destructive" of the two. 2. Change the markup of the radio buttons to allow clicking on the text: <p> <label for="cancelreserve">Cancel hold</label> <input type="radio" checked="checked" value="cancel" name="cancelreserve" id="cancelreserve" /><br /> <label for="revertreserve">Revert waiting status</label> <input type="radio" value="revert" name="cancelreserve" id="revertreserve" /> </p> The label must match the id of the input field. Note that the input name and the input id do not have to match. Created attachment 12279 [details] [review] Bug 8559 - conflicting item statuses - Force cancel or revert If a librarian checks out a waiting hold to a different patron it gives the item conflicting statuses. The item will show as both checked out to the different patron, and waiting for the original patron. This patch fixes this by not allowing this situation to occurr. If a librarian attempts to issue an item that is waiting for a different patron, the system will force the librarian to choose to a) not issue the item b) issue the item, and cancel the waiting hold c) issue the item, and revert the waiting hold In this scenario, reverting the waiting hold means to push it back on the reserves queue as a hold with a priority of 1, which will push the priorities of any existing holds back by 1 as well. It will become an item level hold for the given item, as we cannot know if the hold was item-level or bib-level given the data we have about the hold. (In reply to comment #8) > I think this patch represents the best possible solution given the way we > handle holds right now. I wish we didn't have to force the hold to be an > item-level hold, but it's a limitation of how we store hold information. > > I would ask for two changes: > > 1. Make "revert" the default option. That is the less "destructive" of the > two. > 2. Change the markup of the radio buttons to allow clicking on the text: > > <p> > <label for="cancelreserve">Cancel hold</label> > <input type="radio" checked="checked" value="cancel" > name="cancelreserve" id="cancelreserve" /><br /> > <label for="revertreserve">Revert waiting status</label> > <input type="radio" value="revert" name="cancelreserve" > id="revertreserve" /> > </p> > > The label must match the id of the input field. Note that the input name and > the input id do not have to match. Good idea, and very nice improvements to the html. Created attachment 12281 [details] [review] [SIGNED-OFF] Bug 8559 - conflicting item statuses - Force cancel or revert If a librarian checks out a waiting hold to a different patron it gives the item conflicting statuses. The item will show as both checked out to the different patron, and waiting for the original patron. This patch fixes this by not allowing this situation to occurr. If a librarian attempts to issue an item that is waiting for a different patron, the system will force the librarian to choose to a) not issue the item b) issue the item, and cancel the waiting hold c) issue the item, and revert the waiting hold In this scenario, reverting the waiting hold means to push it back on the reserves queue as a hold with a priority of 1, which will push the priorities of any existing holds back by 1 as well. It will become an item level hold for the given item, as we cannot know if the hold was item-level or bib-level given the data we have about the hold. Signed-off-by: Owen Leonard <oleonard@myacpl.org> All three cases tested, correct outcome each time (In reply to comment #11) > Created attachment 12281 [details] [review] > [SIGNED-OFF] Bug 8559 - conflicting item statuses - Force cancel or revert > Signed-off-by: Owen Leonard <oleonard@myacpl.org> > > All three cases tested, correct outcome each time just a little QA thing... please fix patch so RevertWaitingStatus takes a scalar (In reply to comment #12) > (In reply to comment #11) > > Created attachment 12281 [details] [review] > > [SIGNED-OFF] Bug 8559 - conflicting item statuses - Force cancel or revert > > Signed-off-by: Owen Leonard <oleonard@myacpl.org> > > > > All three cases tested, correct outcome each time > > > just a little QA thing... > > please fix patch so RevertWaitingStatus takes a scalar I don't see a compelling reason to do that, there are plenty of subroutines that pass hashes for parameters. Perhaps its because the subroutine only takes one parameter now, but we don't know what parameters will be added in the future. I believe that we should move to using hashes for all future subroutines. How many subroutines does Koha have that have a ton of parameters? And it's even better when half the passed in params are undef. If we switch to hashes for parameters, rather than arrays, it will not only simplify our code, but make it more self-documenting. (In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > Created attachment 12281 [details] [review] > > > [SIGNED-OFF] Bug 8559 - conflicting item statuses - Force cancel or revert > > > Signed-off-by: Owen Leonard <oleonard@myacpl.org> > > > > > > All three cases tested, correct outcome each time > > > > > > just a little QA thing... > > > > please fix patch so RevertWaitingStatus takes a scalar > > I don't see a compelling reason to do that, there are plenty of subroutines > that pass hashes for parameters. hmm, no those 'plently of subroutines' are passing *hashrefs*, not hashes so, if you want to pass a hash, pass a hashref instead why? its faster and uses less memory :) "Use references If you work with large arrays or hashes and use them as arguments to functions, use a reference instead of the variable directly. By using a reference, you tell the function to point to the information. Without a reference, you copy the entire array or hash onto the function call stack, and then copy it again in the function. References also save memory (which reduces footprint and management overheads) and simplify your programming." http://www.ibm.com/developerworks/library/l-optperl/index.html >
> hmm, no
>
> those 'plently of subroutines' are passing *hashrefs*, not hashes
> so, if you want to pass a hash, pass a hashref instead
>
> why? its faster and uses less memory :)
>
>
> "Use references
>
> If you work with large arrays or hashes and use them as arguments to
> functions, use a reference instead of the variable directly. By using a
> reference, you tell the function to point to the information. Without a
> reference, you copy the entire array or hash onto the function call stack,
> and then copy it again in the function. References also save memory (which
> reduces footprint and management overheads) and simplify your programming."
>
> http://www.ibm.com/developerworks/library/l-optperl/index.html
Point taken : )=
I'll post a followup to switch it to a hashref then!
Created attachment 12679 [details] [review] Bug 8559 - conflicting item statuses - QA Followup QA comment: * This patch fails koha-qa.pl, but I don't understand well what it means. Mason, explanation welcomed : * C4/Reserves.pm FAIL pod OK forbidden patterns OK valid FAIL Subroutine RevertWaitingStatus redefined critic OK (In reply to comment #17) > QA comment: > * This patch fails koha-qa.pl, but I don't understand well what it means. > Mason, explanation welcomed : > * C4/Reserves.pm FAIL > pod OK > forbidden patterns OK > valid FAIL > Subroutine RevertWaitingStatus redefined > critic OK hi Paul the 'valid FAIL' means Reserves.pm failed a 'perl --warnings $FILE' test if we google 'perl warning Subroutine redefined' there are some good discussions of this problem http://stackoverflow.com/questions/3428264/perl-subroutine-redefined (In reply to comment #18) > (In reply to comment #17) > > QA comment: > > * This patch fails koha-qa.pl, but I don't understand well what it means. > > Mason, explanation welcomed : > > * C4/Reserves.pm FAIL > > pod OK > > forbidden patterns OK > > valid FAIL > > Subroutine RevertWaitingStatus redefined > > critic OK > > hi Paul > > the 'valid FAIL' means Reserves.pm failed a 'perl --warnings $FILE' test > > if we google 'perl warning Subroutine redefined' there are some good > discussions of this problem > > http://stackoverflow.com/questions/3428264/perl-subroutine-redefined hmmm, after some more testing, i discover something quite strange... it seems many/most of the subs() in Reserves.pm have the same error?! atm, i am not sure what is causing this problem :/ $ perl -w ./C4/Reserves.pm Subroutine AddReserve redefined at ./C4/Reserves.pm line 143. Subroutine GetReservesFromBiblionumber redefined at ./C4/Reserves.pm line 256. Subroutine GetReservesFromItemnumber redefined at ./C4/Reserves.pm line 337. Subroutine GetReservesFromBorrowernumber redefined at ./C4/Reserves.pm line 362. Subroutine CanBookBeReserved redefined at ./C4/Reserves.pm line 394. Subroutine CanItemBeReserved redefined at ./C4/Reserves.pm line 418. Subroutine GetReserveCount redefined at ./C4/Reserves.pm line 509. Subroutine GetOtherReserves redefined at ./C4/Reserves.pm line 533. Subroutine GetReserveFee redefined at ./C4/Reserves.pm line 583. Subroutine GetReservesToBranch redefined at ./C4/Reserves.pm line 686. Subroutine GetReservesForBranch redefined at ./C4/Reserves.pm line 711. Subroutine GetReserveStatus redefined at ./C4/Reserves.pm line 738. Subroutine CheckReserves redefined at ./C4/Reserves.pm line 777. Subroutine CancelExpiredReserves redefined at ./C4/Reserves.pm line 871. Subroutine AutoUnsuspendReserves redefined at ./C4/Reserves.pm line 914. Subroutine CancelReserve redefined at ./C4/Reserves.pm line 942. Subroutine ModReserve redefined at ./C4/Reserves.pm line 1057. Subroutine ModReserveFill redefined at ./C4/Reserves.pm line 1126. Subroutine ModReserveStatus redefined at ./C4/Reserves.pm line 1194. Subroutine ModReserveAffect redefined at ./C4/Reserves.pm line 1224. Subroutine ModReserveCancelAll redefined at ./C4/Reserves.pm line 1282. Subroutine ModReserveMinusPriority redefined at ./C4/Reserves.pm line 1304. Subroutine GetReserveInfo redefined at ./C4/Reserves.pm line 1330. Subroutine IsAvailableForItemLevelRequest redefined at ./C4/Reserves.pm line 1403. Subroutine AlterPriority redefined at ./C4/Reserves.pm line 1457. Subroutine ToggleLowestPriority redefined at ./C4/Reserves.pm line 1493. Subroutine ToggleSuspend redefined at ./C4/Reserves.pm line 1522. Subroutine SuspendAll redefined at ./C4/Reserves.pm line 1563. Subroutine _FixPriority redefined at ./C4/Reserves.pm line 1617. Subroutine _Findgroupreserve redefined at ./C4/Reserves.pm line 1722. Subroutine _koha_notify_reserve redefined at ./C4/Reserves.pm line 1831. Subroutine _ShiftPriorityByDateAndPriority redefined at ./C4/Reserves.pm line 1939. Subroutine MoveReserve redefined at ./C4/Reserves.pm line 1979. Subroutine MergeHolds redefined at ./C4/Reserves.pm line 2028. Subroutine RevertWaitingStatus redefined at ./C4/Reserves.pm line 2078. Subroutine ReserveSlip redefined at ./C4/Reserves.pm line 2132. String changes. Will hold for post-3.10.0. Previous QA comments must be ignored. The warning comes from a new defined routine (and caused by the circulars dependencies). Marked as Passed QA. Jonathan, (In reply to comment #21) > Previous QA comments must be ignored. The warning comes from a new defined > routine (and caused by the circulars dependencies). > > Marked as Passed QA. Did you QA this, or did you merely change the status to "Passed QA" because the error message Paul reported turned out to not be relevant? Your message does not make it clear whether you reviewed the patch and approved it or not. (In reply to comment #22) > Jonathan, > > (In reply to comment #21) > > Previous QA comments must be ignored. The warning comes from a new defined > > routine (and caused by the circulars dependencies). > > > > Marked as Passed QA. > > Did you QA this, or did you merely change the status to "Passed QA" because > the error message Paul reported turned out to not be relevant? Your message > does not make it clear whether you reviewed the patch and approved it or not. I change the status to "Passed QA" because I thought Paul and Mason didn't because of the error message. But now I realise that perhaps I misunderstood So I just reviewed these patchs and now I really pass QA it! :) Created attachment 13668 [details] [review] Bug 8559 - conflicting item statuses - Force cancel or revert If a librarian checks out a waiting hold to a different patron it gives the item conflicting statuses. The item will show as both checked out to the different patron, and waiting for the original patron. This patch fixes this by not allowing this situation to occurr. If a librarian attempts to issue an item that is waiting for a different patron, the system will force the librarian to choose to a) not issue the item b) issue the item, and cancel the waiting hold c) issue the item, and revert the waiting hold In this scenario, reverting the waiting hold means to push it back on the reserves queue as a hold with a priority of 1, which will push the priorities of any existing holds back by 1 as well. It will become an item level hold for the given item, as we cannot know if the hold was item-level or bib-level given the data we have about the hold. Signed-off-by: Owen Leonard <oleonard@myacpl.org> All three cases tested, correct outcome each time Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 13669 [details] [review] Bug 8559 - conflicting item statuses - QA Followup Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> This patch has been pushed to master. Bugfix, was held because of the string freeze, pushed to 3.10.x will be in 3.10.1 |
Created attachment 11293 [details] conflicting statuses if you check out a waiting hold to a different patron it gives the item conflicting statuses See the attached screenshot. Item was waiting for Rob, but staff checked it out Jessie, and it kept the "waiting status". So now it is both "waiting" and "checked out" - we need a way to handle this better.