[Bug 14337] New: AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Bug ID: 14337 Summary: AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability Change sponsored?: --- Product: Koha Version: 3.18 Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | Severity|enhancement |normal -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 39898 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39898&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Amit Gupta <amitddng135@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |amitddng135@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #2 from Amit Gupta <amitddng135@gmail.com> --- Created attachment 41262 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41262&action=edit Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability Works OK. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Amit Gupta <amitddng135@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Amit Gupta <amitddng135@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #39898|0 |1 is obsolete| | Attachment #41262|0 |1 is obsolete| | --- Comment #3 from Amit Gupta <amitddng135@gmail.com> --- Created attachment 41263 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41263&action=edit Bug 14337 - AllowRenewalIfOtherItemsAvailable counts holdable not for loan items when checking renewability Works OK. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Kyle, please provide tests for this change. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41569 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41569&action=edit Bug 14337 [QA Followup] - Add unit test -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #6 from Kyle M Hall <kyle.m.hall@gmail.com> --- (In reply to Jonathan Druart from comment #4)
Kyle, please provide tests for this change.
Done! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|3.18 |master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #8 from Kyle M Hall <kyle.m.hall@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #9 from Kyle M Hall <kyle.m.hall@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41263|0 |1 is obsolete| | Attachment #41569|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41708 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41708&action=edit Bug 14337 - Add Unit Tests -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41709 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41709&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #13 from Kyle M Hall <kyle.m.hall@gmail.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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 41708 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41708 Bug 14337 - Add Unit Tests Review of attachment 41708: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14337&attachment=41708) ----------------------------------------------------------------- ::: 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 receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14526 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Be care, AddReserve prototype changed by bug 14526 (contrainst param removed). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|http://bugs.koha-community. |http://bugs.koha-community. |org/bugzilla3/show_bug.cgi? |org/bugzilla3/show_bug.cgi? |id=14526 |id=9809 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I meant bug 9809. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41708|0 |1 is obsolete| | Attachment #41709|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41787 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41787&action=edit Bug 14337 - Add Unit Tests -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #18 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 41788 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41788&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #19 from Kyle M Hall <kyle.m.hall@gmail.com> --- (In reply to Jonathan Druart from comment #14)
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?
You are correct. Unit test patch is updated based on your comments! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41787|0 |1 is obsolete| | --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 41904 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41904&action=edit Bug 14337 - Add Unit Tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41788|0 |1 is obsolete| | --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 41905 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41905&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 41906 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41906&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 41907 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41907&action=edit Bug 14337: Add 2 more tests to be complete Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Depends on| |9809 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809 [Bug 9809] Get rid of reserveconstraints -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 --- Comment #24 from Kyle M Hall <kyle.m.hall@gmail.com> --- Thanks for the followups Jonathan! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Pushed to Master --- Comment #25 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks Kyle and Jonathan! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org