Bug 8559

Summary: conflicting item statuses
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P3 CC: chris, gmcharlt, jcamins, jonathan.druart, kyle.m.hall, kyle, mtj, paul.poulain
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: conflicting statuses
Bug 8559 - conflicting item statuses
Bug 8559 - Conflicting item statuses - Force cancel waiting hold
Bug 8559 - Conflicting item statuses - Make issuing a waiting item impossible.
Bug 8559 - conflicting item statuses - Force cancel or revert
Bug 8559 - conflicting item statuses - Force cancel or revert
Bug 8559 - conflicting item statuses - Force cancel or revert
[SIGNED-OFF] Bug 8559 - conflicting item statuses - Force cancel or revert
Bug 8559 - conflicting item statuses - QA Followup
Bug 8559 - conflicting item statuses - Force cancel or revert
Bug 8559 - conflicting item statuses - QA Followup

Description Nicole C. Engard 2012-08-02 12:22:58 UTC
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.
Comment 1 Kyle M Hall 2012-09-14 18:44:50 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-09-14 18:46:17 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-09-14 19:04:17 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-09-14 19:06:39 UTC
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.
Comment 5 Owen Leonard 2012-09-14 19:51:59 UTC
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?
Comment 6 Kyle M Hall 2012-09-17 13:10:57 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2012-09-17 13:11:52 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2012-09-17 14:24:46 UTC
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.
Comment 9 Kyle M Hall 2012-09-17 14:44:17 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-09-17 14:45:03 UTC
(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.
Comment 11 Owen Leonard 2012-09-17 15:03:48 UTC Comment hidden (obsolete)
Comment 12 Mason James 2012-10-03 01:27:55 UTC
(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
Comment 13 Kyle M Hall 2012-10-03 11:48:47 UTC
(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.
Comment 14 Mason James 2012-10-03 20:49:48 UTC
(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
Comment 15 Kyle M Hall 2012-10-04 11:09:14 UTC
> 
> 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!
Comment 16 Kyle M Hall 2012-10-04 11:11:46 UTC Comment hidden (obsolete)
Comment 17 Paul Poulain 2012-10-28 17:55:36 UTC
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
Comment 18 Mason James 2012-11-01 10:33:30 UTC
(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
Comment 19 Mason James 2012-11-01 10:53:20 UTC
(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.
Comment 20 Jared Camins-Esakov 2012-11-01 19:34:22 UTC
String changes. Will hold for post-3.10.0.
Comment 21 Jonathan Druart 2012-11-19 14:10:51 UTC
Previous QA comments must be ignored. The warning comes from a new defined routine (and caused by the circulars dependencies).

Marked as Passed QA.
Comment 22 Jared Camins-Esakov 2012-11-25 23:23:37 UTC
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.
Comment 23 Jonathan Druart 2012-11-26 09:24:00 UTC
(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! :)
Comment 24 Jonathan Druart 2012-11-26 09:24:32 UTC
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>
Comment 25 Jonathan Druart 2012-11-26 09:24:39 UTC
Created attachment 13669 [details] [review]
Bug 8559 - conflicting item statuses - QA Followup

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 26 Jared Camins-Esakov 2012-11-28 22:09:55 UTC
This patch has been pushed to master.
Comment 27 Chris Cormack 2012-11-29 07:40:12 UTC
Bugfix, was held because of the string freeze, pushed to 3.10.x will be in 3.10.1