Bug 8365

Summary: Renewal duration in issuingules
Product: Koha Reporter: Paul Poulain <paul.poulain>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: new feature    
Priority: P5 - low CC: claire.hernandez, gmcharlt, jean-manuel.broust, jonathan.druart+koha, katrin.fischer, kyle.m.hall, kyle
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Large patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 8362, 9919    
Attachments: Bug 8365: Add a renewal duration in the issuing rules
Bug 8365: Add a renewal duration in the issuing rules
Bug 8365: Add a renewal duration in the issuing rules
[SIGNED-OFF] Bug 8365: Add a renewal duration in the issuing rules
Bug 8365: Add unit tests
Bug 8365: Add a renewal duration in the issuing rules
Bug 8365: Add unit tests
Bug 8365: Add unit tests and fix QA issues
Bug 8365: Add a renewal duration in the issuing rules
Bug 8365: Add unit tests and fix QA issues
[PASSED QA] Bug 8365: Add a renewal duration in the issuing rules
[PASSED QA] Bug 8365: Add unit tests and fix QA issues
Bug 8365: Add a renewal duration in the issuing rules
Bug 8365: Add unit tests and fix QA issues
Bug 8365: Followup Force DateTime type for the datedue variable
Bug 8365: FIX Return issue values instead of item values
Bug 8365: FIX Return issue values instead of item values
[PASSED QA] Bug 8365: Add a renewal duration in the issuing rules
[PASSED QA] Bug 8365: Add unit tests and fix QA issues
[PASSED QA] Bug 8365: Followup Force DateTime type for the datedue variable
[PASSED QA] Bug 8365: FIX Return issue values instead of item values

Description Paul Poulain 2012-07-05 13:27:00 UTC
It could be interesting to have a renewal duration, to let a library say:
"books checked-out for 3 weeks, with 1 renewal of 1 week"

this renewal duration should be at issuing rule level
Comment 1 Jonathan Druart 2012-09-07 15:27:32 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-09-19 16:51:09 UTC
How is the renewal date calculated if the renewals period is set to zero (as it will be when the feature is added on top of an existing installation) ?

Shouldn't this feature accommodate renewal periods in hours or days? How can a library set a renewal period if their loan period is in hours?
Comment 3 Jonathan Druart 2012-09-20 09:39:57 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-09-20 09:43:10 UTC
(In reply to comment #2)

Hello Owen,
Thanks for your question. It permits me to improve this patch :)

> How is the renewal date calculated if the renewals period is set to zero (as
> it will be when the feature is added on top of an existing installation) ?

A new query is added to the updatedatabase. It set the renewalperiod field with the issuelength value.

> Shouldn't this feature accommodate renewal periods in hours or days? How can
> a library set a renewal period if their loan period is in hours?

Now it is possible, the renewalperiod unit is based on the unit value (days or hours)
Comment 5 Kyle M Hall 2012-12-07 14:54:33 UTC
Large merge conflict in C4/Circulation.pm
Comment 6 Jonathan Druart 2012-12-10 10:03:54 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2012-12-14 16:37:48 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2013-02-12 22:04:39 UTC
Hi Jonathan, 

I have started testing this - could you please add some unit tests in a follow up for the changed routines?
Comment 9 Katrin Fischer 2013-02-12 22:58:53 UTC
Hi Jonathan, 

I read through your code and have some questions, mostly about documentation:

1) I was wondering if this line in the POD for CanBookBeRenewed makes sense:
C<$dbh> is a DBI handle to the Koha database.

Just a note, little typo a few lines below:
C<$CanBookBeRenewed> returns a true value _iff_ the item may be renewed. The ...

2) POD for CalcDateDue misses $isrenewal

3) In the circulation matrix the new field is named "Renewals period" - "Renewal period" looks a bit better to me, but we are both not native speakers, so I will leave that to someone else. :)

