Bug 14395 - Two different ways to calculate 'No renewal before' (days or hours)
Summary: Two different ways to calculate 'No renewal before' (days or hours)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P3 enhancement (vote)
Assignee: Holger Meißner
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 15666 (view as bug list)
Depends on: 14101
Blocks:
  Show dependency treegraph
 
Reported: 2015-06-16 07:51 UTC by Holger Meißner
Modified: 2017-12-07 22:15 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14395: Add new syspref NoRenewalBeforePrecision (2.59 KB, patch)
2015-07-16 14:01 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Code changes (2.94 KB, patch)
2015-07-16 14:01 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Code changes (2.94 KB, patch)
2015-07-16 14:10 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Add new syspref NoRenewalBeforePrecision (3.99 KB, patch)
2015-07-17 09:02 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Code changes (3.15 KB, patch)
2015-07-17 09:02 UTC, Holger Meißner
Details | Diff | Splinter Review
No renewal before TEST IN DAYS AND HOURS UNIT (159.48 KB, application/pdf)
2015-07-25 15:16 UTC, Claudio
Details
Bug 14395: Unit tests (4.82 KB, patch)
2015-07-27 13:27 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Unit tests (5.21 KB, patch)
2015-09-22 13:26 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Add new syspref NoRenewalBeforePrecision (3.99 KB, patch)
2015-10-23 09:05 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Code changes (3.15 KB, patch)
2015-10-23 09:05 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Unit tests (5.36 KB, patch)
2015-10-23 09:05 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Add new syspref NoRenewalBeforePrecision (3.99 KB, patch)
2015-11-18 09:30 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Code changes (3.15 KB, patch)
2015-11-18 09:30 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Unit tests (5.33 KB, patch)
2015-11-18 09:30 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14395: Add new syspref NoRenewalBeforePrecision (4.04 KB, patch)
2016-02-08 21:42 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 14395: Code changes (3.20 KB, patch)
2016-02-08 21:43 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 14395: Unit tests (5.39 KB, patch)
2016-02-08 21:43 UTC, Jesse Weaver
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Meißner 2015-06-16 07:51:41 UTC
The 'No renewal before' setting is based on the exact DateTime of the due date. This can be confusing when the loan period is measured in days, because Koha sets the time for the due date to 23:59.

So with 'No renewal before' set to one day (exactly 24 hours) the loan will be renewable on the next day, but not before 23:59 when there is only one minute left of the day.

