|
Description
Marcel de Rooy
2013-03-07 12:31:29 UTC
Created attachment 15932 [details] [review] Bug 9761: Make it possible to confirm future hold requests at checkin time Description: A new pref ConfirmFutureHolds is added. When confirming a hold at checkin time, the number of days in this pref is taken into account when looking for reserves. Note that this pref does not interfere with renewing, issuing or transferring a book. For report Holds to pull, the default end date is calculated with this new preference. The use of ConfirmFutureHolds is useful only when future holds are allowed. Test plan: 1) Enable future holds. Add a number of days into ConfirmFutureHolds. 2) Place a future hold within this number of days. 3) Run holds to pull report. Check default startdate and enddate. 4) Check this book in. Can you confirm the hold? Do not confirm. 5) Issue the book to another patron. You should not see a warning. 6) Renew the book for this patron via opac or staff. No warning either. 7) Check in again. Warning pops up again. 8) Transfer book. Switch branch. Check in. Hold found pops up. Do not confirm. 9) Back to first branch. Check in (with popup). Remove the hold. Add new future hold past the number of days. Check in (no warn). Created attachment 15933 [details] [review] Bug 9761: dbrev for ConfirmFutureHolds Db rev for new pref ConfirmFutureHolds. Test plan: Run [your server]/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure Optionally, run a new install to test sysprefs.sql. Or: Edit sysprefs.sql, leave at least the last few lines including the new one. Will also add some comments as results of code analysis for these patches here too. Created attachment 15951 [details] [review] Bug 9761: Reformulating the preference description After some thought, I had to reformulate what this pref does. No code changes here. Created attachment 15980 [details] [review] Bug 9761: Make it possible to confirm future hold requests at checkin time Description: A new pref ConfirmFutureHolds is added. When confirming a hold at checkin time, the number of days in this pref is taken into account when looking for reserves. Note that this pref does not interfere with renewing, issuing or transferring a book. For report Holds to pull, the default end date is calculated with this new preference. The use of ConfirmFutureHolds is useful only when future holds are allowed. Test plan: 1) Enable future holds. Add a number of days into ConfirmFutureHolds. 2) Place a future hold within this number of days. 3) Run holds to pull report. Check default startdate and enddate. 4) Check this book in. Can you confirm the hold? Do not confirm. 5) Issue the book to another patron. You should not see a warning. 6) Renew the book for this patron via opac or staff. No warning either. 7) Check in again. Warning pops up again. 8) Transfer book. Switch branch. Check in. Hold found pops up. Do not confirm. 9) Back to first branch. Check in (with popup). Remove the hold. Add new future hold past the number of days. Check in (no warn). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 15981 [details] [review] Bug 9761: dbrev for ConfirmFutureHolds Db rev for new pref ConfirmFutureHolds. Test plan: Run [your server]/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure Optionally, run a new install to test sysprefs.sql. Or: Edit sysprefs.sql, leave at least the last few lines including the new one. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 15982 [details] [review] Bug 9761: Reformulating the preference description After some thought, I had to reformulate what this pref does. No code changes here. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Just some comments on the changed code (making life somewhat easier for QA or RM most hopefully :) Main change is in _Findgroupreserve: the where clause is adjusted to optionally include future reserve dates. This routine is *only* used in CheckReserves :) The circulation module calls CheckReserves for transferbook, CanBookBeIssued, AddReturn, CanBookBeRenewed. This change is only meant for AddReturn. When writing this patch, the Search and XSLT module used CheckReserves also for displaying status information. Bug 9367 converts that to GetReserveStatus, implicitly supporting this change too. Note that bug 9788 makes some adjustments on top of that, improving item status for future waits. In the Reserves module, CheckReserves is used by GetOtherReserves (when confirming the hold: but you first need to recognize it!) and MoveReserve (for checkout: not relevant here). The call of CheckReserves after calling CanBookBeRenewed in circulation.pl is somewhat dubious. The calls in opac-user.pl and opac-detail.pl are also for displaying status info (see above with reference to the other reports again). Comment on attachment 15980 [details] [review] Bug 9761: Make it possible to confirm future hold requests at checkin time Review of attachment 15980 [details] [review]: ----------------------------------------------------------------- ::: C4/Reserves.pm @@ +1810,4 @@ > AND reserves.reservedate = reserveconstraints.reservedate ) > OR reserves.constrainttype='a' ) > AND (reserves.itemnumber IS NULL OR reserves.itemnumber = ?) > + AND reserves.reservedate <= DATE_ADD(NOW(),INTERVAL ? DAY) I was a little worried about adding this constraint, because this column is not indexed, but I tested with some explains and it should be ok. Passes all QA tests, testing for regressions now Created attachment 18609 [details] [review] Bug 9761: Make it possible to confirm future hold requests at checkin time Description: A new pref ConfirmFutureHolds is added. When confirming a hold at checkin time, the number of days in this pref is taken into account when looking for reserves. Note that this pref does not interfere with renewing, issuing or transferring a book. For report Holds to pull, the default end date is calculated with this new preference. The use of ConfirmFutureHolds is useful only when future holds are allowed. Test plan: 1) Enable future holds. Add a number of days into ConfirmFutureHolds. 2) Place a future hold within this number of days. 3) Run holds to pull report. Check default startdate and enddate. 4) Check this book in. Can you confirm the hold? Do not confirm. 5) Issue the book to another patron. You should not see a warning. 6) Renew the book for this patron via opac or staff. No warning either. 7) Check in again. Warning pops up again. 8) Transfer book. Switch branch. Check in. Hold found pops up. Do not confirm. 9) Back to first branch. Check in (with popup). Remove the hold. Add new future hold past the number of days. Check in (no warn). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 18610 [details] [review] Bug 9761: dbrev for ConfirmFutureHolds Db rev for new pref ConfirmFutureHolds. Test plan: Run [your server]/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure Optionally, run a new install to test sysprefs.sql. Or: Edit sysprefs.sql, leave at least the last few lines including the new one. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 18611 [details] [review] Bug 9761: dbrev for ConfirmFutureHolds Db rev for new pref ConfirmFutureHolds. Test plan: Run [your server]/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure Optionally, run a new install to test sysprefs.sql. Or: Edit sysprefs.sql, leave at least the last few lines including the new one. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 18612 [details] [review] Bug 9761: dbrev for ConfirmFutureHolds Db rev for new pref ConfirmFutureHolds. Test plan: Run [your server]/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure Optionally, run a new install to test sysprefs.sql. Or: Edit sysprefs.sql, leave at least the last few lines including the new one. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 18613 [details] [review] Bug 9761: Make it possible to confirm future hold requests at checkin time Description: A new pref ConfirmFutureHolds is added. When confirming a hold at checkin time, the number of days in this pref is taken into account when looking for reserves. Note that this pref does not interfere with renewing, issuing or transferring a book. For report Holds to pull, the default end date is calculated with this new preference. The use of ConfirmFutureHolds is useful only when future holds are allowed. Test plan: 1) Enable future holds. Add a number of days into ConfirmFutureHolds. 2) Place a future hold within this number of days. 3) Run holds to pull report. Check default startdate and enddate. 4) Check this book in. Can you confirm the hold? Do not confirm. 5) Issue the book to another patron. You should not see a warning. 6) Renew the book for this patron via opac or staff. No warning either. 7) Check in again. Warning pops up again. 8) Transfer book. Switch branch. Check in. Hold found pops up. Do not confirm. 9) Back to first branch. Check in (with popup). Remove the hold. Add new future hold past the number of days. Check in (no warn). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 18614 [details] [review] Bug 9761: dbrev for ConfirmFutureHolds Db rev for new pref ConfirmFutureHolds. Test plan: Run [your server]/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure Optionally, run a new install to test sysprefs.sql. Or: Edit sysprefs.sql, leave at least the last few lines including the new one. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 18615 [details] [review] Bug 9761: Reformulating the preference description After some thought, I had to reformulate what this pref does. No code changes here. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 21193 [details] [review] Bug 9761: Make it possible to confirm future hold requests at checkin time Description: A new pref ConfirmFutureHolds is added. When confirming a hold at checkin time, the number of days in this pref is taken into account when looking for reserves. Note that this pref does not interfere with renewing, issuing or transferring a book. For report Holds to pull, the default end date is calculated with this new preference. The use of ConfirmFutureHolds is useful only when future holds are allowed. Test plan: 1) Enable future holds. Add a number of days into ConfirmFutureHolds. 2) Place a future hold within this number of days. 3) Run holds to pull report. Check default startdate and enddate. 4) Check this book in. Can you confirm the hold? Do not confirm. 5) Issue the book to another patron. You should not see a warning. 6) Renew the book for this patron via opac or staff. No warning either. 7) Check in again. Warning pops up again. 8) Transfer book. Switch branch. Check in. Hold found pops up. Do not confirm. 9) Back to first branch. Check in (with popup). Remove the hold. Add new future hold past the number of days. Check in (no warn). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 21197 [details] [review] Bug 9761: dbrev for ConfirmFutureHolds Db rev for new pref ConfirmFutureHolds. Test plan: Run [your server]/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure Optionally, run a new install to test sysprefs.sql. Or: Edit sysprefs.sql, leave at least the last few lines including the new one. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 21198 [details] [review] Bug 9761: Reformulating the preference description After some thought, I had to reformulate what this pref does. No code changes here. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Rebased two patches for trivial reasons: withdrawn change, lexical order of sysprefs and updatedatabase. Note that this set of patches is already in the Passed QA queue since June 3 without any visible attention after that time.. Created attachment 21208 [details] [review] Bug 9761: Preliminary measures for adding a unit test Optionally add some branches and categories that may not exist. Test plan: Run the test with or without CPL branch or ST category. Verify that the test does not fail. Created attachment 21209 [details] [review] Bug 9761: Preliminary measures for adding a unit test Optionally add some branches and categories that may not exist. Test plan: Run the test with or without CPL branch or S (staff) category. Verify that the test does not fail. Please wait for another followup adding a few specific tests Created attachment 21227 [details] [review] Bug 9761: Unit tests for ConfirmFutureHolds changes Adds tests for CheckReserves with lookahead parameter. Adds tests for AddReturn with regard to future reserve messages. The following test cases are added, resulting in 8 new tests: a) Add a reserve without date, CheckReserve should return it b) Add a reserve with future date, CheckReserve should not return it c) Add a reserve with future date, CheckReserve should return it if lookahead is high enough d) Check ResFound message of AddReturn for future hold Test plan: Run the test. No fails? Will send a message if Chris and/or Kyle would be willing to run the adjusted Reserves.t unit test and provide an additional signoff. Since these two (QA) followups just serve the purpose of getting these patches further, I did not change the report status for now. Created attachment 21280 [details] [review] Bug 9761: Unit tests for ConfirmFutureHolds changes Adds tests for CheckReserves with lookahead parameter. Adds tests for AddReturn with regard to future reserve messages. The following test cases are added, resulting in 8 new tests: a) Add a reserve without date, CheckReserve should return it b) Add a reserve with future date, CheckReserve should not return it c) Add a reserve with future date, CheckReserve should return it if lookahead is high enough d) Check ResFound message of AddReturn for future hold Test plan: Run the test. No fails? Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> (In reply to Kyle M Hall from comment #27) > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Great. Thanks. Pushed to master. Thanks, Marcel! |