Closed Bug 554267 Opened 14 years ago Closed 13 years ago

Deleting a single item of an recurring event, does not delete the alarm of this event.

Categories

(Calendar :: Alarms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: software.architekt, Assigned: mmecca)

References

Details

(Whiteboard: [needed beta][no l10n impact])

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2 GTBDFff GTB7.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.1.8) Gecko/20100227 Lightning/1.0b1 Thunderbird/3.0.3

Deleting a single item of an recurring event, does not delete the alarm of this event.
After the alarm occurs, you can't quit or snooze the alarm.
You have to restart Thunderbird to get rid of this alarm.

Reproducible: Always
Confirming - this happens with repeating tasks also if an occurrence is completed before the alarm is set to fire. Errors:

Error: An error occurred when writing to the calendar test! Error code: MODIFICATION_FAILED. Description: 
Source File: resource://calendar/modules/calUtils.jsm -> file:///D:/mozilla/objdir-comm-central/mozilla/dist/xpi-stage/lightning/calendar-js/calCalendarManager.js
Line: 1035

Error: uncaught exception: [Exception... "'<error>' when calling method: [calICalendar::modifyItem]"  nsresult: "0x804a0002 (<unknown>)"  location: "JS frame :: resource://calendar/modules/calUtils.jsm -> file:///D:/mozilla/objdir-comm-central/mozilla/dist/xpi-stage/lightning/calendar-js/calAlarmService.js :: cAS_snoozeAlarm :: line 233"  data: no]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Matthew, do you want to look in to fixing this bug?
Assignee: nobody → matthew.mecca
OS: Windows 7 → All
Hardware: x86_64 → All
Attached patch [checked in] fix v1 (obsolete) β€” β€” Splinter Review
Makes sure all old alarms are removed when an occurrence is modified/deleted.
Attachment #523805 - Flags: review?(philipp)
Status: NEW → ASSIGNED
Comment on attachment 523805 [details] [diff] [review]
[checked in] fix v1

Looks good, r=philipp
Attachment #523805 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/1384d22c46ec>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to comm-miramar <http://hg.mozilla.org/releases/comm-miramar/rev/ef46f99a2684>
Target Milestone: Trunk → 1.0b4
Backed out on comm-central rev 7dab38a4d5ce and comm-miramar revs cb5678d306db and db993ddcd0b8, this breaks alarms and creates an empty alarm window (see bug 668450)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #523805 - Attachment description: fix v1 → [checked in] fix v1
The root cause of this is that in some cases timers are never removed - when modifications to repeating items are made on the parent item, the occurrences are not considered when removing old alarms and their associated timers. The problem with the first fix is that the alarm monitor and dialogs sometimes rely on this to handle display and updating when there are multiple occurrences of repeating items with unack'd alarms.

While working on this I stumbled across a few other issues related to how the alarm dialog handles repeating items (see bug 496897). While not totally necessary to fix this bug, I think the more comprehensive fix would be to move batch alarm handling (ie snooze all / dismiss all with multiple entries in the dialog) into calAlarmService.
Attached patch WIP fix v2 (obsolete) β€” β€” Splinter Review
This work in progress patch adds batch alarm handling into calAlarmService, which I think will make alarm handling on repeating items more reliable, and rely less on the alarm dialog for snoozing/dismissing multiple occurrences.

One effect of this is that multiple occurrences can be snoozed together if there are several missed occurrences in the dialog. For example, if there are 2 events set to repeat daily, and lightning isn't started for 3 days, we will get 6 reminders on next startup. With this patch applied, we can select Snooze All for 5 minutes, and in 5 minutes all 6  reminders will pop up again. Based on the discussion in Bug 397030 I'm not sure if we want this behavior, or if it would be better to just allow one occurrence to snooze and have a single alarm fire per event.

I'd appreciate some feedback before I go further with this approach.
Attachment #523805 - Attachment is obsolete: true
Attachment #547974 - Flags: feedback?(philipp)
Comment on attachment 547974 [details] [diff] [review]
WIP fix v2

