I agree Holger

On second thought, it's kind of pointless to have hourly loans with date-based 'No renewal before'. I'll change it, so that the syspref is only relevant for loans calculated in days. Setting back to ASSIGNED.

Note: With 'date' setting both OPAC user account and staff client display soonest possible renewal including time. With 'exact time' setting the staff client only displays the date. But OPAC shows the time. I couldn't figure out why yet.

(In reply to Holger Meißner from comment #5)
> On second thought, it's kind of pointless to have hourly loans with
> date-based 'No renewal before'. I'll change it, so that the syspref is only
> relevant for loans calculated in days. Setting back to ASSIGNED.

I think so...

(In reply to Holger Meißner from comment #8)
> Note: With 'date' setting both OPAC user account and staff client display
> soonest possible renewal including time. With 'exact time' setting the staff
> client only displays the date. But OPAC shows the time. I couldn't figure
> out why yet.

I think this is not relevant for now...

Holger!

Great job! I have tested all cases in a local virtual machine under Debian Jessie and Koha 3.20.1. All seem to work perfectly!

I have tested both 14101 bug and 14395 bug together.

I think it is ready for signoff.

Thank you so much Holger!!

bondiurbano - if you have tested the patches, why not sign off? :)

Here I go...

One step closer :) But we miss your signature in the patches - do you know that you can use the sandboxes for adding your sign off lines? (http://wiki.koha-community.org/wiki/Sandboxes)

Glup!

I'm soooooo sorry. I wrote above that I tested this issue in a local virtual machine. I did not know about what you are informing me (I am realizing I'm not the best tester!!)

I'm no a programmer nor perl technician. I don't know how to procedure now...

Don't panic, just trying to help :) We can add your sign-off manually for these patches during the QA step.

For adding your sign-off line you can use git-bz if you are using a git installation for your local testing or you can use a sandbox. The sandboxes are targetted at non-developers to help with the process of testing and signing-off - maybe something to give a try sometime :)

(In reply to Katrin Fischer from comment #16)
> Don't panic, just trying to help :) We can add your sign-off manually for
> these patches during the QA step.
> 
> For adding your sign-off line you can use git-bz if you are using a git
> installation for your local testing or you can use a sandbox. The sandboxes
> are targetted at non-developers to help with the process of testing and
> signing-off - maybe something to give a try sometime :)

Thank you Katrin! Of course that I will give it a try as a step beyond of my learning!

Thank you for testing, bondiurbano! :)

Please sign off 14101 as well. Git bz is a really comfortable way to do this, once you have set it up.
http://wiki.koha-community.org/wiki/Git_bz_configuration

Holger,

Please sign off for me this time. Git or sandboxes are misteries for me for now, despite I know that I must to learn its use as a best practice. 

I beg your patient.

Please add a unit tests for the two subroutines changed.

Created attachment 41189 [details]
No renewal before TEST IN DAYS AND HOURS UNIT

No renewal before TEST IN DAYS AND HOURS UNIT

Tested in Debian Jessie, Koha 3.20.02

Sorry, I forgot about the unit tests. Note that two test cases (related to bug 11634) still fail and I think they are not affected by anything I did. I'm not sure how to fix them and it probably should be done in another bug.

Thanks for looking at this, Kyle! Setting back to Signed Off.

Applying: Bug 14395: Add new syspref NoRenewalBeforePrecision
Applying: Bug 14395: Code changes
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.

