389251 - Cannot dismiss alarm on single overridden instance of recurring item

Status: VERIFIED FIXED

(Calendar :: Alarms, defect)

Description

[Daniel Boelzle [:dbo]]

1. Create a recurring event.
2. Define an alarm on a *single* instance (only) and let it fire.
=> alarm cannot be dismissed.

The current alarm code always saves the last ack stamp at the *parent* item. Due to the inherent saving of X-MOZ-LASTACK inside the VALARM component, you cannot save X-MOZ-LASTACK without an alarm defined.

Comment 3

[Daniel Boelzle [:dbo]]

Going on vacation, I won't have time for this; this is wanted for 0.8 though.

Comment 6

[Daniel Boelzle [:dbo]]

Attachment: fix

Comment 7

[Daniel Boelzle [:dbo]]

The fix moves X-MOZ-LASTACK property from the inner VALARM to the VEVENT/VTODO component.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

What happens to existing alarms with the change? I fear that this will force everyone upgrading from 0.8 to 0.9 to dismiss their already dismissed alarms.

Also, do we really need to force the lastack timezone to be utc? Are there cases where this might cause a 1 hr offset on a DST boundary or such? If we do need to force this, then I'd suggest either throwing something if the timezone is not UTC, or just converting the date to UTC. 

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
> What happens to existing alarms with the change? I fear that this will force
> everyone upgrading from 0.8 to 0.9 to dismiss their already dismissed alarms.
Yes I know, this might happen for recent alarms (on ics calendars, not storage). But I really don't think we should move over the old alarm lastAcks. This adds more code and cycles on parsing for little outcome: users just have to dismiss their recent reminders once again.

