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.
Created attachment 39898 [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
Created attachment 41262 [details] [review] Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability Works OK.
Created attachment 41263 [details] [review] Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability Works OK.
Kyle, please provide tests for this change.
Created attachment 41569 [details] [review] Bug 14337 [QA Followup] - Add unit test
(In reply to Jonathan Druart from comment #4) > Kyle, please provide tests for this change. Done!
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?
(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.
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.
(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.
Created attachment 41708 [details] [review] Bug 14337 - Add Unit Tests
Created attachment 41709 [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>
(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 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?
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
I meant bug 9809.
Created attachment 41787 [details] [review] Bug 14337 - Add Unit Tests
Created attachment 41788 [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>
(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!
Created attachment 41904 [details] [review] Bug 14337 - Add Unit Tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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>
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>
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>
Thanks for the followups Jonathan!
Patches pushed to master. Thanks Kyle and Jonathan!
Created attachment 50840 [details] [review] Bug 14337 [QA Followup] - Fix conditional
Created attachment 50841 [details] [review] Bug 14337 [QA Followup] - Fix conditional