Bug 11211

Summary: Move calculation code out of C4::Calendar
Product: Koha Reporter: Jesse Weaver <jweaver>
Component: CirculationAssignee: Jesse Weaver <jweaver>
Status: CLOSED WONTFIX QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: brendan, eric.begin, francois.charbonnier, gmcharlt, indradg, jonathan.druart, jschmidt, katrin.fischer, kholten, kyle.m.hall, kyle, m.de.rooy, magnus, marjorie.barry-vila, matted-34813, philippe.blouin, tmisilo, tomascohen, wizzyrea
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12450, 8133    
Attachments: Bug 11211 - Move calculation code out of C4::Calendar
Bug 11211 - Move calculation code out of C4::Calendar
0001-SIGNED-OFF-Bug-11211-Move-calculation-code-out-of-C4
Bug 11211: Fix QA issues
Bug 11211 - Move calculation code out of C4::Calendar
Bug 11211 - Move calculation code out of C4::Calendar
Bug 11211 - Move calculation code out of C4::Calendar
Bug 11211 - Move calculation code out of C4::Calendar
Bug 11211 - Move calculation code out of C4::Calendar
Bug 11211 [QA Followup] - Remove use of dt_add_type_uk_date
Dates are not sorted properly
Bug 11211 - Move calculation code out of C4::Calendar
Bug 11211 [QA Followup] - Remove use of dt_add_type_uk_date
Bug 11211 - Move calculation code out of C4::Calendar
Bug 11211 [QA Followup] - Remove use of dt_add_type_uk_date
Errors in the firebug console when calendar.pl only shows name of weekday
Bug 11211 - Followups for calendar.tt

Description Jesse Weaver 2013-11-06 23:14:14 UTC
Koha::Calendar should contain all of the calendar calculation code, and C4::Calendar should only have admin subs.

Created to slim down bug 8133.
Comment 1 Jesse Weaver 2014-01-29 01:46:10 UTC Comment hidden (obsolete)
Comment 2 Jesse Weaver 2014-01-29 01:47:17 UTC
CC'ing Liz, as she was on the original bug.
Comment 3 Jesse Weaver 2014-02-20 22:37:10 UTC Comment hidden (obsolete)
Comment 4 wajasu 2014-02-22 20:40:53 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-03-18 10:33:09 UTC
Jesse,
Could you explain why you changed "holiday" with "event" and you removed "Holiday exception" please?
Comment 6 Jonathan Druart 2014-03-18 10:33:27 UTC Comment hidden (obsolete)
Comment 7 Jesse Weaver 2014-03-18 19:04:30 UTC
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.
Comment 8 Jonathan Druart 2014-03-19 10:32:21 UTC
(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?
Comment 9 Jesse Weaver 2014-03-25 18:25:08 UTC
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.
Comment 10 Jonathan Druart 2014-03-26 12:08:56 UTC
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)?
Comment 11 Eric Bégin 2014-07-15 19:18:24 UTC
Jessy, 

Do you know if and when you'll have time to work again on this feature ?

Cheers !
Comment 12 Jesse Weaver 2014-07-15 21:25:49 UTC
Now that I'm done with school and on-site work for a little while, I'll have some time to get this through.
Comment 13 Jesse Weaver 2014-07-16 04:09:28 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2014-08-13 15:37:54 UTC
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
Comment 15 Jesse Weaver 2014-08-19 02:19:50 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2014-10-09 19:31:27 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2014-10-28 20:20:26 UTC
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
Comment 18 Jesse Weaver 2014-10-28 20:54:40 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2014-10-28 22:37:29 UTC
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
Comment 20 Katrin Fischer 2014-10-28 22:41:58 UTC
All good, too late here and my clue to stop QAing for tonight.
Comment 21 Katrin Fischer 2014-10-30 06:53:30 UTC
Working on this now.
Comment 22 Katrin Fischer 2014-10-30 07:00:04 UTC
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
Comment 23 Katrin Fischer 2014-10-30 07:15:29 UTC
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!
Comment 24 Kyle M Hall 2014-10-30 15:33:59 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2014-10-30 17:38:14 UTC Comment hidden (obsolete)
Comment 26 Magnus Enger 2014-11-10 11:03:59 UTC
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
Comment 27 Magnus Enger 2014-11-10 12:48:12 UTC
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.
Comment 28 Jesse Weaver 2014-11-13 02:38:11 UTC
(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?
Comment 29 Jonathan Druart 2014-11-13 08:10:44 UTC
(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.
Comment 30 Magnus Enger 2014-11-14 20:42:07 UTC
(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.
Comment 31 Magnus Enger 2014-11-24 14:37:13 UTC
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
Comment 32 Magnus Enger 2014-11-25 10:25:56 UTC
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.
Comment 33 Kyle M Hall 2014-11-25 12:43:11 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2014-11-25 12:43:25 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2014-11-25 12:46:06 UTC
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>
Comment 36 Kyle M Hall 2014-11-25 12:46:15 UTC
Created attachment 33899 [details] [review]
Bug 11211 [QA Followup] - Remove use of dt_add_type_uk_date
Comment 37 Magnus Enger 2014-11-25 13:08:30 UTC
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.
Comment 38 Kyle M Hall 2014-11-25 13:23:14 UTC
> 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.
Comment 39 Magnus Enger 2014-11-25 14:03:46 UTC
(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...
Comment 40 Magnus Enger 2014-11-25 14:35:35 UTC
Created attachment 33908 [details]
Errors in the firebug console when calendar.pl only shows name of weekday
Comment 41 Magnus Enger 2014-11-26 13:53:42 UTC
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.
Comment 42 Owen Leonard 2014-11-26 13:56:55 UTC
(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?
Comment 43 Magnus Enger 2014-11-26 14:10:13 UTC
(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 44 Magnus Enger 2014-11-26 14:58:15 UTC
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
}));
Comment 45 Magnus Enger 2014-12-01 13:34:30 UTC
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
Comment 46 Magnus Enger 2014-12-01 13:40:08 UTC
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?
Comment 47 Jesse Weaver 2015-03-02 06:37:37 UTC
(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;
Comment 48 Magnus Enger 2015-03-03 10:44:08 UTC
(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?
Comment 49 Katrin Fischer 2015-05-14 22:07:24 UTC
Magnuse, reading your last comment - should this be failed qa for a regression on making 'exceptions'?
Comment 50 Magnus Enger 2015-05-18 07:31:21 UTC
(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. ;-)
Comment 51 Jesse Weaver 2015-05-29 21:07:15 UTC
(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.
Comment 52 Jesse Weaver 2015-05-29 21:10:26 UTC
Poking this bug into what I believe is right. What would be the best next step (once I've addressed Magnus's concerns)?
Comment 53 Katrin Fischer 2015-05-31 13:17:08 UTC
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.
Comment 54 Blou 2015-07-21 13:36:56 UTC
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.
Comment 55 Katrin Fischer 2015-11-30 17:23:37 UTC
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?
Comment 56 Karl Holten 2016-06-16 16:05:11 UTC
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.
Comment 57 Francois Charbonnier 2017-02-03 17:22:25 UTC
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