Bug 14954 - Remove C4::Dates from holiday related files in folder tools
Summary: Remove C4::Dates from holiday related files in folder tools
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 14870
  Show dependency treegraph
 
Reported: 2015-10-04 20:27 UTC by Marc Véron
Modified: 2016-06-21 21:40 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14954 - Remove C4::Dates from holiday related files in folder tools (5.31 KB, patch)
2015-10-04 20:35 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14954 - Remove C4::Dates from holiday related files in folder tools (8.33 KB, patch)
2015-10-09 14:33 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14954 - Remove C4::Dates from holiday related files in folder tools (10.01 KB, patch)
2015-10-14 21:10 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14954 - Remove C4::Dates from holiday related files in folder tools (11.31 KB, patch)
2015-10-25 19:06 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14954 - Remove C4::Dates from holiday related files in folder tools (11.42 KB, patch)
2015-10-27 15:10 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14954 - (followup) Remove C4::Dates from holiday related files (5.03 KB, patch)
2015-10-28 23:01 UTC, Marc Véron
Details | Diff | Splinter Review
Exception dates formatted in ISO (23.04 KB, image/png)
2015-11-04 14:58 UTC, Josef Moravec
Details
Bug 14954: (followup) Display exceptions in syspref format (1.20 KB, patch)
2015-11-04 15:16 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14954 - Remove C4::Dates from holiday related files in folder tools (11.48 KB, patch)
2015-11-06 16:05 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14954 - (followup) Remove C4::Dates from holiday related files (5.10 KB, patch)
2015-11-06 16:05 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14954: (followup) Display exceptions in syspref format (1.27 KB, patch)
2015-11-06 16:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14954 - (followup) Remove C4::Dates from holiday related files (5.09 KB, patch)
2015-11-06 16:07 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14954: (followup) Display exceptions in syspref format (1.26 KB, patch)
2015-11-06 16:07 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14954 - (followup) Remove C4::Dates from holiday related files (5.14 KB, patch)
2015-11-06 16:18 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14954: (followup) Display exceptions in syspref format (1.31 KB, patch)
2015-11-06 16:18 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14954 - Remove C4::Dates from holiday related files in folder tools (11.54 KB, patch)
2015-11-11 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14954 - (followup) Remove C4::Dates from holiday related files (5.21 KB, patch)
2015-11-11 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14954: (followup) Display exceptions in syspref format (1.38 KB, patch)
2015-11-11 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14954: Remove unused C4::Calendar::addDate subroutine (2.02 KB, patch)
2015-11-11 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14954 - Remove C4::Dates from holiday related files in folder tools (11.60 KB, patch)
2015-11-18 00:58 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 14954 - (followup) Remove C4::Dates from holiday related files (5.27 KB, patch)
2015-11-18 00:58 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 14954: (followup) Display exceptions in syspref format (1.44 KB, patch)
2015-11-18 00:58 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 14954: Remove unused C4::Calendar::addDate subroutine (2.08 KB, patch)
2015-11-18 00:58 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-10-04 20:27:28 UTC
Remove C4::Dates from:

- tools/exceptionHolidays.pl
- tools/holidays.pl
- tools/newHolidays.pl
Comment 1 Marc Véron 2015-10-04 20:35:44 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-10-09 14:33:26 UTC Comment hidden (obsolete)
Comment 3 Héctor Eduardo Castro Avalos 2015-10-13 22:07:35 UTC
A problem when creating a new holiday

The 'day' parameter ("32") to DateTime::new did not pass the 'an integer which is a possible valid day of month' callback
 at /usr/lib/perl5/DateTime.pm line 199
	DateTime::new(undef, 'year', 2015, 'month', 10, 'day', 32) called at /home/koha/kohaclone/tools/newHolidays.pl line 67

The error was launched when enter an invalid date, for example

From date: Fridays, 30/10/2015
To date: 32/10/2015

this is DD/MM/YYYY
Comment 4 Héctor Eduardo Castro Avalos 2015-10-13 22:31:38 UTC
Another thing observed is: if you edit a holiday you can see in the first date, the form set it in dateformat syspref, but when you include a new date in the text field 'To Date:' from datepicker you'll see another format with hyphens not following the syspref.

Example:

From date: Thursdays, 2015/12/24
To Date : 2015-12-24

