Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability
Summary: AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when che...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 9809
Blocks:
  Show dependency treegraph
 
Reported: 2015-06-05 11:48 UTC by Kyle M Hall
Modified: 2016-12-05 21:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability (1.61 KB, patch)
2015-06-05 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability (1.66 KB, patch)
2015-07-31 12:30 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability (1.66 KB, patch)
2015-07-31 12:32 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14337 [QA Followup] - Add unit test (1.54 KB, patch)
2015-08-17 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14337 - Add Unit Tests (4.16 KB, patch)
2015-08-20 11:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability (1.68 KB, patch)
2015-08-20 11:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14337 - Add Unit Tests (4.15 KB, patch)
2015-08-21 14:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability (1.68 KB, patch)
2015-08-21 14:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14337 - Add Unit Tests (4.22 KB, patch)
2015-08-25 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability (1.73 KB, patch)
2015-08-25 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14337: Fix conflict with bug 9809 (911 bytes, patch)
2015-08-25 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14337: Add 2 more tests to be complete (2.89 KB, patch)
2015-08-25 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14337 [QA Followup] - Fix conditional (1.02 KB, patch)
2016-04-27 17:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14337 [QA Followup] - Fix conditional (1.02 KB, patch)
2016-04-27 17:37 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 2015-06-05 11:48:42 UTC
AllowRenewalIfOtherItemsAvailable checks C4::Reserves::IsAvailableForItemLevelRequest to see if the item is holdable, which catches not for loan values less than 0 ( i.e. holdable, but not circ-able ). However, since this feature is about actually checking out items to patrons, we should not count *any* not for loan items when deciding if the available items will satisfy all current holds.
Comment 1 Kyle M Hall 2015-06-05 11:53:53 UTC Comment hidden (obsolete)
Comment 2 Amit Gupta 2015-07-31 12:30:48 UTC Comment hidden (obsolete)
Comment 3 Amit Gupta 2015-07-31 12:32:03 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-08-05 07:53:16 UTC
Kyle, please provide tests for this change.
Comment 5 Kyle M Hall 2015-08-17 16:53:24 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2015-08-17 16:54:08 UTC
(In reply to Jonathan Druart from comment #4)
> Kyle, please provide tests for this change.

Done!
Comment 7 Jonathan Druart 2015-08-19 13:59:35 UTC
Kyle, the test you have provided passes against master.
I would expect something to fail, and then to be fixed by your patch.

Why is the version set to 3.18?
Comment 8 Kyle M Hall 2015-08-19 17:45:55 UTC
(In reply to Jonathan Druart from comment #7)
> Kyle, the test you have provided passes against master.
> I would expect something to fail, and then to be fixed by your patch.
> 
> Why is the version set to 3.18?

My bad. I assumed the test would fail on master but failed to test my assumption!

Not sure why the version was set that way. Updated to master.
Comment 9 Kyle M Hall 2015-08-19 18:47:53 UTC
I just wanted to publicly observe that these unit tests are such a mess that I'm spending hours writing unit tests for a patch that took minutes to complete. We really need to think about how to clean these tests up.
Comment 10 Jonathan Druart 2015-08-20 07:35:23 UTC
(In reply to Kyle M Hall from comment #9)
> I just wanted to publicly observe that these unit tests are such a mess that
> I'm spending hours writing unit tests for a patch that took minutes to
> complete. We really need to think about how to clean these tests up.

I totally agree with that.
Comment 11 Kyle M Hall 2015-08-20 11:16:36 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2015-08-20 11:16:44 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-08-20 11:17:58 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to Kyle M Hall from comment #9)
> > I just wanted to publicly observe that these unit tests are such a mess that
> > I'm spending hours writing unit tests for a patch that took minutes to
> > complete. We really need to think about how to clean these tests up.
> 
> I totally agree with that.

Success! I created a new block of tests at the end of the file and by starting from scratch ( mostly ) I believe I have now crafted a correct unit test!
Comment 14 Jonathan Druart 2015-08-20 13:11:57 UTC
Comment on attachment 41708 [details] [review]
Bug 14337 - Add Unit Tests

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

::: t/db_dependent/Circulation.t
@@ +649,5 @@
> +    my $borrower2 = GetMember( borrowernumber => $borrowernumber2 );
> +
> +    my $issue = AddIssue( $borrower1, $barcode1 );
> +
> +    my ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber2, $itemnumber1 );

Shouldn't be borrowernumber1?
Chelsea didn't check out itemnumber1.

@@ +667,5 @@
> +
> +    ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber2, $itemnumber1 );
> +    is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled' );
> +
> +    diag("Setting item not checked out to be not for loan but holdable");

No need to overload the output, just a comment would be enough, don't you think?
Comment 15 Jonathan Druart 2015-08-21 10:34:53 UTC
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
Comment 16 Jonathan Druart 2015-08-21 10:40:21 UTC
I meant bug 9809.
Comment 17 Kyle M Hall 2015-08-21 14:38:33 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2015-08-21 14:38:41 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2015-08-21 14:39:13 UTC
(In reply to Jonathan Druart from comment #14)
> Comment on attachment 41708 [details] [review] [review]
> Bug 14337 - Add Unit Tests
> 
> Review of attachment 41708 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: t/db_dependent/Circulation.t
> @@ +649,5 @@
> > +    my $borrower2 = GetMember( borrowernumber => $borrowernumber2 );
> > +
> > +    my $issue = AddIssue( $borrower1, $barcode1 );
> > +
> > +    my ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber2, $itemnumber1 );
> 
> Shouldn't be borrowernumber1?
> Chelsea didn't check out itemnumber1.
> 
> @@ +667,5 @@
> > +
> > +    ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber2, $itemnumber1 );
> > +    is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled' );
> > +
> > +    diag("Setting item not checked out to be not for loan but holdable");
> 
> No need to overload the output, just a comment would be enough, don't you
> think?

