Bug 11580 - If returnBeforeExpiry is on, holidays are not taken into account to calculate the due date
Summary: If returnBeforeExpiry is on, holidays are not taken into account to calculate...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Baptiste
QA Contact: Kyle M Hall
URL:
Whiteboard:
Keywords:
: 17806 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-01-17 15:50 UTC by Marjorie Barry-Vila
Modified: 2017-12-07 22:24 UTC (History)
10 users (show)

See Also:
Change 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 11580 - If returnBeforeExpiry is ON, use calendar to calculate return date not on closed day (depending on useDaysMode) (1.26 KB, patch)
2016-11-16 16:04 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 11580 - If returnBeforeExpiry is ON, use calendar to calculate return date not on closed day (depending on useDaysMode) (1.32 KB, patch)
2017-01-10 11:57 UTC, Claire Gravely
Details | Diff | Splinter Review
BUG 11580 : Added unit test (11.43 KB, patch)
2017-02-23 16:28 UTC, Baptiste
Details | Diff | Splinter Review
BUG 11580 : Added unit test (2.67 KB, patch)
2017-06-27 15:30 UTC, Baptiste
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11580 - If returnBeforeExpiry is ON, use calendar to calculate return date not on closed day (depending on useDaysMode) (1.39 KB, patch)
2017-08-09 20:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] BUG 11580 : Added unit test (2.74 KB, patch)
2017-08-09 20:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11580 - If returnBeforeExpiry is ON, use calendar to calculate return date not on closed day (depending on useDaysMode) (1.43 KB, patch)
2017-09-01 10:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
BUG 11580 : Added unit test (2.79 KB, patch)
2017-09-01 10:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11580 [QA Followup] (879 bytes, patch)
2017-09-01 10:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11580 - Fix expiry calculation and rewrite tests (4.77 KB, patch)
2017-09-06 15:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 11580: Add one more test and clear the cache (2.22 KB, patch)
2017-09-06 15:53 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 Marjorie Barry-Vila 2014-01-17 15:50:21 UTC
Hi,

If ReturnBeforeExpiry is set to "ON", closed day of calendar is not factor in.

Test plan:
1. ReturnBeforeExpiry is activated
1. Set a closed day on calendar 2014-01-26
2. Set expiry date of a patron like 2014-01-26
3. Do a checkout, due date will be 2014-01-26

Is it a normal behavior?
I dont think so. A due date should not be a closed day?

Thanks,


Marjorie
Comment 1 Sophie MEYNIEUX 2016-11-16 16:04:29 UTC
Created attachment 57562 [details] [review]
Bug 11580 - If returnBeforeExpiry is ON, use calendar to calculate return date not on closed day (depending on useDaysMode)

Test plan:
1. ReturnBeforeExpiry is activated
2. useDaysMode is different from "circulation rules only"
3. Set expiry date of a patron to a near date
4. Set a closed day on calendar for this date
5. Do a checkout

Without patch, return date will be patron expiration date
With the patch, return date will be last open day before patron expiration day
Comment 2 Claire Gravely 2017-01-10 11:57:04 UTC
Created attachment 58740 [details] [review]
Bug 11580 - If returnBeforeExpiry is ON, use calendar to calculate return date not on closed day (depending on useDaysMode)

Test plan:
1. ReturnBeforeExpiry is activated
2. useDaysMode is different from "circulation rules only"
3. Set expiry date of a patron to a near date
4. Set a closed day on calendar for this date
5. Do a checkout

Without patch, return date will be patron expiration date
With the patch, return date will be last open day before patron expiration day

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>
Comment 3 Jonathan Druart 2017-01-11 08:28:17 UTC
Hi Sophie,
Please provide tests for this change.
Comment 4 Katrin Fischer 2017-01-17 23:04:59 UTC
*** Bug 17806 has been marked as a duplicate of this bug. ***
Comment 5 Baptiste 2017-02-23 10:42:16 UTC
I take it to add the tests, but I don't understand why there are 3 options for UseDaysModes despite 2 of them have exactly the same behaviour in all function in which they are called, shouldn't we delete the "caleandar", which doesn't have the specified behaviour ?
Comment 6 Katrin Fischer 2017-02-23 10:56:18 UTC
Hi Baptiste, 

I think all three options should have a specific behaviour for calculating due dates when the patron _doesn't_ expire before the calculated due date:

- ... the calendar to push the due date to the next open day
- ... the calendar to sktip all closed days
- ... circulation rules only (ignore the calendar altogether)

But when you use ReturnExpiry to shorten the due date and it's before the calculated due date I think you are right that the behaviour of the first 2 will be the same.
Comment 7 Baptiste 2017-02-23 16:28:27 UTC
Created attachment 60621 [details] [review]
BUG 11580 : Added unit test

