Koha::Calendar should contain all of the calendar calculation code, and C4::Calendar should only have admin subs. Created to slim down bug 8133.
Created attachment 24856 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly.
CC'ing Liz, as she was on the original bug.
Created attachment 25528 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly. (Rebased version.)
Created attachment 25556 [details] [review] 0001-SIGNED-OFF-Bug-11211-Move-calculation-code-out-of-C4 I think you might need to get the radio button initially set on "Open" or "Closed" after clicking a day, so one knows the default. Less confusion if one selects repeating events. Noticed: You can only save one event per day. But I guess thats the design, events are ONLY open/closed. 5) Check out items to verify that the calculation code is using the events you just created. Opened patron's checkout tab and say that the Total charges accumulted correctly. (No fines are shown but that seems the current behavior. But that could be another issue for another day). 6) If possible, verify that the two affected cronjobs still function correctly. ---- staticfines.pl test Administration finesmode = calc and charge Cataloged an item. (or search for one and note barcode number). Administration->Circulation and fine rules. Add a definition rule line in frst table: Patron All 1 10 days Exact 1.00 1 0 overdue cap=emtpyfield zeros for the rest. Search for patron ( Lillie is a test one). Checkout book with noted barcode, but with due date 5 days prior to today. Note: the patron's library. Centerville is CPL PT = patron category command line: koha@biblio:~/kohaclone/misc/cronjobs$ ./staticfines.pl -v -c PT -l CPL xsub: [(($6||0),($5||0),($4||0),$3, $2 - 1, $1 - 1900)] CalcFine returning (1, , 1, 1) at /home/koha/kohaclone//C4/Overdues.pm line 261. Creating fine for borrower 28 with amount : 1 at ./staticfines.pl line 220. query: INSERT INTO accountlines (borrowernumber,itemnumber,date,amount,description,accounttype,amountoutstanding,lastincrement,accountno) VALUES (?,?,now(),?,?,'F',?,?,?) w/ args: 28, 1, 1, staticfine, 1, 1, 12 Fines assessment -- 2014-02-22 Number of Overdue Items: counted 1 reported 1 you can run mysql -uroot -p koha_biblio and select * from accountlines; to see results. Now navigate to Tools-> Calendar. selected days with "closed" checked and saved between the item due date and today's date. Could see corrent adjustments to amount with more days closed, etc. Did this for weekley, yearly, range. with correct results. ---- also ran fines.pl -v and saw a fine with accountype FU update when holidays changed. NOTE: koha@biblio:~/kohaclone$ ./koha_perl_deps.pl -a | grep Plugin Template::Plugin::JavaScript 0.01* 0.02 Yes 0.01 in squeeze worked. Just need libtemplate-plugin-javascript-perl to be added to control file.
Jesse, Could you explain why you changed "holiday" with "event" and you removed "Holiday exception" please?
Created attachment 26434 [details] [review] Bug 11211: Fix QA issues FAIL tools/calendar.pl FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 7) FAIL critic Loop iterator is not lexical at line 126, column 5. See page 108 of PBP. FAIL koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc FAIL forbidden patterns forbidden pattern: tab char (line 90)
Silly tab characters. Cosmic rays, I swear. The change from "our" to "my" doesn't cause any problems with the embedded sub's, correct? Even under plack? (I admit I put that "our" there without being sure.) Thanks for the QA followup. As far as the change in terminology, it is largely intended as a bridge to bug 8133, where it makes more sense. I admit the interface is a bit confusing at the moment, but once this is in, that bug should help improve things.
(In reply to Jesse Weaver from comment #7) > Silly tab characters. Cosmic rays, I swear. The change from "our" to "my" > doesn't cause any problems with the embedded sub's, correct? Even under > plack? (I admit I put that "our" there without being sure.) Thanks for the > QA followup. It produced a perlcritic error: "Loop iterator is not lexical at line 126, column 5. See page 108 of PBP." > As far as the change in terminology, it is largely intended as a bridge to > bug 8133, where it makes more sense. I admit the interface is a bit > confusing at the moment, but once this is in, that bug should help improve > things. So, what is the next step? Will you wait for it to be pushed before submitting a new patch for bug 8133?
Yes, that's the plan. If the interface is truly unusable as it stands, I'll submit a followup to make it better, but otherwise the plan is to get in 8133 as soon as possible afterwards.
Jesse, Could you please: - execute db dependent unit tests into a transaction. - change the prototype for routines in C4::Calendar with more than 1 parameter (using hashref). - remove unused routines in Koha::Calendar (at least single_holidays). Do you think this patch should be integrated into 3.16 or it is useless and it could be wait for the next release (since bug 8133 is not ready yet)?
Jessy, Do you know if and when you'll have time to work again on this feature ? Cheers !
Now that I'm done with school and on-site work for a little while, I'll have some time to get this through.
Created attachment 29731 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly. (Merges in old and some new QA fixes.)
Applying: Bug 11211 - Move calculation code out of C4::Calendar Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Removing tools/newHolidays.pl CONFLICT (modify/delete): tools/holidays.pl deleted in Bug 11211 - Move calculation code out of C4::Calendar and modified in HEAD. Version HEAD of tools/holidays.pl left in tree. Removing tools/exceptionHolidays.pl Removing tools/copy-holidays.pl Removing koha-tmpl/intranet-tmpl/prog/en/modules/tools/holidays.tt Auto-merging C4/Overdues.pm Auto-merging C4/Installer/PerlDependencies.pm Auto-merging C4/Circulation.pm Failed to merge in the changes. Patch failed at 0001 Bug 11211 - Move calculation code out of C4::Calendar
Created attachment 30959 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly. (Rebased.)
Created attachment 32110 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
This patch no longer applies :( I'd love to see this moving, please rebase quickly! Bug 11211 - Move calculation code out of C4::Calendar 32110 - Bug 11211 - Move calculation code out of C4::Calendar Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11211 - Move calculation code out of C4::Calendar Using index info to reconstruct a base tree... M C4/Circulation.pm M C4/Installer/PerlDependencies.pm M Koha/Calendar.pm M t/db_dependent/Holidays.t Falling back to patching base and 3-way merge... Removing tools/newHolidays.pl Removing tools/holidays.pl Removing tools/exceptionHolidays.pl Removing tools/copy-holidays.pl Auto-merging t/db_dependent/Holidays.t CONFLICT (content): Merge conflict in t/db_dependent/Holidays.t Removing koha-tmpl/intranet-tmpl/prog/en/modules/tools/holidays.tt Auto-merging Koha/Calendar.pm CONFLICT (content): Merge conflict in Koha/Calendar.pm Auto-merging C4/Installer/PerlDependencies.pm CONFLICT (content): Merge conflict in C4/Installer/PerlDependencies.pm Auto-merging C4/Circulation.pm Failed to merge in the changes. Patch failed at 0001 Bug 11211 - Move calculation code out of C4::Calendar The copy of the patch that failed is found in: /home/katrin/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-11211---Move-calculation-code-out-of-C4Calenda-4wLOOf.patch
Created attachment 32867 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly. (Rebased.)
Jesse, did you rebase on a current master branch? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11211 - Move calculation code out of C4::Calendar Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... error: The following untracked working tree files would be overwritten by merge: koha-tmpl/intranet-tmpl/prog/en/modules/tools/calendar.tt t/db_dependent/Calendar.t tools/calendar.pl Please move or remove them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 Bug 11211 - Move calculation code out of C4::Calendar The copy of the patch that failed is found in: /home/katrin/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-11211---Move-calculation-code-out-of-C4Calenda-ieK16X.patch
All good, too late here and my clue to stop QAing for tonight.
Working on this now.
Ok, some first findings, noting here, I will keep testing, but please fix: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/tools/calendar.tt OK forbidden patterns OK tt_valid FAIL valid_template JavaScript: plugin not found FAIL tools/calendar.pl OK critic FAIL forbidden patterns forbidden pattern: You should not use a .tmpl extension for the template name (see bug 11349) (line 35) OK pod OK valid
Hm, I can't keep testing this, because when I get to the calendar tool page, I have a perl error blocking my way: Template process failed: file error - /home/katrin/kohaclone/koha-tmpl/intranet-tmpl///modules/tools/calendar.tmpl: not found at /home/katrin/kohaclone/C4/Templates.pm line 127. The same still happens with 8133 appied on top. Please fix, I can continue testing tonight. I will probably be testing both bugs together, please make sure 8133 still applies with a follow-up here!
Created attachment 32978 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly.
Created attachment 32992 [details] [review] Bug 11211 [QA Followup] - Remove use of dt_add_type_uk_date
When I apply the two patches on this bug to master and try to run the tests, I get this: $ sudo koha-shell -c "prove t/db_dependent/Calendar.t" kohadev t/db_dependent/Calendar.t .. Undefined subroutine &main::ModRepeatingEvent called at t/db_dependent/Calendar.t line 22. # Looks like your test exited with 255 before it could output anything. t/db_dependent/Calendar.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 14/14 subtests Test Summary Report ------------------- t/db_dependent/Calendar.t (Wstat: 65280 Tests: 0 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 14 tests but ran 0. Files=1, Tests=0, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.13 cusr 0.01 csys = 0.16 CPU) Result: FAIL The calendar page in the intranet seems to work as it should. Other db_dependent tests do pass... I cannot for the life of me understand what is happening here, and it does not seem to happen for others, so I am just leaving this as a note, without changing the status of the bug. Holidays.t also fails: $ sudo koha-shell -c "prove t/db_dependent/Holidays.t" kohadev t/db_dependent/Holidays.t .. 1/6 # Failed test 'Sunday is a closed day' # at t/db_dependent/Holidays.t line 44. # got: '0' # expected: '1' # Failed test 'Christmas is a closed day' # at t/db_dependent/Holidays.t line 46. # got: '0' # expected: '1' # Failed test 'New Years day is a closed day' # at t/db_dependent/Holidays.t line 47. # got: '0' # expected: '1' # Looks like you failed 3 tests of 6. t/db_dependent/Holidays.t .. Dubious, test returned 3 (wstat 768, 0x300) Failed 3/6 subtests Test Summary Report ------------------- t/db_dependent/Holidays.t (Wstat: 768 Tests: 6 Failed: 3) Failed tests: 3, 5-6 Non-zero exit status: 3 Files=1, Tests=6, 0 wallclock secs ( 0.02 usr 0.01 sys + 0.18 cusr 0.02 csys = 0.23 CPU) Result: FAIL
Created attachment 33415 [details] Dates are not sorted properly I added dates covering several months in a row, and the dates are not sorted properly, when the format is dd/mm/yyyy. See attached screenshot.
(In reply to Magnus Enger from comment #26) > When I apply the two patches on this bug to master and try to run the tests, > I get this: > > $ sudo koha-shell -c "prove t/db_dependent/Calendar.t" kohadev > t/db_dependent/Calendar.t .. Undefined subroutine &main::ModRepeatingEvent > called at t/db_dependent/Calendar.t line 22. > # Looks like your test exited with 255 before it could output anything. > t/db_dependent/Calendar.t .. Dubious, test returned 255 (wstat 65280, 0xff00) > Failed 14/14 subtests > > Test Summary Report > ------------------- > t/db_dependent/Calendar.t (Wstat: 65280 Tests: 0 Failed: 0) > Non-zero exit status: 255 > Parse errors: Bad plan. You planned 14 tests but ran 0. > Files=1, Tests=0, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.13 cusr 0.01 > csys = 0.16 CPU) > Result: FAIL > > The calendar page in the intranet seems to work as it should. Other > db_dependent tests do pass... I cannot for the life of me understand what is > happening here, and it does not seem to happen for others, so I am just > leaving this as a note, without changing the status of the bug. Just to verify, do you have the sample branches and holidays installed?
(In reply to Jesse Weaver from comment #28) > Just to verify, do you have the sample branches and holidays installed? Jesse, I am not sure it's a data problem. The error is: > t/db_dependent/Calendar.t .. Undefined subroutine &main::ModRepeatingEvent > called at t/db_dependent/Calendar.t line 22.
(In reply to Jesse Weaver from comment #28) > Just to verify, do you have the sample branches and holidays installed? I was testing on a NORMARC/nb-NO setup, so possibly not.
I get conflicts now: $ git bz apply 11211 Bug 11211 - Move calculation code out of C4::Calendar 32978 - Bug 11211 - Move calculation code out of C4::Calendar 32992 - Bug 11211 [QA Followup] - Remove use of dt_add_type_uk_date Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11211 - Move calculation code out of C4::Calendar Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Removing tools/newHolidays.pl Removing tools/holidays.pl Removing tools/exceptionHolidays.pl Removing tools/copy-holidays.pl Auto-merging misc/cronjobs/staticfines.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/en/modules/tools/holidays.tt deleted in Bug 11211 - Move calculation code out of C4::Calendar and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/modules/tools/holidays.tt left in tree. Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc Auto-merging Koha/Calendar.pm Auto-merging C4/Overdues.pm Auto-merging C4/Installer/PerlDependencies.pm CONFLICT (content): Merge conflict in C4/Installer/PerlDependencies.pm Auto-merging C4/Circulation.pm Failed to merge in the changes. Patch failed at 0001 Bug 11211 - Move calculation code out of C4::Calendar When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-11211---Move-calculation-code-out-of-C4Calenda-SuvqdA.patch The first one is probably caused by bug 13289: http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=c474b0f645fecb2c1663907904c6b9aa583e4513
Looks like the failing tests i reported in comment 26 were due to 1. How I was running the tests 2. Missing sample data (I had the nb-NO sample data, not the en ones) $ sudo -u kohadev-koha KOHA_CONF=/etc/koha/sites/kohadev/koha-conf.xml prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t t/Calendar.t t/db_dependent/Calendar.t .. ok t/db_dependent/Holidays.t .. ok t/Calendar.t ............... ok All tests successful. Files=3, Tests=54, 5 wallclock secs ( 0.03 usr 0.01 sys + 0.84 cusr 0.95 csys = 1.83 CPU) Result: PASS But I still see some problems: 1. The sorting of dates from comment 27 and attachment 33415 [details] 2. The merge conflicts reported in comment 31 I made a branch before the conflicts were introduced, to do some more testing: 3. If I add a weekly repeating event, like saying the library is closed every Friday, I click on "Save" and then I get a page that just says "Fridays". No Koha UI or anything, just that word. If I view source, the source also contains nothing but the word "Fridays". No HTML or anything like that. If I open the URL for the intranet main page and navigate to Tools > Calendar the same thing happens, I get just the word "Fridays". If I go into the database and delete the relevant entry from repeatable_holidays, the calendar displays as it should again. 4. With this patch applied you now get to choose if the library is open or closed when you add an event. As far as I can tell, the special_holidays.isexception field is used to record open/closed. But the description for this field is "is this a holiday exception to a repeatable holiday (1 for yes, 0 for no)". Is open/closed a sensible use/interpretation of this field? I am not sure of the usage, so I am only raising this as a question.
Created attachment 33895 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly.
Created attachment 33896 [details] [review] Bug 11211 [QA Followup] - Remove use of dt_add_type_uk_date
Created attachment 33898 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar This patch moves the calculation code out of C4::Calendar, replacing any references to it with Koha::Calendar. Additionally, it moves the administration interface to a single script, tools/calendar.pl. NOTE: This is intended as a bridge to bug 8133, so the UI is a bit awkward, and C4::Calendar has an API designed for adding hours to the calendar. All features should still work, and 8133 will make things better. Test plan: 1) Install the new dependency, Template::Plugin::JavaScript. 2) prove t/db_dependent/Calendar.t t/db_dependent/Holidays.t to verify that the new C4::Calendar API works correctly. 3) prove t/Calendar.t 4) Add, remove and modify events of each type using the administrative interface. In this version, changing a repeating event to "Open" deletes it. 5) Check out items to verify that the calculation code is using the events you just created. 6) If possible, verify that the two affected cronjobs still function correctly. Signed-off-by: wajasu <matted-34813@mypacks.net>
Created attachment 33899 [details] [review] Bug 11211 [QA Followup] - Remove use of dt_add_type_uk_date
Thanks for a quick rebase! I still have problems with 1, 3 and 4 from comment 32. 1 and 3 makes me fail QA this - 4 is more of an open question.
> 1. The sorting of dates from comment 27 and attachment 33415 [details] We'll look into this. > 3. If I add a weekly repeating event, like saying the library is closed > every Friday, I click on "Save" and then I get a page that just says > "Fridays". No Koha UI or anything, just that word. If I view source, the > source also contains nothing but the word "Fridays". No HTML or anything > like that. If I open the URL for the intranet main page and navigate to > Tools > Calendar the same thing happens, I get just the word "Fridays". If I > go into the database and delete the relevant entry from repeatable_holidays, > the calendar displays as it should again. I think you may be seeing bug 13097, which I don't believe is directly related to this patch. However, if you are not seeing this problem without this patch, I am probably wrong ; ) > 4. With this patch applied you now get to choose if the library is open or > closed when you add an event. As far as I can tell, the > special_holidays.isexception field is used to record open/closed. But the > description for this field is "is this a holiday exception to a repeatable > holiday (1 for yes, 0 for no)". Is open/closed a sensible use/interpretation > of this field? I am not sure of the usage, so I am only raising this as a > question. Good question. I'll ask Jesse to take a look at this.
(In reply to Kyle M Hall from comment #38) > > 3. If I add a weekly repeating event, like saying the library is closed > > every Friday, I click on "Save" and then I get a page that just says > > "Fridays". No Koha UI or anything, just that word. If I view source, the > > source also contains nothing but the word "Fridays". No HTML or anything > > like that. If I open the URL for the intranet main page and navigate to > > Tools > Calendar the same thing happens, I get just the word "Fridays". If I > > go into the database and delete the relevant entry from repeatable_holidays, > > the calendar displays as it should again. > > I think you may be seeing bug 13097, which I don't believe is directly > related to this patch. However, if you are not seeing this problem without > this patch, I am probably wrong ; ) The result looks the same as in the video on bug 13097, but it happens even if I do not have an ampersand in the title or description. Yearly works. But if I click on a date, choose "Closed", give it a title like "test" and choose "Event repeated weekly" I get this weird error, just showing the day of the week. There is nothing in the intranet error log when i save the weekly event, but when I try to view the calendar.pl page later, I get this: [Tue Nov 25 13:50:05 2014] [error] [client 10.0.2.2] [Tue Nov 25 13:50:05 2014] calendar.pl: Use of uninitialized value in split at /home/vagrant/kohaclone/tools/calendar.pl line 105. Line 104 etc looks like this: if ( $input->param( 'allBranches' ) || !$input->param( 'branchName' ) ) { @branches = split /\|/, $input->param( 'branchcodes' ); } else { @branches = ( $input->param( 'branchName' ) ); } It looks slightly suspicious that it checks for "allBranches" and then splits "branchcodes", but that is just a hunch, really. The intranet access log has: 10.0.2.2 - - [25/Nov/2014:13:58:20 +0000] "GET /cgi-bin/koha/tools/calendar.pl HTTP/1.1" 200 10319 If i remove the weekly event from the database and reload the page, I get this: 10.0.2.2 - - [25/Nov/2014:14:02:16 +0000] "GET /cgi-bin/koha/tools/calendar.pl HTTP/1.1" 200 10114 So it looks like the size of those pages is pretty similar, even when the page and the source is only showing the name of the week...
Created attachment 33908 [details] Errors in the firebug console when calendar.pl only shows name of weekday
Comment on attachment 33898 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar Review of attachment 33898 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/calendar.tt @@ +490,5 @@ > + [% FOREACH event IN weekly_events %] > + <tr> > + <td> > +<script type="text/javascript"> > + document.write(weekdays[ [% event.weekday %]]); I think this line is causing the problem I was seeing with the page just showing the name of a weekday. I think document.write does not just inject some text where it is executed, but writes to the top level window (or something like that), thus replacing the whole HTML page with a single word in this instance. Commenting out this line makes the page render OK.
(In reply to Magnus Enger from comment #41) > I think document.write does not just inject > some text where it is executed, but writes to the top level window Sounds like Bug 13289. Does this patch reverse the correction made by that patch?
(In reply to Owen Leonard from comment #42) > (In reply to Magnus Enger from comment #41) > > I think document.write does not just inject > > some text where it is executed, but writes to the top level window > > Sounds like Bug 13289. Does this patch reverse the correction made by that > patch? It does, indeed. Probably because the original patch for the present bug is 10 months old. :-) The patch should definitely be updated with the fix from bug 13289. That would take care of my concern number 3 from comment 32, leaving 1 and 4 to be dealt with.
Comment on attachment 33898 [details] [review] Bug 11211 - Move calculation code out of C4::Calendar Review of attachment 33898 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/calendar.tt @@ +183,5 @@ > + $("#branch").change(function(){ > + changeBranch(); > + }); > + $("#weekly-events,#single-events").dataTable($.extend(true, {}, dataTablesDefaults, { > + "sDom": 't', Looks like my problem with incorrect sorting of dates (comment 27 and attachment 33415 [details], a.k.a. problem number 1 in comment 32) can be solved by adding this here: "aoColumns": [ { "sType": "title-string" },null,null,null ], This would be very similar to what is done below on line 190 for $("#yearly-events"). Perhaps these could be combined into something like this? $("#weekly-events,#single-events,#yearly-events").dataTable($.extend(true, {}, dataTablesDefaults, { "sDom": 't', "aoColumns": [ { "sType": "title-string" },null,null,null ], "bPaginate": false }));
Created attachment 34040 [details] [review] Bug 11211 - Followups for calendar.tt - Make sure dates are sorted correctly in the table of unique/single events (This is the same fix that was done on bug 13289.) - Make sure weekdays are sorted correctly in the table of "Events repeating weekly", including taking the CalendarFirstDayOfWeek syspref into account - Some minor changes in the "Hints" text - Replace "Unique event" with "Single event" in the colour coded legend, since that is what is used on the rest of the page - Make headers above tables and tooltips in the main calendar more in line with the language used in the colour coded legend To test: - Apply this patch on top of the other patches on this bug - Add single events that span a month boundary - Check that dates are sorted chronologically in the "Single events" table - Add several weekly events and check that sorting is by the order of days in the week, not alphabetical - Toggle CalendarFirstDayOfWeek and check that the sorting in the "Events repeating weekly" table behaves as it should - Check that the other changes described above makes sense
I have provided a followup in attachment 34040 [details] [review] that should take care of most of the problems I have found. This probably needs a signoff. If this followup meets with approval, I think there is still one question that needs to be addressed: Jonatahan, comment 5: "Could you explain why you changed "holiday" with "event" and you removed "Holiday exception" please?" Me, comment 32: "4. With this patch applied you now get to choose if the library is open or closed when you add an event. As far as I can tell, the special_holidays.isexception field is used to record open/closed. But the description for this field is "is this a holiday exception to a repeatable holiday (1 for yes, 0 for no)". Is open/closed a sensible use/interpretation of this field? I am not sure of the usage, so I am only raising this as a question." To rephrase: Does the repurposing of special_holidays.isexception to store open/closed make sense, and does it mean we loose some functionality connected to making exceptions from repeating holidays?
(In reply to Magnus Enger from comment #46) > I have provided a followup in attachment 34040 [details] [review] [review] that > should take care of most of the problems I have found. This probably needs a > signoff. If this followup meets with approval, I think there is still one > question that needs to be addressed: > > Jonatahan, comment 5: "Could you explain why you changed "holiday" with > "event" and you removed "Holiday exception" please?" > > Me, comment 32: "4. With this patch applied you now get to choose if the > library is open or closed when you add an event. As far as I can tell, the > special_holidays.isexception field is used to record open/closed. But the > description for this field is "is this a holiday exception to a repeatable > holiday (1 for yes, 0 for no)". Is open/closed a sensible use/interpretation > of this field? I am not sure of the usage, so I am only raising this as a > question." > > To rephrase: Does the repurposing of special_holidays.isexception to store > open/closed make sense, and does it mean we loose some functionality > connected to making exceptions from repeating holidays? RE: comment 5: I used event instead of holiday because you can now record that a given day is open or closed, seperately of the precedence order of specific day > day/month > weekday. RE: comment 32: I will at least cite lines 219-220 of Koha/Calendar.pm to argue for this patch's interpretation of isexception == open: # exceptions are not holidays return 0;
(In reply to Jesse Weaver from comment #47) > RE: comment 5: I used event instead of holiday because you can now record > that a given day is open or closed, seperately of the precedence order of > specific day > day/month > weekday. That change makes sense to me - it looks like a good generalization from holiday (which to me is something like Easter or "summer hliday") to "event" (which to me could be something like "the library is closed for renovation", which would be hard to fit into the "holiday" concept). > RE: comment 32: I will at least cite lines 219-220 of Koha/Calendar.pm to > argue for this patch's interpretation of isexception == open: > > # exceptions are not holidays > return 0; I can sort of see that, but I have two concerns: 1) That future developers will find it hard to understand the name 2) That we are loosing some functionality. The way I understand the current functionality, you can say that there is a holiday and the library is closed from date A to date C, BUT on date B (which is between A and C) the library is open anyway. If I remember correctly, you can also say that the library is closed for a month, but open on Wednesdays. Is it still possible to do something like that after applying the patch on this bug?
Magnuse, reading your last comment - should this be failed qa for a regression on making 'exceptions'?
(In reply to Katrin Fischer from comment #49) > Magnuse, reading your last comment - should this be failed qa for a > regression on making 'exceptions'? I think this needs some more clarification, yes. Setting it to FQA to see if anything happens. ;-)
(In reply to Magnus Enger from comment #48) > (In reply to Jesse Weaver from comment #47) > > RE: comment 5: I used event instead of holiday because you can now record > > that a given day is open or closed, seperately of the precedence order of > > specific day > day/month > weekday. > > That change makes sense to me - it looks like a good generalization from > holiday (which to me is something like Easter or "summer hliday") to "event" > (which to me could be something like "the library is closed for renovation", > which would be hard to fit into the "holiday" concept). > > > RE: comment 32: I will at least cite lines 219-220 of Koha/Calendar.pm to > > argue for this patch's interpretation of isexception == open: > > > > # exceptions are not holidays > > return 0; > > I can sort of see that, but I have two concerns: > > 1) That future developers will find it hard to understand the name This is reasonable. I'd offer the interpretation that the underlying tables are named *_holidays, and an exception to a holiday is not being a holiday -> open. (And, in theory, this is a stepping stone to bug 8133, though of course we want to make the situation tenable now.) > > 2) That we are loosing some functionality. The way I understand the current > functionality, you can say that there is a holiday and the library is closed > from date A to date C, BUT on date B (which is between A and C) the library > is open anyway. If I remember correctly, you can also say that the library > is closed for a month, but open on Wednesdays. Is it still possible to do > something like that after applying the patch on this bug? I believe so; the way that creating events on a range works is that the code simply creates a set of single-date events. It is then possible to edit any of these events individually. The UI and calendar display are both a bit non-ideal, but are functional.
Poking this bug into what I believe is right. What would be the best next step (once I've addressed Magnus's concerns)?
Hi Jesse, this still needs a sign-off. I think first would be to confirm that the entering of exceptions still works as it should/add a follow-up and then give this another round of testing.
9 files do not apply on the first patch. The diffs are too big for me to "just do it" not knowing what I'm doing. Jesse, if you rebase the patches, I'll be happy to sign them off ASAP.
If we want to revive this, it will need a bit work. I have taken a quick look at the patches and identified 2 things already: - take into account that every day can be first day of the week now (pref) - remove all use of C4::Dates, as this no longer exists What's the best way to move forward?
Just updating to say that our institutions have interest in this. Bug 8133 is still blocked by this, and when we circulate equipment right now we have to manually set the return to be before the library closes.
Hello, If you're still interested by this patch, please have a look at this one who fixes the issue, but also add the feature from bug 8133. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17015#c46 Any help to help us get it upstreamed will be much appreciated! Thanks! François