368976 - Cannot undefine alarm for an exception

Status: VERIFIED FIXED

(Calendar :: Alarms, defect)

Description

[Daniel Boelzle [:dbo]]

1. I create a recurring event series, defining an alarm on the master item.
2. I create an exception for a specific occurrence of the series *without* an alarm.
=> Although the alarm is undefined for that specific occurrence, it gets fired.

Comment 1

[Daniel Boelzle [:dbo]]

I suspect the reason is code like the below in calAlarmService.js:

            function hasAlarm(a) {
                return a.alarmOffset || a.parentItem.alarmOffset;
            }

taking the bug.

Comment 2

[Joey Minta]

I think I've written about this elsewhere, but to me, this is actually a hole in the RFC.  If a property (or in this case a component) is undefined, the RFC says to consult the parent item.  There doesn't seem to be any way to insert an anti-alarm property into an exception, in order to generate the desired behavior.  In this case, the bug is more general, you can't undefine any properties for exceptions.

Someone want to correct my understanding of the RFC?

Comment 3

[Daniel Boelzle [:dbo]]

(In reply to comment #2)
> I think I've written about this elsewhere, but to me, this is actually a hole
> in the RFC.  If a property (or in this case a component) is undefined, the RFC
> says to consult the parent item.  There doesn't seem to be any way to insert 

I have never read about that in the spec. Where is it?

> anti-alarm property into an exception, in order to generate the desired
> behavior.  In this case, the bug is more general, you can't undefine any
> properties for exceptions.

IMO an exception item states a full/complete item, so it's wrong to consult the parent item. Moreover, this also means we should correct some code in calItemBase where mIsProxy is used. Effectively a true "proxy" only holds a RECURRENCE-ID; all other data is taken from its parent, isn't it?
(proxy != exception)

Comment 4

[Matthew (lilmatt) Willis]

(In reply to comment #2)
> Someone want to correct my understanding of the RFC?

Have we posed this question to the calsify list?

Comment 5

[Daniel Boelzle [:dbo]]

(In reply to comment #4)
> Have we posed this question to the calsify list?

Excuse my ignorance, but I don't know about that list. What/where is it?

Comment 6

[Matthew (lilmatt) Willis]

Check out:
http://lists.osafoundation.org/mailman/listinfo/ietf-calsify

Comment 7

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

I don't see anything in the RFC that indicates that the parent item must be considered. So unless somebody shows me why it must be done, I think it is not the way to go.

Comment 8

[Stefan Sitter [:ssitter]]

This is apparently a follow up bug for Bug 329985 Comment #10.

Comment 9

[Daniel Boelzle [:dbo]]

IMO it's worth to review the current base code regarding this; proposing for 0.7.

Comment 10

[cmtalbert]

Marking blocking for 0.7.  The RFC has gone to final draft, so it should be pretty easy to resolve this issue.  But, if this turns into a giant amount of work, this should probably be timeboxed out of 0.7

Comment 11

[Daniel Boelzle [:dbo]]

Attachment: first cut

calItemBase's mIsProxy flag determines whether getProperty et al fall back to their parent item and some other explicit places do the same like calAlarmService, probing for alarms.
This patch:
- removes enforcement of mIsProxy if any parentItem is set
- assures a deep copy in cloneItemBaseInto() which is called by cloneShallow() which called by calRecurrenceInfo.modifyException (and other)
- refurbishes calItemBase.propertyEnumerator which caused exception during when called from cloneItemBaseInto()
- shifts alarmOffset fallback mimic to calItemBase

Works good for me, although only *rudimentarily* tested. IMO this patch is likely to cause regressions; the impact is quite hard to foresee. Thus I'd appreciate more testing. I don't think it makes sense to put it on review before.

Comment 12

[Daniel Boelzle [:dbo]]

This is a good candidate for regressions, but the bug is IMO not that urging. Thus this won't stop 0.7, but should be done ASAP post 0.7.

Comment 13

[Simon Paquet [:sipaq]]

Not going to happen for 0.8

Comment 14

[Daniel Boelzle [:dbo]]

Attachment: patch - v1

This patch fixes
- buggy cloning of overridden items and proxies (changes to a proxy or overridden got lost) => I suspect the foremost reason for bug 427276.
- fixes property modifications the way that undefined properties are tracked and overridden items (exceptions) are properly serialized in their full form (this bug and bug 402539)
- removes the 'unproxied' API form calIItemBase, because it must not be used.
- shifts createProxy() to internal API, because it's an optimization detail of calRecurrenceInfo.
- fixes alarmOffset and parent lookup of that property (this bug and bug 381998)
- some code cleanup that I stumbled over revisiting getProperty et al

I changed cloning the way that occurrences no longer also clone their parent item and recurrence info and all overridden/exceptional items. I see no need for that, because modification of the parent (entering the overridden item etc) could be postponed until the item is actually stored (-> providers calling modifyException). Moreover we can't rely on having a parent at all (bug 357399), e.g. from iTIP invitations (yet not fixed). Further I enhanced modifyException to allow taking over ownership of the passed item. All this could bring us some performance.

I did some rudimentary tests with storage, ics, wcap and have run the unit tests. Since these changes are good candidates for regressions (and gdata and caldav tests are missing), volunteers are welcome for further testing! thanks!

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 318561 [details] [diff] [review]
patch - v1

>-          if ((item.alarmOffset || item.parentItem.alarmOffset) &&
>-              getPrefSafe("calendar.alarms.indicator.show", true)) {
>+          if (item.alarmOffset && getPrefSafe("calendar.alarms.indicator.show", true)) {
So this means that if an alarm is set on the parent item, it will not be displayed unless the item is a proxy?

>-  void modifyException (in calIItemBase anItem);
>+  void modifyException (in calIItemBase anItem, in boolean aTakeOverOwnership);
Comments about the parameters, especially the second would be nice.

>+    clone: function () {
>+        return this.cloneShallow(this.mParentItem);
>+    },
Cloning returns a shallow copy of the parent item? What if mParentItem is null ?

>-        this.mProperties.setProperty(aName.toUpperCase(), aValue);
>+        if (aValue === null) {
>+            this.deleteProperty(aName);
>+        } else {
>+            this.mProperties.setProperty(aName.toUpperCase(), aValue);
>+        }
is null === undefined? If not we might also want to cover that case.


>-    get icalComponent() {
>-        throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>-    },
>-
Why just get rid of the icalComponent? If its because its implemented in the event and todo classes, I think we should keep the icalComponent getter, to make clear that it needs to be reimplemented, and was not forgotten.

>-        // the item must be an occurrence
>-        if (anItem.parentItem == anItem)
>-            throw Components.results.NS_ERROR_UNEXPECTED;
>-
Why can the item now also be a parent item?

>+                ex = ex.clone();
>+                // xxx todo: isn't the below questionable w.r.t DST changes?
>                 ex.recurrenceId.addDuration(timeDiff);
That depends on if subtractDate works correctly or not.

>+    getProperty_: function cpb_getProperty(aName) {
>+        return this.mData[aName];
>+    },
>     getProperty: function cpb_getProperty(aName) {
>         var aValue = this.mData[aName];
>         if (aValue === undefined) {
>             aValue = null;
>         }
>         return aValue;
>     },
Why do we need this?

>Index: calendar/providers/gdata/components/calGoogleCalendar.js
I know this change is quite minimal and it won't be too much work to merge that change, but since the gdata interfaces patch consists of a small number of hunks with many lines, making it harder to merge, it would be great if we could get the interfaces checked in soon.


>-                item.parentItem.recurrenceInfo.modifyException(item);
>+                item.parentItem.recurrenceInfo.modifyException(item, true);
Why do you take over ownership for gdata, and not for caldav and parts of storage? Could be answered with IDL comment for aTakeoverOwnership


>+        // xxx todo: why?
>+//         var rec = aItem.recurrenceInfo;
>+//         if (rec) {
>+//             var exceptions = rec.getExceptionIds({});
>+//             for each (var exid in exceptions) {
>+//                 var exception = rec.getExceptionFor(exid, false);
>+//                 if (exception) {
>+//                     if (!exception.isMutable) {
>+//                         exception = exception.clone();
>+//                     }
>+//                     exception.calendar = this.superCalendar;
>+//                     rec.modifyException(exception);
>+//                 }
>+//             }
>+//         }
I'm not sure. It seems the calendar is being changed to the supercalendar, but I don' see why its needed. We should find out before checking in though.


r=philipp

Comment 16

[Daniel Boelzle [:dbo]]

(In reply to comment #15)
> (From update of attachment 318561 [details] [diff] [review])
> >-          if ((item.alarmOffset || item.parentItem.alarmOffset) &&
> >-              getPrefSafe("calendar.alarms.indicator.show", true)) {
> >+          if (item.alarmOffset && getPrefSafe("calendar.alarms.indicator.show", true)) {
> So this means that if an alarm is set on the parent item, it will not be
> displayed unless the item is a proxy?
...or the occurrence is not a proxy (but an exceptional/overridden item), and has an alarmOffset set.

> >+    clone: function () {
> >+        return this.cloneShallow(this.mParentItem);
> >+    },
> Cloning returns a shallow copy of the parent item? What if mParentItem is null
> ?
No, the parent item just get's passed.

> >-        this.mProperties.setProperty(aName.toUpperCase(), aValue);
> >+        if (aValue === null) {
> >+            this.deleteProperty(aName);
> >+        } else {
> >+            this.mProperties.setProperty(aName.toUpperCase(), aValue);
> >+        }
> is null === undefined? If not we might also want to cover that case.
We only need to cover IDL types; thus it's illegal to call setProperty with undefined.

> >-    get icalComponent() {
> >-        throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> >-    },
> >-
> Why just get rid of the icalComponent? If its because its implemented in the
> event and todo classes, I think we should keep the icalComponent getter, to
> make clear that it needs to be reimplemented, and was not forgotten.
Wouldn't we run into errors either way, even without explicitly throwing NOT_IMPL? What's the benefit?

> >-        // the item must be an occurrence
> >-        if (anItem.parentItem == anItem)
> >-            throw Components.results.NS_ERROR_UNEXPECTED;
> >-
> Why can the item now also be a parent item?
I want to get rid of these kind of checks, because we need to cope with parent-less items in the future. The check whether the item has a RECURRENCE-ID fits better.

> >+                ex = ex.clone();
> >+                // xxx todo: isn't the below questionable w.r.t DST changes?
> >                 ex.recurrenceId.addDuration(timeDiff);
> That depends on if subtractDate works correctly or not.
No, I think not. If DTSTART has a timezone with DST specified, and an occurrence falls right before the DST boundary, the patched RECURRENCE-ID would be wrong, isn't it?

> >+    getProperty_: function cpb_getProperty(aName) {
> >+        return this.mData[aName];
> >+    },
> >     getProperty: function cpb_getProperty(aName) {
> >         var aValue = this.mData[aName];
> >         if (aValue === undefined) {
> >             aValue = null;
> >         }
> >         return aValue;
> >     },
> Why do we need this?
I need to distinguish null-properties form undefined ones (for the property enumerator).

> >Index: calendar/providers/gdata/components/calGoogleCalendar.js
> I know this change is quite minimal and it won't be too much work to merge that
> change, but since the gdata interfaces patch consists of a small number of
> hunks with many lines, making it harder to merge, it would be great if we could
> get the interfaces checked in soon.
Yes, I promise I'll do that review right start of next week, and the other reviews, too. Sorry, folks... :/

> >-                item.parentItem.recurrenceInfo.modifyException(item);
> >+                item.parentItem.recurrenceInfo.modifyException(item, true);
> Why do you take over ownership for gdata, and not for caldav and parts of
> storage? Could be answered with IDL comment for aTakeoverOwnership
We could let calRecurrenceInfo take over the exception item in case the provider has a freshly parsed instance it would get rid of anyway. I'll add some comments.

> >+        // xxx todo: why?
> >+//         var rec = aItem.recurrenceInfo;
> >+//         if (rec) {
> >+//             var exceptions = rec.getExceptionIds({});
> >+//             for each (var exid in exceptions) {
> >+//                 var exception = rec.getExceptionFor(exid, false);
> >+//                 if (exception) {
> >+//                     if (!exception.isMutable) {
> >+//                         exception = exception.clone();
> >+//                     }
> >+//                     exception.calendar = this.superCalendar;
> >+//                     rec.modifyException(exception);
> >+//                 }
> >+//             }
> >+//         }
> I'm not sure. It seems the calendar is being changed to the supercalendar, but
> I don' see why its needed. We should find out before checking in though.
Yes.

> r=philipp
thanks for reviewing that promptly, philipp!

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

> > So this means that if an alarm is set on the parent item, it will not be
> > displayed unless the item is a proxy?
> ...or the occurrence is not a proxy (but an exceptional/overridden item), and
> has an alarmOffset set.
But isn't it a valid case that I would like to set a general alarm for all instances, on the parent item? Shouldn't this alarm be displayed?

> > Why just get rid of the icalComponent? If its because its implemented in the
> > event and todo classes, I think we should keep the icalComponent getter, to
> > make clear that it needs to be reimplemented, and was not forgotten.
> Wouldn't we run into errors either way, even without explicitly throwing
> NOT_IMPL? What's the benefit?
Showing this in the source gives developers the hint that its not implemented. If its just not there, I would probably try to use it first and then find out that its not implemented. The same happened to me with previousOccurrence and nextOccurrence. I wrote half of the method I wanted to implement without knowing its not implemented. Would NS_ERROR_NOT_IMPLEMENTED be written in calItemBase, I would have noticed it some other time before and would have known its not implemented.


> 
> > >-        // the item must be an occurrence
> > >-        if (anItem.parentItem == anItem)
> > >-            throw Components.results.NS_ERROR_UNEXPECTED;
> > >-
> > Why can the item now also be a parent item?
> I want to get rid of these kind of checks, because we need to cope with
> parent-less items in the future. The check whether the item has a RECURRENCE-ID
> fits better.
In that case, should you not check if (!aItem.recurrenceId) here? Also, we should probably get rid of them everywhere? If you don't want to do that here, go ahead and file a followup bug. 

> 
> > >+                ex = ex.clone();
> > >+                // xxx todo: isn't the below questionable w.r.t DST changes?
> > >                 ex.recurrenceId.addDuration(timeDiff);
> > That depends on if subtractDate works correctly or not.
> No, I think not. If DTSTART has a timezone with DST specified, and an
> occurrence falls right before the DST boundary, the patched RECURRENCE-ID would
> be wrong, isn't it?
Well, the patching uses subtractDate, so it kind of depends on if subtractDate gives you the right difference between the boundaries. Its possible though that I don't quite understand the issue. In any case, it should be investigated. Please file a followup bug.

> > >+    getProperty_: function cpb_getProperty(aName) {
> > Why do we need this?
> I need to distinguish null-properties form undefined ones (for the property
> enumerator).
Sounds good. A comment on that function would be great though.

> thanks for reviewing that promptly, philipp!
Sure, no problem :-)

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #17)
> > > So this means that if an alarm is set on the parent item, it will not be
> > > displayed unless the item is a proxy?
> > ...or the occurrence is not a proxy (but an exceptional/overridden item), and
> > has an alarmOffset set.
> But isn't it a valid case that I would like to set a general alarm for all
> instances, on the parent item? Shouldn't this alarm be displayed?
I think you get me wrong: In case the item is a proxy it inherits its alarmOffset from its parentItem. In case it's an individual item (non-parent or exceptional/overridden), it governs it's own alarmOffset. If an exceptional/overridden item is created, the alarm setting is taken over.

> > > Why just get rid of the icalComponent? If its because its implemented in the
> > > event and todo classes, I think we should keep the icalComponent getter, to
> > > make clear that it needs to be reimplemented, and was not forgotten.
> > Wouldn't we run into errors either way, even without explicitly throwing
> > NOT_IMPL? What's the benefit?
> Showing this in the source gives developers the hint that its not implemented.
> If its just not there, I would probably try to use it first and then find out
> that its not implemented. The same happened to me with previousOccurrence and
> nextOccurrence. I wrote half of the method I wanted to implement without
> knowing its not implemented. Would NS_ERROR_NOT_IMPLEMENTED be written in
> calItemBase, I would have noticed it some other time before and would have
> known its not implemented.
Ok, it's even a bit better than just placing a comment.

> > > >-        // the item must be an occurrence
> > > >-        if (anItem.parentItem == anItem)
> > > >-            throw Components.results.NS_ERROR_UNEXPECTED;
> > > >-
> > > Why can the item now also be a parent item?
> > I want to get rid of these kind of checks, because we need to cope with
> > parent-less items in the future. The check whether the item has a RECURRENCE-ID
> > fits better.
> In that case, should you not check if (!aItem.recurrenceId) here? Also, we
> should probably get rid of them everywhere? If you don't want to do that here,
> go ahead and file a followup bug. 
That check is still in; a few lines below.

> > > >+                ex = ex.clone();
> > > >+                // xxx todo: isn't the below questionable w.r.t DST changes?
> > > >                 ex.recurrenceId.addDuration(timeDiff);
> > > That depends on if subtractDate works correctly or not.
> > No, I think not. If DTSTART has a timezone with DST specified, and an
> > occurrence falls right before the DST boundary, the patched RECURRENCE-ID would
> > be wrong, isn't it?
> Well, the patching uses subtractDate, so it kind of depends on if subtractDate
> gives you the right difference between the boundaries. Its possible though that
> I don't quite understand the issue. In any case, it should be investigated.
> Please file a followup bug.
I've filed a spin-off bug 432417.

Comment 19

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Comment 20

[Andreas Treumann]

Checked in lightning build 2008050618 and sunbird 20080506 -> VERIFIED.

Comment 21

[Daniel Boelzle [:dbo]]

I just want to remind everybody that until this fix people have written incomplete ics data (dataloss). This had only be obvious when using other calendar apps or servers.

So this is probably worth relnoting. Opinions?

Comment 22

[Simon Paquet [:sipaq]]

> I just want to remind everybody that until this fix people have written
> incomplete ics data (dataloss). This had only be obvious when using other
> calendar apps or servers.
> 
> So this is probably worth relnoting. Opinions?

We normally just relnote unfixed bugs. Why should we move away from that?