Closed Bug 420228 Opened 16 years ago Closed 16 years ago

Cannot dismiss alarms on CalDAV calendars

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: browning, Assigned: browning)

Details

Attachments

(1 file, 1 obsolete file)

The (!aListener) test in the CalDAV provider's getUpdatedItem() is inappropriate, since the alarm service calls modifyItem() with a null listener.
Attachment #306438 - Flags: review?(ctalbert)
Flags: wanted-calendar0.8?
You say that modifyItem calls this with a null listener, but if aItem is null, the listener is called regardless. I don't quite understand the interaction with modifyItem though. 
Right you are, we need to handle the case where both aItem and aListener are null.

What's happening is that the alarm service calls modifyItem when dismissing the alarm, which with CalDAV results in the item being PUT to the server. CalDAV servers often alter items upon PUT (with X-props and the like), so we always refetch the item immediately after a PUT. This is done by calling getUpdatedItem with the item and the listener from the original modifyItem() call. On an alarm dismissal, that listener will be null, so getUpdatedItem must not bail when being called with a null listener.
Assignee: nobody → browning
Attachment #306438 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #306592 - Flags: review?(ctalbert)
Attachment #306438 - Flags: review?(ctalbert)
Comment on attachment 306592 [details] [diff] [review]
handle case where both aItem and aListener are null

>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/caldav/calDavCalendar.js,v
>retrieving revision 1.45.2.69
>diff -p -8 -u -r1.45.2.69 calDavCalendar.js
>--- calendar/providers/caldav/calDavCalendar.js	23 Feb 2008 20:53:52 -0000	1.45.2.69
>+++ calendar/providers/caldav/calDavCalendar.js	29 Feb 2008 21:32:31 -0000
>@@ -689,27 +689,29 @@ calDavCalendar.prototype = {
>     /**
>      * Retrieves a specific item from the CalDAV store.
>      * Use when an outdated copy of the item is in hand.
>      *
>      * @param aItem       item to fetch
>      * @param aListener   listener for method completion
>      */
>     getUpdatedItem: function caldavGUI(aItem, aListener) {
>-        if (!aListener) {
>-            return;
>-        }
> 
>         if (aItem == null) {
>-            aListener.onOperationComplete(this.superCalendar,
>-                                          Components.results.NS_ERROR_FAILURE,
>-                                          aListener.GET,
>-                                          null,
>-                                          "passed in null item");
>-            return;
>+            if (aListener) {
>+                aListener.onOperationComplete(this.superCalendar,
>+                                              Components.results.NS_ERROR_FAILURE,
>+                                              aListener.GET,
>+                                              null,
>+                                              "passed in null item");
>+                return;
>+            } else {
>+                LOG("passed in null item and null listener for getUpdatedItem");
>+                return;
>+            }
I'd just shorten this to

if (aItem == null) {
  if (aListener) {
    aListener.onOp...
  }
  return;
}

No need for the double return, and I don't think a LOG message is needed since this is a quite common case.

r=philipp with those changes
(Clint, I hope its ok I'm taking your reviews, I was just available and since the rc is coming up on monday I thought I'd take over :)

>         }
> 
>         var itemType = "VEVENT";
>         if (aItem instanceof Components.interfaces.calITodo) {
>             itemType = "VTODO";
>         }
> 
>         var queryStatuses = new Array();
Attachment #306592 - Flags: review?(ctalbert) → review+
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Keywords: checkin-needed
patch checked in on HEAD and MOZILLA_1_8_BRANCH with reviewer's changes

->FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: wanted-calendar0.8+ → wanted-calendar0.8?
Resolution: --- → FIXED
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Keywords: checkin-needed
Target Milestone: --- → 0.8
You need to log in before you can comment on or make changes to this bug.