http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Savitra Sirohi <savitra.sirohi@osslabs.biz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |melia@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #2 from Margo Duncan <mduncan@uttyler.edu> --- 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?

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P1 - high CC| |wizzyrea@gmail.com Severity|normal |blocker --- Comment #3 from Liz Rea <wizzyrea@gmail.com> --- Calculating fines correctly is insanely important. This is a big deal. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=8253 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #5 from Kyle M Hall <kyle.m.hall@gmail.com> --- Created attachment 10349 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10349&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #7 from Kyle M Hall <kyle.m.hall@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10353|0 |1 is obsolete| | --- Comment #9 from Liz Rea <wizzyrea@gmail.com> --- Created attachment 10356 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10356&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #12 from Liz Rea <wizzyrea@gmail.com> --- Created attachment 10368 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10368&action=edit bug 8110 - add branchcode MPL to sample holidays to facilitate automated testing. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #14 from Kyle M Hall <kyle.m.hall@gmail.com> --- Created attachment 10383 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10383&action=edit [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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #15 from Kyle M Hall <kyle.m.hall@gmail.com> --- 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! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10349|0 |1 is obsolete| | Attachment #10384|0 |1 is obsolete| | --- Comment #17 from Mason James <mtj@kohaaloha.com> --- Created attachment 10385 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10385&action=edit [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 :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Comment on attachment 10382 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10382 [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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #21 from Mason James <mtj@kohaaloha.com> --- (In reply to comment #19)

Comment on attachment 10382 [details] [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 :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10387|0 |1 is obsolete| | --- Comment #23 from Mason James <mtj@kohaaloha.com> --- Created attachment 10388 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10388&action=edit bug 8110 - add 'sample_holidays.sql' files for all languages -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Ian Walls <koha.sekjal@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion CC| |koha.sekjal@gmail.com --- Comment #25 from Ian Walls <koha.sekjal@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #26 from Ian Walls <koha.sekjal@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |julian.maurice@biblibre.com --- Comment #28 from Julian Maurice <julian.maurice@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #30 from Kyle M Hall <kyle.m.hall@gmail.com> --- Created attachment 10608 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10608&action=edit Bug 8110 - Followup - Make days_between return a number of days, instead of an object. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=8348 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #33 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Dobrica Pavlinusic <dpavlin@rot13.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dpavlin@rot13.org -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #35 from Dobrica Pavlinusic <dpavlin@rot13.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10608|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10383|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.poulain@biblibre.com Depends on| |8486 --- Comment #38 from Paul Poulain <paul.poulain@biblibre.com> --- Isn't this bug/patch also related to bug 8486 ? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #40 from Kyle M Hall <kyle.m.hall@gmail.com> --- 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

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #42 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #43 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA QA Contact| |mtj@kohaaloha.com --- Comment #45 from Mason James <mtj@kohaaloha.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #47 from Paul Poulain <paul.poulain@biblibre.com> --- (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 ! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 --- Comment #49 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8110 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #51 from Paul Poulain <paul.poulain@biblibre.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.