Bug 8133 - hourly loans doesn't know when library closed
Summary: hourly loans doesn't know when library closed
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Jesse Weaver
QA Contact:
URL:
Keywords:
Depends on: 11211
Blocks: 6796 11055 12450 8528
  Show dependency treegraph
 
Reported: 2012-05-21 15:33 UTC by Nicole C. Engard
Modified: 2019-02-05 14:14 UTC (History)
27 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Large patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8133 - hourly loans doesn't know when library closed (184.67 KB, patch)
2013-07-01 21:05 UTC, Jesse Weaver
Details | Diff | Splinter Review
Followup to bug 8133 - fix JS escaping in data literals (2.86 KB, patch)
2013-07-08 21:52 UTC, Jesse Weaver
Details | Diff | Splinter Review
Followup to bug 8133 - fix JS escaping in data literals (3.39 KB, patch)
2013-07-18 01:16 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8133 - New fix: hourly loans doesn't know when library closed (42.38 KB, patch)
2014-06-27 20:54 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 8133 - hourly loans doesn't know when library closed (77.14 KB, patch)
2014-10-02 08:51 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 8133 - hourly loans doesn't know when library closed (77.20 KB, patch)
2014-10-10 15:44 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8133 [QA Followup] - Fix koha-qa.pl violations (12.69 KB, patch)
2014-10-15 11:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8133 - hourly loans doesn't know when library closed (75.35 KB, patch)
2014-10-30 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8133 [QA Followup] - Fix koha-qa.pl violations (12.69 KB, patch)
2014-10-30 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8133 [QA Followup 2] - Fix unit tests (1.79 KB, patch)
2014-10-30 16:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8133 [QA Followup 2] - Fix unit tests (1.77 KB, patch)
2014-10-30 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-05-21 15:33:59 UTC
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.
Comment 1 Elliott Davis 2012-07-24 19:49:31 UTC
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
Comment 2 Jesse Weaver 2013-07-01 21:05:38 UTC Comment hidden (obsolete)
Comment 3 Liz Rea 2013-07-05 00:33:06 UTC
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
Comment 4 Jesse Weaver 2013-07-08 21:52:45 UTC Comment hidden (obsolete)
Comment 5 Liz Rea 2013-07-18 01:16:29 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2013-09-09 06:21:03 UTC
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.
Comment 7 Jonathan Druart 2013-09-20 09:39:16 UTC
(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.
Comment 8 Blou 2013-10-23 20:10:52 UTC
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).
Comment 9 Jesse Weaver 2013-11-22 22:09:18 UTC
(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.
Comment 10 Maxime Beaulieu 2014-06-27 20:54:14 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2014-07-08 17:31:44 UTC
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
Comment 12 Blou 2014-07-08 17:56:46 UTC
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.
Comment 13 Blou 2014-07-08 18:27:31 UTC
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...
Comment 14 Jesse Weaver 2014-10-02 08:51:07 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2014-10-09 13:10:00 UTC
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.
Comment 16 Chris Cormack 2014-10-09 14:29:14 UTC
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
Comment 17 Brendan Gallagher 2014-10-09 19:17:10 UTC
(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.
Comment 18 Chris Cormack 2014-10-10 15:44:11 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2014-10-11 19:32:29 UTC
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).
Comment 20 Kyle M Hall 2014-10-15 11:20:09 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2014-10-28 20:23:09 UTC
Currently blocked by bug 11211 not applying.
Comment 22 Brendan Gallagher 2014-10-28 21:00:17 UTC
bug 11211 now apply's cleanly.  Changing back to signed off :)
Comment 23 Katrin Fischer 2014-10-30 07:06:44 UTC
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
Comment 24 Katrin Fischer 2014-10-30 07:16:07 UTC
Blocked by 11211 being failed QA right now.
Comment 25 Kyle M Hall 2014-10-30 16:30:25 UTC
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>
Comment 26 Kyle M Hall 2014-10-30 16:30:32 UTC
Created attachment 32986 [details] [review]
Bug 8133 [QA Followup] - Fix koha-qa.pl violations
Comment 27 Kyle M Hall 2014-10-30 16:55:02 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2014-10-30 16:58:17 UTC
Created attachment 32991 [details] [review]
Bug 8133 [QA Followup 2] - Fix unit tests
Comment 29 Katrin Fischer 2014-10-30 20:08:51 UTC
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.
Comment 30 Katrin Fischer 2014-10-30 20:50:59 UTC
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.
Comment 31 Viktor Sarge 2014-11-06 14:11:57 UTC
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.)
Comment 32 Magnus Enger 2014-11-10 12:57:44 UTC
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.
Comment 33 Magnus Enger 2014-11-10 13:45:15 UTC
(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.
Comment 34 Jonathan Druart 2014-12-19 13:35:58 UTC
This is blocked by bug 11211.
Comment 35 Francois Charbonnier 2017-02-03 17:23:33 UTC
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