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)
Created attachment 23286 [details] [review] 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 (clic on the red cross) The CancelReserve calls _FixPriority with a reserveid. _FixPriority try to retrieve the reserve with this reserveid but it does not exist anymore...
The fix I propose is quite dirty but I did not find another fix without rewriting a big part of code.
Looks like it does what is expected. Thanks for addressing this issue. Christopher
Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 23384 [details] [review] 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 (clic on the red cross) The CancelReserve calls _FixPriority with a reserveid. _FixPriority try to retrieve the reserve with this reserveid but it does not exist anymore... Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
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?
(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.
Created attachment 23522 [details] [review] 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 (clic on the red cross)
It looks good. I add regression test.
Created attachment 23531 [details] [review] 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 (clic on the red cross) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 23532 [details] [review] Bug 11336: [QA Followup] Priority is not updated on deleting holds Patch does not fix issue when canceling hold from moremember.pl. This followup fixes that issue.
Jonathan, could you check the follow-up so I can do QA?
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.
Created attachment 23567 [details] [review] Bug 11336: (follow-up) Priority is not updated on deleting holds - moremember Patch does not fix issue when canceling hold from moremember.pl. This followup fixes that issue.
Created attachment 23629 [details] [review] Bug 11336: (follow-up) Priority is not updated on deleting holds - moremember Patch does not fix issue when canceling hold from moremember.pl. This followup fixes that issue. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 23706 [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 (clic on the red cross) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Testing notes on second patch.
Created attachment 23707 [details] [review] [PASSED QA] Bug 11336: (follow-up) Priority is not updated on deleting holds - moremember Patch does not fix issue when canceling hold from moremember.pl. This followup fixes that issue. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> 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
(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' })
Created attachment 23721 [details] [review] Bug 11336: WIP add GetOldReserve
Is this patch serves your purpose?
No, I read too quickly.
Created attachment 23787 [details] [review] 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 (clic on the red cross)
Created attachment 23831 [details] [review] 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 (clic on the red cross) Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Works as defined. Christopher
Working on this now...
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.
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
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=.
Pushed to master. Thanks, Jonathan!
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.
Fixed by commit 7af64ff7bd26eaa0d8cdb688f0db1526e438f087 Bug 11336: (follow-up) fix typo in previous follow-up
Patches pushed to 3.14.x, will be in 3.14.4.