Currently RenewalPeriodBase can be set to 2 different options: 1) When renewing checkouts, base the new due date on current date. This means that when a patron renews the item at the checkout date, the renewal counter will increment, but the due date will not change - the loan period is still the same. If he renews one day after checkout - he will get a reneal of one day and so on. 2) When renewing checkouts, base the new due date on the old date of the checkout. This means the renewals can be done immediately after checkout. So if you allow 3 renewals and a loan period of 4 weeks, the patron can log into his account and do all the renewals immediately. This will mean he effectively gets a loan period of 4 + 3*4 weeks = 16 weeks. I see no reason why a clever student shouldn't do this to make sure he can keep the text books as long as possible without any holds forcing him/her to return the books. One solution here would be to allow renewals only x days before the book is due. As the loan periods can be very different (overnight loans, 4 weeks, 7 days...) it would be cool if this could be implemented in circulation rules.
(In reply to comment #0) > 2) When renewing checkouts, base the new due date on the old date of the > checkout. I thought that meant your renewal due date was based on the old due date. So if I check something out for two weeks which is due 9 June 2011 and I renew it on 19 June 2011 it's due on 23 June 2011 (two weeks after the original due date) rather than 3 July 2011 (two weeks from the date I renewed). Is that incorrect?
Owen, reading it again I think the description of the system preference is wrong here. I think you are right - it's based on the old due date, not checkout date in that case.
Assigned this to myself.
Created attachment 24685 [details] [review] [PATCH 1/5] Bug 7413: Allow OPAC renewal x days before due date Currently multiple renewals can be done in immediate succsession. To optionally prevent this, a new parameter "No renewals before" is introduced. This patch adds issuingrules.norenewalbefore to the database. Default value is NULL. To test: 1) Run installer/data/mysql/updatedatabase.pl. 2) Create a SQL report like: SELECT * FROM issuingrules 3) Confirm that norenewalbefore was added after renewalperiod. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24686 [details] [review] [PATCH 2/5] Bug 7413: Allow OPAC renewal x days before due date This patch adds a column "No renewal before" to the circulation and fine rules. To test: 1) Add or edit some issuing rules. 2) Confirm that an empty entry, zero and non-numerical values for "No renewal before" are converted into undefined. 3) Confirm that numerical values can be entered and saved. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24687 [details] [review] [PATCH 3/5] Bug 7413: Allow OPAC renewal x days before due date This patch modifies CanBookBeRenewed in Ciculation.pm, so that based on issuingrules.norenewalbefore a new error "too_soon" can be returned. For this error intranet HTML templates will display "Not renewable yet" instead of "Not renewable". To test: 1) Create an issuing rule with "No renewal before" set to value X and "Unit" set to days. 2) Confirm that items can't be renewed if current date is more than X days before due date. 3) Confirm that items can be renewed if "No renewal before" is undefined or current date is X or less days before due due. 4) Confirm that "Renewals allowed (count)" has priority, i.e. if the number of allowed renewals is exceeded "Not renewable" is displayed, no matter what "No renewal before" is set to. 5) Test the same things with "Unit" set to hours. 6) Test for both circulation.tt and moremember.tt. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24688 [details] [review] [PATCH 4/5] Bug 7413: Allow OPAC renewal x days before due date This patch makes the bootstrap version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to bootstrap. 2) Test the sames things as in previous patch, only for opac-user.tt. 3) Confirm that additionally to "Not renewable yet", the number of remaining renewals is displayed. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24689 [details] [review] [PATCH 5/5] Bug 7413: Allow OPAC renewal x days before due date This patch makes the prog version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to prog. 2) Test the same things as in previous patch. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24926 [details] [review] Bug 7413: Add issuingrules.norenewalbefore Currently multiple renewals can be done in immediate succsession. To optionally prevent this, a new parameter "No renewal before" is introduced. This patch adds issuingrules.norenewalbefore to the database. Default value is NULL. To test: 1) Run installer/data/mysql/updatedatabase.pl. 2) Create a SQL report like: SELECT * FROM issuingrules 3) Confirm that norenewalbefore was added after renewalperiod. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24927 [details] [review] Bug 7413: Add "No renewal before" to circulation and fine rules This patch adds a column "No renewal before" to the circulation and fine rules table. To test: 1) Add or edit some issuing rules. 2) Confirm that an empty entry, zero and non-numerical values for "No renewal before" are converted into undefined. 3) Confirm that numerical values can be entered and saved. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24928 [details] [review] Bug 7413: Add new error "too_soon" in CanBookBeRenewed This patch modifies CanBookBeRenewed in Ciculation.pm, so that based on issuingrules.norenewalbefore a new error "too_soon" can be returned. For this error intranet HTML templates will display "Not renewable yet" instead of "Not renewable". To test: 1) Create an issuing rule with "No renewal before" set to value X and "Unit" set to days. 2) Confirm that items can't be renewed if current date is more than X days before due date. 3) Confirm that items can be renewed if "No renewal before" is undefined or current date is X or less days before due date. 4) Confirm that "Renewals allowed (count)" has priority, i.e. if the number of allowed renewals is exceeded "Not renewable" is displayed, no matter what "No renewal before" is set to. 5) Test the same things with "Unit" set to hours. 6) Test for both circulation.tt and moremember.tt. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24929 [details] [review] Bug 7413: Bootstrap theme changes This patch makes opac-user.pl and the bootstrap version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to bootstrap. 2) Test the sames things as in previous patch, only for opac-user.tt. 3) Confirm that additionally to "Not renewable yet", the number of remaining renewals is displayed. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 24930 [details] [review] Bug 7413: Prog theme change This patch makes the prog version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to prog. 2) Test the same things as in previous patch. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Improved commit messages and fixed a few typos.
Hi Holger, It's great to see another new developer! Your code looks good to me, and I'm just running through testing it for bugs now. As a quick comment, I'de love to see a minor enhancement to this. It would be great if you could pass the number of days before renewal allowed in the error, and display this useful piece of information in the error message given to users? Otherwise, I'm pretty happy to sign-off once I've completed my testing. Great to have you on-board.
Thank you, Martin! (In reply to Martin Renvoize from comment #15) > As a quick comment, I'de love to see a minor enhancement to this. It would > be great if you could pass the number of days before renewal allowed in the > error, and display this useful piece of information in the error message > given to users? Yes, that's a good idea. I'll do this as a follow-up. I also noticed that unit test t/db_dependent/Circulation_Issuingrule.t fails. I'll do a follow-up for this too.
Hi Holger, Well spotted on the unit tests, I hadn't found that issue yet.. I had a bit of a mare getting my test environment circulation working to test the feature, I'm hoping to test properly later today. I'll hold off on a signoff until I see the unit tests patch go in, as failing unit test would only get caught at the QA level, so it'll save us both hassle if it passes those before QA give it the evil eye ;) Great work, Martin
Created attachment 25166 [details] [review] Bug 7413: Unit Test This patch makes the new parameter norenewalbefore known to Circulation_Issuingrule.t To test: 1) prove t/db_dependent/Circulation_Issuingrule.t 2) Notice that the test fails with: $got->{nornewalbefore} = undef $expected->{norenewalbefore} = Does not exist 3) Apply patch. 4) Run test again and it should pass. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Thought about it and decided to do a rebase rather than a follow-up. To avoid race conditions that might lead to QA having to apply the malocchio twice, I set the status back to assigned. ;) I'm a bit busy, but I'll try to complete it this week.
All great news Holger, Thanks for keeping us informed, I look forward to the updated patch for my next round of tests. Thus far I'm happy with the results and the code.. it's looking great. Martin
Created attachment 25247 [details] [review] Bug 7413: Add issuingrules.norenewalbefore to database Currently multiple renewals can be done in immediate succsession. To optionally prevent this, a new parameter "No renewal before" is introduced. This patch adds issuingrules.norenewalbefore to the database. Default value is NULL. To test: 1) Run installer/data/mysql/updatedatabase.pl 2) Create a SQL report like: SELECT * FROM issuingrules 3) Confirm that norenewalbefore was added after renewalperiod. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 25248 [details] [review] Bug 7413: Add "No renewal before" to the circulation and fine rules This patch adds a column "No renewal before" to the circulation and fine rules table. To test: 1) Add or edit some issuing rules. 2) Confirm that an empty entry, zero and non-numerical values for "No renewal before" are saved as undefined. 3) Confirm that numerical values can be entered and saved. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 25249 [details] [review] Bug 7413: Code and intranet template changes This patch modifies CanBookBeRenewed, so that based on issuingrules.norenewalbefore a new error "too_soon" can be returned. Also adds a new subroutine GetSoonestRenewDate. To test: 1) Create an issuing rule with "No renewal before" set to value X and "Unit" set to days. 2) Test the following steps for both: Home > Patron > Patron details Home > Circulation > Checkouts 3) On the checkout page, test for today's issues as well as previous issues. (Check something out on one day and something else on the next day, then do the testing.) 4) Confirm that items can't be renewed if current date is more than X days before due date. 5) Confirm that the date and time of the soonest possible renewal are displayed in the format specified by global sysprefs "dateformat" and "TimeFormat". 6) Confirm that items can be renewed if "No renewal before" is undefined or current date is X or less days before due date. 7) Confirm that if the number of allowed renewals is exceeded "Not renewable" is displayed, no matter what "No renewal before" is set to. 8) Test the same things with "Unit" set to hours. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 25250 [details] [review] Bug 7413: OPAC bootstrap theme changes This patch makes opac-user.pl and the bootstrap version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to bootstrap. 2) Set global syspref "OpacRenewalAllowed" to Allow. 3) Test the same things as in previous patch, this time for the OPAC summary page. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 25251 [details] [review] Bug 7413: OPAC prog theme changes This patch makes the prog version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to prog. 2) Test the same things as in previous patch. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Created attachment 25252 [details] [review] Bug 7413: Unit test This patch makes the new parameter norenewalbefore known to unit test Circulation_Issuingrule.t. To test: 1) Before applying the patch: prove t/db_dependent/Circulation_Issuingrule.t 2) Notice that the test fails with: $got->{norenewalbefore} = undef $expected->{norenewalbefore} = Does not exist 3) Apply the patch. 4) Run test again and it should pass. Sponsored-by: Hochschule für Gesundheit (hsg), Germany
Good thing I did the rebase. Found a major flaw, I didn't notice the distinction between todayissues and previssues. But that should work now. :) Thanks a lot for testing, Martin! Don't hurry, I'm now on vacation until march.
Created attachment 25855 [details] [review] [SIGNED OFF] Bug 7413: Add issuingrules.norenewalbefore to database Currently multiple renewals can be done in immediate succsession. To optionally prevent this, a new parameter "No renewal before" is introduced. This patch adds issuingrules.norenewalbefore to the database. Default value is NULL. To test: 1) Run installer/data/mysql/updatedatabase.pl 2) Create a SQL report like: SELECT * FROM issuingrules 3) Confirm that norenewalbefore was added after renewalperiod. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 25856 [details] [review] [SIGNED OFF] Bug 7413: Add "No renewal before" to the circulation and fine rules This patch adds a column "No renewal before" to the circulation and fine rules table. To test: 1) Add or edit some issuing rules. 2) Confirm that an empty entry, zero and non-numerical values for "No renewal before" are saved as undefined. 3) Confirm that numerical values can be entered and saved. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 25857 [details] [review] [SIGNED OFF] Bug 7413: Code and intranet template changes This patch modifies CanBookBeRenewed, so that based on issuingrules.norenewalbefore a new error "too_soon" can be returned. Also adds a new subroutine GetSoonestRenewDate. To test: 1) Create an issuing rule with "No renewal before" set to value X and "Unit" set to days. 2) Test the following steps for both: Home > Patron > Patron details Home > Circulation > Checkouts 3) On the checkout page, test for today's issues as well as previous issues. (Check something out on one day and something else on the next day, then do the testing.) 4) Confirm that items can't be renewed if current date is more than X days before due date. 5) Confirm that the date and time of the soonest possible renewal are displayed in the format specified by global sysprefs "dateformat" and "TimeFormat". 6) Confirm that items can be renewed if "No renewal before" is undefined or current date is X or less days before due date. 7) Confirm that if the number of allowed renewals is exceeded "Not renewable" is displayed, no matter what "No renewal before" is set to. 8) Test the same things with "Unit" set to hours. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 25858 [details] [review] [SIGNED OFF] Bug 7413: OPAC bootstrap theme changes This patch makes opac-user.pl and the bootstrap version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to bootstrap. 2) Set global syspref "OpacRenewalAllowed" to Allow. 3) Test the same things as in previous patch, this time for the OPAC summary page. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 25859 [details] [review] [SIGNED OFF] Bug 7413: OPAC prog theme changes This patch makes the prog version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to prog. 2) Test the same things as in previous patch. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 25860 [details] [review] [SIGNED OFF] Bug 7413: Unit test This patch makes the new parameter norenewalbefore known to unit test Circulation_Issuingrule.t. To test: 1) Before applying the patch: prove t/db_dependent/Circulation_Issuingrule.t 2) Notice that the test fails with: $got->{norenewalbefore} = undef $expected->{norenewalbefore} = Does not exist 3) Apply the patch. 4) Run test again and it should pass. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice work Holger, all works as expected and you've delivered nicely with the follow-ups. Signing off, lets see what they make of it at QA ;)
Thank you, Martin! If there's anything you need tested, let me know and I'll see what I can do. :)
Hello Holger Meißner - I do have one comment and maybe you can test this. If you set the "Loan period" to say 4 day loan and you set the "No renewal before" to 5 days, shouldn't you always be able to renew the material? I have tested with that set up and I can not renew the item. I have set it for something crazy like loan period of 4 and a "no renewal before" to 30 days and that works fine. I'm not sure if that really is a bug. But I could be wrong with my logic on the 4 loan 5 no renewal before. Perhaps maybe you can test and let me know if you can repeat the problem. Besides that weird little case, everything else looks great. IT maybe that I found one little case that we can't really get at with programing because of the timestamps or something like that. and just need to add a comment for documentation, that perhaps the renewal before amount should be smaller than the loan period OR greater than 1 :) Let me know what you think - I am ready to pass QA on this, and will probably just mention to the RM about a special documentation needed. -Brendan
Hello Brendan, thanks for looking at this patch! (In reply to Brendan Gallagher from comment #36) > I do have one comment and maybe you can test this. If you set the "Loan > period" to say 4 day loan and you set the "No renewal before" to 5 days, > shouldn't you always be able to renew the material? Yes, you should definitely be able to renew. I'm unable to reproduce this problem. When I check something out with a loan period of 4 days and "No renewal before" set to 5, I can renew immediately. I have no idea what might cause this problem for you. Could you supply a screenshot of the checkout page? What exactly is displayed in the columns "Due date" and "Renew"?
(In reply to Holger Meißner from comment #37) > Hello Brendan, > > thanks for looking at this patch! > > (In reply to Brendan Gallagher from comment #36) > > I do have one comment and maybe you can test this. If you set the "Loan > > period" to say 4 day loan and you set the "No renewal before" to 5 days, > > shouldn't you always be able to renew the material? > > Yes, you should definitely be able to renew. I'm unable to reproduce this > problem. When I check something out with a loan period of 4 days and "No > renewal before" set to 5, I can renew immediately. > > I have no idea what might cause this problem for you. > > Could you supply a screenshot of the checkout page? What exactly is > displayed in the columns "Due date" and "Renew"? Hmm... Holger - I am not able to repeat the problem I saw earlier. Let me do a little more testing and see if I can repeat it. hang with me a few...
There is of course one thing that may be a bit confusing and depends on the chosen unit. If you use hours, a loan period of 4 and norenewalbefore 4, you can renew instantly. If you use days, a loan period of 4 and norenewalbefore 4, you can't. Unless the checkout is just before midnight. Because the time of the due date is always set to 23:59. And the functionality is based solely on the exact datetime of the due date. This might not always be desired or expected, but I guess it's better than making assumptions about what libraries want instead. And it won't break, if at some point in the future someone introduces a patch that makes it possible to set a different time for the due date.
Created attachment 26726 [details] [review] Bug 7413: Unit test This patch makes the new parameter norenewalbefore known to unit test Circulation_Issuingrule.t. To test: 1) Before applying the patch: prove t/db_dependent/Circulation_Issuingrule.t 2) Notice that the test fails with: $got->{norenewalbefore} = undef $expected->{norenewalbefore} = Does not exist 3) Apply the patch. 4) Run test again and it should pass. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 26727 [details] [review] Bug 7413: Unit test This patch makes the new parameter norenewalbefore known to unit test Circulation_Issuingrule.t. To test: 1) Before applying the patch: prove t/db_dependent/Circulation_Issuingrule.t 2) Notice that the test fails with: $got->{norenewalbefore} = undef $expected->{norenewalbefore} = Does not exist 3) Apply the patch. 4) Run test again and it should pass. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 26728 [details] [review] Bug 7413: Add issuingrules.norenewalbefore to database Currently multiple renewals can be done in immediate succsession. To optionally prevent this, a new parameter "No renewal before" is introduced. This patch adds issuingrules.norenewalbefore to the database. Default value is NULL. To test: 1) Run installer/data/mysql/updatedatabase.pl 2) Create a SQL report like: SELECT * FROM issuingrules 3) Confirm that norenewalbefore was added after renewalperiod. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 26729 [details] [review] Bug 7413: Add "No renewal before" to the circulation and fine rules This patch adds a column "No renewal before" to the circulation and fine rules table. To test: 1) Add or edit some issuing rules. 2) Confirm that an empty entry, zero and non-numerical values for "No renewal before" are saved as undefined. 3) Confirm that numerical values can be entered and saved. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 26730 [details] [review] Bug 7413: Code and intranet template changes This patch modifies CanBookBeRenewed, so that based on issuingrules.norenewalbefore a new error "too_soon" can be returned. Also adds a new subroutine GetSoonestRenewDate. To test: 1) Create an issuing rule with "No renewal before" set to value X and "Unit" set to days. 2) Test the following steps for both: Home > Patron > Patron details Home > Circulation > Checkouts 3) On the checkout page, test for today's issues as well as previous issues. (Check something out on one day and something else on the next day, then do the testing.) 4) Confirm that items can't be renewed if current date is more than X days before due date. 5) Confirm that the date and time of the soonest possible renewal are displayed in the format specified by global sysprefs "dateformat" and "TimeFormat". 6) Confirm that items can be renewed if "No renewal before" is undefined or current date is X or less days before due date. 7) Confirm that if the number of allowed renewals is exceeded "Not renewable" is displayed, no matter what "No renewal before" is set to. 8) Test the same things with "Unit" set to hours. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 26731 [details] [review] Bug 7413: OPAC bootstrap theme changes This patch makes opac-user.pl and the bootstrap version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to bootstrap. 2) Set global syspref "OpacRenewalAllowed" to Allow. 3) Test the same things as in previous patch, this time for the OPAC summary page. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 26732 [details] [review] Bug 7413: OPAC prog theme changes This patch makes the prog version of opac-user.tt handle the new renewal error "too_soon". To test: 1) Set global syspref "opacthemes" to prog. 2) Test the same things as in previous patch. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 26733 [details] [review] Bug 7413: Unit test This patch makes the new parameter norenewalbefore known to unit test Circulation_Issuingrule.t. To test: 1) Before applying the patch: prove t/db_dependent/Circulation_Issuingrule.t 2) Notice that the test fails with: $got->{norenewalbefore} = undef $expected->{norenewalbefore} = Does not exist 3) Apply the patch. 4) Run test again and it should pass. Sponsored-by: Hochschule für Gesundheit (hsg), Germany Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Pushed to master, along with several follow-ups. Thanks, Holger!