Bug 16387 - Incorrect loan period calculation when using decreaseLoanHighHolds feature
Summary: Incorrect loan period calculation when using decreaseLoanHighHolds feature
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Whiteboard:
Keywords: Academy
Depends on:
Blocks: 17143
  Show dependency treegraph
 
Reported: 2016-04-29 01:03 UTC by Katrin Fischer
Modified: 2017-02-09 09:41 UTC (History)
8 users (show)

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


Attachments
Bug 16387: Fix default shortened loan period time (3.13 KB, patch)
2016-12-12 15:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16387: Fix default shortened loan period time (3.19 KB, patch)
2017-01-19 20:17 UTC, Gracie
Details | Diff | Splinter Review
Bug 17917: Fix path issue in t/db_dependent/check_sysprefs.t (2.69 KB, patch)
2017-01-19 21:06 UTC, Gracie
Details | Diff | Splinter Review
Bug 16387: Fix default shortened loan period time (3.25 KB, patch)
2017-01-27 13:51 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2016-04-29 01:03:28 UTC
When a loan period is shortened due to using decreaseLoanHighHolds* the time is always set to the current time in X days, even if the original loan period is given in days and not in hours.

It should default to 23:59 as is normal for loan periods given in days.
Comment 1 Christopher Brannon 2016-08-19 20:52:13 UTC
I was just noticing this.  I talked with Nick Clemens, and he thinks it might be this in circulation.pm:

1178         my $issuedate = DateTime->now( time_zone => C4::Context->tz() );
1179 
1180         my $calendar = Koha::Calendar->new( branchcode => $branch );
1181 
1182         my $itype =
1183           ( C4::Context->preference('item-level_itypes') )
1184           ? $biblio->{'itype'}
1185           : $biblio->{'itemtype'};
1186 
1187         my $orig_due = C4::Circulation::CalcDateDue( $issuedate, $itype, $branch, $borrower );
1188 
1189         my $decreaseLoanHighHoldsDuration = C4::Context->preference('decreaseLoanHighHoldsDuration');
1190 
1191         my $reduced_datedue = $calendar->addDate( $issuedate, $decreaseLoanHighHoldsDuration );
1192 
1193         if ( DateTime->compare( $reduced_datedue, $orig_due ) == -1 ) {
1194             $return_data->{exceeded} = 1;
1195             $return_data->{duration} = $decreaseLoanHighHoldsDuration;
1196             $return_data->{due_date} = $reduced_datedue;

Line 1196 is assigning the value in $reduced_datedue.
Line 1191 is basing the value of $reduced_datedue from $issuedate
Line 1178 is assigning $issuedate a value based on now(the time at this very moment).

So somehow we need $reduced_datedue to use the same time as $orig_due, but only adjust the date, and not reference $issuedate.

I would do this, but I barely understand what I am looking at, thanks to Nick's help.

Hope this helps.
Comment 2 Christopher Brannon 2016-10-05 15:03:21 UTC
I would love to see this resolved.  I've even provided some clues to the problem.  Any takers?
Comment 3 Lari Taskula 2016-12-12 15:21:37 UTC
Created attachment 58127 [details] [review]
Bug 16387: Fix default shortened loan period time

When a loan period is shortened due to using decreaseLoanHighHolds* the time is
always set to the current time in X days, even if the original loan period is
given in days and not in hours.

It should default to 23:59 as is normal for loan periods given in days.

As original due date time defaults to 23:59 when given in days, this patch
modifies the hours and minutes of shortened due date to be equal to original due
date.

To test:
1. prove t/db_dependent/DecreaseLoanHighHolds.t
Comment 4 Claire Gravely 2017-01-09 08:55:42 UTC
tests failed for me:

t/db_dependent/DecreaseLoanHighHolds.t (Wstat: 768 Tests: 17 Failed: 3)
  Failed tests:  6-8
  Non-zero exit status: 3
Files=1, Tests=17,  5 wallclock secs ( 0.04 usr  0.00 sys +  4.19 cusr  0.36 csys =  4.59 CPU)
Result: FAIL
Comment 5 Gracie 2017-01-19 20:17:03 UTC
Created attachment 59278 [details] [review]
Bug 16387: Fix default shortened loan period time

When a loan period is shortened due to using decreaseLoanHighHolds* the time is
always set to the current time in X days, even if the original loan period is
given in days and not in hours.

It should default to 23:59 as is normal for loan periods given in days.

As original due date time defaults to 23:59 when given in days, this patch
modifies the hours and minutes of shortened due date to be equal to original due
date.

To test:
1. prove t/db_dependent/DecreaseLoanHighHolds.t

Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>
Comment 6 Gracie 2017-01-19 20:17:51 UTC
The test plan passed for me

t/db_dependent/DecreaseLoanHighHolds.t .. ok     
All tests successful.
Files=1, Tests=14,  4 wallclock secs ( 0.05 usr  0.01 sys +  3.42 cusr  0.14 csys =  3.62 CPU)
Result: PASS
Comment 7 Gracie 2017-01-19 21:06:29 UTC
Created attachment 59285 [details] [review]
Bug 17917: Fix path issue in t/db_dependent/check_sysprefs.t

C4::Context->config('intranet') is returning different
values based on whether it is a full t/db_dependent test
or a specific check_sysprefs.t test run.

Tests are only run in kohaclone, so it makes no sense to
reference config at all. Brought in rel2abs logic to build
a reasonable $root_dir.

TEST PLAN
---------
-- on a kohadevbox

sudo apt-get update
sudo apt-get upgrade
-- need to make sure the koha-commonpackage is more current

git checkout master
git pull
sudo koha-shell -c "prove t/db_dependent" kohadev
-- this should have a check_sysprefs.t failure
sudo koha-shell -c "prove t/db_dependent/check_sysprefs.t" kohadev
-- this should strangely pass

git checkout -b bug_17917 origin/master
git bz apply 17917
sudo koha-shell -c "prove t/db_dependent" kohadev
-- there will not be a check_sysprefs.t failure
sudo koha-shell -c "prove t/db_dependent/check_sysprefs.t" kohadev
-- this should pass

-- run koha qa test tools

Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>


Current status: Signed Off
Comment 8 Jonathan Druart 2017-01-27 13:51:19 UTC
Created attachment 59619 [details] [review]
Bug 16387: Fix default shortened loan period time

When a loan period is shortened due to using decreaseLoanHighHolds* the time is
always set to the current time in X days, even if the original loan period is
given in days and not in hours.

It should default to 23:59 as is normal for loan periods given in days.

As original due date time defaults to 23:59 when given in days, this patch
modifies the hours and minutes of shortened due date to be equal to original due
date.

To test:
1. prove t/db_dependent/DecreaseLoanHighHolds.t

Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 9 Kyle M Hall 2017-02-07 17:54:20 UTC
Pushed to master for 17.04, thanks Lari!
Comment 10 Katrin Fischer 2017-02-07 21:22:58 UTC
Love the tests!
This patch has been pushed to 16.11.x and will be in 16.11.04.
Comment 11 Julian Maurice 2017-02-09 09:41:48 UTC
Pushed to 3.22.x for 3.22.17