Added one unit test when the syspref useDaysMode is active.  To perform it I
had to put the function add_holiday in the perl module Koha/Calendar.pm from
the script tools/newHolidays.pl, in order to use the function in the tests.
Comment 8 Srdjan Jankovic 2017-03-21 02:29:13 UTC
This kind of looks wrong. Koha namespace should not be depending on C4 (apart from unfortunate Context). Koha::Calendar should not use C4::Calendar. The other way around is OK.
Comment 9 Owen Leonard 2017-04-24 15:37:09 UTC
Setting to 'Failed QA' based on Comment 8
Comment 10 Baptiste 2017-05-19 07:48:16 UTC
I don't quite understand what is supposed to be done.
Shall we move the whole fonction in which this code is added toward Koha::Calendar ?
Comment 11 Baptiste 2017-06-27 15:30:36 UTC
Created attachment 64681 [details] [review]
BUG 11580 : Added unit test

Added one unit test when the syspref useDaysMode is active.
This does not move code anymore
Comment 12 Baptiste 2017-06-27 15:32:39 UTC
Moving code isn't the purpose of this patch, I removed my changes concerning Koha::Calendar and used a function from C4::Calendar
Comment 13 Katrin Fischer 2017-08-09 20:07:59 UTC
Created attachment 65779 [details] [review]
[SIGNED OFF] Bug 11580 - If returnBeforeExpiry is ON, use calendar to calculate return date not on closed day (depending on useDaysMode)

Test plan:
1. ReturnBeforeExpiry is activated
2. useDaysMode is different from "circulation rules only"
3. Set expiry date of a patron to a near date
4. Set a closed day on calendar for this date
5. Do a checkout

Without patch, return date will be patron expiration date
With the patch, return date will be last open day before patron expiration day

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2017-08-09 20:08:35 UTC
Created attachment 65780 [details] [review]
[SIGNED OFF] BUG 11580 : Added unit test

Added one unit test when the syspref useDaysMode is active.
This does not move code anymore

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Kyle M Hall 2017-09-01 10:52:55 UTC
Created attachment 66705 [details] [review]
Bug 11580 - If returnBeforeExpiry is ON, use calendar to calculate return date not on closed day (depending on useDaysMode)

Test plan:
1. ReturnBeforeExpiry is activated
2. useDaysMode is different from "circulation rules only"
3. Set expiry date of a patron to a near date
4. Set a closed day on calendar for this date
5. Do a checkout

Without patch, return date will be patron expiration date
With the patch, return date will be last open day before patron expiration day

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2017-09-01 10:53:03 UTC
Created attachment 66706 [details] [review]
BUG 11580 : Added unit test

Added one unit test when the syspref useDaysMode is active.
This does not move code anymore

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2017-09-01 10:53:06 UTC
Created attachment 66707 [details] [review]
Bug 11580 [QA Followup]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Jonathan Druart 2017-09-01 16:03:07 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 19 Jonathan Druart 2017-09-01 19:16:29 UTC
This change breaks test, please fix ASAP:

t/db_dependent/Circulation/CalcDateDue.t .. 1/5 
#   Failed test 'date expiry'
#   at t/db_dependent/Circulation/CalcDateDue.t line 54.
#          got: '2012-12-31T23:59:00'
#     expected: '2013-01-01T23:59:00'
# Looks like you failed 1 test of 5.
Comment 20 Jonathan Druart 2017-09-01 19:39:08 UTC
I think the correct change is:

+            my $calendar = Koha::Calendar->new( branchcode => $branch );
+            if ( $calendar->is_holiday($datedue) ) {
+                # Don't return on a closed day
+                $datedue = $calendar->prev_open_day($datedue);
+            }

Because if expiry date is not a close day, the date due must be the expiry date.

But then tests from t/db_dependent/Circulation/CalcDateDue.t are failing
Comment 21 Alex Arnaud 2017-09-06 15:38:18 UTC
Created attachment 66886 [details] [review]
Bug 11580 - Fix expiry calculation and rewrite tests
Comment 22 Alex Arnaud 2017-09-06 15:42:30 UTC
Here is a fix about Jonathan's comment.
I don't know which status to choose for this BZ since it has been pushed to master...
Comment 23 Jonathan Druart 2017-09-06 15:53:51 UTC
Created attachment 66888 [details] [review]
Bug 11580: Add one more test and clear the cache

Make sure we are not just returning J-1 and clear the cache before and
after the tests.
Comment 24 Jonathan Druart 2017-09-06 16:32:24 UTC
(In reply to Alex Arnaud from comment #21)
> Created attachment 66886 [details] [review] [review]
> Bug 11580 - Fix expiry calculation and rewrite tests

(In reply to Jonathan Druart from comment #23)
> Created attachment 66888 [details] [review] [review]
> Bug 11580: Add one more test and clear the cache
> 
> Make sure we are not just returning J-1 and clear the cache before and
> after the tests.

These last 2 patches have been pushed to master, thanks Alex!