Remove C4::Dates from: - tools/exceptionHolidays.pl - tools/holidays.pl - tools/newHolidays.pl
Created attachment 43098 [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 To test: - Go to Home > Tools > Calendar - Add, edit, delete the different types of hilidays and exceptions
Created attachment 43300 [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
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
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.
Created attachment 43410 [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)
Still applies
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
Created attachment 43957 [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
Created attachment 44077 [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
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"
Created attachment 44136 [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
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.
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.
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)
Created attachment 44390 [details] Exception dates formatted in ISO
Created attachment 44403 [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]
Created attachment 44585 [details] [review] [SIGNED OFF] 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>
Created attachment 44586 [details] [review] [SIGNED OFF] 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>
Created attachment 44587 [details] [review] [SIGNED OFF] 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>
Created attachment 44588 [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: Mirko Tietgen <mirko@abunchofthings.net>
Created attachment 44589 [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: Mirko Tietgen <mirko@abunchofthings.net>
Woops, Nick was a little faster. I'll restore the patches and add my signoff there
Created attachment 44591 [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>
Created attachment 44592 [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>
Created attachment 44749 [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>
Created attachment 44750 [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>
Created attachment 44751 [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>
Created attachment 44752 [details] [review] Bug 14954: Remove unused C4::Calendar::addDate subroutine Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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
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
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
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
Patches pushed to master. Thanks Marc and Jonathan!