Bug 7413 - Allow OPAC renewal x days before due date
Summary: Allow OPAC renewal x days before due date
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Holger Meißner
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-06 16:07 UTC by Katrin Fischer
Modified: 2014-12-07 20:07 UTC (History)
4 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
[PATCH 1/5] Bug 7413: Allow OPAC renewal x days before due date (2.58 KB, patch)
2014-01-24 13:56 UTC, Holger Meißner
Details | Diff | Splinter Review
[PATCH 2/5] Bug 7413: Allow OPAC renewal x days before due date (6.33 KB, patch)
2014-01-24 13:58 UTC, Holger Meißner
Details | Diff | Splinter Review
[PATCH 3/5] Bug 7413: Allow OPAC renewal x days before due date (5.04 KB, patch)
2014-01-24 13:59 UTC, Holger Meißner
Details | Diff | Splinter Review
[PATCH 4/5] Bug 7413: Allow OPAC renewal x days before due date (3.85 KB, patch)
2014-01-24 14:00 UTC, Holger Meißner
Details | Diff | Splinter Review
[PATCH 5/5] Bug 7413: Allow OPAC renewal x days before due date (3.58 KB, patch)
2014-01-24 14:01 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Add issuingrules.norenewalbefore (2.57 KB, patch)
2014-01-31 10:51 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Add "No renewal before" to circulation and fine rules (6.35 KB, patch)
2014-01-31 10:52 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Add new error "too_soon" in CanBookBeRenewed (5.04 KB, patch)
2014-01-31 10:52 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Bootstrap theme changes (3.84 KB, patch)
2014-01-31 10:52 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Prog theme change (3.56 KB, patch)
2014-01-31 10:52 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Unit Test (3.42 KB, patch)
2014-02-10 09:00 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Add issuingrules.norenewalbefore to database (2.62 KB, patch)
2014-02-14 09:58 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Add "No renewal before" to the circulation and fine rules (6.35 KB, patch)
2014-02-14 09:58 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Code and intranet template changes (11.88 KB, patch)
2014-02-14 09:58 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: OPAC bootstrap theme changes (5.49 KB, patch)
2014-02-14 09:58 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: OPAC prog theme changes (3.15 KB, patch)
2014-02-14 09:59 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 7413: Unit test (3.47 KB, patch)
2014-02-14 09:59 UTC, Holger Meißner
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7413: Add issuingrules.norenewalbefore to database (2.69 KB, patch)
2014-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7413: Add "No renewal before" to the circulation and fine rules (6.42 KB, patch)
2014-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7413: Code and intranet template changes (11.96 KB, patch)
2014-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7413: OPAC bootstrap theme changes (5.57 KB, patch)
2014-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7413: OPAC prog theme changes (3.23 KB, patch)
2014-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7413: Unit test (3.54 KB, patch)
2014-03-05 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7413: Unit test (3.59 KB, patch)
2014-04-01 21:34 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 7413: Unit test (3.59 KB, patch)
2014-04-01 21:50 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 7413: Add issuingrules.norenewalbefore to database (2.70 KB, patch)
2014-04-01 21:56 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 7413: Add "No renewal before" to the circulation and fine rules (6.47 KB, patch)
2014-04-01 21:56 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 7413: Code and intranet template changes (12.01 KB, patch)
2014-04-01 21:56 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 7413: OPAC bootstrap theme changes (5.62 KB, patch)
2014-04-01 21:57 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 7413: OPAC prog theme changes (3.28 KB, patch)
2014-04-01 21:57 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 7413: Unit test (3.59 KB, patch)
2014-04-01 21:57 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2012-01-06 16:07:13 UTC
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.
Comment 1 Owen Leonard 2012-01-06 16:20:24 UTC
(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?
Comment 2 Katrin Fischer 2012-01-06 23:11:39 UTC
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.
Comment 3 Holger Meißner 2014-01-20 13:15:04 UTC
Assigned this to myself.
Comment 4 Holger Meißner 2014-01-24 13:56:17 UTC Comment hidden (obsolete)
Comment 5 Holger Meißner 2014-01-24 13:58:09 UTC Comment hidden (obsolete)
Comment 6 Holger Meißner 2014-01-24 13:59:19 UTC Comment hidden (obsolete)
Comment 7 Holger Meißner 2014-01-24 14:00:25 UTC Comment hidden (obsolete)
Comment 8 Holger Meißner 2014-01-24 14:01:42 UTC Comment hidden (obsolete)
Comment 9 Holger Meißner 2014-01-31 10:51:59 UTC Comment hidden (obsolete)
Comment 10 Holger Meißner 2014-01-31 10:52:03 UTC Comment hidden (obsolete)
Comment 11 Holger Meißner 2014-01-31 10:52:08 UTC Comment hidden (obsolete)
Comment 12 Holger Meißner 2014-01-31 10:52:11 UTC Comment hidden (obsolete)
Comment 13 Holger Meißner 2014-01-31 10:52:15 UTC Comment hidden (obsolete)
Comment 14 Holger Meißner 2014-01-31 10:55:42 UTC
Improved commit messages and fixed a few typos.
Comment 15 Martin Renvoize 2014-02-06 16:22:20 UTC
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.
Comment 16 Holger Meißner 2014-02-07 08:54:56 UTC
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.
Comment 17 Martin Renvoize 2014-02-07 09:40:26 UTC
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
Comment 18 Holger Meißner 2014-02-10 09:00:25 UTC Comment hidden (obsolete)
Comment 19 Holger Meißner 2014-02-11 08:34:04 UTC
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.
Comment 20 Martin Renvoize 2014-02-11 11:38:01 UTC
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
Comment 21 Holger Meißner 2014-02-14 09:58:46 UTC Comment hidden (obsolete)
Comment 22 Holger Meißner 2014-02-14 09:58:50 UTC Comment hidden (obsolete)
Comment 23 Holger Meißner 2014-02-14 09:58:53 UTC Comment hidden (obsolete)
Comment 24 Holger Meißner 2014-02-14 09:58:57 UTC Comment hidden (obsolete)
Comment 25 Holger Meißner 2014-02-14 09:59:00 UTC Comment hidden (obsolete)
Comment 26 Holger Meißner 2014-02-14 09:59:05 UTC Comment hidden (obsolete)
Comment 27 Holger Meißner 2014-02-14 10:17:41 UTC
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.
Comment 28 Martin Renvoize 2014-03-05 15:09:08 UTC Comment hidden (obsolete)
Comment 29 Martin Renvoize 2014-03-05 15:09:12 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2014-03-05 15:09:15 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2014-03-05 15:09:19 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2014-03-05 15:09:22 UTC Comment hidden (obsolete)
Comment 33 Martin Renvoize 2014-03-05 15:09:26 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2014-03-05 15:12:34 UTC
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 ;)
Comment 35 Holger Meißner 2014-03-07 08:58:35 UTC
Thank you, Martin!

