Description
Marjorie Barry-Vila
2014-01-17 15:50:21 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 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. 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! |