I also ran the test suite - so far it looks good. I will continue with testing the interface next.
Comment 10 Katrin Fischer 2013-02-13 11:39:28 UTC
I learned that iff is not a typo, so ignore that :)
Comment 11 Jonathan Druart 2013-02-13 14:09:39 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2013-02-13 14:11:24 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2013-02-13 14:11:34 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2013-02-13 14:16:37 UTC
I switch back to needs signoff, I made some modifications into the CalcDateDue routine (in my second patch).
The main modification is I removed the return statement (return $dt) when the syspref useDaysMode eq 'Days'. I think we want to check the ReturnBeforeExpiry syspref value in all cases.
Comment 15 Katrin Fischer 2013-02-13 14:43:03 UTC
Hi Jonathan, thx for taking my comments into account! The follow up looks good to me, but I think the commit message belongs to another patch?
Comment 16 Jonathan Druart 2013-02-13 14:57:56 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-02-13 14:58:29 UTC
(In reply to comment #15)
> Hi Jonathan, thx for taking my comments into account! The follow up looks
> good to me, but I think the commit message belongs to another patch?

Hi Katrin,
In fact I forgot to add the new file :-/
Comment 18 Kyle M Hall 2013-02-15 13:20:51 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-02-15 13:21:02 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2013-03-14 09:43:28 UTC
Hi Jonathan, 
the new unit tests don't pass the QA script - can you take a look please? 
But I will continue testing :)

* t/db_dependent/Circulation_issuingrules.t    FAIL
	pod                         OK
	forbidden patterns          OK
	valid                       OK
	critic                      FAIL
Code before strictures are enabled at line 11, column 1. See page 429 of PBP.
Comment 21 Jonathan Druart 2013-03-14 10:41:12 UTC
(In reply to comment #20)
> Hi Jonathan, 
> the new unit tests don't pass the QA script - can you take a look please? 
> But I will continue testing :)
> 
> * t/db_dependent/Circulation_issuingrules.t    FAIL
> 	pod                         OK
> 	forbidden patterns          OK
> 	valid                       OK
> 	critic                      FAIL
> Code before strictures are enabled at line 11, column 1. See page 429 of PBP.

It is caused by the "use Modern::Perl;".
You should add to your ~/.perlcriticrc :
  [TestingAndDebugging::RequireUseWarnings]
  equivalent_modules = Modern::Perl

  [TestingAndDebugging::RequireUseStrict]
  equivalent_modules = Modern::Perl

Take a look at the perlcriticrc file present in the qa-tools repository.
Comment 22 Katrin Fischer 2013-03-14 11:11:49 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2013-03-14 11:14:28 UTC Comment hidden (obsolete)
Comment 24 Jared Camins-Esakov 2013-03-17 01:59:34 UTC
This no longer applies. Please rebase.
Comment 25 Jonathan Druart 2013-03-18 09:51:10 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2013-03-18 09:51:15 UTC Comment hidden (obsolete)
Comment 27 jmbroust 2013-03-18 15:58:54 UTC
When allowing 2 renewals for X days, the first renewal due date is ok but when you renew for the second time the due date doesn't change. Tested with System Pref Renewalperiodbase turned to "the old due date of the checkout"
Comment 28 Katrin Fischer 2013-03-18 16:01:42 UTC
Hi,
I think this might be a configuation issue - could you check the value in the system preference RenewalPeriodBase? If it's set to calculate from today's date, the second renewal has the same date as the first and that would be correct.
Comment 29 jmbroust 2013-03-19 09:44:02 UTC
(En réponse au commentaire 28)
> Hi,
> I think this might be a configuation issue - could you check the value in
> the system preference RenewalPeriodBase? If it's set to calculate from
> today's date, the second renewal has the same date as the first and that
> would be correct.

