Fines accrue on days set as closed in the calendar.
The code to skip holidays during fine calculation seems to be present. Has the user set finescalendar syspref to noFinesWhenClosed?
Yes, the finescalendar syspref is set to not include the days the library is closed. (In reply to comment #1) > The code to skip holidays during fine calculation seems to be present. Has > the user set finescalendar syspref to noFinesWhenClosed?
Calculating fines correctly is insanely important. This is a big deal.
*** Bug 8243 has been marked as a duplicate of this bug. ***
Created attachment 10349 [details] [review] Holidays Test Plan - DB Dependent This test file is meant to be used with the sample database with a minimum of the sample libraries and sample holidays installed.
While the non-db-dependent Kalendar.t passes all tests, this new db-dependent test plan fails all tests where it should return 1 ( i.e. yes, this is a holiday ).
It appears that *most* of my test fails because the default holidays data has no branchcode. There is no way to set pan-branch holidays within Koha, so I will consider the lack of branchcodes in the test data to be the bug. However, the plan is still failing on some tests.
Created attachment 10353 [details] [review] 8110 - Fines accruing on closed days - Fix Day/Month Holidays Bug This patch fixes a bug in which day/month holidays are stored in the Koha::Calendar internal hash not by month/day but incorrectly by day/month. To test: 1) Apply 'Holidays Test Plan - DB Dependent' patch, create MPL holidays for Sunday, New Years Day, and Christmas. 2) Run test plan, only Christmas should fail ( New Years does not because 1-1 means the day and month position do not matter ). 3) Apply this patch 4) Run test plan again
Created attachment 10356 [details] [review] 8110 - Fines accruing on closed days - Fix Day/Month Holidays Bug This patch fixes a bug in which day/month holidays are stored in the Koha::Calendar internal hash not by month/day but incorrectly by day/month. To test: 1) Apply 'Holidays Test Plan - DB Dependent' patch, create MPL holidays for Sunday, New Years Day, and Christmas. 2) Run test plan, only Christmas should fail ( New Years does not because 1-1 means the day and month position do not matter ). 3) Apply this patch 4) Run test plan again Signed-off-by: Liz Rea <wizzyrea@gmail.com> Test plan works as advertised.
I've set a test install with this settings: mysql> SELECT * FROM repeatable_holidays; +----+------------+---------+------+-------+-------+----------------+ | id | branchcode | weekday | day | month | title | description | +----+------------+---------+------+-------+-------+----------------+ | 2 | TEST | 0 | NULL | NULL | | Sundays | | 3 | TEST | NULL | 1 | 1 | | New Year's Day | | 4 | TEST | NULL | 25 | 12 | | Christmas | +----+------------+---------+------+-------+-------+----------------+ 3 rows in set (0.00 sec) And got: tomas@tomas-desktop:~/Work/koha/devel/koha-community-src$ perl t/db_dependent/Holidays.t 1..8 ok 1 - use Koha::Calendar; ok 2 - use C4::Calendar; ok 3 - Koha::Calendar class returned isa Koha::Calendar ok 4 - C4::Calendar class returned isa C4::Calendar not ok 5 - Sunday is a closed day # Failed test 'Sunday is a closed day' # at t/db_dependent/Holidays.t line 42. # got: '0' # expected: '1' ok 6 - Monday is not a closed day not ok 7 - Christmas is a closed day # Failed test 'Christmas is a closed day' # at t/db_dependent/Holidays.t line 44. # got: '0' # expected: '1' not ok 8 - New Years day is a closed day # Failed test 'New Years day is a closed day' # at t/db_dependent/Holidays.t line 45. # got: '0' # expected: '1' # Looks like you failed 3 tests of 8.
Created attachment 10363 [details] [review] 8110 - Fines accruing on closed days - Fix Day/Month Holidays Bug This patch fixes a bug in which day/month holidays are stored in the Koha::Calendar internal hash not by month/day but incorrectly by day/month. To test: 1) Apply 'Holidays Test Plan - DB Dependent' patch, create MPL holidays for Sunday, New Years Day, and Christmas. 2) Run test plan, only Christmas should fail ( New Years does not because 1-1 means the day and month position do not matter ). 3) Apply this patch 4) Run test plan again Signed-off-by: Liz Rea <wizzyrea@gmail.com> Test plan works as advertised. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Test runs flawlessly. I originaly missed to set the expected branchcode
Created attachment 10368 [details] [review] bug 8110 - add branchcode MPL to sample holidays to facilitate automated testing.
Created attachment 10382 [details] [review] [SIGNED-OFF] bug 8110 - add branchcode MPL to sample holidays to facilitate automated testing. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 10383 [details] [review] [SIGNED-OFF] 8110 - Fines accruing on closed days - Fix Day/Month Holidays Bug This patch fixes a bug in which day/month holidays are stored in the Koha::Calendar internal hash not by month/day but incorrectly by day/month. To test: 1) Apply 'Holidays Test Plan - DB Dependent' patch, create MPL holidays for Sunday, New Years Day, and Christmas. 2) Run test plan, only Christmas should fail ( New Years does not because 1-1 means the day and month position do not matter ). 3) Apply this patch 4) Run test plan again Signed-off-by: Liz Rea <wizzyrea@gmail.com> Test plan works as advertised. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Test runs flawlessly. I originaly missed to set the expected branchcode
Liz, Tomás, would one or both of you consider the "Holidays Test Plan - DB Dependent" patch to be signed off as well, considering it was necessary for testing. I think it should be included in Koha proper. I think we need for more db dependent testing than we have had in the past. That's why this bug exists, after all!
Created attachment 10384 [details] [review] Holidays Test Plan - DB Dependent This test file is meant to be used with the sample database with a minimum of the sample libraries and sample holidays installed. Holidays.t works as stated. signing-off :) mason@xen1:~/g/head$ mysql head < installer/data/mysql/en/optional/sample_holidays.sql mason@xen1:~/g/head$ prove ./t/db_dependent/Holidays.t ./t/db_dependent/Holidays.t .. ok All tests successful. Files=1, Tests=8, 1 wallclock secs ( 0.02 usr 0.01 sys + 0.28 cusr 0.03 csys = 0.34 CPU) Result: PASS !!!!!!!!!!!!!
Created attachment 10385 [details] [review] [SIGNED-OFF] Holidays Test Plan - DB Dependent mason@xen1:~/g/head$ mysql head < installer/data/mysql/en/optional/sample_holidays.sql mason@xen1:~/g/head$ prove ./t/db_dependent/Holidays.t ./t/db_dependent/Holidays.t .. ok All tests successful. Files=1, Tests=8, 1 wallclock secs ( 0.02 usr 0.01 sys + 0.28 cusr 0.03 csys = 0.34 CPU) Result: PASS Holidays.t works as stated. signing-off :)
QA Comment: this patch-set passes all perl-critic and prove tests. --------------------------------------- $ perl ~/bin/git-perlcritic.pl Koha/Calendar.pm: OK t/db_dependent/Holidays.t: OK $ mysql head < installer/data/mysql/en/optional/sample_holidays.sql $ prove ./t/db_dependent/Holidays.t ./t/db_dependent/Holidays.t .. ok All tests successful. Files=1, Tests=8, 1 wallclock secs ( 0.02 usr 0.01 sys + 0.28 cusr 0.03 csys = 0.34 CPU) Result: PASS --------------------------------------- Passed QA.
Comment on attachment 10382 [details] [review] [SIGNED-OFF] bug 8110 - add branchcode MPL to sample holidays to facilitate automated testing. This patch only updates the English sample files, it would avoid problems if we could update all sample holiday files for all languages.
Created attachment 10387 [details] [review] bug 8110 - add 'sample_holidays.sql' files for all languages
(In reply to comment #19) > Comment on attachment 10382 [details] [review] > [SIGNED-OFF] bug 8110 - add branchcode MPL to sample holidays to facilitate > automated testing. > > This patch only updates the English sample files, it would avoid problems if > we could update all sample holiday files for all languages. oops, good spotting! - i've just added those files now :)
Thx for the patch Mason :)
Created attachment 10388 [details] [review] bug 8110 - add 'sample_holidays.sql' files for all languages
Created attachment 10389 [details] [review] bug 8110 - add 'sample_holidays.sql' files for all languages
The test plan for this patch has drifted from the original problem. Most testing seems to be running the new Holidays DB dependent unit test, rather than testing fines.pl to see if fines are accruing on holidays. This fix looks good and solid for what it is, but I'm not 100% convinced that it fixes the actual problem reported. Please test against fines.pl, and confirm this patch prevents fines from accruing on closed days.
Further investigation: the test script tests 'is_holiday', which is what's used in fines.pl to determine whether or not to charge the fine. If the tests pass, then is_holiday works as expected (at least for the 4 tested dates), and fines.pl shouldn't calculate the fine for today (if today is a holiday). This should be relatively easy to test: 1. Mark today as a holiday for a branch 2. Find an overdue material that should be accruing fines for this branch 3. Run fines.pl 4. confirm that the fine is NOT added
(In reply to comment #26) > Further investigation: the test script tests 'is_holiday', which is what's > used in fines.pl to determine whether or not to charge the fine. If the > tests pass, then is_holiday works as expected (at least for the 4 tested > dates), and fines.pl shouldn't calculate the fine for today (if today is a > holiday). > > This should be relatively easy to test: > > 1. Mark today as a holiday for a branch > 2. Find an overdue material that should be accruing fines for this branch > 3. Run fines.pl > 4. confirm that the fine is NOT added 5. Switch dateformat syspref and try again Chris
This is not working but this is not related to the patch. Subroutine Koha::Calendar::days_between "truncate to days" both start date and end date. So if my item is due since 2012-05-21 and I want to calculate fine at 2012-06-21, $duration will be 0 instead of 30. So this ends in days_between returning -4 (because of 4 sundays defined in calendar as holidays). Why truncating date here? I mark this bug as Failed QA because fines.pl still don't work correctly.
In fact, truncation is not the problem. It's how DateTime::Duration is handled. In C4::Overdues::_get_chargeable_units, only days part of duration is used. months and years are ignored. The problem is a DateTime::Duration object with months and years cannot be used to calculate the total number of days because we don't know if years are leap years, or if months contains 28 or 31 days. I think the solution is to modify days_between so that it returns a number of days instead of a DateTime::Duration object (same for hours_between I think: it should return a number of hours)
Created attachment 10608 [details] [review] Bug 8110 - Followup - Make days_between return a number of days, instead of an object.
Julian, is this new patch what you are looking for, or have I just moved code around without solving the problem?
We had not yet took into account theese fines problems (the use of fines is very marginal at Scd Lyon3), but we cope the same question of duration calculation in in days_between routine from another bug Bug 8251 Patrons incorrectly debarred with another way to solve it. About the calculation of holidays, there's another error to get aware in _init routine of Calendar.pm : the boolean parameter has to be 1 in the second execution of $repeat_sth query (line 45) to get day_month_closed_days. I had just post a fix that include the correction : bug 8348 useDaysMode syspref does not apply. As theese problems are closely intricated, Chris Cormack counselled me to tie bug 8348 with this one but as a bugzilla beginner, I am not sure of the best way to do it.
Sorry for having added confusion : the error I meant to talk about is on second execution of "$special" query (not $repeat_sth), line 68 of _init routine and the parameter has to be 0 instead of 1 to get single_holidays dates.
(In reply to comment #31) > Julian, is this new patch what you are looking for, or have I just moved > code around without solving the problem? DateTime::Duration is just a hash that looks like this: $VAR1 = bless( { 'seconds' => 20, 'minutes' => 106, 'end_of_month' => 'wrap', 'nanoseconds' => 0, 'days' => 8, 'months' => 380 }, 'DateTime::Duration' ); You can't say with only these informations how many days it is. This is the problem: we cannot use DateTime::Duration for telling us how many days there are between two dates. I think we can use Date::Calc::Delta_Days for this (and Delta_DHMS for hours_between). I would be happy to propose a patch, but I've not enough time to do it right now and can't do it next week either. So I hope this helps.
I have objection to code change: - $self->{day_month_closed_days}->{ $tuple->{day} }->{ $tuple->{month} } = + $self->{day_month_closed_days}->{ $tuple->{month} }->{ $tuple->{day} } = which basically shifts values inside hash so that hey are in REVERSE from name! This problem has to be solved in another part of code, see first patch at: Bug 8418 - Koha::Calendar is_holiday ignores repeating holidays
Created attachment 11027 [details] [review] [SIGNED-OFF] Bug 8110 - add 'sample_holidays.sql' files for all languages Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
It appears that 8418 will resolve this bug. I still propose we include these test-related patches in Koha.
Isn't this bug/patch also related to bug 8486 ?
QA comments: One test does not pass: # Failed test 'Christmas is a closed day' # at t/db_dependent/Holidays.t line 44. # got: '0' # expected: '1' After a quick debug, in Koha::Calendar, l.153: if ( exists $self->{day_month_closed_days}->{$month}->{$day} ) { should be if ( exists $self->{day_month_closed_days}->{$day}->{$month} ) { Marked as Failed QA
Jonathan, I failed to see how this is a problem with this patch. The test failing does not mean this patch is bad, it *is* the test. The test failing means there is something wrong with the calendar modules. (In reply to comment #39) > QA comments: > > One test does not pass: > # Failed test 'Christmas is a closed day' > # at t/db_dependent/Holidays.t line 44. > # got: '0' > # expected: '1' > > After a quick debug, in Koha::Calendar, l.153: > if ( exists $self->{day_month_closed_days}->{$month}->{$day} ) { > should be > if ( exists $self->{day_month_closed_days}->{$day}->{$month} ) { > > Marked as Failed QA
(In reply to comment #40) > Jonathan, I failed to see how this is a problem with this patch. The test > failing does not mean this patch is bad, it *is* the test. The test failing > means there is something wrong with the calendar modules. Based on Dobrica's comment #35, there may already be a patch that fixes the problem. If that is true, the tests are wrong, and should be updated to test for correct functioning (because they will fail when the code is working, and pass only when it is not working in a very particular way). If that is not true, the patch fails to fix the problem, and therefore does not pass QA for that reason. If the intention is only to provide a test suite that highlights where the code is broken, the patches should probably just be moved to a different bug, so that no one is expecting a fix for the "Fines accruing on closed days issue" when they see this mentioned in the release notes, and so that the QA team is not confused. Also, I am in agreement with Dobrica that day_month_closed_days should probably be renamed to month_day_closed_days if we are switching the order. Not that either name makes all that much sense, but at least there's some argument in favor of the latter.
(In reply to comment #41) > (In reply to comment #40) > > Jonathan, I failed to see how this is a problem with this patch. The test > > failing does not mean this patch is bad, it *is* the test. The test failing > > means there is something wrong with the calendar modules. > > Based on Dobrica's comment #35, there may already be a patch that fixes the > problem. If that is true, the tests are wrong, and should be updated to test > for correct functioning (because they will fail when the code is working, > and pass only when it is not working in a very particular way). If that is > not true, the patch fails to fix the problem, and therefore does not pass QA > for that reason. If the intention is only to provide a test suite that > highlights where the code is broken, the patches should probably just be > moved to a different bug, so that no one is expecting a fix for the "Fines > accruing on closed days issue" when they see this mentioned in the release > notes, and so that the QA team is not confused. You are referring to a patch that was deprecated a long time ago. Unless Jonathan decided to pull and test, and fail an already deprecated patch, what you are saying makes not sense. Based on Dobrica's comment I deprecated my patch that actually fixed the problem. A tester would have to ignore the title and description for all three patches here to assume one of them fixed the problem. That would be poor testing, not poor patch writing. I will, however, change the title of the bug as you have suggested.
I was trying to figure out how any of the attached patches could fix the problem, but I figured maybe I had missed something, and "sample" meant something specific in the context of the calendar.
*** Bug 8348 has been marked as a duplicate of this bug. ***
t/db_dependent/Holidays.t(In reply to comment #39) > QA comments: > > One test does not pass: > # Failed test 'Christmas is a closed day' > # at t/db_dependent/Holidays.t line 44. > # got: '0' > # expected: '1' > > Marked as Failed QA ok, i think this one is looking good now... all tests pass, after applying 8418 and 8486 everyone.. marking this one as 'paseed QA' $ perl t/db_dependent/Holidays.t 1..8 ok 1 - use Koha::Calendar; ok 2 - use C4::Calendar; ok 3 - Koha::Calendar class returned isa Koha::Calendar ok 4 - C4::Calendar class returned isa C4::Calendar ok 5 - Sunday is a closed day ok 6 - Monday is not a closed day ok 7 - Christmas is a closed day ok 8 - New Years day is a closed day ---------------------------------------------- $ koha-qa.pl -c 3 testing 3 commit(s) (applied to commit 15cf307) * 47deb47 Bug 8656 Calendar needs to load both exception and nonexception d Koha/Calendar.pm * 51efc88 Bug 8110 - add 'sample_holidays.sql' files for all languages installer/data/mysql/de-DE/optional/sample_holidays.sql installer/data/mysql/es-ES/optional/sample_holidays.sql installer/data/mysql/fr-FR/2-Optionel/sample_holidays.sql installer/data/mysql/it-IT/optional/sample_holidays.sql installer/data/mysql/nb-NO/2-Valgfritt/sample_holidays.sql installer/data/mysql/pl-PL/optional/sample_holidays.sql installer/data/mysql/ru-RU/optional/sample_holidays.sql installer/data/mysql/uk-UA/optional/sample_holidays.sql * ed34abb Holidays Test Plan - DB Dependent t/db_dependent/Holidays.t * Koha/Calendar.pm OK * t/db_dependent/Holidays.t OK
QA comment : the 3rd patch adds some localized files: .../mysql/fr-FR/2-Optionel/sample_holidays.sql | 4 ++++ .../data/mysql/it-IT/optional/sample_holidays.sql | 4 ++++ why are they needed ? in fr-FR, there is no branch in the sample 'MPL' afaik PS: is it still a blocker ? I think no, the blocker was bug 8486, and it's pushed now, this one is just the test suite
(In reply to comment #46) > QA comment : the 3rd patch adds some localized files: > .../mysql/fr-FR/2-Optionel/sample_holidays.sql | 4 ++++ > .../data/mysql/it-IT/optional/sample_holidays.sql | 4 ++++ > > why are they needed ? in fr-FR, there is no branch in the sample 'MPL' afaik > > PS: is it still a blocker ? I think no, the blocker was bug 8486, and it's > pushed now, this one is just the test suite PING !
(In reply to comment #46) > QA comment : the 3rd patch adds some localized files: > .../mysql/fr-FR/2-Optionel/sample_holidays.sql | 4 ++++ > .../data/mysql/it-IT/optional/sample_holidays.sql | 4 ++++ > > why are they needed ? in fr-FR, there is no branch in the sample 'MPL' afaik > > PS: is it still a blocker ? I think no, the blocker was bug 8486, and it's > pushed now, this one is just the test suite Perhaps we should add MPL as a sample branch for fr-FR? It seems like the easiest solution. I would agree, this is no longer a blocker, but should still be a priority, as it is a unit test.
The sample holidays have by themselves a +1 for addition, but its outside the scope of this bug. We need some defaults to test db_dependent stuff, and they were just added to other languages. Maybe those patches should be in another bug and let this ones be pushed.
(In reply to comment #49) > The sample holidays have by themselves a +1 for addition, but its outside > the scope of this bug. We need some defaults to test db_dependent stuff, and > they were just added to other languages. Maybe those patches should be in > another bug and let this ones be pushed. That would be fine by me.
The first 2 patches have been pushed. I haven't pushed the 3rd one, see comment 46 If i'm wrong, please argue ! PS: the test does not work locally for me, it should on jenkins
Released in 3.10.0