> Also, do we really need to force the lastack timezone to be utc? Are there
> cases where this might cause a 1 hr offset on a DST boundary or such? If we do
> need to force this, then I'd suggest either throwing something if the timezone
> is not UTC, or just converting the date to UTC. 
The lastAck stamp is saved as ical string (either floating or UTC). I think documenting this at calIItemBase is OK for now, since there's only code storing UTC, thus no bug yet. However, we might consider ensuring this implementing getters and setters, then I'd prefer your second options (coercion to UTC).

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #9)
> (In reply to comment #8)
> > What happens to existing alarms with the change? I fear that this will force
> > everyone upgrading from 0.8 to 0.9 to dismiss their already dismissed alarms.
> Yes I know, this might happen for recent alarms (on ics calendars, not
> storage). But I really don't think we should move over the old alarm lastAcks.
> This adds more code and cycles on parsing for little outcome: users just have
> to dismiss their recent reminders once again.
I know, being backward compatible is a bitch. But you remember how users complained after their calendars were not visible after the update, and there it was also "just checking the checkbox again".

> 
> > Also, do we really need to force the lastack timezone to be utc? Are there
> > cases where this might cause a 1 hr offset on a DST boundary or such? If we do
> > need to force this, then I'd suggest either throwing something if the timezone
> > is not UTC, or just converting the date to UTC. 
> The lastAck stamp is saved as ical string (either floating or UTC). I think
> documenting this at calIItemBase is OK for now, since there's only code storing
> UTC, thus no bug yet. However, we might consider ensuring this implementing
> getters and setters, then I'd prefer your second options (coercion to UTC).
Yes, please.

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > What happens to existing alarms with the change? I fear that this will force
> > > everyone upgrading from 0.8 to 0.9 to dismiss their already dismissed alarms.
> > Yes I know, this might happen for recent alarms (on ics calendars, not
> > storage). But I really don't think we should move over the old alarm lastAcks.
> > This adds more code and cycles on parsing for little outcome: users just have
> > to dismiss their recent reminders once again.
> I know, being backward compatible is a bitch. But you remember how users
> complained after their calendars were not visible after the update, and there
> it was also "just checking the checkbox again".
I think that's slightly different, because users were missing their calendars at that time (and IMO another indication that the visibility check isn't recognized as such, but that's another story). W.r.t. alarms they get the recent ones from ics again, and I think that's OK and don't reason to further complicate the code.

> > > Also, do we really need to force the lastack timezone to be utc? Are there
> > > cases where this might cause a 1 hr offset on a DST boundary or such? If we do
> > > need to force this, then I'd suggest either throwing something if the timezone
> > > is not UTC, or just converting the date to UTC. 
> > The lastAck stamp is saved as ical string (either floating or UTC). I think
> > documenting this at calIItemBase is OK for now, since there's only code storing
> > UTC, thus no bug yet. However, we might consider ensuring this implementing
> > getters and setters, then I'd prefer your second options (coercion to UTC).
> Yes, please.
Hey, that's just been a suggestion I've done while reading the code; it's off-topic to this bug :-) I think I've done an improvement in this field by adding a simple comment to IDL.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #11)
> I think that's slightly different, because users were missing their calendars
> at that time (and IMO another indication that the visibility check isn't
> recognized as such, but that's another story). W.r.t. alarms they get the
> recent ones from ics again, and I think that's OK and don't reason to further
> complicate the code.
Since the alarms are only from the past weeks, I guess its ok. I didn't think about that. We should add a release note though.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 322799 [details] [diff] [review]
fix

One thing that comes to mind is that the currently unused calIAlarm interface also allows setting lastAck on the VALARM. If this is to be changed also, then setting lastAck depends on the item being set, or the base item needs to keep the method to set the alarm last ack.

Great bonus points for also fixing calIAlarm, although I won't r- based on that.

r=philipp

Comment 14

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> > UTC, thus no bug yet. However, we might consider ensuring this implementing
> > getters and setters, then I'd prefer your second options (coercion to UTC).
> Yes, please.
I did that.

(In reply to comment #13)
> Great bonus points for also fixing calIAlarm, although I won't r- based on
> that.
I think we should fix the yet unused calAlarm in another bug; I've opened bug 437196.

Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED.

Comment 15

[Andreas Treumann]

Checked issue with nightly build 2008073118 and sunbird 20080731 -> VERIFIED

Comment 16

[Simon Paquet [:sipaq]]

Daniel, I need a good description of the open issue with regards to the reappearing old alarms for the release notes (see comment 11 and comment 23). 

Any suggestions?

Comment 17

[Daniel Boelzle [:dbo]]

Maybe: "It may happen that you have to dismiss previous alarms set on recurring instances again.". But to be honest, I don't think it's really relnote-worthy.

Comment 18

[Simon Paquet [:sipaq]]

(In reply to comment #17)
> But to be honest, I don't think it's really relnote-worthy.

I take your word for it :)

Comment 19

[Matt]

Attachment: Special Event Instance Alarm Example ICS

I think there might still be an issue related to this (as of build 2008090218).  I have a recurring event with an alarm (Mondays at 10am).  An instance of this event has been moved to the following day (Tuesday), and the alarm for that instance will not dismiss/dismiss all.  I have to close the reminder box using the X button, but the reminder keeps popping up a few minutes later (I'm assuming it's snoozing). I've even tried deleting the instance, editing the ICS file to delete the EXDATE under the event, and then creating the special instance (moving it to Tuesday) again in Lightning just to make sure it didn't get corrupted somehow.  There's other special instances earlier than this one if that matters.  I'm attaching a sample ICS file that has the problem.

Comment 20

[manager58013421]

I get very similar problems dismissing alarms on a shared gcal using google calendar add on and private XML address.  My recurring event reminders can't be dismissed or snoozed  ....  Never used a bug service, what should I do?

Comment 21

[Daniel Boelzle [:dbo]]

File a bug against gdata-provider, but please leave the blocking flag to release drivers.

Comment 22

[manager58013421]

Just to add some info i found.
Since Dismiss, Dismiss All, Snooze don't do anything and don't produce and error, I tried to turn off the reminder for the individual task.

I clicked details on the reminder pop up.  I edit all instances of recurring item.
I set reminder to none. and save and close.

Now the error console had this:

Error: aItem.calendar has no properties
Source File: chrome://calendar/content/calendar-item-editing.js
Error: aItem.calendar has no properties
Source File: chrome://calendar/content/calendar-item-editing.js
Line: 476
Error: [Exception... "'[JavaScript Error: "aItem.calendar has no properties" {file: "chrome://calendar/content/calendar-item-editing.js" line: 476}]' when calling method: [calIOperationListener::onOperationComplete]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///D:/Program%20Files/Calendar/components/calItemModule.js -> file:///D:/Program%20Files/Calendar/js/calTransactionManager.js :: cT_onOperationComplete :: line 144"  data: yes]
Line: 476


How should I report this bug?

Comment 23

[Stefan Sitter [:ssitter]]

(In reply to comment #20)
This seems to be a known issue for shared Google Calendars, see Bug 424185. According to the last comment it might be fixed using Lightning 0.9 and Provider for Google Calendar 0.5 (not yet released.)

Comment 24

[ubuntumuntu]

This issue still affects me - been extremely annoying for a long while now on multiple OS's (Mac & Linux)

Mac 10.6.8
Ubuntu 12.04.1 64-bit
TB 15.0
Lightning 1.7
Provider 0.16

I subscribe to a number of my Cal's (across several Google accounts); they're shared privately server-side, but *none* of them are either cached or read-only.

I've looked into some suggestions found online, but I think this is essentially a long-running client-side bug.
http://www.consumingexperience.com/2009/01/thunderbird-reminders-keep-popping-up.html