Yes, I think this goes in the right direction. Sorry for the late late feedback, but we really want this for 1.0 (this fixes bug 496897, right?). Do you think you could finish this patch in the next week?
Attachment #547974 - Flags: feedback?(philipp) → feedback+
Flags: blocking-calendar1.0+
Whiteboard: [needed beta][no l10n impact]
(In reply to Philipp Kewisch [:Fallen] from comment #13)
> Comment on attachment 547974 [details] [diff] [review] [diff] [details] [review]
> WIP fix v2
> 
> Yes, I think this goes in the right direction. Sorry for the late late
> feedback, but we really want this for 1.0 (this fixes bug 496897, right?).
> Do you think you could finish this patch in the next week?

If we want to fix this bug for 1.0 I think it would be safer to move the batch handling parts to another bug (that's the part that addresses bug 496897 and a few others). I'm confident that it's the right approach, but it has been difficult to test for all the edge cases and has proven to be regression prone - this already bit us on one release, I'd hate to see a repeat for 1.0.
Attached patch Fix v3 (obsolete) β€” β€” Splinter Review
Fixes alarm removal on repeating items (without batch alarm handling). 

I've been testing this version for a few weeks on my production machine without any noticeable regressions, but please test to make sure I haven't missed anything.
Attachment #547974 - Attachment is obsolete: true
Attachment #567661 - Flags: review?(philipp)
Comment on attachment 567661 [details] [diff] [review]
Fix v3

Review of attachment 567661 [details] [diff] [review]:
-----------------------------------------------------------------

I would love to prevent users from getting an exception when they click dismiss all, this is one of the sharp edges that is hard for users to accept. I think I'm not quite clear what specific issues this patch vs the patch with batch handling would fix. Maybe you could explain this to me in more detail.

Testing the patch I noticed the following issues, please excuse if they can only be fixed with the batch handling part:

* Clicking Snooze All doesn't work at all for me (no error messages)
* Clicking Dismiss All still throws a conflict dialog for caldav and an error message for storage

What does work for me, at least what I specifically tested:

* Clicking Dismiss or Snooze on one item removes all of them from the dialog
* Dismissing a single instance of a recurring event and a single event works fine with dismiss all

r- for now until its clear what we want :-)

The code itself looks fine, aside from:

::: calendar/base/src/calAlarmService.js
@@ +583,5 @@
>      alarmFired: function cAS_alarmFired(aItem, aAlarm) {
>          if (!aItem.calendar.getProperty("suppressAlarms") &&
>              !aItem.calendar.getProperty("disabled") &&
>              aItem.getProperty("STATUS") != "CANCELLED") {
> +            cal.LOG("[calAlarmService] alarmFired: notifying observers for item " + aItem.hashId);

Do we need this log message? If you'd like to keep it in, I'd suggest changing the wording to not include the method name, i.e

"Alarm for item " + item.title + " has fired, notifying observers"
Attachment #567661 - Flags: review?(philipp) → review-
(In reply to Philipp Kewisch [:Fallen] from comment #16)
> Comment on attachment 567661 [details] [diff] [review] [diff] [details] [review]
> Fix v3
> 
> Review of attachment 567661 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I would love to prevent users from getting an exception when they click
> dismiss all, this is one of the sharp edges that is hard for users to
> accept. I think I'm not quite clear what specific issues this patch vs the
> patch with batch handling would fix. Maybe you could explain this to me in
> more detail.

There are actually 2 separate but related issues with alarms on repeating items. One of these is the one you mentioned with dismiss/snooze all, reported in bug 496897, and probably bug 581943 (I'm not sure that one is entirely due to this though).

The other issue is the one reported in this bug, where modifying or deleting an occurrence doesn't properly update the alarm service, sometimes leaving invalid alarms that result in an error when acknowledged. This can be demonstrated with the following STR:

1) Create a repeating task, with a due date in the near future, with a custom alarm set to the moment the task ends.
2) Check the first occurrence of the task to complete it.
3) Wait until the due date/time.

Expected results: The alarm will not fire, as the task is completed. This is the behavior of a non-repeating completed task with an alarm.

Actual results: An alarm fires, and acknowledging the alarm by snoozing, dismissing, or closing the alarm dialog results in a MODIFICATION_FAILED error.


The bigger fix in WIP Fix 2 added batch handling to the alarm service that fixed both issues, but I don't feel that it will be ready in time for the 1.0 release. The current Fix v3 only addresses the second issue, but I have done testing on it in production (not to say this doesn't still need more testing, the nature of the alarm service makes edge cases are hard to test for IMHO).

I have another patch I'm currently doing testing on that addresses the first dismiss/snooze all issue, but in a simpler way that I think will be less risky if we need it for the 1.0 release. I plan to post that patch to bug 496897 (or maybe bug 581943 that it was duped to, but I disagree with that decision) as soon as I finish testing it locally.

I still think we should do the batch handling part, just not for 1.0 to reduce regression risks. The current Fix v3 on this bug still takes care of the first part of the batch handling fix, moving the occurrence handling out of the calendar observer. I'll file a followup bug to finish the batch alarm handling.

> Testing the patch I noticed the following issues, please excuse if they can
> only be fixed with the batch handling part:
> 
> * Clicking Snooze All doesn't work at all for me (no error messages)
> * Clicking Dismiss All still throws a conflict dialog for caldav and an
> error message for storage

This will be fixed by the patch I'm currently testing for bug 496897, barring any issues I find it should be posted in the next day or two.

> Do we need this log message? If you'd like to keep it in, I'd suggest
> changing the wording to not include the method name, i.e

That was a testing remnant, I don't think we need to keep it.
Comment on attachment 567661 [details] [diff] [review]
Fix v3

(In reply to Matthew Mecca [:mmecca] from comment #17)
> There are actually 2 separate but related issues with alarms on repeating
> items. One of these is the one you mentioned with dismiss/snooze all,
> reported in bug 496897, and probably bug 581943 (I'm not sure that one is
> entirely due to this though).
> 
> The other issue is the one reported in this bug, where modifying or deleting
> an occurrence doesn't properly update the alarm service, sometimes leaving
> invalid alarms that result in an error when acknowledged. This can be
> demonstrated with the following STR:

> The bigger fix in WIP Fix 2 added batch handling to the alarm service that
> fixed both issues, but I don't feel that it will be ready in time for the
> 1.0 release. The current Fix v3 only addresses the second issue, but I have
> done testing on it in production (not to say this doesn't still need more
> testing, the nature of the alarm service makes edge cases are hard to test
> for IMHO).
Sure, I agree we shouldn't introduce too many risky patches. Offline support is already enough risk for one release :-)


> I have another patch I'm currently doing testing on that addresses the first
> dismiss/snooze all issue, but in a simpler way that I think will be less
> risky if we need it for the 1.0 release. I plan to post that patch to bug
> 496897 (or maybe bug 581943 that it was duped to, but I disagree with that
> decision) as soon as I finish testing it locally.
Great, good to hear. If you disagree to the duping, go ahead and undo.


Given I understand the situation a bit better now, r+ for this patch with the log comment removed. Also approved for all branches.
Attachment #567661 - Flags: review- → review+
Attached patch Fix v4 β€” β€” Splinter Review
Removes log message.
Attachment #567661 - Attachment is obsolete: true
Attachment #568278 - Flags: review+
What about comm-beta (which is needed for 1.0) ? a=philipp.
Pushed to comm-beta - http://hg.mozilla.org/releases/comm-beta/rev/0384947efa32
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: 1.0b4 → 1.0b8
Problem still (Lightning 1.9) exists.

I can confirm the bug on Windows XP, Windows Vista and Windows 7.
Last Thunderbird is 17.0.

I'm not using any google calendar or extra plugins.

Details:
When CalDav calendars are synced (by button or automatic) the reminder window appears, the sound is played and at once the all the time empty reminder window diappears again.

Tested with Kerio and Davical server.

The debug console shows repeated recv of the same events.
Axel, I believe this is a separate issue. Would you please file a new bug report? If possible, can you also create a clean profile using a test calendar on the CalDAV server and try to reproduce the problem, and if so enable the preferences calendar.debug.log and calendar.debug.log.verbose and attach the output of the error console?
You need to log in before you can comment on or make changes to this bug.