(In reply to Holger Meißner from comment #23)
> Sorry, I forgot about the unit tests. Note that two test cases (related to
> bug 11634) still fail and I think they are not affected by anything I did.
> I'm not sure how to fix them and it probably should be done in another bug.
> 
> Thanks for looking at this, Kyle! Setting back to Signed Off.

Holger, sorry for this comment: this patch is waiting to be fixed and  sent to QA for testing. You have made a great job and it would be nice to have it in next release.

Sry it took so long, bondiurbano!

Now bug 14101 and 14395 have their own unit tests. Setting back to signed off.

(In reply to Holger Meißner from comment #27)
> Sry it took so long, bondiurbano!
> 
> Now bug 14101 and 14395 have their own unit tests. Setting back to signed
> off.

Great job Holger! Thank you very much for your dedication and concern. Hug!

Comment on attachment 41054 [details] [review]
Bug 14395: Code changes

Review of attachment 41054 [details] [review]:
-----------------------------------------------------------------

Please provide a better commit description.

::: C4/Circulation.pm
@@ +2772,5 @@
> +        {
> +            $soonestrenewal->truncate( to => 'day' );
> +        }
> +
> +        if ( $soonestrenewal > DateTime->now( time_zone => C4::Context->tz() ) )

Prefer dt_from_string to get a DateTime object (yes this already existed before your patch, but it's worth to make things consistent).

(In reply to Holger Meißner from comment #27)
> Sry it took so long, bondiurbano!
> 
> Now bug 14101 and 14395 have their own unit tests. Setting back to signed
> off.

Holger! My friend! You did not abandon me, didn't you?

I apologize. We have a bit of staff shortage here. But don't worry, it's on my to-do-list. I hope I'll get it done next week.

(In reply to Holger Meißner from comment #31)
> I apologize. We have a bit of staff shortage here. But don't worry, it's on
> my to-do-list. I hope I'll get it done next week.

Thank you Holger! Nice to read you and take your time...Thanks again!

Here you go, bondiurbano. Have a nice weekend :)

(In reply to Holger Meißner from comment #36)
> Here you go, bondiurbano. Have a nice weekend :)

Thanks Holger! Impressive...

(In reply to Holger Meißner from comment #36)
> Here you go, bondiurbano. Have a nice weekend :)

Holger, the next release is ahead and it seems we have no lucky...

Rebased on master.

(In reply to Holger Meißner from comment #42)
> Rebased on master.

Holger, I do not understand what is the status of this issue...

Don't worry, bondiurbano. Everything is okay. We're just waiting to pass QA. The QA team also has to check a lot of other bugs, so this may take some time. That's all.

(In reply to Holger Meißner from comment #44)
> Don't worry, bondiurbano. Everything is okay. We're just waiting to pass QA.
> The QA team also has to check a lot of other bugs, so this may take some
> time. That's all.

Thanks Holger!

Created attachment 47777 [details] [review]
Bug 14395: Add new syspref NoRenewalBeforePrecision

Currently the 'NoRenwalBefore' setting is always based on the exact
DateTime of the due date. This patch introduces a new global syspref for
choosing if 'NoRenewalBefore' should instead be calculated based on date
only. This is only relevant for loans caluclated in days. Hourly loans
are not affected.

To test:

1) Apply bug 14101, then apply this patch.

2) Run installer/data/mysql/updatedatabase.pl

3) Confirm that a new syspref NoRenewalBeforePrecision is available
   in administration. It should let you choose between 'date' (default)
   and 'exact time'.

Sponsored-by: Hochschule für Gesundheit (hsg), Germany

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>

Created attachment 47778 [details] [review]
Bug 14395: Code changes

This patch updates the calculation of 'No renewal before' to include the
new syspref NoRenewalBeforePrecision.

To test:

1) Check out an hour-based loan with 'No renewal before' set to 1.
   Switch syspref NoRenewalBeforePrecision between 'date' and 'exact
   time'. Confirm that with both settings the item cannot be renewed
   until exactly one hour before due.

2) Check out a day-based loan with 'No renewal before' set to 1 day.
   Confirm that:
   * with NoRenewalBeforePrecision set to 'date', renewal is possible
     at 12:00 AM on the day before due.
   * with NoRenewalBeforePrecision set to 'exact time', renewal is
     possible at 11:59 PM on the day before due.

Sponsored-by: Hochschule für Gesundheit (hsg), Germany

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>

Created attachment 47779 [details] [review]
Bug 14395: Unit tests

This patch adds test cases for the new syspref NoRenewalBeforePrecision.
Tests if GetSoonestRenewDate returns correct values for both 'date' and
'exact_time' settings.
Also adds a few comments related to bugs 7413 and 14101.

To test:

1) prove t/db_dependent/Circulation.t

Sponsored-by: Hochschule für Gesundheit (hsg), Germany

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>

Pushed to master! Should be in Koha 3.24, thanks Holger!

(In reply to Holger Meißner from comment #44)
> Don't worry, bondiurbano. Everything is okay. We're just waiting to pass QA.
> The QA team also has to check a lot of other bugs, so this may take some
> time. That's all.

Holger, please look at bug #15581 by Jonathan Druart

*** Bug 15666 has been marked as a duplicate of this bug. ***