Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds
Summary: Allow renewal of item with unfilled holds if other available items can fill t...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 10207 (view as bug list)
Depends on: 11891
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-29 15:51 UTC by Kyle M Hall
Modified: 2020-10-13 08:44 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds (9.89 KB, patch)
2014-01-29 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds (10.08 KB, patch)
2014-02-04 15:07 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds (10.13 KB, patch)
2014-02-07 18:52 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 11634 [QA Followup] - Enable by default (2.64 KB, patch)
2014-03-04 16:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup] - Unit Tests (1.57 KB, patch)
2014-03-04 17:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Make Koha::Database use C4::Context->dbh (2.22 KB, patch)
2014-03-04 18:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Make Koha::Database use C4::Context->dbh (2.13 KB, patch)
2014-03-04 18:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest (901 bytes, patch)
2014-03-05 12:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds (9.99 KB, patch)
2014-05-13 14:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup] - Unit Tests (1.69 KB, patch)
2014-05-13 14:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest (901 bytes, patch)
2014-05-13 14:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds (9.98 KB, patch)
2014-09-18 12:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup] - Unit Tests (1.70 KB, patch)
2014-09-18 12:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest (901 bytes, patch)
2014-09-18 12:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup 3] - Found holds should be considered unavailable (5.71 KB, patch)
2014-09-18 12:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds (9.98 KB, patch)
2014-10-07 13:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup] - Unit Tests (1.70 KB, patch)
2014-10-07 13:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest (901 bytes, patch)
2014-10-07 13:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11634 [QA Followup 3] - Found holds should be considered unavailable (5.51 KB, patch)
2014-10-07 13:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds (10.04 KB, patch)
2014-10-28 22:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11634 [QA Followup] - Unit Tests (1.76 KB, patch)
2014-10-28 22:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11634 [QA Followup 2] - Implement check for IsAvailableForItemLevelRequest (964 bytes, patch)
2014-10-28 22:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11634 [QA Followup 3] - Found holds should be considered unavailable (5.57 KB, patch)
2014-10-28 22:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11634 [QA Followup] - Make unit tests pass (2.15 KB, patch)
2014-11-12 15:28 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-01-29 15:51:14 UTC
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.
Comment 1 Kyle M Hall 2014-01-29 15:55:55 UTC Comment hidden (obsolete)
Comment 2 Holger Meißner 2014-02-04 15:07:00 UTC Comment hidden (obsolete)
Comment 3 Holger Meißner 2014-02-04 15:09:08 UTC
Works as described.
Comment 4 Chris Rohde 2014-02-07 18:11:21 UTC
Tested and works as described.
Comment 5 Melissa Lefebvre 2014-02-07 18:52:16 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2014-02-12 10:15:36 UTC
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.
Comment 7 Katrin Fischer 2014-02-23 20:45:01 UTC
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.
Comment 8 Marcel de Rooy 2014-02-24 08:27:01 UTC
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..
Comment 9 Kyle M Hall 2014-02-24 16:42:59 UTC
Agreed, I also think it's safer to pref it than to not. We can always remove the preference at a later date.
Comment 10 Katrin Fischer 2014-02-25 07:30:37 UTC
Hi Kyle, could you please add the pref? Failing for now so it shows up in your queue :)
Comment 11 Kyle M Hall 2014-03-04 16:27:57 UTC
(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.
Comment 12 Kyle M Hall 2014-03-04 16:29:14 UTC Comment hidden (obsolete)
Comment 13 Galen Charlton 2014-03-04 16:37:30 UTC
(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.
Comment 14 Kyle M Hall 2014-03-04 16:43:13 UTC
(In reply to Galen Charlton from comment #13)

Thanks for the comments! I've depped my followup.
Comment 15 Galen Charlton 2014-03-04 16:44:04 UTC
Setting to failed QA, nonetheless; such a change to the holds API needs unit tests.
Comment 16 Kyle M Hall 2014-03-04 17:59:22 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-03-04 18:27:11 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2014-03-04 18:28:28 UTC Comment hidden (obsolete)
Comment 19 Galen Charlton 2014-03-04 19:51:08 UTC
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.
Comment 20 Kyle M Hall 2014-03-05 12:32:14 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2014-04-27 13:26:28 UTC
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'
Comment 22 Kyle M Hall 2014-05-13 14:54:03 UTC
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'
Comment 23 Kyle M Hall 2014-05-13 14:56:24 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2014-05-13 14:56:33 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2014-05-13 14:56:42 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2014-09-18 12:23:33 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2014-09-18 12:23:56 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2014-09-18 12:23:59 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2014-09-18 12:24:02 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2014-09-29 08:06:03 UTC
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.
Comment 31 Kyle M Hall 2014-10-07 13:00:08 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2014-10-07 13:00:22 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2014-10-07 13:00:25 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2014-10-07 13:00:27 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2014-10-28 20:47:41 UTC
Working on this now.
Comment 36 Katrin Fischer 2014-10-28 22:11:25 UTC
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>
Comment 37 Katrin Fischer 2014-10-28 22:11:28 UTC
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>
Comment 38 Katrin Fischer 2014-10-28 22:11:32 UTC
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>
Comment 39 Katrin Fischer 2014-10-28 22:11:36 UTC
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>
Comment 40 Katrin Fischer 2014-11-10 00:18:00 UTC
*** Bug 10207 has been marked as a duplicate of this bug. ***
Comment 41 Kyle M Hall 2014-11-12 15:28:14 UTC
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 42 Jonathan Druart 2014-11-12 15:48:06 UTC
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.
Comment 43 Tomás Cohen Arazi 2014-11-12 17:59:05 UTC
Patches pushed to master.

Thanks Kyle!