Bug 7351 - ability to edit a range of holidays
Summary: ability to edit a range of holidays
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: 3.10
Hardware: All All
: P3 enhancement (vote)
Assignee: Christophe Croullebois
QA Contact: Marcel de Rooy
URL:
Keywords:
: 7964 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-12-13 17:10 UTC by Nicole C. Engard
Modified: 2013-05-23 06:23 UTC (History)
9 users (show)

See Also:
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 (11.01 KB, patch)
2012-02-05 19:53 UTC, Christophe Croullebois
Details | Diff | Splinter Review
before (24.22 KB, image/png)
2012-02-08 14:22 UTC, Christophe Croullebois
Details
single (44.45 KB, image/png)
2012-02-08 14:27 UTC, Christophe Croullebois
Details
repeated (52.58 KB, image/png)
2012-02-08 14:30 UTC, Christophe Croullebois
Details
after (23.54 KB, image/png)
2012-02-08 14:30 UTC, Christophe Croullebois
Details
the last patch with delete exceptions on a range (15.65 KB, patch)
2012-03-04 17:58 UTC, Christophe Croullebois
Details | Diff | Splinter Review
patch finalized (15.45 KB, patch)
2012-03-20 16:17 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Delete repeated holidays on a range (183.12 KB, image/jpeg)
2012-03-21 09:10 UTC, Koha Team University Lyon 3
Details
I hope the last one, patch finalised (15.37 KB, patch)
2012-03-22 12:47 UTC, Christophe Croullebois
Details | Diff | Splinter Review
this patch is finalised (16.13 KB, patch)
2012-03-23 11:14 UTC, Christophe Croullebois
Details | Diff | Splinter Review
The patch finalized after cleaning code (16.99 KB, patch)
2012-04-06 15:20 UTC, Christophe Croullebois
Details | Diff | Splinter Review
[SIGN OFF]Bug 7351 : feature that allows to delete a range of dates (17.05 KB, patch)
2012-04-10 13:50 UTC, Paul Poulain
Details | Diff | Splinter Review
Patch for minor textual changes (5.27 KB, patch)
2012-05-16 14:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
screenshot of ugly display (77.11 KB, image/png)
2012-05-22 08:56 UTC, Paul Poulain
Details
the page is ok (80.52 KB, image/png)
2012-06-01 09:07 UTC, Christophe Croullebois
Details
this is an addon after M. de Rooy's patch (7.90 KB, patch)
2012-06-01 14:17 UTC, Christophe Croullebois
Details | Diff | Splinter Review
[SIGN OFF]Bug 7351 : feature that allows to delete a range of dates (19.03 KB, patch)
2012-07-15 14:46 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Bug 7351 : feature that allows to delete a range of dates (19.06 KB, patch)
2012-07-31 08:04 UTC, Chris Cormack
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 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