If there's anything you need tested, let me know and I'll see what I can do. :)
Comment 36 Brendan Gallagher 2014-03-12 13:29:26 UTC
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
Comment 37 Holger Meißner 2014-03-17 11:03:21 UTC
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"?
Comment 38 Brendan Gallagher 2014-03-25 20:36:36 UTC
(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...
Comment 39 Holger Meißner 2014-03-26 09:05:25 UTC
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.
Comment 40 Brendan Gallagher 2014-04-01 21:34:34 UTC Comment hidden (obsolete)
Comment 41 Brendan Gallagher 2014-04-01 21:50:35 UTC Comment hidden (obsolete)
Comment 42 Brendan Gallagher 2014-04-01 21:56:30 UTC
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>
Comment 43 Brendan Gallagher 2014-04-01 21:56:43 UTC
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>
Comment 44 Brendan Gallagher 2014-04-01 21:56:52 UTC
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>
Comment 45 Brendan Gallagher 2014-04-01 21:57:03 UTC
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>
Comment 46 Brendan Gallagher 2014-04-01 21:57:13 UTC
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>
Comment 47 Brendan Gallagher 2014-04-01 21:57:23 UTC
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>
Comment 48 Galen Charlton 2014-04-08 23:32:07 UTC
Pushed to master, along with several follow-ups.  Thanks, Holger!