|
Description
Kyle M Hall (khall)
2014-01-29 15:51:14 UTC
Created attachment 24866 [details] [review] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds The current holds behavior in Koha allows a situation like this: - Patron A has an item currently checked out. - Patron B places a hold on the next available copy of that title. - Then Patron A will not be able to renew his item, even if there are other available copies of that title that could potentially fill Patron B's hold. Since this seems unfair to Patron A, we should allow renewal of items even if there are unfilled holds, but those holds could all be filled with currently available items. Test Plan: 1) Apply this patch 2) Create a record with two items 3) Check out the item to a patron 4) Place a hold on the record 5) Note you cannot renew the item for the patron 6) Enable the new system preference AllowRenewalIfOtherItemsAvailable 7) Note you can now renew the item, as all the holds can be satisfied by available items. 8) Place a second hold on the record 9) Note you can no longer renew the item, as all the holds *cannot* be filled by currently available items Created attachment 25032 [details] [review] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds The current holds behavior in Koha allows a situation like this: - Patron A has an item currently checked out. - Patron B places a hold on the next available copy of that title. - Then Patron A will not be able to renew his item, even if there are other available copies of that title that could potentially fill Patron B's hold. Since this seems unfair to Patron A, we should allow renewal of items even if there are unfilled holds, but those holds could all be filled with currently available items. Test Plan: 1) Apply this patch 2) Create a record with two items 3) Check out the item to a patron 4) Place a hold on the record 5) Note you cannot renew the item for the patron 6) Enable the new system preference AllowRenewalIfOtherItemsAvailable 7) Note you can now renew the item, as all the holds can be satisfied by available items. 8) Place a second hold on the record 9) Note you can no longer renew the item, as all the holds *cannot* be filled by currently available items Signed-off-by: Holger Meißner <h.meissner.82@web.de> Works as described. Tested and works as described. Created attachment 25148 [details] [review] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds Hi Kyle, To me the new syspref does not make sense. Don't you consider the existing behavior as a bug? I don't see a situation where the renew should be refused. Maybe could you send an email to the koha mailing list in order to ask what librarians think about the existing behavior? Switch to in discussion. I am not sure everyone will agree that this is a bug - it seems to have worked like this forever. I think with a pref we would be on the safe side. In this discussion I first tended to agree with Jonathan. We should not need a syspref to have odd behavior stay in Koha. But on the other hand will the potential items really be available? If there is only one other item, and you cannot longer find it? So maybe we should indeed stay on the safer side with Katrin :) yasp.. Agreed, I also think it's safer to pref it than to not. We can always remove the preference at a later date. Hi Kyle, could you please add the pref? Failing for now so it shows up in your queue :) (In reply to Katrin Fischer from comment #10) > Hi Kyle, could you please add the pref? Failing for now so it shows up in > your queue :) This already has a system preference, AllowRenewalIfOtherItemsAvailable! Is there something else that needs to be taken care of? Also, since this is being consider bug-ish, I think maybe we should enable the feature by default. I'll post a followup for that. If you don't think that's a good idea you can just deprecate the patch. Created attachment 25822 [details] [review] Bug 11634 [QA Followup] - Enable by default (In reply to Kyle M Hall from comment #11) > (In reply to Katrin Fischer from comment #10) > > Hi Kyle, could you please add the pref? Failing for now so it shows up in > > your queue :) > > This already has a system preference, AllowRenewalIfOtherItemsAvailable! Is > there something else that needs to be taken care of? AllowRenewalIfOtherItemsAvailable is the minimum, IMO, as I would prefer it as a per-library setting, but as a YASP, it suffices for now. > Also, since this is being consider bug-ish, I think maybe we should enable > the feature by default. I'll post a followup for that. If you don't think > that's a good idea you can just deprecate the patch. I think we've just exposed a difference in library practices, because I know of libraries that absolutely do not want renewals to be allowed if there are pending requests, and who would very much consider it a bug if they were suddenly allowed following an upgrade. Consequently, I will /not/ be pushing the follow-up as it stands now, as it changes existing behavior upon upgrade. I am neutral on the question of the default value for this one for new installations. (In reply to Galen Charlton from comment #13) Thanks for the comments! I've depped my followup. Setting to failed QA, nonetheless; such a change to the holds API needs unit tests. Created attachment 25825 [details] [review] Bug 11634 [QA Followup] - Unit Tests These new unit tests will fail due to the fact that Koha::Database uses a separate dbh handle than C4::Context->dbh Created attachment 25826 [details] [review] Make Koha::Database use C4::Context->dbh Created attachment 25827 [details] [review] Make Koha::Database use C4::Context->dbh Comment on attachment 25148 [details] [review] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds Review of attachment 25148 [details] [review]: ----------------------------------------------------------------- ::: C4/Circulation.pm @@ +2516,5 @@ > + # Get all other reserves that could have been filled by this item > + my @borrowernumbers; > + while (1) { > + my ( $reserve_found, $reserve, undef ) = > + C4::Reserves::CheckReserves( $itemnumber, undef, undef, I'm not convinced that CheckReserves() is a sufficient test -- IsAvailableForItemLevelRequest() may need to be checked as well. Otherwise, damaged, lost, and withdrawn items could "count" as available for filling the hold when that is not actually the case. Created attachment 25837 [details] [review] Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest Some older tests fail on applying these patches: # Biblio-level hold, renewal test not ok 21 - (Bug 10663) Cannot renew, reserved # Failed test '(Bug 10663) Cannot renew, reserved' # at t/db_dependent/Circulation.t line 266. # got: '0' # expected: '1' not ok 22 - (Bug 10663) Cannot renew, reserved # Failed test '(Bug 10663) Cannot renew, reserved' # at t/db_dependent/Circulation.t line 268. # got: '0' # expected: '1' These unit tests will continue to fail until both C4::Context::dbh and Koha::Database share a database handle. Galen is planning a patch to make C4::Context use the database handle from Koha::Database, but hasn't posted his patch yet. He has told me it should be ready by this Friday. (In reply to Katrin Fischer from comment #21) > Some older tests fail on applying these patches: > > # Biblio-level hold, renewal test > not ok 21 - (Bug 10663) Cannot renew, reserved > # Failed test '(Bug 10663) Cannot renew, reserved' > # at t/db_dependent/Circulation.t line 266. > # got: '0' > # expected: '1' > not ok 22 - (Bug 10663) Cannot renew, reserved > # Failed test '(Bug 10663) Cannot renew, reserved' > # at t/db_dependent/Circulation.t line 268. > # got: '0' > # expected: '1' Created attachment 28212 [details] [review] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds The current holds behavior in Koha allows a situation like this: - Patron A has an item currently checked out. - Patron B places a hold on the next available copy of that title. - Then Patron A will not be able to renew his item, even if there are other available copies of that title that could potentially fill Patron B's hold. Since this seems unfair to Patron A, we should allow renewal of items even if there are unfilled holds, but those holds could all be filled with currently available items. Test Plan: 1) Apply this patch 2) Create a record with two items 3) Check out the item to a patron 4) Place a hold on the record 5) Note you cannot renew the item for the patron 6) Enable the new system preference AllowRenewalIfOtherItemsAvailable 7) Note you can now renew the item, as all the holds can be satisfied by available items. 8) Place a second hold on the record 9) Note you can no longer renew the item, as all the holds *cannot* be filled by currently available items Signed-off-by: Holger Meissner <h.meissner.82@web.de> Signed-off-by: Chris Rohde <crohde@roseville.ca.us> Created attachment 28213 [details] [review] Bug 11634 [QA Followup] - Unit Tests These new unit tests will fail due to the fact that Koha::Database uses a separate dbh handle than C4::Context->dbh Created attachment 28214 [details] [review] Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest Created attachment 31706 [details] [review] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds The current holds behavior in Koha allows a situation like this: - Patron A has an item currently checked out. - Patron B places a hold on the next available copy of that title. - Then Patron A will not be able to renew his item, even if there are other available copies of that title that could potentially fill Patron B's hold. Since this seems unfair to Patron A, we should allow renewal of items even if there are unfilled holds, but those holds could all be filled with currently available items. Test Plan: 1) Apply this patch 2) Create a record with two items 3) Check out the item to a patron 4) Place a hold on the record 5) Note you cannot renew the item for the patron 6) Enable the new system preference AllowRenewalIfOtherItemsAvailable 7) Note you can now renew the item, as all the holds can be satisfied by available items. 8) Place a second hold on the record 9) Note you can no longer renew the item, as all the holds *cannot* be filled by currently available items Signed-off-by: Holger Meissner <h.meissner.82@web.de> Signed-off-by: Chris Rohde <crohde@roseville.ca.us> Created attachment 31707 [details] [review] Bug 11634 [QA Followup] - Unit Tests These new unit tests will fail due to the fact that Koha::Database uses a separate dbh handle than C4::Context->dbh Created attachment 31708 [details] [review] Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest Created attachment 31709 [details] [review] Bug 11634 [QA Followup 3] - Found holds should be considered unavailable Applying: Bug 11634 [QA Followup 3] - Found holds should be considered unavailable fatal: sha1 information is lacking or useless (C4/Reserves.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Created attachment 32034 [details] [review] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds The current holds behavior in Koha allows a situation like this: - Patron A has an item currently checked out. - Patron B places a hold on the next available copy of that title. - Then Patron A will not be able to renew his item, even if there are other available copies of that title that could potentially fill Patron B's hold. Since this seems unfair to Patron A, we should allow renewal of items even if there are unfilled holds, but those holds could all be filled with currently available items. Test Plan: 1) Apply this patch 2) Create a record with two items 3) Check out the item to a patron 4) Place a hold on the record 5) Note you cannot renew the item for the patron 6) Enable the new system preference AllowRenewalIfOtherItemsAvailable 7) Note you can now renew the item, as all the holds can be satisfied by available items. 8) Place a second hold on the record 9) Note you can no longer renew the item, as all the holds *cannot* be filled by currently available items Signed-off-by: Holger Meissner <h.meissner.82@web.de> Signed-off-by: Chris Rohde <crohde@roseville.ca.us> Created attachment 32035 [details] [review] Bug 11634 [QA Followup] - Unit Tests These new unit tests will fail due to the fact that Koha::Database uses a separate dbh handle than C4::Context->dbh Created attachment 32036 [details] [review] Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest Created attachment 32037 [details] [review] Bug 11634 [QA Followup 3] - Found holds should be considered unavailable Working on this now. Created attachment 32875 [details] [review] [PASSED QA] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds The current holds behavior in Koha allows a situation like this: - Patron A has an item currently checked out. - Patron B places a hold on the next available copy of that title. - Then Patron A will not be able to renew his item, even if there are other available copies of that title that could potentially fill Patron B's hold. Since this seems unfair to Patron A, we should allow renewal of items even if there are unfilled holds, but those holds could all be filled with currently available items. Test Plan: 1) Apply this patch 2) Create a record with two items 3) Check out the item to a patron 4) Place a hold on the record 5) Note you cannot renew the item for the patron 6) Enable the new system preference AllowRenewalIfOtherItemsAvailable 7) Note you can now renew the item, as all the holds can be satisfied by available items. 8) Place a second hold on the record 9) Note you can no longer renew the item, as all the holds *cannot* be filled by currently available items Signed-off-by: Holger Meissner <h.meissner.82@web.de> Signed-off-by: Chris Rohde <crohde@roseville.ca.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 32876 [details] [review] [PASSED QA] Bug 11634 [QA Followup] - Unit Tests These new unit tests will fail due to the fact that Koha::Database uses a separate dbh handle than C4::Context->dbh Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 32877 [details] [review] [PASSED QA] Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 32878 [details] [review] [PASSED QA] Bug 11634 [QA Followup 3] - Found holds should be considered unavailable Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> *** Bug 10207 has been marked as a duplicate of this bug. *** Created attachment 33492 [details] [review] Bug 11634 [QA Followup] - Make unit tests pass * Allow on shelf holds needed to be enabled * Added some error supression code for undefined string comparison Comment on attachment 33492 [details] [review] Bug 11634 [QA Followup] - Make unit tests pass Review of attachment 33492 [details] [review]: ----------------------------------------------------------------- ::: Koha/DateUtils.pm @@ +128,4 @@ > my $pref = > defined $force_pref ? $force_pref : C4::Context->preference('dateformat'); > > + my $time_format = $force_time || C4::Context->preference('TimeFormat') || q{}; Hum, why? I am not sure it is a good idea to consider that TimeFormat can be '', 0 or undef. Patches pushed to master. Thanks Kyle! |