And again, if you alter the date to 2015-12-32 the same error is shown in the screen.
Comment 5 Marc Véron 2015-10-14 21:10:33 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2015-10-23 09:40:55 UTC
Still applies
Comment 7 Héctor Eduardo Castro Avalos 2015-10-23 20:51:39 UTC
Sorry, but when editing an existing "Holiday repeating yearly" for example, christmas i get an error

Software error:

The 'day' parameter ("32") to DateTime::new did not pass the 'an integer which is a possible valid day of month' callback
 at /usr/lib/perl5/DateTime.pm line 199
	DateTime::new(undef, 'year', 2015, 'month', 10, 'day', 32) called at /home/koha/kohaclone/tools/exceptionHolidays.pl line 64

When enter an invalid date like 32/10/2015
Comment 8 Marc Véron 2015-10-25 19:06:22 UTC Comment hidden (obsolete)
Comment 9 Mirko Tietgen 2015-10-27 15:10:29 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-10-28 15:30:48 UTC
Comment on attachment 44077 [details] [review]
Bug 14954 - Remove C4::Dates from holiday related files in folder tools

Review of attachment 44077 [details] [review]:
-----------------------------------------------------------------

Looking at the patch, some changes smell false.
I might be wrong.

::: C4/Calendar.pm
@@ +707,5 @@
>  
>  sub addDate {
>      my ($self, $startdate, $offset) = @_;
> +    $startdate = eval { output_pref( { dt => dt_from_string( $startdate ), dateonly => 1, dateformat => 'iso' } ); };
> +    my ( $year, $month, $day) = split( "-", $startdate );

$startdate is already a dt!

@@ +730,5 @@
>  	} else { ## ($daysMode eq 'Days') 
>          ($year, $month, $day) = &Date::Calc::Add_Delta_Days($year, $month, $day, $offset );
>      }
> +    my $date_ret = sprintf(ISO_DATE_FORMAT,$year,$month,$day);
> +    $date_ret =  eval { output_pref( { dt => dt_from_string( $date_ret), dateonly => 1, dateformat => 'iso' } ); };

This looks wrong, $date_ret is not in the pref format (what dt_from_string is expecting).
There is a fallback on iso, but when you know the format is iso, you have to provide it.

@@ +736,3 @@
>  }
>  
>  =head2 daysBetween

This subroutine is not used anywhere, prefer to remove it before this patch.

::: tools/holidays.pl
@@ +49,3 @@
>  # if the url has an invalid date default to 'now.'
> +$calendardate = output_pref( { dt => $calendarinput_dt, dateonly => 1 } );
> +$keydate = output_pref( { dt => $calendarinput_dt, dateonly => 1, dateformat => 'iso' } );

That's not what it was done before.

