329985 - alarms on recurring items don't really work

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Nidheesh Dubey]

Double click an open an even from agenda pane for any event that has alarm set to go off say 30 min before event. The event detail does not show this alarm info though calendar event shows it. Weird since I would imagine both to be pulling the info from same store.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I can reproduce this; it happens only on recurring events.  Furthermore, it happens in the views too, but if you edit the individual occurrence and not all events.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

This is a side-effect of not using makeMemberAttr for alarmOffset and friends.  Requests for properties generated with makeMemberAttr pass through to the parent if they are not present on the proxy by virtue of using explicit getter and setter methods.  Just using makeMemberAttr is not enough to fix this, however, as that only does pass-through on things that are in the property bag, which the alarm stuff is not.  

The possible fixes that I can think of seem relatively risky to me, so at this point, my inclination is to relnote this for 0.1.  This is unfortunate, since it means that any exceptions created via the event dialog will lose their alarm.

I am still not clear on why the calAlarmService seems to fire alarms for occurrences despite the existence of this bug, so my understanding of whats going on here is not yet complete.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

After discussion with Joey, we've concluded that in addition to the makeMemberAttr problem, it's also the case the onAddItem incorrectly uses the parent rather than the occurrences to fire alarms.  This will really be solved when the current infrastructure is replaced with calIAlarm.

Comment 4

[Joey Minta]

Attachment: occurrences and parents

There are 2 parts to this patch.  First, we need to expand items to occurrences, if a new recurring item is added. Second, we need to check both the item and its parent for alarm information, since those properties aren't automatically forwarded on to child-items.

Comment 5

[Matthew (lilmatt) Willis]

Comment on attachment 232653 [details] [diff] [review]
occurrences and parents

"jminta: any time you have 2 instances of the same parent with alarms in the same 6hr window you're in trouble"

r=lilmatt with a followup filed to fix the above case per our IRC discussion (and remove the dumps)

Comment 6

[Joey Minta]

Comment on attachment 232653 [details] [diff] [review]
occurrences and parents

+            if (!aItem.alarmOffset &&
+                !(aItem.parentItem && aItem.parentItem.alarmOffset)) {
+                return;
+            }
dmose: this can be factored into a helper function.
jminta: this will need to be done for snooze, since there will be extra checks.
dmose: ok.

+                start.month -= 2;
Comment here, or at least somewhere useful.

+        var offset = aItem.alarmOffset;
+        if (!offset) {
+            offset = aItem.parentItem.alarmOffset;
+        }
This should just be an ||.

+            var lastAck = aItem.alarmLastAck || aItem.parentItem.alarmLastAck
semicolon would be nice.

r=dmose with that.

Comment 7

[Joey Minta]

patch checked in.

Comment 8

[Daniel Boelzle [:dbo]]

(In reply to comment #6)
> (From update of attachment 232653 [details] [diff] [review] [edit])
> +            if (!aItem.alarmOffset &&
> +                !(aItem.parentItem && aItem.parentItem.alarmOffset)) {
> +                return;
> +            }
> dmose: this can be factored into a helper function.

Why not implement this right in calItemBase once for all, like proxied items do for attendees, organizer etc...?

Comment 9

[Joey Minta]

(In reply to comment #8)
> Why not implement this right in calItemBase once for all, like proxied items do
> for attendees, organizer etc...?
> 
You're welcome to write that patch.  mvl also mentioned he may want to refactor calItemBase to make it a bit more sane.  I don't know if the intended to address this issue or not in that patch.

Comment 10

[Daniel Boelzle [:dbo]]

(In reply to comment #9)
> You're welcome to write that patch.  mvl also mentioned he may want to refactor
> calItemBase to make it a bit more sane.  I don't know if the intended to
> address this issue or not in that patch.

Thinking further about that "alarmOffset = item.alarmOffset || item.parentItem.alarmOffset" mimic, I think that it is a bug: People might want to define exception items explicitly switching off the alarm. Currently they will get the parent's alarm setting.
So the fix IMO tends to be in calItemBase::initializeProxy(): Setting the parent's alarmOffset, alarmRelated[, alarmLastAck] and reviewing all code with the stated mimic.
What do you think?

Comment 11

[Stefan Sitter [:ssitter]]

Bug 368976 was filed apparently for the issue mentioned in Comment #10.