Bug 7351

Summary: ability to edit a range of holidays
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: ToolsAssignee: Christophe Croullebois <christophe.croullebois>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P3 CC: chris, colin.campbell, katrin.fischer, koha, leila.arkab, m.de.rooy, mtj, paul.poulain, stephane.delaye
Version: 3.10   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6663
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Delete on a range of dates
before
single
repeated
after
the last patch with delete exceptions on a range
patch finalized
Delete repeated holidays on a range
I hope the last one, patch finalised
this patch is finalised
The patch finalized after cleaning code
[SIGN OFF]Bug 7351 : feature that allows to delete a range of dates
Patch for minor textual changes
screenshot of ugly display
the page is ok
this is an addon after M. de Rooy's patch
[SIGN OFF]Bug 7351 : feature that allows to delete a range of dates
Bug 7351 : feature that allows to delete a range of dates

Description Nicole C. Engard 2011-12-13 17:10:10 UTC
The new feature that allows you to enter a range of holidays does not let you edit the range. So if you have a long range of dates and you messed up you have to edit them one by one.  A way to edit/delete a range of dates would be nice.
Comment 1 Christophe Croullebois 2012-02-05 19:53:58 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-02-07 19:38:07 UTC
I was able to use this to delete a range of repeated yearly holidays but not repeated single holidays or repeated weekly holidays.
Comment 3 Christophe Croullebois 2012-02-08 12:57:17 UTC
Hello Owen,
right, about repeated weekly holidays it is deliberate, because you have max 7 days to edit/delete, I think it don't needs to be changed.
However with the patch you must be able to delete either the singles holidays or the yearly repeated holidays...
Have you checked the corresponding radio button ?
I'll test and try to understand.
thx
Comment 4 Christophe Croullebois 2012-02-08 14:13:30 UTC
Sry Owen I can't reproduce the bug.
I'll join some snapshots, no idea why you can't delete a range of single holidays...
Comment 5 Christophe Croullebois 2012-02-08 14:22:55 UTC Comment hidden (obsolete)
Comment 6 Christophe Croullebois 2012-02-08 14:27:35 UTC Comment hidden (obsolete)
Comment 7 Christophe Croullebois 2012-02-08 14:30:11 UTC Comment hidden (obsolete)
Comment 8 Christophe Croullebois 2012-02-08 14:30:47 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2012-02-08 17:24:23 UTC
(In reply to comment #3)
> right, about repeated weekly holidays it is deliberate, because you have max 7
> days to edit/delete, I think it don't needs to be changed.

I think I see what you mean: It's easy enough to select one of the repeated weekly events and delete the whole range from there. I think what I expected was that if the weekly event fell within the range I specified it would remove just those instances of the weekly event (i.e. create exceptions).

I am similarly confused about how the "Delete the repeated holidays on a range" option works. Example: I specify a range of holidays repeating yearly (24 and 25 December, for instance). Then I select a range within that repeating pattern: 24 December 2015 through 25 December 2015 and choose "Delete the repeated holidays on a range." Doing so deletes the entire set of repeating yearly holidays defined in my original operation. Again, I expected it to delete only the dates I specified in the range I gave in the delete operation.

I would be nice to get some other testers' input on that functionality to see if it's necessary to mitigate the confusion with some interface changes (labels, hints, whatever).

> However with the patch you must be able to delete either the singles holidays
> or the yearly repeated holidays...
> Have you checked the corresponding radio button ?

I think I must have not had the correct radio button selected because it is working now.

I'm putting this back to "needs signoff" because I'd like to get a second opinion.
Comment 10 Christophe Croullebois 2012-02-09 10:46:10 UTC
Hello Owen,
yes it's the reason why I said "needs deep testing", I think this patch must have multiple advices.
For the first point : the weekly repeatable days, I think we are ok, we can stay on what we have, day by day for deleting or exceptions.
A part of the choices I have made was in regard of the technical structure and my lazy side. ;o)
The patch can on a range of date either delete the single or repeated days, depending of the radio button selected. BUT if 'single' selected, and if in your range you have other kinds of holidays, only 'single' will be deleted, the same for 'repeated'...
I have made the functions able to exit if there is some bad values, so if you don't put a value in 'to date:' for example or if the values are not in the scope, koha does nothing, I have chosen this option because you have the visual of dates on the calandar and if no action is done you can see it on calandar, and at my sense it's more ergonomic than some warnings messages with a clic to close them...
But it's just my advice.
On the other hand, you gave me an idea, I think I could add a third option to put exceptions on a range of date regarding the yearly repeated holidays...
What do you think about ?
thx
Comment 11 Owen Leonard 2012-02-09 13:53:45 UTC
(In reply to comment #10)
> On the other hand, you gave me an idea, I think I could add a third option to
> put exceptions on a range of date regarding the yearly repeated holidays...
> What do you think about ?

What I'm not sure about is which is overall easier for the user. I think selecting a range within a yearly repeated holiday to create exceptions is a good feature. But then how does the user delete an *entire* range of repeated yearly holidays? Do they have to input the exact range? That sounds potentially difficult.
Comment 12 Christophe Croullebois 2012-02-09 16:26:07 UTC
In fact Owen,
in his actual state the patch is permissive, no obligations to put the exact range. eg:
you have a range of yearly repeated holidays from 15 august to 31 august.
You can :
1° select from 29 august to 4 september :result => only 29/30/31 august will be deleted.....
2° select from 25 july to 16 august :result => only 15/16 august will be deleted.....
3° you want the exact periode, then => select from 15 august to 31 august 
I wanted to make a light patch the first time, to edit the range, now we are further...
But inside, all the job is done one day by one day, I don't change the DB structure, so we can delete one day of a range and so on.
All the job for a range of 10 days is the same in the base, that you obtain with 10 edit/create...
So I'll create an option to put exceptions on a range, and we'll see.
Thx.
Comment 13 Christophe Croullebois 2012-03-04 17:58:45 UTC Comment hidden (obsolete)
Comment 14 Koha Team University Lyon 3 2012-03-20 15:11:17 UTC
Tested on sand-box

Something went wrong !
Applying: Bug 7351 : feature that allows to delete a range of dates
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Calendar.pm
CONFLICT (content): Merge conflict in C4/Calendar.pm
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/holidays.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 7351 : feature that allows to delete a range of dates
Comment 15 Christophe Croullebois 2012-03-20 16:17:49 UTC Comment hidden (obsolete)
Comment 16 Koha Team University Lyon 3 2012-03-21 09:08:53 UTC
I try this :
- Add Holidays on a range.
- Add Holidays repeated yearly on a range
I have the error : 
Software error:

Missing right curly or square bracket at /home/koha/src/tools/newHolidays.pl line 149, at end of line
syntax error at /home/koha/src/tools/newHolidays.pl line 149, at EOF
Execution of /home/koha/src/tools/newHolidays.pl aborted due to compilation errors.

For help, please send mail to the webmaster (webmaster@test4-sandbox), giving this error message and the time and date 
of the error.  

I try : "Generate an exception for this repeated holiday" : ok for the date where I have clic.

I try : "Delete the repeated holidays on a range." nothing is done. (cf. attachement)
Comment 17 Koha Team University Lyon 3 2012-03-21 09:10:24 UTC Comment hidden (obsolete)
Comment 18 Christophe Croullebois 2012-03-21 13:11:17 UTC
Ok, thx,
a stupid bug due to a lack of a semicolon..
But more, a bug on "Delete the exceptions on a range".
So, new patch will follow soon.
Comment 19 Christophe Croullebois 2012-03-22 12:47:38 UTC Comment hidden (obsolete)
Comment 20 Christophe Croullebois 2012-03-23 11:14:01 UTC Comment hidden (obsolete)
Comment 21 Leila 2012-03-23 14:30:22 UTC
Hi
Tested again. We can apply holidays (1 day off, on a range), and undo this in different cases (Unique holiday, Holiday repeating weekly, Holiday repeating yearly, Holiday exception). I also tried to copy this to all libraries. OK.
Thanks Christophe for this patch, and soon a new bugzilla for some little improvments.
Comment 22 Christophe Croullebois 2012-04-06 15:20:34 UTC Comment hidden (obsolete)
Comment 23 Paul Poulain 2012-04-10 13:50:35 UTC Comment hidden (obsolete)
Comment 24 delaye 2012-04-10 13:53:13 UTC
test in a sandbox : it works
we can create and delete holidays on a range.
Comment 25 Marc Véron 2012-04-27 19:59:49 UTC
*** Bug 7964 has been marked as a duplicate of this bug. ***
Comment 26 Marcel de Rooy 2012-05-16 14:20:19 UTC
QA Comments:
In general: I do not object to adding these options, but I am not convinced that we make life easier for the average user. When clicking a holiday, I now have 7 options instead of 3 options. Descriptions like "Deleting single holidays on a range" are not very intuitive.
Since editing the holiday table is not a daily task, this may not need community discussions. I will set the status to Passed QA. I have no technical objections, but pass my "functional doubts" to the RM.

Please note too: If you leave the To field empty, and choose for Deleting on a range, nothing happens. This behavior could be confusing for the user. IMO it still needs to be improved. Currently, we leave the user in a state wondering if something happened or not. A simple javascript check may already be sufficient.

And a minor remark about a POD line: "Is the day month to make the date to delete."
This description is not very clear. What is "day month"? IMO if you have three pars, day, month and year, you do not need to explain.

While looking at the options, I detected some spelling issues too. Will add a patch for that.

Passed QA
Comment 27 Marcel de Rooy 2012-05-16 14:21:22 UTC Comment hidden (obsolete)
Comment 28 Paul Poulain 2012-05-22 08:55:14 UTC
Testing this patch, I get an ugly display (screenshot attached).
I also tried with chrome, flushed the cache in case of a CSS problem, same problem
Comment 29 Paul Poulain 2012-05-22 08:56:04 UTC
Created attachment 9691 [details]
screenshot of ugly display
Comment 30 Christophe Croullebois 2012-06-01 09:06:02 UTC
Hum, Paul,
I have tested again, on a branch up to date, I have applyed my patch and the pages are ok...
I join a snapshot
Could anyone test ?
Thx
Comment 31 Christophe Croullebois 2012-06-01 09:07:16 UTC
Created attachment 9869 [details]
the page is ok
Comment 32 Christophe Croullebois 2012-06-01 09:45:21 UTC
I have seen with Paul, maybe due to his hook git...
Tested with two others PC.
thx
Comment 33 Christophe Croullebois 2012-06-01 14:17:19 UTC Comment hidden (obsolete)
Comment 34 Paul Poulain 2012-06-27 15:27:20 UTC
Sorry Christophe, but this patch make me crazy. Now it does not apply anymore.


Could you please rebase and squash the 3 patches ?

git rebase master
git rebase -i HEAD~3
replace on line 2 and 3 "pick" by "squash"
(ask me if needed)
Comment 35 Christophe Croullebois 2012-07-08 15:54:19 UTC
Paul, too much time has passed, the calendar has changed, I have to rewrite code.
Comment 36 Christophe Croullebois 2012-07-15 14:46:37 UTC Comment hidden (obsolete)
Comment 37 Mason James 2012-07-16 04:36:58 UTC
QA comment, passing QA

all QA tests pass
$ koha-qa.pl
starting perlcritic-progressive tests... OK
starting 'perl -c' syntax tests... OK
starting xt/tt_valid.t tests... OK
starting xt/author/vaild-template.t tests... OK

patch applies clean to 60b6c187c46614431f109512c740a3bcf077f342
Comment 38 Mason James 2012-07-16 22:42:33 UTC
(In reply to comment #35)
> Paul, too much time has passed, the calendar has changed, I have to rewrite
> code.

QA comment...
hmm, i think we are still waiting for a sign-off for Christophe's rewrite here

can someone do a quick test on it, and sign-off please

setting status to 'needs sign-off'
Comment 39 Christophe Croullebois 2012-07-27 09:18:15 UTC
please sign-off ;o(
thx
Comment 40 Chris Cormack 2012-07-28 08:18:38 UTC
Colin, do you have any thoughts about this and Koha::Calendar?
Comment 41 Colin Campbell 2012-07-30 13:58:10 UTC
Koha::Calendar left the entry and editing of Holidays to C4::Calendar basically reading the data only. The reason was that while the way the holidays were stored was not the best, if we tried to address that we'd also have to rewrite the interface doing the holiday create/update/delete. So I dont think this introduces any conflict. I'd still like one day (but not soon I expect) to have the whole logic overhauled and the user interface totally overhauled, possibly then Holiday could be separated out into its own object Holiday.pm. But until then....

In summary from a Koha::Calendar view I dont think this alters the interface it addresses.
Comment 42 Chris Cormack 2012-07-31 04:32:17 UTC
Cool, in that case im happy to sign off, will do that tonight.
Comment 43 Chris Cormack 2012-07-31 08:04:57 UTC
Created attachment 11214 [details] [review]
Bug 7351 : feature that allows to delete a range of dates

Four new options, one for single holidays, one for the repeatable holidays.
One to create exceptions on a range of dates, one to delete exceptions in a range of dates.
Note that the exceptions are not deleted if you delete a range of repeatable dates.
But if you delete a range of single holidays the exeptions inside will be deleted.

Signed-off-by: Stephane Delaye <stephane.delaye@biblibre.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 44 Marcel de Rooy 2012-08-23 10:14:19 UTC
Depending on tests, code looks good to me. Passed QA
Comment 45 Paul Poulain 2012-09-03 16:49:04 UTC
mmm... trying to add a range of holiday, I get nothing.
I can choose a range of dates, select "Holidays on a range", can click OK, but the holiday is not saved :(((
Comment 46 Christophe Croullebois 2012-09-17 14:48:20 UTC
hum Paul, this patch just works on the second feature about a range a date, the deletion.
The first one I have made allows to create holidays on a range, and it is pushed.
This one allows to delete on a range, so I don't understand, you says you can't add holidays on a range ? But it's not the purpose of this patch, anyway, I have just tested again few minutes ago, all is ok, even creating holidays on a range...
Could you test again ?
I see an other trouble with you there is some time => 

Christophe Croullebois 2012-06-01 09:45:21 UTC
I have seen with Paul, maybe due to his hook git...
Tested with two others PC.
Comment 47 Paul Poulain 2012-09-28 13:01:40 UTC
Works like a charm.
However, i've discovered some problems under plack, i'll open another bug for that.

Patch pushed to master