@@ +132,4 @@
>  my $single_holidays = $calendar->get_single_holidays();
>  my @holidays;
>  foreach my $yearMonthDay (keys %$single_holidays) {
> +    my $holidaydate = eval { dt_from_string( $single_holidays->{$yearMonthDay}{date} ) };

specify iso.

@@ +136,4 @@
>      my %holiday;
>      %holiday = (KEY => $yearMonthDay,
>                  DATE_SORT => $single_holidays->{$yearMonthDay}{date},
> +                DATE => output_pref( { dt => $holidaydate, dateonly => 1, dateformat => 'iso' } ),

before the output was "syspref"
Comment 11 Marc Véron 2015-10-28 23:01:37 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-10-29 08:24:08 UTC
Marc, are you sure all the subroutines you have changed with these patches are triggered by your test plan?
I would like to see a more complete test plan and a second signoff here please.
Comment 13 Marc Véron 2015-10-29 09:14:09 UTC
More complete test plan as requested by comment #12

- Prove t/Calendar.t
- Go to Home > Tools > Calendar

- Verify that adding holidays works as before: 
  - Holiday only on one day.
  - Holiday repeated every same day of the week.
  - Holiday repeated yearly on the same date.
  - Holidays on a range.
  - Holidays repeated yearly on a range.

- Verify that editing (including deleting) holidays and exceptions
  work as before:
  - For repeated holidays:
     - Generate an exception for this repeated holiday.
     - Generate exceptions on a range for this repeated holiday.
     - Generate exceptions on a range of dates.
     - Delete this holiday.
     - Delete the single holidays on a range.
     - Delete the repeated holidays on a range.
     - Delete the exceptions on a range.
  - For a single holiday:
     - Generate exceptions on a range of dates.
     - Delete this holiday.
     - Delete the single holidays on a range.
     - Delete the repeated holidays on a range.
     - Delete the exceptions on a range.
     - Edit this holiday

Hint: There is a [?] after each function with explanation how it works.
Comment 14 Josef Moravec 2015-11-04 14:58:00 UTC
I have this setup:
    First day of week monday
    Date format dd/mm/rrrr


When I try to add a range holiday from 01/01/2015 to 10/01/2015 and I end with this days set close:

01/01/2015
02/01/2015
03/01/2015
04/01/2015
05/01/2015
06/01/2015
07/01/2015
08/01/2015
09/01/2015
10/01/2015
01/11/2015
02/11/2015
03/11/2015
04/11/2015
24/12/2015
25/12/2015
26/12/2015
27/12/2015
28/12/2015
29/12/2015
30/12/2015
31/12/2015

The dates for exceptions are formatted in ISO, not in right date format (see attachment)
Comment 15 Josef Moravec 2015-11-04 14:58:36 UTC
Created attachment 44390 [details]
Exception dates formatted in ISO
Comment 16 Marc Véron 2015-11-04 15:16:14 UTC Comment hidden (obsolete)
Comment 17 Nick Clemens 2015-11-06 16:05:15 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2015-11-06 16:05:26 UTC Comment hidden (obsolete)
Comment 19 Nick Clemens 2015-11-06 16:05:33 UTC Comment hidden (obsolete)
Comment 20 Mirko Tietgen 2015-11-06 16:07:29 UTC Comment hidden (obsolete)
Comment 21 Mirko Tietgen 2015-11-06 16:07:48 UTC Comment hidden (obsolete)
Comment 22 Mirko Tietgen 2015-11-06 16:10:23 UTC
Woops, Nick was a little faster. I'll restore the patches and add my signoff there
Comment 23 Mirko Tietgen 2015-11-06 16:18:35 UTC Comment hidden (obsolete)
Comment 24 Mirko Tietgen 2015-11-06 16:18:50 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-11-11 13:01:46 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-11-11 13:01:49 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2015-11-11 13:01:54 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2015-11-11 13:02:00 UTC Comment hidden (obsolete)
Comment 29 Brendan Gallagher 2015-11-18 00:58:35 UTC
Created attachment 44938 [details] [review]
Bug 14954 - Remove C4::Dates from holiday related files in folder tools

This patch removes C4::Dates from:
Remove C4::Dates from:

- tools/exceptionHolidays.pl
- tools/holidays.pl
- tools/newHolidays.pl
- C4/Calendar.pm

To test:
- Go to Home > Tools > Calendar
- Add, edit, delete the different types of holidays and exceptions
- git grep 'C4::Calendar' and test such files
- prove t/Calendar.t

(Amended following comments #3 and #4 / mv)
(Amended following comment #7 / 25.10.2015 / mv

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

http://bugs.koha-community.org/show_bug.cgi?id=14945
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Brendan Gallagher brendan@bywatersolutions.com
Comment 30 Brendan Gallagher 2015-11-18 00:58:42 UTC
Created attachment 44939 [details] [review]
Bug 14954 - (followup) Remove C4::Dates from holiday related files

Followup for comment #10

To test:
- Go to Home > Tools > Calendar
- Add, edit, delete the different types of holidays and exceptions
- git grep 'C4::Calendar' and test such files
- git grep daysBetween to make sure that this sub is not used
- prove t/Calendar.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Brendan Gallagher brendan@bywatersolutions.com
Comment 31 Brendan Gallagher 2015-11-18 00:58:46 UTC
Created attachment 44940 [details] [review]
Bug 14954: (followup) Display exceptions in syspref format

This patch fixes the display of exceptions (was iso, is now syspref).
See comment #14 and attachment #15 [details] [review]

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Brendan Gallagher brendan@bywatersolutions.com
Comment 32 Brendan Gallagher 2015-11-18 00:58:52 UTC
Created attachment 44941 [details] [review]
Bug 14954: Remove unused C4::Calendar::addDate subroutine

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Brendan Gallagher brendan@bywatersolutions.com
Comment 33 Tomás Cohen Arazi 2015-11-18 02:47:11 UTC
Patches pushed to master.

Thanks Marc and Jonathan!