Closed Bug 409921 Opened 17 years ago Closed 16 years ago

Implement CalDAV scheduling

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: browning, Assigned: browning)

References

Details

Attachments

(4 files, 6 obsolete files)

We want to support the draft caldav-sched spec, which is modelled closely enough on iTIP that at this point it's unclear that it needs a separate interface. This patch is the (very raw) beginning; it adds:
* sending invitations by POSTing to the caldav-scheduling outbox
* displaying items from the caldav-scheduling inbox in the views
* moving items from the caldav-scheduling inbox to the calendar proper when they are modified
There's a good bit more to do before this actually becomes useful, like locking the inbox when appropriate, wiring up iTIP REPLY and friends, etc. So I've put this behind a pref: it's too raw to try to use and we don't want to create expectations that it will be usable by 0.8 release.
Attachment #294622 - Flags: review?(daniel.boelzle)
Status: NEW → ASSIGNED
Attachment #294622 - Flags: review?(daniel.boelzle)
updated patch in hand; it relies on code proposed in a patch for bug 393817 so I'm setting that as a blocker.
Depends on: 393817
Flags: wanted-calendar0.9+
IMO this bug depends on resolving the group scheduling API enhancements.
Depends on: 370150
Attached patch Work in progress patch (obsolete) — Splinter Review
Posting work-in-progress patch on request. This is not yet ready for review, but the basic transport-level parts of caldav-sched draft 4 work:
* send invitation by POSTing to scheduling-outbox
* poll scheduling-inbox on refresh() and display invitations in UI
* send iTIP REPLYs by POSTing to scheduling-outbox
* process iTIP REPLYs found in scheduling-inbox
It also does silly things like send invitations to the ORGANIZER, does not have working error dialogs, and in general is not yet milspec. If we decide that we will always ship CalDAV with Sb/Ltn we might want to structure it differently, with code somewhere other than calendar/providers/caldav/itip/ And so forth.
Attachment #294622 - Attachment is obsolete: true
Flags: wanted-calendar0.9+ → blocking-calendar0.9+
Summary: CalDAV provider should implement calIITipTransport → Implement CalDAV scheduling
Blocks: 434354
Comment on attachment 324382 [details] [diff] [review]
Work in progress patch

random comments:

>Index: calendar/base/content/calendar-item-editing.js
>     // ask the provide if this item is an invitation. if this is the case
>     // we'll open the summary dialog since the user is not allowed to change
>     // the details of the item.
>     var isInvitation = false;
>+    if (calendarItem.calendar.type == "caldav") {
>+        calendar = calendar.QueryInterface(Components.interfaces.calICalDavCalendar);
>+    }
>     try {
>         isInvitation = calendar.isInvitation(calendarItem);
>     }

ought be obsolete with calISchedulingSupport in bug 370150