I tested one more time on a sandbox but I confirm the problem : the due date doesn't change when you renew more than one time with syspref renewalperiodbase turned on : "the old due date of the checkout". The due date should change any time.
Comment 30 Jonathan Druart 2013-03-19 13:23:04 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2013-03-19 13:25:28 UTC
(In reply to comment #29)
> I tested one more time on a sandbox but I confirm the problem : the due date
> doesn't change when you renew more than one time with syspref
> renewalperiodbase turned on : "the old due date of the checkout". The due
> date should change any time.

Yep, Good catch! Please retest, it should be fixed with this last patch.
Comment 32 Jonathan Druart 2013-03-19 13:43:24 UTC Comment hidden (obsolete)
Comment 33 Paul Poulain 2013-03-19 14:10:23 UTC
Patch tested with a sandbox, by Broust <jean-manuel.broust@univ-lyon2.fr>
Comment 34 Paul Poulain 2013-03-19 14:10:45 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2013-03-19 17:15:08 UTC
Created attachment 16433 [details] [review]
[PASSED QA] Bug 8365: Add a renewal duration in the issuing rules

Renew an issue for a number of days (filled in the issuing rules).

Test if rules work for any i[item]types and if there is no regression.

- new column issuingrules.renewalperiod
- remove all occurrences of an already removed syspref (globalDueDate)
- remove an unused routine (Overdues::GetIssuingRules)

How it works:
- On existing installations, the issuingrules.renewalperiod =
  issuingrules.loanlength. So the behaviour is the same before and after
  this patch.
- when you add a rule, you can choose a renewal period (the unit value
  is the issuingrules.unit). So you can have a renewal period in hours
  or days.
- The default value for the renewal period is 21 days (same as
  loanlength)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Test comments on second patch.
Comment 36 Katrin Fischer 2013-03-19 17:15:39 UTC
Created attachment 16434 [details] [review]
[PASSED QA] Bug 8365: Add unit tests and fix QA issues

This patch adds some unit tests for CalcDateDue and GetLoanLength

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests and QA script pass.

Tests done:
- Checked update works correctly for existing circulation rules.
- Adding, deleting and overwriting circulation rules works.
- Renewals work for different circulation rules and changes
  to the holiday calendar.
Comment 37 Katrin Fischer 2013-03-19 17:16:10 UTC
Created attachment 16435 [details] [review]
[PASSED QA] Bug 8365: Followup Force DateTime type for the datedue variable

Signed-off-by: Broust <jean-manuel.broust@univ-lyon2.fr>
I tested one more time on a sandbox but I confirm the problem:
the due date doesn't change when you renew more than one time
with syspref renewalperiodbase turned on: "the old due date of
the checkout". The due date should change any time.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Added signed-off line and problem description from bugzilla.
Comment 38 Katrin Fischer 2013-03-19 17:16:32 UTC
Created attachment 16436 [details] [review]
[PASSED QA] Bug 8365: FIX Return issue values instead of item values

Before this patch, the GetItemIssue routine returns items.renewals
instead of issues.renewals

Signed-off-by: Broust <jean-manuel.broust@univ-lyon2.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests and QA script pass.
Comment 39 Jared Camins-Esakov 2013-03-22 01:30:05 UTC
This looks good except for one thing: if I leave renewal period blank, it gets set to zero when the circ rule is saved. When renewal period is left blank, it should probably be set to the same as the regular issue period, based on the principle of least surprise.
Comment 40 Katrin Fischer 2013-03-22 05:54:04 UTC
Hi Jared, I can see what you mean, but 0 is the default behaviour for the other fields (max holds, max renwals) so from this point of view it's consistent.
Comment 41 Jonathan Druart 2013-03-22 08:57:26 UTC
(In reply to comment #40)
> Hi Jared, I can see what you mean, but 0 is the default behaviour for the
> other fields (max holds, max renwals) so from this point of view it's
> consistent.

Jared, I confirm too, it is the current default behavior.
Comment 42 Katrin Fischer 2013-03-22 09:20:21 UTC
Hm thinking more about this, it looks to me like it's out of the scope of this bug, because that's just what happens now if you create a completely new rule. And when editing a rule it should not do something unexpected.
Comment 43 Jared Camins-Esakov 2013-03-22 11:32:29 UTC
(In reply to comment #40)
> Hi Jared, I can see what you mean, but 0 is the default behaviour for the
> other fields (max holds, max renwals) so from this point of view it's
> consistent.

I think we are inconsistent, because leaving some fields blank results in "unlimited." However, I'll push this as-is if you're comfortable with this and have considered the issue.
Comment 44 Katrin Fischer 2013-03-22 11:57:46 UTC
I agree with you - just saying that the inconsistency was not introduced by this patch - it's there already and there is no regression. :)
Comment 45 Jared Camins-Esakov 2013-03-22 11:58:40 UTC
This patch has been pushed to master.