326077 - cannot delete modified events from a series

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Stephan Schaefer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

It seems that the exception handling (regarding recurrence rules) is not water proof. I would expect an exception to be generated once you move a single event from a series but in fact it is not. If you later want to delete this single event an exception is generated for the original occurrence-id which does not exist anymore, at least not at the original time. So an exception is generated for the wrong instance and the changed instance survives.

Reproducible: Always

Steps to Reproduce:
1. Generate a recurring event: daily, 5 occurences
2. Move one instance using drag'n'drop in the week view to another time.
3. Right-click this changed instance and choose delete.

Actual Results:  
The instance is not deleted. But if you open the recurrences dialog of the parent (ctrl-doubleclick) you see an exception for the original instance (i.e. before moving it). Moreover, if you try to delete it again another exception at the same time is generated. 

Expected Results:  
The instance should definitely be deleted. I wonder if after moving the event, the exception list in the dialog should also contain this information. But it seems that the exception list contains only events that do not take place and not arbitrary exceptions from the rule (like a changed start/end date).

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: fixes cloning, calculateDates()

fixes calRecurrenceInfo:
- when item is set (upon cloning), the exception items need
  to be patched using this (new) base item
- calculateDates() fix:
  1. dates of exception objects are added
  2. positive recurrence items are applied
  3. finally negative recurrence items filter the list

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

> - when item is set (upon cloning), the exception items need
>   to be patched using this (new) base item

Why is this? To me. an exception sounds like it is an exception to the normal rules, so should be applied after the normal rules. Not before.

Comment 3

[Daniel Boelzle [:dbo]]

Reason is that a master's EXDATE entry (=> neg calIRecurrenceDate item) may affect an exception item being filtered out.
E.g. create a recurring event and modify one occurrence (e.g. changing its title), thus effectively creating an exception item ("different from ordinary occurrences").  If you delete that (exception) occurrence, an EXDATE will be added to its master with its RECURRENCE-ID.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #3)
> E.g. create a recurring event and modify one occurrence (e.g. changing its
> title), thus effectively creating an exception item ("different from ordinary
> occurrences").  If you delete that (exception) occurrence, an EXDATE will be
> added to its master with its RECURRENCE-ID.

Shouldn't that also delete the exception? Having an exception for a deleted occurence sounds silly.

Comment 5

[Daniel Boelzle [:dbo]]

+1
Yes, IMO removeOccurrenceAt() should also call removeExceptionFor(), cleaning up.  Currently it just appends an EXDATE item.

Comment 6

[OOO until Apr 20; Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #5)
> +1
> Yes, IMO removeOccurrenceAt() should also call removeExceptionFor(), cleaning
> up.  Currently it just appends an EXDATE item.
> 

Agreed.

Comment 7

[OOO until Apr 20; Dan Mosedale (:dmosedale, :dmose)]

After more discussion with mvl and jminta, our plan is to only allow editing the parent items in 0.1 and relnote that so that we can ship sooner.  Removing from blocker list.

Comment 8

[OOO until Apr 20; Dan Mosedale (:dmosedale, :dmose)]

The bug that actually needs to be referenced in the relnote is bug 320178; removing [cal relnote] from here.

Comment 9

[Daniel Boelzle [:dbo]]

> After more discussion with mvl and jminta, our plan is to only allow editing
> the parent items in 0.1 and relnote that so that we can ship sooner.  Removing

@dmose: This implies there won't be any exception items anymore for 0.1, right?  IMO a hard restriction. Nevertheless, it is still possible that exception items come with some ics file. In that case, they need to be properly filtered when an EXRULE/EXDATE is set at the master item.
I really don't understand why this patch won't be in, because it obviously fixes the stated problems.  Or do I miss some point?

Comment 10

[OOO until Apr 20; Dan Mosedale (:dmosedale, :dmose)]

Daniel: we've already slipped Lightning 0.1 multiple times, so we're trying very very hard to hit this end-of-February date, which means cutting things that look risky.  At this point, it seems somewhat unlikely that we'll be able to handle the various edge cases involved in exceptions in addition to the rest of the bugs in the blocker list and still ship by the end of this month.

You're right that ICS that comes from non-Lightning sources might well contain exceptions.  It turns that we've got a whole class of bugs related to not handling, roundtripping, or properly understanding various valid ICS constructs (search for bugs in the Calendar product with the keyword 'dataloss' to see them).  As a result, the (draft) release notes (at <http://wiki.mozilla.org/Calendar:Lightning:0.1:Release_Notes>) specifically recommend that users not try to edit ICS that was not generated by Lightning 0.1 just yet.

That said, all I've done for this particular bug is remove it from the blocker list, not actually forbid it from landing.  If it gets reviewed and is deemed sufficiently non-risky by the reviewer, this could potentially land before 0.1.  That said, I rather hope that most reviewers have the 0.1-blockers higher on their priority list of things to review than non-blockers.

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #9)
> I really don't understand why this patch won't be in, because it obviously
> fixes the stated problems.  Or do I miss some point?

Comments 4, 5 and 6 seems to indicate that the patch isn't the right approach. The code that tries to delete an item should be fixed, not the recurrence calculation.

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #1)
>   1. dates of exception objects are added
>   2. positive recurrence items are applied
>   3. finally negative recurrence items filter the list

In that order, if you have a recurrance rule and an exception, won't you get both?  first the exception, then the recurrence rule is applied, even if it is actually an exception.
At least with the patch, where the check for exceptions is removed from the code that adds recurrence rules.

Comment 13

[Daniel Boelzle [:dbo]]

The check is not removed.  The inflated dates (out of recurrence items) are checked whether they are already in. As the exceptions are in first, they won't be added again.

Comment 14

[Daniel Boelzle [:dbo]]

[replying to #11]

I think it is rather error-prone when clients ("code that delete items...need to be fixed") need to care whether an occurrence to be deleted is an exception or not. I would simply propose that calRecurrenceInfo's removeOccurrenceAt() also removes a potential exception object for that RECURRENCE-ID, calling removeExceptionFor() (as I have already stated in #5, not in patch).

I know that removing the exception item (previous paragraph) would fix this issue, but by reordering the date calculation the proposed way, it will be more *robust*.  In case of a provider bug, e.g. a provider has defined an exception item although the occurrence is to be filtered out by its master's EXDATE, the code still behaves correct, which IMO is to be favored.  Of course, I would like to see asserts/warnings when this situation is detected.

Please don't forget the "item setter" fix in this patch, as currently all exceptions' base item become wrong when cloning the master item.

Comment 15

[Daniel Boelzle [:dbo]]

Concerning #11:
I redeem my vote from #5: Having a once closer look at calRecurrenceInfo.js revealed that deleting an exception item when removing the occurrence seems to be wrong. There is currently the possibility to restore a once deleted occurrence, aka restoreOccurrenceAt(). This is used to manage the "exception list" in the recurring dialog.  This explains why removeOccurrenceAt() just adds an EXDATE to the info, while restoreRecurrenceAt() removes it again.  Any "exceptional" occurrence that has been defined ought to be preserved.
That said, I would say that the attached patch is the only correct fix.

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 211010 [details] [diff] [review]
fixes cloning, calculateDates()

Ok, you convinced me that this patch is right.
I missed the if (!dates.some(... line, so that's where my confusion came from.

r=mvl

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

Patch checked in.
If we decide that we won't be allowing editing of items in a recurrence set, this code won't be called, so it's safe enough to land for 0.1.
Landing now, so that we can get some testing on it, also by people who don't have a build env.

Comment 18

[Andreas Treumann]

Cecked in latest nightly build -> task is fixed and verified.