You are correct. Unit test patch is updated based on your comments!
Comment 20 Jonathan Druart 2015-08-25 11:48:30 UTC
Created attachment 41904 [details] [review]
Bug 14337 - Add Unit Tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2015-08-25 11:48:33 UTC
Created attachment 41905 [details] [review]
Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability

AllowRenewalIfOtherItemsAvailable checks
C4::Reserves::IsAvailableForItemLevelRequest to see if the item is
holdable, which catches not for loan values less than 0 ( i.e. holdable,
but not circ-able ). However, since this feature is about
actually checking out items to patrons, we should not count *any* not
for loan items when deciding if the available items will satisfy all
current holds.

Test Plan:
 1) Enable AllowRenewalIfOtherItemsAvailable
 2) Create a record with two items
 3) Check out one item to a patron
 4) Ensure the item is renewable
 5) Place a hold on the record
 6) The item should now be non-renewable
 7) Add a second item to the record, but with a not for loan value < 0
 8) Note the checkout is still renewable
 9) Apply this patch
10) Note the checkout is now non-renewable

Works ok.

Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2015-08-25 11:48:35 UTC
Created attachment 41906 [details] [review]
Bug 14337: Fix conflict with bug 9809

The reserves.constrainttype does not exist anymore, it has been removed
by bug 9809.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2015-08-25 11:48:38 UTC
Created attachment 41907 [details] [review]
Bug 14337: Add 2 more tests to be complete

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Kyle M Hall 2015-08-25 12:32:33 UTC
Thanks for the followups Jonathan!
Comment 25 Tomás Cohen Arazi 2015-08-28 13:44:43 UTC
Patches pushed to master.

Thanks Kyle and Jonathan!
Comment 26 Kyle M Hall 2016-04-27 17:36:51 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2016-04-27 17:37:29 UTC Comment hidden (obsolete)