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
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
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>
Hi Sophie, Please provide tests for this change.
*** Bug 17806 has been marked as a duplicate of this bug. ***
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 ?
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.
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.
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.
Setting to 'Failed QA' based on Comment 8
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 ?
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
Moving code isn't the purpose of this patch, I removed my changes concerning Koha::Calendar and used a function from C4::Calendar
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>
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>
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>
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>
Created attachment 66707 [details] [review] Bug 11580 [QA Followup] Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 17.11, thanks to everybody involved!
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.
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
Created attachment 66886 [details] [review] Bug 11580 - Fix expiry calculation and rewrite tests
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...
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.
(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!