The calendar only asks for dates the library is open closed, so if you have an hourly loan it doesn't know that the library is closed between 8pm and 9am for example and is marking things overdue that maybe the library doesn't consider overdue.
Fines Calculation 1) Don’t charge fines when library is closed (Ex: after 5 pm) a) Syspref for how to handle item returns if the return falls during a closed time. b) Make materials due at closed time i) Make materials due next day at time open ii) Open/Closed Rules Creation - This may require that we move everything over to Koha:: from C4:: 2) Recurring times (Ex: Weekdays closed at 5 pm, weekends closed at 3 pm) a) Exceptions (Ex: Closed at noon on Easter) Possible Useful Links: http://search.cpan.org/~fglock/DateTime-Set-0.31/lib/DateTime/Span.pm http://search.cpan.org/~fglock/DateTime-Set-0.31/lib/DateTime/SpanSet.pm
Created attachment 19326 [details] [review] Bug 8133 - hourly loans doesn't know when library closed This adds support for storing library hours in the calendar, and using those hours to compute hourly loan duetimes. A large amount of cleanup was also performed to expunge duplication of code between C4::Calendar and Koha::Calendar. Test plan: 1) Apply patch. 2) Update database. 3) Verify that the calendar administration interface still exists and works as expected. 4) Run t/Calendar.t and t/db_dependent/Calendar.t.
Hi Jesse, I couldn't seem to see the regular calendar interface after applying this patch. This is what I saw: http://img542.imageshack.us/img542/6643/4xij.png This is how it usually looks: http://img812.imageshack.us/img812/5074/8n85.png Liz
Created attachment 19494 [details] [review] Followup to bug 8133 - fix JS escaping in data literals There was some unescaped data being put into JS literals, which could cause the calendar interface to fail to load. Note for testing: Template::Plugin::JavaScript must be installed, either from CPAN or libtemplate-plugin-javascript-perl.
Created attachment 19740 [details] [review] Followup to bug 8133 - fix JS escaping in data literals There was some unescaped data being put into JS literals, which could cause the calendar interface to fail to load. Note for testing: Template::Plugin::JavaScript must be installed, either from CPAN or libtemplate-plugin-javascript-perl. Signed-off-by: Liz Rea <liz@catalyst.net.nz> Interface seems to work, I don't note any regressions. I tested the following: Normal, non holiday circulation with no hours set - due dates seem correct and consistent. Holiday set for today, library opens at 9am, closes at 10:30am. Item checked out at 10am, due 10:30am - seems right! UseDaysMode was set to "calendar" Holiday set for today, library opens at 9am, closes at 10:30am. Item checked out at 10am, due tomorrow at 9:30 - seems right! UseDaysMode was set to "days" Interface looks nice, thanks!
Wow - this is huge! I am a bit worried about the test plan, although I see that Liz has gone beyond it already. I think it's not only the calendar interface we need to check here, but also due date calculation, overdue fine calculation (on checkin, using the cronjobs, etc.) I think getting some more eyes on this would be good. And also some documentation about the new feature and how it will impact circulation.
(In reply to Katrin Fischer from comment #6) > Wow - this is huge! Yes, it is. Jesse, do you think it is possible to divide this patch into smaller ones? Creating many iterations will help moving ahead this patch. This patch does not apply anymore, so I just look at the code. Here are some notes: 1/ Use qa-tools in order to flush out some wrong lines (e.g. Koha is under the GPLv3 license, not v2). 2/ my $hours = { %{ $self->get_hours( $dt ) } }; is not it the same as $self->get_hours( $dt ) ? 3/ IN DUPLICATE is a Mysqlism. 4/ in updatedb: use a transaction to avoid a lost of data. 5/ Please use "git mv" for renaming files (e.g. tools/holidays.tt and tools/calendar.tt). The changes in the git history will be easier to read. 6/ try to provide several small patches (if possible). 7/ provide a more detailed test plan Marked as Failed QA.
I this dead? I suppose not, considering the time invested to do this massive change. But I ask because the patch will get harder to rebase/merge as time goes by. I would like to do some work on the calendar, but I don't want to conflict with this if it's going to be merged "sonn" (all relative to current time-space continuum).
(In reply to Jonathan Druart from comment #7) > (In reply to Katrin Fischer from comment #6) > > Wow - this is huge! > > Yes, it is. > > Jesse, do you think it is possible to divide this patch into smaller ones? > Creating many iterations will help moving ahead this patch. Yes, going to separate it into 2 bugs and 2 or 3 patches. > This patch does not apply anymore, so I just look at the code. > Here are some notes: > 1/ Use qa-tools in order to flush out some wrong lines (e.g. Koha is under > the GPLv3 license, not v2). Will do. > 2/ my $hours = { %{ $self->get_hours( $dt ) } }; > is not it the same as $self->get_hours( $dt ) ? This is used to clone the hash in question. > 3/ IN DUPLICATE is a Mysqlism. > 4/ in updatedb: use a transaction to avoid a lost of data. Good idea, changing this. > 5/ Please use "git mv" for renaming files (e.g. tools/holidays.tt and > tools/calendar.tt). The changes in the git history will be easier to read. I did so, the changes afterwards were too much for git to keep track of. The separated patches should show the rename if the changes are less drastic. > 6/ try to provide several small patches (if possible). > 7/ provide a more detailed test plan Will do so, but within reason. None of the circulation or overdues UI has been changed at all (except for a couple minor cronjobs), and testing them separately from the unit tests for Koha::Calendar accomplishes nothing. The only changes to C4::Circulation and C4::Overdues were to remove unused subs that depended on C4::Calendar (this can be verified with a quick grep). > > Marked as Failed QA.
Created attachment 29336 [details] [review] Bug 8133 - New fix: hourly loans doesn't know when library closed Overdues now take into account opening and closing time. Fines will not be applied when the library is closed. A test script 't/Calendar1.t' is provided for preleminary testing.
Maxime, Is the patch a followup to Jesse's patches? Or an alternative solution? If it is the latter, does it depend on bug 11211? I'm having a hard time applying your patch to test! Kyle
I can help there. I asked Maxime to provide an alternative solution to Jesse's. We've had this solution ready for over a year now, but put it aside with the objective to use the community's instead (at that point Jesse had started working on it) Our solution is not as ambitious as Jesse's, and does not offer the same amount of flexibility. But it's available. And it was required for use to push our "minute loans" patch (see #11055). I see that the old patches are still there, so this creates confusion. Maxime's patch is "standalone". This provides a calendar, and the overdue calculation function are modified to use the hours to determines the fines. In short.
And to be clear (after being short) I was not aware that #11211 had moved forward (although still not through). I suppose that long term, there is more benefit to go ahead with Jesse's complete solution. But some ETA would be nice...
Created attachment 31973 [details] [review] Bug 8133 - hourly loans doesn't know when library closed This adds support for storing library hours in the calendar, and using those hours to compute hourly loan duetimes. A large amount of cleanup was also performed to expunge duplication of code between C4::Calendar and Koha::Calendar. Test plan: 1) Apply patch. 2) Update database. 3) Verify that the calendar administration interface still exists and works as expected. 4) Run t/Calendar.t and t/db_dependent/Calendar.t. (This version is truly based on bug 11211, and folds in Liz's followup.)
Comment on attachment 29336 [details] [review] Bug 8133 - New fix: hourly loans doesn't know when library closed Obsoleting for now. Jesse has posted an updated patch.
Applying: Bug 8133 - hourly loans doesn't know when library closed fatal: sha1 information is lacking or useless (C4/Calendar.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 8133 - hourly loans doesn't know when library closed
(In reply to Chris Cormack from comment #16) > Applying: Bug 8133 - hourly loans doesn't know when library closed > fatal: sha1 information is lacking or useless (C4/Calendar.pm). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 Bug 8133 - hourly loans doesn't know when library closed Chris - I think you need to make sure you apply 11211 first - because it applied cleanly for me after that.
Created attachment 32176 [details] [review] Bug 8133 - hourly loans doesn't know when library closed This adds support for storing library hours in the calendar, and using those hours to compute hourly loan duetimes. A large amount of cleanup was also performed to expunge duplication of code between C4::Calendar and Koha::Calendar. Test plan: 1) Apply patch. 2) Update database. 3) Verify that the calendar administration interface still exists and works as expected. 4) Run t/Calendar.t and t/db_dependent/Calendar.t. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Hi Jesse, Please use the qa script and fix the errors, most of them are trailing spaces and tab chars and license. One is about datatables, you must use the include file. I have one concern about the new dependency: the module has not been updated since 2010, maybe it could worth to write our own module (or retrieve the one on the CPAN).
Created attachment 32327 [details] [review] Bug 8133 [QA Followup] - Fix koha-qa.pl violations
Currently blocked by bug 11211 not applying.
bug 11211 now apply's cleanly. Changing back to signed off :)
Running first tests on top of 11211, please fix and make sure both still apply on top of each other: 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 FAIL valid given is experimental when is experimental
Blocked by 11211 being failed QA right now.
Created attachment 32985 [details] [review] Bug 8133 - hourly loans doesn't know when library closed This adds support for storing library hours in the calendar, and using those hours to compute hourly loan duetimes. A large amount of cleanup was also performed to expunge duplication of code between C4::Calendar and Koha::Calendar. Test plan: 1) Apply patch. 2) Update database. 3) Verify that the calendar administration interface still exists and works as expected. 4) Run t/Calendar.t and t/db_dependent/Calendar.t. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 32986 [details] [review] Bug 8133 [QA Followup] - Fix koha-qa.pl violations
Created attachment 32990 [details] [review] Bug 8133 [QA Followup 2] - Fix unit tests
Created attachment 32991 [details] [review] Bug 8133 [QA Followup 2] - Fix unit tests
We still need to get rid of given-when here: FAIL tools/calendar.pl OK critic OK forbidden patterns OK pod FAIL valid given is experimental when is experimental But I will keep testing.
There is no documenation about the new options in the GUI and how this will change how things work. Please, provide a description how the calculations for hourly loans will be made.
Will receipts adjust their timestamp for "return by" based on opening hours or should that be a separate patch? The current situation with the receipts asking people to return books a minute to midnight is a bit silly. (btw - I set a "see also" from 8822 that asks for a public calendar with opening hours.)
I get an error when I try to apply the patches: $ git bz apply 8133 Bug 8133 - hourly loans doesn't know when library closed 32985 - Bug 8133 - hourly loans doesn't know when library closed 32986 - Bug 8133 [QA Followup] - Fix koha-qa.pl violations 32991 - Bug 8133 [QA Followup 2] - Fix unit tests Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 8133 - hourly loans doesn't know when library closed /home/vagrant/kohaclone/.git/rebase-apply/patch:62: trailing whitespace. }, { Slice => {} }, $branchcode ); /home/vagrant/kohaclone/.git/rebase-apply/patch:314: trailing whitespace. }, 'weekday', { Slice => {} }, $branch ); /home/vagrant/kohaclone/.git/rebase-apply/patch:399: trailing whitespace. if ( $self->{days_mode} eq 'Calendar' ) { /home/vagrant/kohaclone/.git/rebase-apply/patch:429: trailing whitespace. if ( $self->{days_mode} eq 'Calendar' ) { /home/vagrant/kohaclone/.git/rebase-apply/patch:595: trailing whitespace. warning: squelched 36 whitespace errors warning: 41 lines add whitespace errors. Using index info to reconstruct a base tree... <stdin>:62: trailing whitespace. }, { Slice => {} }, $branchcode ); <stdin>:314: trailing whitespace. }, 'weekday', { Slice => {} }, $branch ); <stdin>:399: trailing whitespace. if ( $self->{days_mode} eq 'Calendar' ) { <stdin>:429: trailing whitespace. if ( $self->{days_mode} eq 'Calendar' ) { <stdin>:595: trailing whitespace. warning: squelched 36 whitespace errors warning: 41 lines applied after fixing whitespace errors. Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/calendar.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc Auto-merging installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/kohastructure.sql Auto-merging C4/Circulation.pm Applying: Bug 8133 [QA Followup] - Fix koha-qa.pl violations fatal: sha1 information is lacking or useless (C4/Calendar.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 8133 [QA Followup] - Fix koha-qa.pl violations 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-8133-QA-Followup---Fix-koha-qapl-violations-admQOe.patch But I did get some weirdness when running the tests from bug 11211 (see bug 11211 comment 26), so perhaps there is something about my setup that is borken.
(In reply to Magnus Enger from comment #32) > I get an error when I try to apply the patches: Sorry for not being clear about this: it happens when I apply the patches from bug 8133 on top of the patches from bug 11211.
This is blocked by bug 11211.
Hello, If you're still interested by this patch, please have a look at this one who add this feature and fixes the issue from bug 11211. 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