Bug 11336

Summary: Priority is not updated on deleting holds
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Hold requestsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: major    
Priority: P5 - low CC: cbrannon, fridolin.somers, gmcharlt, katrin.fischer, kyle, sandboxes
Version: Main   
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: Bug 11336: Priority is not updated on deleting holds
Bug 11336: Priority is not updated on deleting holds
Bug 11336: Priority is not updated on deleting holds
Bug 11336: Priority is not updated on deleting holds
Bug 11336: [QA Followup] Priority is not updated on deleting holds
Bug 11336: (follow-up) Priority is not updated on deleting holds - moremember
Bug 11336: (follow-up) Priority is not updated on deleting holds - moremember
[PASSED QA] Bug 11336: Priority is not updated on deleting holds
[PASSED QA] Bug 11336: (follow-up) Priority is not updated on deleting holds - moremember
Bug 11336: WIP add GetOldReserve
Bug 11336: Priority is not updated on deleting holds
Bug 11336: Priority is not updated on deleting holds
[PASSED QA] Bug 11336: Priority is not updated on deleting holds
Bug 11336: Follow-up - fixing capitalization in templates

Description Jonathan Druart 2013-12-04 11:32:27 UTC
There are a lot of places where a hold deletion is possible. But I just found 1 place where it works!

To reproduce:
- select or create 2 users U1 and U2
- select or create an holdable item
- place on hold for both U1 and U2. U1 has priority 1 and U2 has priority 2.
- delete the hold for U1
- go on circ/circulation.pl?borrowernumber=XXXX for U2 (or in the DB directly) and verify the priority has not been set to 1


The issue is repeatable (at least) on these 2 pages:
 * circ/circulation.pl?borrowernumber=XXXX (tab 'Holds', select "yes" in the dropdown list and submit the form)
 * reserve/request.pl?biblionumber=XXXX (clic on the red cross)
Comment 1 Jonathan Druart 2013-12-04 11:35:36 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-12-04 11:40:59 UTC
The fix I propose is quite dirty but I did not find another fix without rewriting a big part of code.
Comment 3 Christopher Brannon 2013-12-10 23:06:47 UTC
Looks like it does what is expected.  Thanks for addressing this issue.

Christopher
Comment 4 Biblibre Sandboxes 2013-12-10 23:07:05 UTC
Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>
Comment 5 Biblibre Sandboxes 2013-12-10 23:07:23 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-12-13 15:53:26 UTC
I agree this is a bit of a dirty fix. Wouldn't it be better to modify GetReserve to look for the reserve in reserves first, and then old_reserves if it doesn't find it there?
Comment 7 Jonathan Druart 2013-12-13 16:03:13 UTC
(In reply to Kyle M Hall from comment #6)
> I agree this is a bit of a dirty fix. Wouldn't it be better to modify
> GetReserve to look for the reserve in reserves first, and then old_reserves
> if it doesn't find it there?

Hum... It could work. But how to be sure this won't introduce a regression? For ex. a logic like: if GetReserve returns undef, the reserve is deleted.
Comment 8 Jonathan Druart 2013-12-13 16:45:29 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-12-13 16:46:07 UTC
It looks good. I add regression test.
Comment 10 Kyle M Hall 2013-12-13 18:02:54 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-12-13 18:04:06 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2013-12-15 21:08:15 UTC
Jonathan, could you check the follow-up so I can do QA?
Comment 13 Jonathan Druart 2013-12-16 09:45:57 UTC
Kyle,
I disagree with your patch. The call stack could be weird:
If ModReserve is called for deleting an hold, _FixPriority will be called with $rank=del which will call CancelReserve which will call again _FixPriority.
Since the code in ModReserve (when call with rank=del) cancels a reserve, maybe the best is to call CancelReserve :)
I am submitting a new patch.
Comment 14 Jonathan Druart 2013-12-16 09:46:38 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-12-18 12:23:38 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2013-12-20 06:49:08 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-12-20 06:49:29 UTC Comment hidden (obsolete)
Comment 18 Galen Charlton 2013-12-20 15:48:26 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Kyle M Hall from comment #6)
> > I agree this is a bit of a dirty fix. Wouldn't it be better to modify
> > GetReserve to look for the reserve in reserves first, and then old_reserves
> > if it doesn't find it there?
> 
> Hum... It could work. But how to be sure this won't introduce a regression?
> For ex. a logic like: if GetReserve returns undef, the reserve is deleted.

Upon review, I prefer Jonathan's original approach.  Yes, it's a bit "dirty", but that's mostly because _FixPriority() does not have all that great an interface: if it's passed a single $reserve_id, it's really meant to fix the priority for all holds on that hold's bib; if it's passed a rank and a $reserve_id, it touches that specific hold first.

No other accessor routine currently falls back to looking in the old_* or deleted* table if it can't find a row in the main table, and I don't see a good reason to make GetReserve() different when all that is needed is to pass the biblionumber to _FixPriority.

Also, making hold priority updates after cancellation contingent on there being a row in old_reserves opens  up the possibility (admittedly, a remote one) of a race condition if anybody ever writes a cronjob that purges old_reserves.

I suggest going back to Jonathan's original approach -- most of the test cases and the follow-up can be used as is, I suspect.  For extra credit, it might be a good time to make _FixPriority accept a hashref, so that you can say:

_FixPriority({ biblionumber => $foo});

vs.

