Bug 8110

Summary: Add tests for Calendar modules
Product: Koha Reporter: Margo Duncan <mduncan>
Component: HolidaysAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Mason James <mtj>
Severity: normal    
Priority: P1 - high CC: chris, dpavlin, jonathan.druart, julian.maurice, katrin.fischer, koha.sekjal, koha, kyle.m.hall, kyle, melia, mtj, nengard, paul.poulain, savitra.sirohi, tomascohen, wizzyrea
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8243
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8253
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8348
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 8418, 8486    
Bug Blocks:    
Attachments: Holidays Test Plan - DB Dependent
8110 - Fines accruing on closed days - Fix Day/Month Holidays Bug
8110 - Fines accruing on closed days - Fix Day/Month Holidays Bug
8110 - Fines accruing on closed days - Fix Day/Month Holidays Bug
bug 8110 - add branchcode MPL to sample holidays to facilitate automated testing.
[SIGNED-OFF] bug 8110 - add branchcode MPL to sample holidays to facilitate automated testing.
[SIGNED-OFF] 8110 - Fines accruing on closed days - Fix Day/Month Holidays Bug
Holidays Test Plan - DB Dependent
[SIGNED-OFF] Holidays Test Plan - DB Dependent
bug 8110 - add 'sample_holidays.sql' files for all languages
bug 8110 - add 'sample_holidays.sql' files for all languages
bug 8110 - add 'sample_holidays.sql' files for all languages
Bug 8110 - Followup - Make days_between return a number of days, instead of an object.
[SIGNED-OFF] Bug 8110 - add 'sample_holidays.sql' files for all languages

Description Margo Duncan 2012-05-16 15:20:22 UTC
Fines accrue on days set as closed in the calendar.
Comment 1 Savitra Sirohi 2012-05-22 06:53:33 UTC
The code to skip holidays during fine calculation seems to be present. Has the user set finescalendar syspref to noFinesWhenClosed?
Comment 2 Margo Duncan 2012-05-22 16:08:52 UTC
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?
Comment 3 Liz Rea 2012-06-15 14:10:22 UTC
Calculating fines correctly is insanely important. This is a big deal.
Comment 4 Kyle M Hall 2012-06-18 11:26:56 UTC
*** Bug 8243 has been marked as a duplicate of this bug. ***
Comment 5 Kyle M Hall 2012-06-18 11:28:41 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-06-18 11:30:23 UTC
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 ).
Comment 7 Kyle M Hall 2012-06-18 12:02:26 UTC
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.
Comment 8 Kyle M Hall 2012-06-18 12:19:41 UTC Comment hidden (obsolete)
Comment 9 Liz Rea 2012-06-18 16:38:26 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2012-06-19 13:54:51 UTC
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.
Comment 11 Tomás Cohen Arazi 2012-06-19 14:30:08 UTC Comment hidden (obsolete)
Comment 12 Liz Rea 2012-06-19 18:41:48 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2012-06-20 13:02:18 UTC
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>
Comment 14 Kyle M Hall 2012-06-20 13:03:35 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2012-06-20 13:06:05 UTC
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!
Comment 16 Mason James 2012-06-20 14:07:49 UTC Comment hidden (obsolete)
Comment 17 Mason James 2012-06-20 14:09:53 UTC
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 :)
Comment 18 Mason James 2012-06-20 14:15:11 UTC
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 19 Katrin Fischer 2012-06-20 14:19:06 UTC
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.
Comment 20 Mason James 2012-06-20 14:29:22 UTC Comment hidden (obsolete)
Comment 21 Mason James 2012-06-20 14:30:58 UTC
(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 :)
Comment 22 Katrin Fischer 2012-06-20 14:32:46 UTC
Thx for the patch Mason :)
Comment 23 Mason James 2012-06-20 14:53:11 UTC Comment hidden (obsolete)
Comment 24 Mason James 2012-06-20 14:57:13 UTC Comment hidden (obsolete)
Comment 25 Ian Walls 2012-06-20 20:12:52 UTC
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.
Comment 26 Ian Walls 2012-06-20 20:25:42 UTC
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
Comment 27 Chris Cormack 2012-06-20 20:52:26 UTC
(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
Comment 28 Julian Maurice 2012-06-21 13:32:40 UTC
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.
Comment 29 Julian Maurice 2012-06-21 14:49:28 UTC
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)
Comment 30 Kyle M Hall 2012-07-02 15:30:32 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2012-07-02 15:31:32 UTC
Julian, is this new patch what you are looking for, or have I just moved code around without solving the problem?
Comment 32 Koha Team University Lyon 3 2012-07-03 10:19:54 UTC
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.
Comment 33 Koha Team University Lyon 3 2012-07-05 07:38:18 UTC
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.
Comment 34 Julian Maurice 2012-07-06 14:32:19 UTC
(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.
Comment 35 Dobrica Pavlinusic 2012-07-11 17:34:43 UTC
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
Comment 36 Kyle M Hall 2012-07-19 18:42:32 UTC
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>
Comment 37 Kyle M Hall 2012-07-19 18:44:49 UTC
It appears that 8418 will resolve this bug. I still propose we include these test-related patches in Koha.
Comment 38 Paul Poulain 2012-08-02 14:55:55 UTC
Isn't this bug/patch also related to bug 8486 ?
Comment 39 Jonathan Druart 2012-08-09 15:02:05 UTC
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
Comment 40 Kyle M Hall 2012-08-11 12:40:35 UTC
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
Comment 41 Jared Camins-Esakov 2012-08-11 18:32:53 UTC
(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.
Comment 42 Kyle M Hall 2012-08-13 11:18:01 UTC
(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.
Comment 43 Jared Camins-Esakov 2012-08-13 11:24:33 UTC
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.
Comment 44 Chris Cormack 2012-08-19 08:00:35 UTC
*** Bug 8348 has been marked as a duplicate of this bug. ***
Comment 45 Mason James 2012-09-14 06:14:14 UTC
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
Comment 46 Paul Poulain 2012-09-17 15:44:47 UTC
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
Comment 47 Paul Poulain 2012-09-26 16:57:08 UTC
(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 !
Comment 48 Kyle M Hall 2012-09-27 13:45:18 UTC
(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.
Comment 49 Tomás Cohen Arazi 2012-09-27 13:48:37 UTC
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.
Comment 50 Kyle M Hall 2012-09-27 13:56:30 UTC
(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.
Comment 51 Paul Poulain 2012-10-01 14:45:42 UTC
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
Comment 52 Chris Cormack 2013-04-20 23:02:12 UTC
Released in 3.10.0