There should be a syspref to ignore the exact time and only check the date. So that an item issued with 1 day 'No renewal before' will be renewable any time on the next day.
Comment 1 Claudio 2015-06-26 01:28:37 UTC
I agree Holger
Comment 2 Holger Meißner 2015-07-16 14:01:01 UTC Comment hidden (obsolete)
Comment 3 Holger Meißner 2015-07-16 14:01:04 UTC Comment hidden (obsolete)
Comment 4 Holger Meißner 2015-07-16 14:10:05 UTC Comment hidden (obsolete)
Comment 5 Holger Meißner 2015-07-17 07:25:14 UTC
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.
Comment 6 Holger Meißner 2015-07-17 09:02:22 UTC Comment hidden (obsolete)
Comment 7 Holger Meißner 2015-07-17 09:02:26 UTC Comment hidden (obsolete)
Comment 8 Holger Meißner 2015-07-17 09:05:03 UTC
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.
Comment 9 Claudio 2015-07-17 16:10:03 UTC
(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...
Comment 10 Claudio 2015-07-17 16:11:42 UTC
(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...
Comment 11 Claudio 2015-07-17 16:20:52 UTC
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!!
Comment 12 Katrin Fischer 2015-07-19 20:49:45 UTC
bondiurbano - if you have tested the patches, why not sign off? :)
Comment 13 Claudio 2015-07-19 20:51:52 UTC
Here I go...
Comment 14 Katrin Fischer 2015-07-19 21:43:06 UTC
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)
Comment 15 Claudio 2015-07-19 21:52:34 UTC
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...
Comment 16 Katrin Fischer 2015-07-19 22:11:47 UTC
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 :)
Comment 17 Claudio 2015-07-19 22:19:09 UTC
(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!
Comment 18 Holger Meißner 2015-07-21 07:25:52 UTC
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
Comment 19 Claudio 2015-07-21 11:40:14 UTC
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.
Comment 20 Kyle M Hall 2015-07-24 15:56:30 UTC
Please add a unit tests for the two subroutines changed.
Comment 21 Claudio 2015-07-25 15:16:59 UTC
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
Comment 22 Holger Meißner 2015-07-27 13:27:39 UTC Comment hidden (obsolete)
Comment 23 Holger Meißner 2015-07-27 13:37:26 UTC
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.
Comment 24 Jonathan Druart 2015-08-19 11:28:17 UTC
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.
Comment 25 Claudio 2015-09-02 12:48:39 UTC
(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.
Comment 26 Holger Meißner 2015-09-22 13:26:39 UTC Comment hidden (obsolete)
Comment 27 Holger Meißner 2015-09-22 13:30:53 UTC
Sry it took so long, bondiurbano!

Now bug 14101 and 14395 have their own unit tests. Setting back to signed off.
Comment 28 Claudio 2015-09-22 15:41:32 UTC
(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 29 Jonathan Druart 2015-10-01 10:49:13 UTC
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).
Comment 30 Claudio 2015-10-14 19:07:14 UTC
(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?
Comment 31 Holger Meißner 2015-10-15 09:34:37 UTC
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.
Comment 32 Claudio 2015-10-15 12:34:18 UTC
(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!
Comment 33 Holger Meißner 2015-10-23 09:05:36 UTC Comment hidden (obsolete)
Comment 34 Holger Meißner 2015-10-23 09:05:39 UTC Comment hidden (obsolete)
Comment 35 Holger Meißner 2015-10-23 09:05:42 UTC Comment hidden (obsolete)
Comment 36 Holger Meißner 2015-10-23 09:13:58 UTC
Here you go, bondiurbano. Have a nice weekend :)
Comment 37 Claudio 2015-10-23 11:06:13 UTC
(In reply to Holger Meißner from comment #36)
> Here you go, bondiurbano. Have a nice weekend :)

Thanks Holger! Impressive...
Comment 38 Claudio 2015-11-10 12:53:19 UTC
(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...
Comment 39 Holger Meißner 2015-11-18 09:30:49 UTC Comment hidden (obsolete)
Comment 40 Holger Meißner 2015-11-18 09:30:52 UTC Comment hidden (obsolete)
Comment 41 Holger Meißner 2015-11-18 09:30:55 UTC Comment hidden (obsolete)
Comment 42 Holger Meißner 2015-11-18 09:33:23 UTC
Rebased on master.
Comment 43 Claudio 2015-12-06 20:06:19 UTC
(In reply to Holger Meißner from comment #42)
> Rebased on master.

Holger, I do not understand what is the status of this issue...
Comment 44 Holger Meißner 2015-12-10 13:54:28 UTC
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.
Comment 45 Claudio 2015-12-10 13:57:44 UTC
(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!
Comment 46 Jesse Weaver 2016-02-08 21:42:59 UTC
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>
Comment 47 Jesse Weaver 2016-02-08 21:43:03 UTC
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>
Comment 48 Jesse Weaver 2016-02-08 21:43:08 UTC
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>
Comment 49 Kyle M Hall 2016-02-26 14:37:50 UTC
Pushed to master! Should be in Koha 3.24, thanks Holger!
Comment 50 Claudio 2016-05-02 12:46:08 UTC
(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
Comment 51 Claudio 2016-08-29 00:05:55 UTC
*** Bug 15666 has been marked as a duplicate of this bug. ***