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.
Created attachment 7459 [details] [review] Delete on a range of dates You have two options : Delete the single holidays on a range Delete the repeated holidays on a range The exceptions are not deleted. Need deep testing. Thx
I was able to use this to delete a range of repeated yearly holidays but not repeated single holidays or repeated weekly holidays.
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
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...
Created attachment 7502 [details] before
Created attachment 7503 [details] single
Created attachment 7504 [details] repeated
Created attachment 7505 [details] after
(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.
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
(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.
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.
Created attachment 8005 [details] [review] the last patch with delete exceptions on a range I think I have finished the job. Thanks for testing.
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
Created attachment 8370 [details] [review] patch finalized Corrected, it's ok now. Please retest. Thx
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)
Created attachment 8382 [details] Delete repeated holidays on a range
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.
Created attachment 8506 [details] [review] I hope the last one, patch finalised I hope no more bugs. Needs hard testing. thank you in advance
Created attachment 8585 [details] [review] this patch is finalised correct a bug if no "from date" given and "Holidays on a range" selected. Now we don't have a page with "koha error", just the code do nothing and you see it on the calendar. Thx Leila for your tests.
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.
Created attachment 8964 [details] [review] The patch finalized after cleaning code The same after cleaning the code. Could you sign off again Leila ? Thank you in advance.
Created attachment 9075 [details] [review] [SIGN OFF]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>
test in a sandbox : it works we can create and delete holidays on a range.
*** Bug 7964 has been marked as a duplicate of this bug. ***
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
Created attachment 9621 [details] [review] Patch for minor textual changes
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
Created attachment 9691 [details] screenshot of ugly display
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
Created attachment 9869 [details] the page is ok
I have seen with Paul, maybe due to his hook git... Tested with two others PC. thx
Created attachment 9882 [details] [review] this is an addon after M. de Rooy's patch As stated in patch comment, I have just put a space between the radio button and the text for the new lines.. And I have replaced tabulations by space. You must have applied before : "[SIGN OFF]Bug 7351 : feature that allows to delete a range of dates" "Patch for minor textual changes"
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)
Paul, too much time has passed, the calendar has changed, I have to rewrite code.
Created attachment 10862 [details] [review] [SIGN OFF]Bug 7351 : feature that allows to delete a range of dates Ok Paul, now it must be ok. I hope... ;o) Just one patch, it's enough.
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
(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'
please sign-off ;o( thx
Colin, do you have any thoughts about this and Koha::Calendar?
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.
Cool, in that case im happy to sign off, will do that tonight.
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>
Depending on tests, code looks good to me. Passed QA
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 :(((
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.
Works like a charm. However, i've discovered some problems under plack, i'll open another bug for that. Patch pushed to master