> function checkForAttendees(aItem, aOriginalItem)
> {
>-    // iTIP is only supported in Lightning right now
>-    if (isSunbird()) {
>+    // iMIP is only supported in Lightning right now
>+    var caldavSched = false;
>+    if (aItem.calendar.type == "caldav"
>+        && aItem.calendar.QueryInterface(Components.interfaces.calICalDavCalendar)
>+            .hasScheduling) {
>+        caldavSched = true;
>+    }
>+    if (isSunbird() && !caldavSched) {
>         return;
...

Why doesn't the caldav provider just set calICalendar::sendItipInvitations to false in that case?

>Index: calendar/base/content/calendar-summary-dialog.js
>-            var provider = calendar.getProperty("private.wcapCalendar")
>-                                   .QueryInterface(Components.interfaces.calIWcapCalendar);
>+            if (calendar.type == "wcap") {
>+                var provider = calendar.getProperty("private.wcapCalendar")
>+                                       .QueryInterface(Components.interfaces
>+                                        .calIWcapCalendar);
>+            }
>+            if (calendar.type == "caldav") {
>+                provider = calendar.QueryInterface(Components.interfaces
>+                                   .calICalDavCalendar);
>+            }
>             var attendee = provider.getInvitedAttendee(item);

IMO obsolete with bug 370150, too

>Index: calendar/base/public/calIErrors.idl
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/public/calIErrors.idl,v
>retrieving revision 1.1.2.8
>diff -p -8 -u -r1.1.2.8 calIErrors.idl
>--- calendar/base/public/calIErrors.idl	11 May 2008 19:10:03 -0000	1.1.2.8
>+++ calendar/base/public/calIErrors.idl	10 Jun 2008 01:22:38 -0000
>@@ -121,10 +121,11 @@ interface calIErrors : nsISupports
>    */
>   const unsigned long DAV_ERROR_BASE = ERROR_BASE + 0x301;
>   const unsigned long DAV_NOT_DAV = DAV_ERROR_BASE +  0;
>   const unsigned long DAV_DAV_NOT_CALDAV = DAV_ERROR_BASE + 1;
>   const unsigned long DAV_NO_PROPS = DAV_ERROR_BASE +  2;
>   const unsigned long DAV_PUT_ERROR = DAV_ERROR_BASE + 3;
>   const unsigned long DAV_REMOVE_ERROR = DAV_ERROR_BASE + 4;
>   const unsigned long DAV_REPORT_ERROR = DAV_ERROR_BASE + 5;
>+  const unsigned long DAV_POST_ERROR = DAV_ERROR_BASE + 6;
why do we need this here?

>Index: calendar/base/src/calUtils.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/src/calUtils.js,v
>retrieving revision 1.1.2.103
>diff -p -8 -u -r1.1.2.103 calUtils.js
>--- calendar/base/src/calUtils.js	6 Jun 2008 11:09:45 -0000	1.1.2.103
>+++ calendar/base/src/calUtils.js	10 Jun 2008 01:22:40 -0000
>@@ -2171,59 +2171,84 @@ calPropertyBagEnumerator.prototype = {
> // Send iTIP invitation
> function sendItipInvitation(aItem, aTypeOfInvitation, aRecipientsList) {
>     // XXX Until we rethink attendee support and until such support
>     // is worked into the event dialog (which has been done in the prototype
>     // dialog to a degree) then we are going to simply hack in some attendee
>     // support so that we can round-trip iTIP invitations.
>     var transportType = aItem.calendar.getProperty("itip.transportType") || "email";
> 
>-    var transport = Components.classes["@mozilla.org/calendar/itip-transport;1?type=" + transportType]
>-                           .getService(Components.interfaces.calIItipTransport);
>+     // select the iTIP binding du jour
>+     var isCalDAVSched = false;
>+     var sender = null;
>+     if (aItem.calendar.type == "caldav"
>+         && aItem.calendar.QueryInterface(Components.interfaces.calICalDavCalendar)
>+         .hasScheduling) {
>+         isCalDAVSched = true;
>+         var transport = Components.classes["@mozilla.org/calendar/itip-transport;1?type=caldav"]
>+                            .createInstance(Components.interfaces.calIItipCalDavTransport);
>+         transport.calDavCalendar = aItem.calendar;
>+         sender = transport.defaultIdentity;
>+     } else {
>+         var transport = Components.classes["@mozilla.org/calendar/itip-transport;1?type=email"]
>+                            .createInstance(Components.interfaces.calIItipTransport);
>+     }

Isn't it more flexible if we return the transport via calendar property instead of using a xpcom name, e.g.

just transport = cal.getProperty("itip.transport");

That way, the provider could glue together the itip with session information etc.

>     var organizer = Components.classes["@mozilla.org/calendar/attendee;1"]
>                               .createInstance(Components.interfaces.calIAttendee);
>-    organizer.id = transport.scheme + ":" + transport.defaultIdentity;
>+    // CalDAV organizer URLs can be of various schemes, so for CalDAV we'll
>+    // handle the scheme in the transport
>+    if (isCalDAVSched) {
>+        organizer.id = transport.defaultIdentity;
>+    } else {
>+        organizer.id = transport.scheme + ":" + transport.defaultIdentity;
>+    }

IMO we should enforce that the scheme is part of the identity, i.e. being a CAL-ADDRESS.

>+     // we don't need the text for caldav-sched
>+     var subject;
>+     var body;
>+     if (!isCalDAVSched) {
>+         subject = sb.formatStringFromName("itipRequestSubject",
>+                                               [summary], 1);
>+         body = sb.formatStringFromName("itipRequestBody",
>+                                            [itipSvc.defaultIdentity, summary],
>+                                            2);
>+     }

let's keep it for now, or does it hurt? We should refrain form adding provider-specifics, and add API if necessary.  I don't want to do the same mistake again (like I did with WCAP) ;-)

further:
- I don't quite understand the extra iTIP transport component implementation. It looks doubled to me.
- If I get it right, you are mixing in the inbox items, which looks wrong to me since those haven't been accepted (and put into the calendar) yet. I've added ITEM_FILTER_REQUEST_NEEDS_ACTION in bug 370150, which is used by the calendar invitations link (below calendar list). It opens a dialog for the open invitations to be accepted/denied. I think you should return those items only on that filter bit.
to be used for errors; yet unused
Attachment #329158 - Flags: review?(philipp)
- some slight changes/additions for recent API changes
- does not implement iTIP transport anymore (sendItipInvitations=false, no iMIP triggering)
- untested, cannot get the calendar user's address on my local darwin server
Comment on attachment 329158 [details] [diff] [review]
[checked in] strings

RSVP sounds strange, afaik RSVP means that the recipient is asked to reply. Maybe just caldavResponseError

r=philipp
Attachment #329158 - Flags: review?(philipp) → review+
Comment on attachment 329158 [details] [diff] [review]
[checked in] strings

agreed; I changed the identifiers to caldavRequestError and 
caldavResponseError.
Attachment #329158 - Attachment description: strings → [checked in] strings
Attachment #329159 - Attachment is obsolete: true
Attached patch debitrot, wip - v2 (obsolete) — Splinter Review
I think I broke invitations, but I just wanted to upload this before I hit the sheets.
Attachment #330392 - Attachment is obsolete: true
Attached patch another wip patch (obsolete) — Splinter Review
posting another wip patch per dbo request on irc. This is successfully sending invitations; also already slightly bitrotted.
Okay, I think this is ready for review and wider testing. Applies cleanly to current tree.
* works well with my Darwin CalendarServer-on-Linux test setup
* sending invitations works on my Bedework 3.4.1 setup, sending REPLYs fails due to what is apparently a Bedework bug which I have reported
* not tested with Oracle due to unavailabilty of test server (Bernard, could you or Simon goose that thing?)
* not working with SOGo; possible configuration problem on test machine, have contacted Ludovic
Attachment #324382 - Attachment is obsolete: true
Attachment #331363 - Attachment is obsolete: true
Attachment #333107 - Attachment is obsolete: true
Attachment #333160 - Flags: review?(daniel.boelzle)
Comment on attachment 333160 [details] [diff] [review]
debitrot, refactor

Since I really look forward to this patch, here a code-level review. I haven't tested anything yet, so I'll leave the rest to daniel.


>+    var subject = "";
>+    var body = "";
>     var summary = (item.getProperty("SUMMARY") || "");
>+    if (transport.type != "caldav") {
No subject or body for caldav? Please add a comment explaining why. Unless this is a workaround that can be solved in a different way on short term, we might want to find a way to generally allow this.


>+            eventUri = this.mInBoxUrl.clone();
>+            eventUri.spec = this.mInBoxUrl.spec + this.mItemInfoCache[aItem.id].locationPath;
Since you're using the whole url, you could probably optimize by doing:
              eventUri = makeURL(this.mInBoxUrl.spec + this.mItemInfoCache[aItem.id].locationPath);

>+                    try {
>+                        if (etag != thisCalendar.mItemInfoCache[itemuid].etag) {
>+                            // we don't have a current copy in cache; fetch the item
>+                            aRefreshEvent.itemsNeedFetching.push(aResource.path);
>+                        }
>+                    } catch(ex) {
>+                        // no mItemInfoCache for this item; fetch it
>                         aRefreshEvent.itemsNeedFetching.push(resourcePath);
>                     }
>                 } else {
>                     aRefreshEvent.itemsNeedFetching.push(resourcePath);
>                 }
I couldn't find aResource anywhere, cruft? Also, you do almost the same thing in most every block. Maybe

if (!(resourcePath in thisCalendar.mHrefIndex) ||
    !(itemuid in thisCalendar.mItemInfoCache) ||
    etag != thisCalendar.mItemInfoCache[itemuid].etag) {
    aRefreshEvent.itemsNeedFetching.push(resourcePath);
}


>                 // avoid sending empty multiget requests
>                 // update views if something has been deleted server-side
>                 if (!aRefreshEvent.itemsNeedFetching.length) {
>                     if (needsRefresh) {
>                         thisCalendar.mObservers.notify("onLoad", [thisCalendar]);
>                     }
>+                    // but do poll the inbox;
>+                    if (thisCalendar.mHaveScheduling &&
>+                        !thisCalendar.isInBox(aRefreshEvent.uri.spec)) {
>+                        thisCalendar.pollInBox();
>+                    }
>                     return;
>                 }
I probably missed something, but doesn't this only poll the inbox if the multiget is empty? I couldn't find the pollinbox in the remaining function.

>+                for each (var prop in propertiesList) {
>+                    if (prop.propertyName == "METHOD") {
>+                    method = prop.value;
>+                    }
>+                }
indent method, also go ahead and |break| after finding the method.

>+            if (!thisCalendar.mCalendarUserAddress || !thisCalendar.mInBoxUrl || !thisCalendar.mOutBoxUrl) {
>                 if (aNameSpaceList.length) {
>                     thisCalendar.checkPrincipalsNameSpace(aNameSpaceList);
>                 } else {
>                     thisCalendar.mHaveScheduling = false;
>                     thisCalendar.refresh();

...

>     getFreeBusyIntervals: function caldav_getFreeBusyIntervals(
>         aCalId, aRangeStart, aRangeEnd, aBusyTypes, aListener) {
> 
>-        if (!this.mHaveScheduling || !this.mOutBoxUrl || !this.mMailToUrl) {
>+        if (!this.mHaveScheduling || !this.mOutBoxUrl || !this.mCalendarUserAddress) {
Since above you set mHaveScheduling to false if there were no namepsaces, isn't it sufficient to check for hasScheduling here?


>+    getInvitedAttendee: function caldav_getInvitedAttendee(aItem) {
>+        for each (var att in aItem.getAttendees({})) {
>+            // compare lower-case because MAILTO != mailto
>+            if (att.id.toLowerCase() == this.mCalendarUserAddress.toLowerCase()) {
>+                return att;
>+            }
>+        }
>+        return null;
Looking at calIItemBase::getAttendeeById(), the lowercase check is already done there. You could just do

return aItem.getAttendeeById(this.mCalendarUserAddress);


>+        var delListener = {};
>+        delListener.onOperationComplete = function caldav_pIR_doOC(aCalendar,
>+                                                                     aStatus,
>+                                                                     aOperationType,
>+                                                                     aItemId,
>+                                                                     aDetail) {
>+        }
Not used?

>+        var attendee = aItem.getAttendeeById(this.mCalendarUserAddress);
>+        if (attendee) {
>+            myPartStat = attendee.participationStatus;
>+            foundMyself = true;
>+        }
>+
>+        if (!foundMyself) {
>+            return;
>+        }

Optional, I find:


        if (!attendee) {
            // Didn't find myself
            return;
        }
        myPartStat = attendee.participationStatus;

better to read.

>+        // let's not be sending responses to ourselves, hey
>+        if (this.mCalendarUserAddress == aItem.organizer.id.toLowerCase()) {
Is it ensured that mCalendarUserAddress is always lowercased?

>+        serializer.addItems([replyItem], 1);
>+        var methodProp = getIcsService().createIcalProperty("METHOD");
>+        methodProp.value = "REPLY";
>+        serializer.addProperty(methodProp);
>+        var ics = serializer.serializeToString();
>+
>+        var ics = ics.replace(/MAILTO:/g, "mailto:")
So if I have an item with description "Fix bug with MAILTO: links", will this destroy the case? ;-)

>+        streamListener.onStreamComplete =
>+            function caldav_sSR_oSC(aLoader, aContext, aStatus, aResultLength,
>+                                    aResult) {
>+                var resultConverter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"]
>+                                             .createInstance(Components
>+                                             .interfaces.nsIScriptableUnicodeConverter);
You could use the new utility function convertByteArray(aResult, aResultLength) and if you want, check for === null to see if anything failed, or pass true as the third (or fourth?) argument, to make it throw. There are other places below that also could have this changed.

>+            var serializer = Components.classes["@mozilla.org/calendar/ics-serializer;1"]
>+                                       .createInstance(Components.interfaces.calIIcsSerializer);
Maybe we want createIcsSerializer() in calUtils?

>+            var queryLines = ics.split("\n");
>+            var changeActive = false;
>+            for (d = 0; d < queryLines.length; d++) {
>+                var org = queryLines[d].search(/^ORGANIZER/);
...
This looks more like the type of parsing function to replace MAILTO, you might want to make an file-scoped helper out of it and use it above.


>+    checkForInvitations: function caldav_CFI(searchStart) {
>+        // CalDAV calendars do this with pollInBox()
>+        throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>+
>+    },
We might want to make sure callers of this function can handle exceptions being thrown here.
Whiteboard: [patch in hand] [needs-review]
(In reply to comment #14)

Couple of quick comments before Daniel's review:

> (From update of attachment 333160 [details] [diff] [review])
> 
> >+    var subject = "";
> >+    var body = "";
> >     var summary = (item.getProperty("SUMMARY") || "");
> >+    if (transport.type != "caldav") {
> No subject or body for caldav? Please add a comment explaining why. Unless this
> is a workaround that can be solved in a different way on short term, we might
> want to find a way to generally allow this.
> 

CalDAV doesn't use these and will ignore them if present (they're for the iMIP wrapper; CalDAV is just pushing the iTIP object around without wrapper). Problem is that Sunbird/CalDAV will throw when trying to look up these Lightning strings. The longer-term solution, I think, is to make them calendar strings instead of Lightning ones -- maybe all Lightning strings, if Sunbird is to become just tbird with the email ui turned off. But that's another bug.

> 

> >+                    // but do poll the inbox;
> >+                    if (thisCalendar.mHaveScheduling &&
> >+                        !thisCalendar.isInBox(aRefreshEvent.uri.spec)) {
> >+                        thisCalendar.pollInBox();
> >+                    }
> >                     return;
> >                 }
> I probably missed something, but doesn't this only poll the inbox if the
> multiget is empty? I couldn't find the pollinbox in the remaining function.
> 

If the multiget is not empty, it gets passed on to getCalendarData and pollInbox() is called when that completes.

> >+        serializer.addItems([replyItem], 1);
> >+        var methodProp = getIcsService().createIcalProperty("METHOD");
> >+        methodProp.value = "REPLY";
> >+        serializer.addProperty(methodProp);
> >+        var ics = serializer.serializeToString();
> >+
> >+        var ics = ics.replace(/MAILTO:/g, "mailto:")
> So if I have an item with description "Fix bug with MAILTO: links", will this
> destroy the case? ;-)
> 

Fortunately our iTIP REPLYs don't include much more than UIDs and ATTENDEEs, so I think we're safe to do a global replace here

> >+            var queryLines = ics.split("\n");
> >+            var changeActive = false;
> >+            for (d = 0; d < queryLines.length; d++) {
> >+                var org = queryLines[d].search(/^ORGANIZER/);
> ...
> This looks more like the type of parsing function to replace MAILTO, you might
> want to make an file-scoped helper out of it and use it above.
> 

... or we could just fix the mailto:->MAILTO: madness  :)

(In reply to comment #15)
> (In reply to comment #14)
> 
> Couple of quick comments before Daniel's review:
> 
> > (From update of attachment 333160 [details] [diff] [review] [details])
> > 
> > >+    var subject = "";
> > >+    var body = "";
> > >     var summary = (item.getProperty("SUMMARY") || "");
> > >+    if (transport.type != "caldav") {
> > No subject or body for caldav? Please add a comment explaining why. Unless this
> > is a workaround that can be solved in a different way on short term, we might
> > want to find a way to generally allow this.
> > 
> 
> CalDAV doesn't use these and will ignore them if present (they're for the iMIP
> wrapper; CalDAV is just pushing the iTIP object around without wrapper).
> Problem is that Sunbird/CalDAV will throw when trying to look up these
> Lightning strings. The longer-term solution, I think, is to make them calendar
> strings instead of Lightning ones -- maybe all Lightning strings, if Sunbird is
> to become just tbird with the email ui turned off. But that's another bug.

Right, before adding them to calendar.properties, I think it's even better to guard those using isSunbird().

(In reply to comment #14)
> >+            eventUri = this.mInBoxUrl.clone();
> >+            eventUri.spec = this.mInBoxUrl.spec + this.mItemInfoCache[aItem.id].locationPath;
> Since you're using the whole url, you could probably optimize by doing:
>               eventUri = makeURL(this.mInBoxUrl.spec +
> this.mItemInfoCache[aItem.id].locationPath);

...or change makeUri to actually really return an nsIURI, because all code I see wants exactly that.


> 
> >+                    try {
> >+                        if (etag != thisCalendar.mItemInfoCache[itemuid].etag) {
> >+                            // we don't have a current copy in cache; fetch the item
> >+                            aRefreshEvent.itemsNeedFetching.push(aResource.path);
> >+                        }
> >+                    } catch(ex) {
> >+                        // no mItemInfoCache for this item; fetch it
> >                         aRefreshEvent.itemsNeedFetching.push(resourcePath);
> >                     }
> >                 } else {
> >                     aRefreshEvent.itemsNeedFetching.push(resourcePath);
> >                 }
> I couldn't find aResource anywhere, cruft? Also, you do almost the same thing
> in most every block. Maybe
> 
> if (!(resourcePath in thisCalendar.mHrefIndex) ||
>     !(itemuid in thisCalendar.mItemInfoCache) ||
>     etag != thisCalendar.mItemInfoCache[itemuid].etag) {
>     aRefreshEvent.itemsNeedFetching.push(resourcePath);
> }

Yes, should be resourcePath, and IMO could even be a bit shorter:

                var itemuid = thisCalendar.mHrefIndex[resourcePath];
                if (!itemuid || etag != thisCalendar.mItemInfoCache[itemuid].etag) {
                    aRefreshEvent.itemsNeedFetching.push(resourcePath);
                }


> >-        if (!this.mHaveScheduling || !this.mOutBoxUrl || !this.mMailToUrl) {
> >+        if (!this.mHaveScheduling || !this.mOutBoxUrl || !this.mCalendarUserAddress) {
> Since above you set mHaveScheduling to false if there were no namepsaces, isn't
> it sufficient to check for hasScheduling here?

I think we could remove this check, because the free-busy provider is only registered in case we have scheduling. I am missing code that removes the provider BTW. Also, is it assured that the provider isn't registered multiple times?

> >+        var delListener = {};
> >+        delListener.onOperationComplete = function caldav_pIR_doOC(aCalendar,
> >+                                                                     aStatus,
> >+                                                                     aOperationType,
> >+                                                                     aItemId,
> >+                                                                     aDetail) {
> >+        }
> Not used?

Used some lines above, but since it doesn't do anything, we could pas in null as listener with doDeleteItem() IMO.

> >+        var attendee = aItem.getAttendeeById(this.mCalendarUserAddress);
> >+        if (attendee) {
> >+            myPartStat = attendee.participationStatus;
> >+            foundMyself = true;
> >+        }
> >+
> >+        if (!foundMyself) {
> >+            return;
> >+        }
> 
> Optional, I find:
> 
> 
>         if (!attendee) {
>             // Didn't find myself
>             return;
>         }
>         myPartStat = attendee.participationStatus;
> 
> better to read.

did that.

> >+        // let's not be sending responses to ourselves, hey
> >+        if (this.mCalendarUserAddress == aItem.organizer.id.toLowerCase()) {
> Is it ensured that mCalendarUserAddress is always lowercased?
changed that

> >+        streamListener.onStreamComplete =
> >+            function caldav_sSR_oSC(aLoader, aContext, aStatus, aResultLength,
> >+                                    aResult) {
> >+                var resultConverter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"]
> >+                                             .createInstance(Components
> >+                                             .interfaces.nsIScriptableUnicodeConverter);
> You could use the new utility function convertByteArray(aResult, aResultLength)
> and if you want, check for === null to see if anything failed, or pass true as
> the third (or fourth?) argument, to make it throw. There are other places below
> that also could have this changed.
did that.

> >+    checkForInvitations: function caldav_CFI(searchStart) {
> >+        // CalDAV calendars do this with pollInBox()
> >+        throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> >+
> >+    },
> We might want to make sure callers of this function can handle exceptions being
> thrown here.
AFAIR there's still no client code, it's unused. We may consider remove it from the API.


I am doing some more changes/tests, then continue on the review. I just wanted to give a short progress update.
Comment on attachment 333160 [details] [diff] [review]
debitrot, refactor

>@@ -415,51 +444,64 @@ calDavCalendar.prototype = {
>             this.notifyOperationComplete(aListener,
>                                          Components.results.NS_ERROR_FAILURE,
>                                          Components.interfaces.calIOperationListener.MODIFY,
>                                          aItem.id,
>                                          "ID for modifyItem doesn't exist or is null");
>             return;
>         }
> 
>+        var wasInBoxItem = false;
>+        if (this.mItemInfoCache[aNewItem.id].isInBoxItem) {
>+            aIgnoreEtag = true;
>+            wasInBoxItem = true;
>+        }
>+        var isInvitation = this.isInvitation(aNewItem);
>+        if (isInvitation) {
>+            this.simpleSendResponse(aNewItem);
>+        }
>+

I think we should use the iTIP code we already have here instead of implementing it twice, i.e. the code in calendar-item-editing.js. That code already deals with a lot of scenarios and I rather would like to weave it in here, so we only have little stuff to implement here, namely the itip transport interface.

That said, I'll come up with changes to calendar-item-editing (and provider base) to do that.

>+    /**
>+     * calISchedulingSupport interface
>+     */
>+    isInvitation: function caldav_isInvitation(aItem) {
>+        var org = aItem.organizer;
>+        if (!org ||
>+            (org.id.toLowerCase() == this.calendarUserAddress.toLowerCase())) {
>+            return false;
>+        }
>+
>+        return (aItem.getAttendeeById(this.calendarUserAddress) != null);
>+    },
>+
>+    getInvitedAttendee: function caldav_getInvitedAttendee(aItem) {
>+        for each (var att in aItem.getAttendees({})) {
>+            // compare lower-case because MAILTO != mailto
>+            if (att.id.toLowerCase() == this.mCalendarUserAddress.toLowerCase()) {
>+                return att;
>+            }
>+        }
>+        return null;
>+    },
>+
>+    canNotify: function caldav_canNotify(aMethod, aItem) {
>+        // This depends on whether the server can send email XXX how to detect that?
>+        return false; // this.hasScheduling;
>+    },

In my upcoming patch, we take this from provider base.

>+    processItipReply: function caldav_processItipReply(aItem, aPath) {
>+        // modify partstat for in-calendar item
>+        // delete item from inbox
>+        var thisCalendar = this;
>+
>+        var path = this.mItemInfoCache[aItem.id].locationPath;
>+        if (!path) {
>+            return;
>+        }
>+
>+        var replyAtt = aItem.getAttendees({});
>+        var getItemListener = {};
>+        getItemListener.onGetResult = function caldav_pIR_oGR(aCalendar,
>+            aStatus, aItemType, aDetail, aCount, aItems) {
>+            var itemToUpdate = aItems[0];
>+            var newItem = itemToUpdate.clone();
>+            for each (var attendee in replyAtt) {
>+                attendee = attendee.clone();
>+                var oldAttendee = newItem.getAttendeeById(attendee.id);
>+                oldAttendee.participationStatus = attendee.participationStatus;
>+                thisCalendar.doModifyItem(newItem, itemToUpdate, modListener, true);
>+            }
The loop wrong, you modify the item for every attendee.

>+    simpleSendResponse: function caldav_simpleSendResponse(aItem) {
>+
>+        if (aItem instanceof Components.interfaces.calIEvent) {
>+            var replyItem = createEvent();
>+        }
>+        if (aItem instanceof Components.interfaces.calITodo) {
>+            var replyItem = createTodo();
>+        }
...

calIItipTransport::simpleSendResponse get passed an calIItipItem, not calIItemBase.

my upcoming patch makes most of the body obsolete, using sendItems.

>+    sendItems: function caldav_SI(aCount, aRecipients, aSubject, aBody, aItem) {
>+        var argRecipients = aRecipients; // need to access inside callback
>+        var recipients = [];
>+        for (var i = 0; i < aCount; i++) {
>+            var recip = aRecipients[i].id;
>+            var recipparts = recip.split(":");
>+            recipparts[0] = recipparts[0].toLowerCase();
>+            recipients.push(recipparts.join(":"));
>+        }

I've fixed calAttendee.js to enforce lowe-case mailto: which makes a lot of code easier.

the patch has a lot of good bits, but still flaws; r-
Attachment #333160 - Flags: review?(daniel.boelzle) → review-
I think this patch makes less changes to the caldav provider, and weaves into the iTIP code we have in calendar-item-editing.js which is more complete.
Attachment #333789 - Flags: review?(browning)
I've been in haste yesterday evening, some comments are missing on the patch:

I don't know what happens if we mix up server users and non-server users in the attendees list. Is it valid to post all REQUESTs to the outbox resp. will the server send out iMIP for the non-server users?
However, I've changed the meaning of "Send attendees invitations via email" to map to a generalized X-MOZ-USE-IMIP and have the clear meaning to use iMIP *only*, excluding any transport implemented by the provider. That way the user could decide what channel to use for sending out the REQUESTs to attendees.
Lightning's mail iTIP/iMIP handler adds that property for incoming iMIP messages, so those are answered using iMIP.
Comment on attachment 333789 [details] [diff] [review]
recycled Bruno's patch, changing more base stuff


> function checkAndSendItipMessage(aItem, aOpType, aOriginalItem) {
>-    var transport = aItem.calendar.getProperty("itip.transport");
>-    if (!transport) { // Only send if there's a transport for the calendar
>-        return;
>+    var transport = null;
>+    if (aItem.getProperty("X-MOZ-USE-IMIP") == "TRUE") { // force to use iMIP if user checked the box
>+        // assure an identity is configured for the calendar
>+        transport = ((!isSunbird() && aItem.calendar.getProperty("imip.identity"))
>+                     ? Components.classes["@mozilla.org/calendar/itip-transport;1?type=email"]
>+                                 .getService(Components.interfaces.calIItipTransport)
>+                     : null);
>+    } else { // use provider specific transport, if available
>+        transport = aItem.calendar.getProperty("itip.transport");
>+        if (!transport || transport.type == "email") {
>+            return; // don't use email transport
>+        }
>     }
>-    transport = transport.QueryInterface(Components.interfaces.calIItipTransport);
>-
>-    if ((transport.type == "email") &&
>-        (aItem.getProperty("X-MOZ-SEND-INVITATIONS") != "TRUE")) { // Only send invitations/cancellations
>-                                                                   // if the user checked the checkbox
>+    if (!transport) {
>         return;
>     }
>+    transport = transport.QueryInterface(Components.interfaces.calIItipTransport);
> 

The UI ends up being a bit misleading here, since we have a checkbox offering to send invitations by email
when we're (maybe) actually sending them by caldav-sched, whether the box is checked or not. A bigger issue,
perhaps, is the case where the attendee set is going to require multiple transports: some caldav-sched, some
email. In that case I think we want to send invitations via caldav-sched where possible and fall back to 
email in cases where the caldav-sched delivery is unsuccessful and email available. IMO this current UI 
is acceptable/relnoteable for 0.9 but should be fixed for 1.0. My gut feeling is that the distinction we'll 
want to make is more "send invitations" than "send email".


>             // 200 = HTTP "OK"
>             // 204 = HTTP "No Content"
>             //
>             var status;
>             try {
>                 status = aContext.responseStatus;
>             } catch (ex) {
>                 status = Components.interfaces.calIErrors.DAV_PUT_ERROR;
>             }
> 
>-            if (status == 204 || status == 200) {
>+            if (status == 204 || status == 201 || status == 200) {

IMO we should not accept a 201 status here indefinitely: it indicates a server error 
of some kind that we want to know about. It's convenient to accept it for now since 
a number of server impls don't get this right yet. I think we should include a comment
to that effect here, and remove the 201 acceptance later.

>                 LOG("CalDAV: Item modified successfully.");
>                 var retVal = Components.results.NS_OK;
>                 // Some CalDAV servers will modify items on PUT (add X-props,
>                 // change location, etc) so we'd best re-fetch in order to know

We no longer allow servers to change item location on PUT, so as long as we're here let's change 
this comment, possibly by changing "change location, etc" to "for instance". There's a similar comment
in doAdoptItem() that could be changed this way as well.

>-    doDeleteItem: function caldavDDI(aItem, aListener, aIgnoreEtag) {
>+     * @param aFromInBox  delete from inbox rather than calendar
>+     * @param aUri        uri of item to delete     */
>+    doDeleteItem: function caldavDDI(aItem, aListener, aIgnoreEtag, aFromInBox, aUri) {
> 
>         if (aItem.id == null) {
>             this.notifyOperationComplete(aListener,
>                                          Components.results.NS_ERROR_FAILURE,
>                                          Components.interfaces.calIOperationListener.DELETE,
>                                          aItem.id,
>                                          "ID doesn't exist for deleteItem");
>             return;
>         }
> 
>-        var eventUri = this.mCalendarUri.clone();
>-        eventUri.spec = this.makeUri(this.mItemInfoCache[aItem.id].locationPath);
>+        var eventUri;
>+        if (aUri) {
>+            eventUri = aUri;
>+        } else if (aFromInBox) {
>+            eventUri = makeURL(this.mInBoxUrl.spec + this.mItemInfoCache[aItem.id].locationPath);
>+        } else {
>+            eventUri = this.makeUri(this.mItemInfoCache[aItem.id].locationPath);
>+        }
> 

We need to make the condition here (aFromInbox || this.mItemInfoCache[aItem.id].isInBoxItem) instead
of simply (aFromInBox), else the user will not be able to delete an item from the scheduling-inbox
without REPLYing to it


>+                            if (thisCalendar.isInBox(aRefreshEvent.uri.path) &&
>+                                thisCalendar.mItemInfoCache[itemToDelete.id].isInBoxItem) {
>+                                delete thisCalendar.mItemInfoCache[itemToDelete.id];
>+                                thisCalendar.mMemoryCalendar.deleteItem(itemToDelete,
>+                                                                        getItemListener);
>+                            }
>+

Further testing shows this to be incomplete. The condition needs to be something like
                             ((thisCalendar.isInBox(aRefreshEvent.uri.path) &&
                                thisCalendar.mItemInfoCache[itemToDelete.id].isInBoxItem) ||
                                (!thisCalendar.isInBox(aRefreshEvent.uri.path) &&
                                !thisCalendar.mItemInfoCache[itemToDelete.id].isInBoxItem))
to properly delete from cache items that have been deleted from the server by other parties, in both the
inbox and the calendar proper.


>+                var propertiesList = parser.getProperties({});
>+                var method;
>+                for each (var prop in propertiesList) {
>+                    if (prop.propertyName == "METHOD") {
>+                    method = prop.value;
>+                    }
>+                }

AFAIK the item can only have one METHOD prop, so we might as well |break| after finding it.

>                 var hrefPath = thisCalendar.ensurePath(href);
>                 thisCalendar.mHrefIndex[hrefPath] = item.id;
>-                thisCalendar.mItemInfoCache[item.id].etag = etag;

Whups! We really really really do not want to delete this line


>     getFreeBusyIntervals: function caldav_getFreeBusyIntervals(
>         aCalId, aRangeStart, aRangeEnd, aBusyTypes, aListener) {
> 
>-        if (!this.mHaveScheduling || !this.mOutBoxUrl || !this.mMailToUrl) {
>-            LOG("CalDAV: Server does not support scheduling; freebusy query not possible");
>-            return;
>-        }
>-

This check is not needed for our own purposes, but has been helpful in troubleshooting for
people adding scheduling code, new authschemes, etc, to server implementations. I'd suggest 
leaving it here for now for that purpose, but it's your call


>+            httpchannel.requestMethod = "POST";
>+            httpchannel.setRequestHeader("Originator", this.calendarUserAddress, false);
>+            for each (var recipient in aRecipients) {
>+                // Houston, we may have a problem.
>+                // What if some recipients are non-server users? Does the server send out iMIP in that case?
>+                // For now, the CU has to decide whether she wants iMIP to be sent out or (knows that
>+                // all attendees are server users) and we will switch up front in calendar-item-editing whether
>+                // we use iMIP or server's outbox only (using X-MOZ-USE-IMIP).
>+                httpchannel.setRequestHeader("Recipient", recipient.id, true);
>+            }
>+
>+            var this_ = this;
>+            var streamListener = {
>+                onStreamComplete: function caldav_sendItems_oSC(aLoader, aContext, aStatus,
>+                                                                aResultLength, aResult) {
>+                    var status;
>+                    try {
>+                        status = aContext.responseStatus;
>+                    } catch (ex) {
>+                        status = Components.interfaces.calIErrors.DAV_POST_ERROR;
>+                        LOG("CalDAV: no response status when sending iTIP.");
>+                    }
>+
>+                    if (status != 200) {
>+                        Components.utils.reportError("Sending iITIP failed with status " + status);
>+                    }
>+
>+                    var str = convertByteArray(aResult, aResultLength, "UTF-8", false);
>+                    if (str) {
>+                        if (this_.verboseLogging()) {
>+                            LOG("CalDAV: recv: " + str);
>+                        }
>+                    } else {
>+                        LOG("CalDAV: Failed to parse iTIP response.");
>+                    }
>+                }
>+            };
>+            if (this.verboseLogging()) {
>+                LOG("CalDAV: send: " + uploadData);
>+            }
>+            var streamLoader = createStreamLoader();
>+            calSendHttpRequest(streamLoader, httpchannel, streamListener);
>+        }
>+    },
>+

I think what we need to do here is collect failed caldav-sched deliveries in onStreamComplete() and
if !isSunbird() send iMIP invitations to those attendees. Probably want to do that in another bug.
I already have code that will get us a list of failed-delivery attendees, so this should be pretty
straightforward. For Sunbird we should just relnote that it cannot deliver to off-server attendees.


>Index: calendar/providers/caldav/public/calICalDavCalendar.idl
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/caldav/public/calICalDavCalendar.idl,v
>retrieving revision 1.1.2.1
>diff -u -8 -p -r1.1.2.1 calICalDavCalendar.idl
>--- calendar/providers/caldav/public/calICalDavCalendar.idl	23 Feb 2008 20:54:00 -0000	1.1.2.1
>+++ calendar/providers/caldav/public/calICalDavCalendar.idl	14 Aug 2008 17:49:06 -0000
>@@ -50,10 +50,30 @@ interface calICalDavCalendar : calICalen
>     readonly attribute AUTF8String authRealm;
> 
>     /**
>      * Refreshes the calendar. Used by calendars in the same authentication
>      * realm to coordinate refreshes
>      *
>      */
>     calIOperation safeRefresh();
>+
>+    /**
>+     * Whether the calendar is the first registered in its authrealm
>+     */
>+    readonly attribute boolean firstInRealm;
>+
>+    /**
>+     * Whether the calendar supports caldav-sched
>+     */
>+    readonly attribute boolean hasScheduling;
>+
>+    /**
>+     * The calendar's principal URI, usually a mailto:
>+     */
>+    readonly attribute AUTF8String calendarUserAddress;
>+
>+    /**
>+     * The calendar's scheduling outbox
>+     */
>+    readonly attribute nsIURI outBoxUrl;
> };
> 

Since we are now implementing calIItipTransport on the provider itself, we no longer 
need these changes to the idl: these properties don't currently need to be available
externally to the provider. The associated getters are thus not really needed, either,
though that strikes me as a matter of taste and is up to you.

r=browning with all that and the followon bug
Attachment #333789 - Flags: review?(browning) → review+
Bah. My fix for detecting items deleted by others regressed handling of items deleted from inbox as part of scheduling. This should work better:

                            var wasInBoxItem = thisCalendar.mItemInfoCache[itemToDelete.id].isInBoxItem;
                            if ((wasInBoxItem &&
                                thisCalendar.isInBox(aRefreshEvent.uri.spec)) ||
                                (!wasInBoxItem && wasInBoxItem !== undefined &&
                                !thisCalendar.isInBox(aRefreshEvent.uri.spec))) {

(in getUpdatedItems::etagListener.onStreamComplete() )
(In reply to comment #20)
> The UI ends up being a bit misleading here, since we have a checkbox offering
> to send invitations by email
> when we're (maybe) actually sending them by caldav-sched, whether the box is
> checked or not. A bigger issue,
> perhaps, is the case where the attendee set is going to require multiple
> transports: some caldav-sched, some
> email. In that case I think we want to send invitations via caldav-sched where
> possible and fall back to 
> email in cases where the caldav-sched delivery is unsuccessful and email
> available. IMO this current UI 
> is acceptable/relnoteable for 0.9 but should be fixed for 1.0. My gut feeling
> is that the distinction we'll 
> want to make is more "send invitations" than "send email".
Right, it would have been better (more generic) if we'd have something like a plain "Notify Attendees" without stating how those are notified. I've filed bug 451018 for that. Thinking twice, I've reverted my changes on adding X-MOZ-USE-IMIP, because those would complicate a further migration later. Instead we will go with the current (slightly misleading) string (stating "email") and add a relnote like

"The event dialog's checkbox "Send attendees invitation via email." not necessarily notifies attendees only by email, but in general switches notifications on or off."

> > 
> >-            if (status == 204 || status == 200) {
> >+            if (status == 204 || status == 201 || status == 200) {
> 
> IMO we should not accept a 201 status here indefinitely: it indicates a server
> error 
> of some kind that we want to know about. It's convenient to accept it for now
> since 
> a number of server impls don't get this right yet. I think we should include a
> comment
> to that effect here, and remove the 201 acceptance later.
done

> >                 LOG("CalDAV: Item modified successfully.");
> >                 var retVal = Components.results.NS_OK;
> >                 // Some CalDAV servers will modify items on PUT (add X-props,
> >                 // change location, etc) so we'd best re-fetch in order to know
> 
> We no longer allow servers to change item location on PUT, so as long as we're
> here let's change 
> this comment, possibly by changing "change location, etc" to "for instance".
> There's a similar comment
> in doAdoptItem() that could be changed this way as well.
done

> >+        } else if (aFromInBox) {
> >+            eventUri = makeURL(this.mInBoxUrl.spec + this.mItemInfoCache[aItem.id].locationPath);
> >+        } else {
> >+            eventUri = this.makeUri(this.mItemInfoCache[aItem.id].locationPath);
> >+        }
> > 
> 
> We need to make the condition here (aFromInbox ||
> this.mItemInfoCache[aItem.id].isInBoxItem) instead
> of simply (aFromInBox), else the user will not be able to delete an item from
> the scheduling-inbox
> without REPLYing to it
done

> >+                            if (thisCalendar.isInBox(aRefreshEvent.uri.path) &&
> >+                                thisCalendar.mItemInfoCache[itemToDelete.id].isInBoxItem) {
> >+                                delete thisCalendar.mItemInfoCache[itemToDelete.id];
> >+                                thisCalendar.mMemoryCalendar.deleteItem(itemToDelete,
> >+                                                                        getItemListener);
> >+                            }
> >+
> 
> Further testing shows this to be incomplete. The condition needs to be
> something like
>                              ((thisCalendar.isInBox(aRefreshEvent.uri.path) &&
>                                
> thisCalendar.mItemInfoCache[itemToDelete.id].isInBoxItem) ||
>                                 (!thisCalendar.isInBox(aRefreshEvent.uri.path)
> &&
>                                
> !thisCalendar.mItemInfoCache[itemToDelete.id].isInBoxItem))
> to properly delete from cache items that have been deleted from the server by
> other parties, in both the
> inbox and the calendar proper.
or
var wasInBoxItem = thisCalendar.mItemInfoCache[itemToDelete.id].isInBoxItem;
if ((wasInBoxItem && thisCalendar.isInBox(aRefreshEvent.uri.spec)) ||
    (wasInBoxItem === false && !thisCalendar.isInBox(aRefreshEvent.uri.spec))) {
done that.

> >+                for each (var prop in propertiesList) {
> >+                    if (prop.propertyName == "METHOD") {
> >+                    method = prop.value;
> >+                    }
> >+                }
> 
> AFAIK the item can only have one METHOD prop, so we might as well |break| after
> finding it.
done

> >                 var hrefPath = thisCalendar.ensurePath(href);
> >                 thisCalendar.mHrefIndex[hrefPath] = item.id;
> >-                thisCalendar.mItemInfoCache[item.id].etag = etag;
> 
> Whups! We really really really do not want to delete this line
back

> >     getFreeBusyIntervals: function caldav_getFreeBusyIntervals(
> >         aCalId, aRangeStart, aRangeEnd, aBusyTypes, aListener) {
> > 
> >-        if (!this.mHaveScheduling || !this.mOutBoxUrl || !this.mMailToUrl) {
> >-            LOG("CalDAV: Server does not support scheduling; freebusy query not possible");
> >-            return;
> >-        }
> >-
> 
> This check is not needed for our own purposes, but has been helpful in
> troubleshooting for
> people adding scheduling code, new authschemes, etc, to server implementations.
> I'd suggest 
> leaving it here for now for that purpose, but it's your call
My thinking was that I don't think it's really necessary, because the freebusy provider is only registered once the checked condition is met. I've put it back in though.

...
> I think what we need to do here is collect failed caldav-sched deliveries in
> onStreamComplete() and
> if !isSunbird() send iMIP invitations to those attendees. Probably want to do
> that in another bug.
> I already have code that will get us a list of failed-delivery attendees, so
> this should be pretty
> straightforward. For Sunbird we should just relnote that it cannot deliver to
> off-server attendees.
filed bug 451020 for this

> >Index: calendar/providers/caldav/public/calICalDavCalendar.idl
> Since we are now implementing calIItipTransport on the provider itself, we no
> longer 
> need these changes to the idl: these properties don't currently need to be
> available
> externally to the provider. The associated getters are thus not really needed,
> either,
> though that strikes me as a matter of taste and is up to you.
removed, though I left the getters/setters in

Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED, more fixes/enh in follow up bugs.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: relnote
Resolution: --- → FIXED
Whiteboard: [patch in hand] [needs-review]
Target Milestone: --- → 0.9
Remove relnote keyword as it has been decided that CalDAV-Sched won't make it into 0.9
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.