_FixPriority({ reserve_id => $foo, rank => 'del' })
Comment 19 Jonathan Druart 2013-12-20 16:09:48 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2013-12-20 16:10:19 UTC
Is this patch serves your purpose?
Comment 21 Jonathan Druart 2013-12-20 16:12:46 UTC
No, I read too quickly.
Comment 22 Jonathan Druart 2013-12-23 15:06:21 UTC Comment hidden (obsolete)
Comment 23 Biblibre Sandboxes 2013-12-27 15:31:57 UTC
Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>
Comment 24 Biblibre Sandboxes 2013-12-27 15:32:42 UTC Comment hidden (obsolete)
Comment 25 Christopher Brannon 2013-12-27 15:33:32 UTC
Works as defined.

Christopher
Comment 26 Katrin Fischer 2013-12-29 12:06:50 UTC
Working on this now...
Comment 27 Katrin Fischer 2013-12-29 12:10:28 UTC
Comment on attachment 23831 [details] [review]
Bug 11336: Priority is not updated on deleting holds

Review of attachment 23831 [details] [review]:
-----------------------------------------------------------------

::: C4/Reserves.pm
@@ +252,4 @@
>  
>      $res = GetReserve( $reserve_id );
>  
> +    Return the current reserve or the old reserve.

This POD change no longer applies - please fix in a follow-up.
Comment 28 Katrin Fischer 2013-12-29 13:01:25 UTC
Created attachment 23862 [details] [review]
[PASSED QA] Bug 11336: Priority is not updated on deleting holds

There are a lot of places where a hold deletion is possible. But I just found
1 place where it works!

To reproduce:
- select or create 2 users U1 and U2
- select or create an holdable item
- place on hold for both U1 and U2. U1 has priority 1 and U2 has priority 2.
- delete the hold for U1
- go on circ/circulation.pl?borrowernumber=XXXX for U2 (or in the DB directly) and
verify the priority has not been set to 1

The issue is repeatable (at least) on these 2 pages:
 * circ/circulation.pl?borrowernumber=XXXX (tab 'Holds', select "yes" in the
dropdown list and submit the form)
 * reserve/request.pl?biblionumber=XXXX (click on the red cross)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Reran my tests:

Preparations:
- Create holds for different patrons on a record:
  * 1st - title level hold
  * 2nd - item level hold
  * 3rd - title level hold
  * 4th - title level hold
- AllowOnShelfHolds = On/Allow (items were not checked out)

Tests:
Deleted holds from various pages, confirming bugs first,
then testing with applied patches. Reloading database
after each test.

1) Cancel holds from OPAC patron account
  /cgi-bin/koha/opac-user.pl#opac-user-holds
- Cancel 4th - ok, before and after applying the patch
- Cancel 2nd - ok, after applying the patch

2) Cancel hold from holds tab on staff detail page
  /cgi-bin/koha/reserve/request.pl?biblionumber=7

  a) Setting priority to 'del', submitting with 'Update holds'
  - Cancel first (1st) - ok, before and after
  - Cancel hold in the middle (was 3rd) - ok, before and after
  - Cancel last (was 4th) -ok, before and after

  b) Using red X
  - Repeating tests from a) - before the patch is applied holds
    get totally 'out of order' - after applying the patch, it works
    correctly

Additional tests done on this page:
- Change priority using up, down, to top, to bottom icons
- Change priority with 'toggle to lowest'

3) Cancel hold from the patron's account

  a) Check out tab - Delete? Yes, 'Cancel marked holds'
    /cgi-bin/koha/circ/circulation.pl?borrowernumber=X
  - Cancel first (1st) - ok, after applying the patch
  - Cancel hold in the middle (was 3rd) - ok, after applying the patch
  - Cancel last (was 4th) - ok, after applying the patch

  b) Details tab - Delete? yes, 'Cancel marked holds'
    /cgi-bin/koha/members/moremember.pl?borrowernumber=X
  - Cancel first (1st) - ok, after applying the patch
  - Cancel hold in the middle (was 3rd) - ok, after applying the patch
  - Cancel last (was 4th) - ok, after applying the patch

  Without the patch, holds priorities get out of order.

Additional tests done:
- Check in one item to trigger first hold
- Check in one item to trigger second hold
- Check out first item
Priorities are kept while the item is waiting, when it's
checked out, priorities of remaining holds get reset correctly.

Conclusion:
Big improvement, no regressions found.

Passes all tests in t, xt and QA script.
Also: t/db_dependent/Holds.t
      t/db_dependent/HoldsQueue.t
      t/db_dependent/Reserves.t
Comment 29 Katrin Fischer 2013-12-29 13:02:14 UTC
Created attachment 23863 [details] [review]
Bug 11336: Follow-up - fixing capitalization in templates

Fixes capitalization on the holds and check in page.

Changes are easy to spot using:
git diff HEAD^ --color-words=.
Comment 30 Galen Charlton 2014-01-04 23:28:42 UTC
Pushed to master.  Thanks, Jonathan!
Comment 31 Jonathan Druart 2014-01-06 12:09:41 UTC
Galen,

commit 543e1dc6731d4a26b777223920bfe4037e7546dd introduces a call to a nonexistent routine:

 =head2 ModReserve
@@ -1184,7 +1181,7 @@ sub ModReserveFill {
     # now fix the priority on the others (if the priority wasn't
     # already sorted!)....
     unless ( $priority == 0 ) {
-        _FixPriority({ reserve_id => $reserve_id });
+        FixPriority({ reserve_id => $reserve_id });
     }
 }

The FixPriority routine does not exist.
Comment 32 Jonathan Druart 2014-01-07 13:41:04 UTC
Fixed by commit 7af64ff7bd26eaa0d8cdb688f0db1526e438f087
Bug 11336: (follow-up) fix typo in previous follow-up
Comment 33 Fridolin Somers 2014-02-18 07:48:53 UTC
Patches pushed to 3.14.x, will be in 3.14.4.