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?

(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)

Large merge conflict in C4/Circulation.pm

Hi Jonathan, 

I have started testing this - could you please add some unit tests in a follow up for the changed routines?

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.

I learned that iff is not a typo, so ignore that :)

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.

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?

(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 :-/

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.

(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.

This no longer applies. Please rebase.

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"

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.

(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.

(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.

Patch tested with a sandbox, by Broust <jean-manuel.broust@univ-lyon2.fr>

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.

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.

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.

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.

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.

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.

(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.

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.

(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.

I agree with you - just saying that the inconsistency was not introduced by this patch - it's there already and there is no regression. :)

This patch has been pushed to master.