355270 - various problems interoperating with cosmo

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[cmtalbert]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20060918 Firefox/2.0
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20061002 Sunbird/0.3

After creating a CalDav calendar and adding events to it, those events disappear once you change the view. I have been able to verify this behavior with both Windows and Mac Sunbird 0.3 builds, and I have verified it with both Oracle CalDav Server as well as OSAF Cosmo CalDav server. With Cosmo, I was able to independently (through the OSAF cosmo WebDav interface) verify that the events were uploaded to the server. It seems to be some kind of issue with the views themselves not refreshing or reloading the calendar data from the CalDav servers.

I was also able to replicate this behavior with Lightning 0.3 RC1.

Reproducible: Always

Steps to Reproduce:
1. Create a CalDav Server account
2. Setup the CalDav calendar on Sunbird (or Lightning)
3. Create events on the CalDav Calendar in the month view
4. Go to the week view corresponding to the week of events you just added
5. Go back to the month view
6. Restart the application.

Actual Results:  
In steps 4, 5, and 6, the events that were added to the CalDav Calendar are no longer displayed.

Even after attempting "Reload remote Calendars", events are still not displayed.

Expected Results:  
The calendar events should be displayed in steps 4, 5, and 6 just as they were when you entered them.

Comment 1

[Sebastian Schwieger]

This works for me. I use a Cosmo 0.4 server and a calendar previously generated by Scooby interface. So maybe the real bug here is that creations of calendars fail (bug 349793; but in this case the events shouldn't show up at (3) ).

using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061002 Sunbird/0.3

Comment 2

[cmtalbert]

This looks like it might be intermittent. I attempted to "Subscribe" to the Cosmo calendar but no new calendar showed up. After attempting to subscribe to the Cosmo calendar two times, the Oracle CalDav calendar (which had not been changed at all) began working. It no longer displays this issue, however, the Cosmo calendar still displays the problem.

Comment 3

[cmtalbert]

Reproducible: Intermittent

Comment 4

[cmtalbert]

I have debugged the reason for this problem. The problem is occuring because getItems is returning from the "if (this.mAuthenticationStatus == kCaldavFirstRequestSent)" statement block. The queued request in mPendingStartupRequests is never getting addressed, and getItems is never run after this point, leaving the user with an empty view. 

Here is the if statement in lxr:
http://lxr.mozilla.org/seamonkey/source/calendar/providers/caldav/calDavCalendar.js#829

Since this has to do with the state of the authentication, that may explain why the problem appears intermittently.

Comment 5

[Matthew (lilmatt) Willis]

Regression from bug 335462

Comment 6

[Bruno Browning]

Using 2006-10-03 builds and Cosmo 0.4, I've tried unsuccessfully to duplicate this on:
Linux FC5
Linux Ubuntu Dapper
Windows XP
MacOS 10.3/PPC
MacOS 10.4/Intel

So I'm wondering: any extensions/add-ins/whatever involved? Also, do you have Sunbird remember your password (or embed the auth info in your URI)?

Comment 7

[cmtalbert]

(In reply to comment #6)
> So I'm wondering: any extensions/add-ins/whatever involved? Also, do you have
> Sunbird remember your password (or embed the auth info in your URI)?
> 

No add-ins or extensions.
I did have Sunbird remember the password. The auth info is not embedded in the URI for Cosmo as far as I know.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

This sure smells like a timing-sensitive thing, so it might be more likely to be reproducible while testing against a CalDAV server that's not on the local network or machine.  If we can get a low-risk patch for this today, we should consider taking it.

Comment 9

[Bruno Browning]

I can reproduce this, but only with cosmo-demo.osafoundation.org, which should be running Cosmo 0.4. I made myself an account, made a calendar in Sunbird pointed at that account, and Sunbird showed the behavior described in this bug. Manually issuing a REPORT with curl (using a query that works with my Cosmo 0.4 installations) produced an empty report, and a manual PROPFIND seemed to show that my Cosmo 'home directory' was a WebDAV but not CalDAV store. So I MKCALENDAR'ed a collection in it with curl, made a Sunbird calendar with that URI, and that seems to work fine (though very slow). 

The return's from that 'if' mentioned in comment #4 are just what Sb should be doing before the server query returns the first item - which I don't think cosmo-demo is currently doing when the REPORT URI is the account directory. Is that the configuration you are using?

Comment 10

[Mike Taylor [:bear]]

I'm with OSAF Cosmo team and wanted to make a note to this bug about Cosmo 0.5

0.5 is going to be a release candidate probably next week and it contains quite a few DAV and CalDAV related fixes along with auth related fixes.  If your seeing intermittant errors they may be fixed if your only seeing them against Cosmo.

Once Cosmo has a 0.5 RC1 build we will be making an instance of it available for testing and I'll try to post either here or in #calendar the URL

Comment 11

[Brian Moseley]

i'm a cosmo developer. here's my take on this bug.

the root issue seems to be that the reporter wasn't aware that the home collection for a cosmo user is not in fact a caldav calendar collection. it's a standard webdav collection that supports MKCALENDAR for creating calendar collections within. when creating a calendar on the server, sunbird apparently doesn't check to make sure that the collection on the server is actually a calendar collection (by checking the DAV:resourcetype property for the <CALDAV:calendar/> xml element). so sunbird puts the events into the home collection, but when it sends a REPORT to populate a view, the REPORT returns no events since that method is not supported on the cosmo home collection.

dmose suggests that that it's not most people's expected behavior for cosmo not to autocreate a default calendar, using the analogy that every mail server autocreates an inbox folder rather than requiring clients to do that. that is certainly true; however, in the calendar world, many clients are sync-based rather than relying on live reporting, and sync-based clients usually have their own default calendars that they want to copy to the server.

it is very likely that cosmo 0.6 will offer users the option of creating a default calendar when they sign up for accounts. it will probably also allow users to create calendars through the web ui as well as import ics files to populate calendars. if not 0.6, then certainly some pre-1.0 release. hopefully this will be more clear to users.

also note that the OSAF wiki has a page for using cosmo with sunbird at <http://wiki.osafoundation.org/bin/view/Documentation/CosmoWithSunbird>. edits to this page to more accurately reflect the current state of cosmo/sunbird interop would be welcome indeed.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

This is really a combination of various different bugs that we need to spin off.  These include:

1) if a collection does not have the DAV:resourcetype property of <CALDAV:calendar/>, we should error out instead of trying to interpret it as a calendar collection

2) when writing events fails like this, we should notice and alert the user

3) when given a URL for a CalDAV calendar, we should first find the 
calendar-home-set collection as per spec, and then enumerate any calendar collections there and add them.  We probably should check on each login to see if any new calendar collections have been created since last time.

4) if there are no calendar collections in the calendar-home-set collection, we should use MKCALENDAR to create one

Scooby, the Web-UI for Cosmo, will autocreate a calendar collection called Scooby on first login.  What we should do for 0.3, I think, is to edit the wiki page that Brian points out, suggesting that people log into Cosmo with Scooby first, then figure out the URL to the Scooby collection, and then configure the Sunbird calendar.  We can then add a relnote to 0.3 pointing people to that page.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

3) should actually refer to a "WebDAV principal" rather than a "CalDAV calendar".

Comment 14

[Brian Moseley]

additional comment: the CALDAV:calendar-home-set property is specified for the webdav principal resource corresponding to the user. so your workflow might be something like:

1) ask the user for the principal url (in cosmo this is just the user's home collection url, which they can find on their account info page when they log into the web ui)

2) PROPFIND the principal url for CALDAV:calendar-home-set

3) PROPFIND each url specified in that property to see a) if it's a calendar collection itself or b) if it's a webdav collection, in which case it might contain calendar collections

4) ask the user what calendar collection to use or MKCALENDAR a new one

note that in cosmo 0.4, the home collection does not function as a webdav principal and does not provide CALDAV:calendar-home-set. this feature will be part of 0.5 which ships in a couple weeks.

also note that in cosmo 0.5, the default calendar created by logging into the cosmo web ui is called "Cosmo". the web app formerly known as scooby was merged into cosmo 0.5 and that name is now no longer to be spoken, upon pain of being struck with a large hammer.

Comment 15

[cmtalbert]

(In reply to comment #9)
> The return's from that 'if' mentioned in comment #4 are just what Sb should be
> doing before the server query returns the first item - which I don't think
> cosmo-demo is currently doing when the REPORT URI is the account directory. Is
> that the configuration you are using?

That's the configuration I was using when I encountered the bug.

Addressing Comment 12 Point 2.
Dan writes: 2) when writing events fails like this, we should notice and alert the user
As near as I was able to determine, by looking both at the Sunbird log and the Cosmo server, the writing of events never failed. The failure was in retrieving the events after writing them. Also, once the Oracle server started working, I saw that all the events I created were on the server. So, it's not a problem writing TO it. It's a problem reading FROM it.

And a general comment on the entire thread:
When I first encountered this problem, I also saw it with an Oracle CalDav calendar. However, the oracle calendar corrected itself as I kept playing with the scenarios, (restarting the app, adding dump statements into JS etc). Once the oracle calendar corrected itself, it has never experienced the problem again, though the Cosmo calendar always demonstrated the problem. I didn't want that bit of information to be lost. To me that says that either Oracle and Cosmo are doing something very simliar in their servers or sunbird is doing something incorrect when it does a GetEvents.

Comment 16

[Bruno Browning]

Yes, that was my only quibble with Dan's comments. The PUT is not failing, per se, but the item is getting PUT to a non-CalDAV-REPORTable location. I think implementing his point #1 will keep that from happening.

So far as the Oracle ... it occurs to me that we'll see this kind of behavior any time REPORTs consistently come back empty, which would could happen for a number of reasons (auth failure, incorrect URI, ...). We're going to need to detect such things and deal with them, if only with e.g. a "Login Failed" dialog. I don't know if that's what happened in this case, but it's something that will need to be dealt with.

Comment 17

[Bruno Browning]

Attachment: propfind calendar uri and set calendar read-only if not caldav

this patch addresses dmose's point 1 in comment 12 by propfinding the calendar uri and setting the calendar readonly if that uri is not a caldav calendar. It also throws hopefully-useful errors for the error console. It doesn't otherwise provide user advisories (dmose's point 2) but I think that's another patch/bug.

This patch is specifically for CalDAV, but the propfind parts (or rather an expanded version of them) will also be needed for ics-on-webdav and e.g. bug 306495, so it seems to make sense to place that in calUtils rather than the CalDAV provider.

Comment 18

[Bruno Browning]

Attachment: patch rev 2, a little more righter

made the propfind function a little more generally useful.

Comment 19

[cmtalbert]

Comment on attachment 244277 [details] [diff] [review]
patch rev 2, a little more righter

Overall, this is a good patch to address the first problem.
I've tried it and it does change the "calendar" to a read only
resource when it realizes that you've used a URI that points at 
a collection instead of a calendar.

I have a few nit-picky comments mostly regarding line folding,
but you have my r+ with those changes.  

Sorry it took so long to get to this review.

My comments are below:

>Index: calendar/base/src/calUtils.js
=======================================================
>+const kDavResourceTypeNone = 0;
>+const kDavResourceTypeCollection = 1;
>+const kDavResourceTypeCalendar = 2;
Nit 1: Since these constants are defined here, but not used 
in this file, can you write a comment here about why they are 
defined here? Perhaps something like: These need to be used in both WebDav and Caldav
providers....

>+    davGetPropsListener.onOperationComplete = function(aStatusCode, aResource, aOperation, aClosure) {
Nit 2: This line ^^^ could folded so that it fits within 80 chars


>+    davGetPropsListener.onOperationDetail = function(aStatusCode, aResource, aOperation, aDetail, aClosure) {
Nit 3: This one too ^^^^

>+       var props = aDetail.QueryInterface(Components.interfaces.nsIProperties);
Nit 4: Up at the top of this file, Ci is defined to be Components.interfaces, use Ci here.

>+       try {
>+              data = props.get(aProps, Components.interfaces.nsISupportsString).toString();
Nit 5: Here too, use Ci to get this line under 80 chars.

>+     var webSvc = Components.classes['@mozilla.org/webdav/service;1']
>+                                       .getService(Components.interfaces.nsIWebDAVService);
Nit 6: Use Cc here to shorten Components.classes

>+     webSvc.getResourceProperties(res, 1, aProps, false,
>+                                             davGetPropsListener, aCalendar, null);
Nit 7: Align your davGetProperties with res to pull that line within 80 chars. It would
       also be good to include a comment here about why you default to 1 for 
       propCount here. It would seem that it would make more sense to use aProps.length

>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>+        if (this.mAuthenticationStatus == kCaldavNoAuthentication) {
>+           this.mAuthenticationStatus = kCaldavFirstRequestSent;
>+           davGetResourceProperties(this.mCalendarUri, ['DAV: resourcetype'], this.onCheckResourceType,
>+                                    this);
Nit 8: At this line ^^^ could you fold the parameters to 
       davGetResourceProperties so that they all fit within 80 chars?

>             var req = new Array(aItemFilter, aCount, aRangeStart, aRangeEnd, aListener);
Nit 9: Also fold here for the req array ^^^

Comment 20

[Bruno Browning]

Attachment: patch rev 3, changes per r1 and a bit more

Thanks. This rev addresses r1's concerns (and fixes a couple of other long lines r1 didn't mention). It also incorporates a couple of other changes to davGetResourceProperties that I found necessary while working on bug 349793, specifically adding an aDepth arg, and using an array rather than string to store data from server. Re-requesting first-review because of those exta changes; sorry.

Note that this patch is not entirely compatible with the proposed patch for bug 357756, and will need to be modified slightly if that goes in first.

Comment 21

[cmtalbert]

Comment on attachment 244973 [details] [diff] [review]
patch rev 3, changes per r1 and a bit more

r+

Comment 22

[Joey Minta]

Comment on attachment 244973 [details] [diff] [review]
patch rev 3, changes per r1 and a bit more


Sadly, I really don't feel qualified to give this code the thorough review it needs, and we're probably going to need to wait for dmose to get back from vacation.  That said, some obvious things that jump out at me

First off, we need some consistency with where we put code.  Right now, calUtils depends on nothing, but a lot of code depends on it.  Because webDavListener is defined in calDavCalendar.js, but you use it in calUtils, you're requiring that we always ship that file.  I'd rather this code just live in calDavCalendar.js until there's a proven need for it elsewhere

+       aResource, aOperation, aClosure) {
+          aFunction(aUri, data, aCalendar);
Maybe this is a different kind of closure, but if it's the kind I know (javascript) then you'll need to do more than just aFunction();

+    davGetPropsListener.onOperationDetail = function(aStatusCode, 
These are all variable assignments, so the final } needs to be followed with a ;

+          try {
+                 data.push(props.get(aProps, Ci.nsISupportsString).toString());
+              } catch(e) {
+                 LOG("error " + e + " fetching properties");
+              }
the '}' for catch aligns with the t in try, not the {

+     var webSvc = Cc['@mozilla.org/webdav/service;1']
+                                       .getService(Ci.nsIWebDAVService);
The . aligns with C

+        if (this.mAuthenticationStatus == kCaldavNoAuthentication) {
+           this.mAuthenticationStatus = kCaldavFirstRequestSent;
All this code should align with the t in (this (4-space indenting should be used throughout please)

Other than that, things look reasonable to my eyes, but my knowledge of webdav is really limited. :-(

Comment 23

[Matthew (lilmatt) Willis]

(In reply to comment #22)
> First off, we need some consistency with where we put code.  Right now,
> calUtils depends on nothing, but a lot of code depends on it.  Because
> webDavListener is defined in calDavCalendar.js, but you use it in calUtils,
> you're requiring that we always ship that file.  I'd rather this code just
> live in calDavCalendar.js until there's a proven need for it elsewhere.

This is a good idea.  Yes we will likely want to use some of this in other places in the future, but until that time it just confuses things and causes dependencies we don't want. Whenever we _do_ decide to use the DAV stuff in other *DAV providers, we could split it out to davUtils.js or something.

All that said, please submit another patch.  dmose is back from vacation, and we'd definitely like to get this stuff in!

Comment 24

[Bruno Browning]

Attachment: move function to CalDAV provider

This moves the propfinding function into the CalDAV provider, and has it check only for the resource type; if we need a more generic version in some future davUtils.js that will be easy enough to do. It just throws some errors for the error console: we'll probably want alerts at some point but I don't think we need them for 0.5, and I rather expect this will get refactored significantly before 1.0 as we figure out subscribe/create/unsubscribe/delete.

Also fixes bug 360076 since we authenticate in the propfind and don't rely on getItems() returning anything first time around.

Comment 25

[Bruno Browning]

Attachment: try/catch for non-net 'caldav' uris

added try/catch to the propfind, pursuant to bug 370139

Comment 26

[Simon Paquet [:sipaq]]

Comment on attachment 254910 [details] [diff] [review]
try/catch for non-net 'caldav' uris

Moving this to an active module peer.

Comment 27

[Matthew (lilmatt) Willis]

Comment on attachment 254910 [details] [diff] [review]
try/catch for non-net 'caldav' uris

>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>+// used in checking calendar URI for (Cal)DAV-ness
>+const kDavResourceTypeNone = 0;
>+const kDavResourceTypeCollection = 1;
>+const kDavResourceTypeCalendar = 2;
>+
> calDavCalendar.prototype = {
>     //
>     // nsISupports interface
>@@ -842,17 +847,20 @@ calDavCalendar.prototype = {
>     //                in calIOperationListener aListener );
>     getItems: function (aItemFilter, aCount, aRangeStart, aRangeEnd, aListener)
>     {
>-        if (!aListener)
>+        if (!aListener) {
>             return;
>+        }
>+
>+        if (this.mAuthenticationStatus == kCaldavNoAuthentication) {
>+           this.mAuthenticationStatus = kCaldavFirstRequestSent;
>+           this.checkDavResourceType();
>+        }
> 
>         if (this.mAuthenticationStatus == kCaldavFirstRequestSent) {
>             var req = new Array(aItemFilter, aCount, aRangeStart, aRangeEnd, aListener);
>             this.mPendingStartupRequests.push(req);
>             return;
>         }
>-        if (this.mAuthenticationStatus == kCaldavNoAuthentication) {
>-            this.mAuthenticationStatus = kCaldavFirstRequestSent;
>-        }
> 
>         // this is our basic report xml
>         var C = new Namespace("C", "urn:ietf:params:xml:ns:caldav");
>@@ -1025,6 +1033,78 @@ calDavCalendar.prototype = {
>             }
>             return;
>         },
>+
>+    /**
>+     * Checks that the calendar URI exists and is a CalDAV calendar
>+     *
>+     */
>+    checkDavResourceType: function checkDavResourceType() {
>+
>+        var listener = new WebDavListener();
>+        var resourceTypeXml = null;
>+        var resourceType = kDavResourceTypeNone;
>+        var thisCalendar = this;
>+        listener.onOperationComplete =
>+            function checkDavResourceType_oOC(aStatusCode, aResource, aOperation,
>+                                              aClosure) {
Move aOperation to the next line (with aClosure)

>+
>+            if (resourceType == null || resourceType == kDavResourceTypeNone) {
>+                thisCalendar.mReadOnly = true;
>+                throw("The resource at " + thisCalendar.mUri.spec +
>+                      " is either not DAV or not available\n");
>+            }
>+
>+            if (resourceType == kDavResourceTypeCollection) {
>+                thisCalendar.mReadOnly = true;
>+                throw("The resource at " + thisCalendar.mUri.spec +
>+                      " is a DAV collection but not a CalDAV calendar\n");
>+            }
>+
>+            // we've authenticated in the process of PROPFINDing and can flush
>+            // the getItems request queue
>+            thisCalendar.mAuthenticationStatus = kCaldavFreshlyAuthenticated;
>+            if (thisCalendar.mPendingStartupRequests.length > 0) {
>+                var req = thisCalendar.mPendingStartupRequests.pop();
>+                thisCalendar.getItems(req[0], req[1], req[2], req[3], req[4]);
>+            }
>+        }
Please unify this with the similar code elsewhere in this file using a helper class, or some other way to prevent you from repeating yourself.

>+
>+        listener.onOperationDetail =
>+            function checkDavResourceType_oOD(aStatusCode, aResource, aOperation,
>+                                              aDetail, aClosure) {
Move aOperation to the next line (with aDetail). Wrap aClosure to next line if necessary.

>+
>+            var prop = aDetail.QueryInterface(Ci.nsIProperties);
>+
>+            try {
>+                resourceTypeXml = prop.get('DAV: resourcetype',
>+                                           Ci.nsISupportsString).toString();
Why are we using single quotes here? If for no pertinent reason, please switch to double quotes for consistency.

>+            } catch(e) {
>+                LOG("error " + e + " fetching resource type");
>+            }
Please add a space after "catch".  I also prefer "ex" as the exception's var name.  It's also used more than (e) in this particular file.

>+            if (resourceTypeXml.length == 0) {
>+                resourceType = kDavResourceTypeNone;
>+            }
>+            if (resourceTypeXml.indexOf("collection") != -1) {
>+                resourceType = kDavResourceTypeCollection;
>+            }
>+            if (resourceTypeXml.indexOf("calendar") != -1) {
>+                resourceType = kDavResourceTypeCalendar;
>+            }
>+        }
Should these instead be stacked as if() else if() else if() ?

>+
>+        var res = new WebDavResource(this.mUri);
>+        var webSvc = Cc['@mozilla.org/webdav/service;1']
>+                     .getService(Ci.nsIWebDAVService);
Put . on the end of the previous line and align "get" beneath "C"

>+        try {webSvc.getResourceProperties(res, 1, ['DAV: resourcetype'], false,
>+                                          listener, this, null);
Put the "try {" on it's own line.  Also it's the same question about the single quotes.

>+        } catch (ex) {
>+            thisCalendar.mReadOnly = true;
>+            Components.utils.reportError(
>+                "Unable to get properties of resource " + thisCalendar.mUri.spec
>+                + " (not a network resource?); setting read-only");
>+        }
>+    },
>+
>     refresh: function calDAV_refresh() {
>         // XXX-fill this in, get a report for modifications+onModifyItem
>     },


I'd like to see this another time and get those questions answered before moving forward on this.

Comment 28

[Bruno Browning]

Attachment: edits per review [checked in]

I think this addresses your concerns. 

I was using singlequotes to be consistent with the way this is done in calICSCalendar.js, but the doublequotes work fine so I changed them. At least there's a choice of what to be consistent with. ;-)

The ifs were intentionally not written as elseifs (since CalDAV calendar collections are inherently collections). I went ahead and reordered things so that they could be written as elseifs.

I've changed the dot location as requested, but I have a question: the prevailing style in this file (and most others I've encountered in calendarland) is to split with the dot beginning the continuation line rather than closing the first one. Are we changing the preferred style?

Comment 29

[Matthew (lilmatt) Willis]

Comment on attachment 257903 [details] [diff] [review]
edits per review [checked in]

r=lilmatt

Comment 30

[Matthew (lilmatt) Willis]

(In reply to comment #28)
I was following the style used elsewhere in toolkit and browser where the dot is on the trailing end.


Patch checked in on MOZILLA_1_8_BRANCH and trunk.

What else needs to be fixed before we can close this bug?

Comment 31

[Bruno Browning]

(In reply to comment #30)

> What else needs to be fixed before we can close this bug?
> 

My take, following dmose's four-point todo list in comment #12:
1. Is addressed by this patch
2. Is, I think, wrong. The PUT does not actually fail, so there's no error to catch. This patch, though, should prevent the provider from doing PUTs to non-CalDAV resources, which accomplishes what dmose wanted here
3. Is I think already in the scope of bug 306495 (which could benefit from some of the comments made here)
4. Is addressed partly by bug 306495 and partly by bug 349793

So I think this bug can be closed and a comment added to bug 306495 pointing back to this discussion. I think the bug subject is misleading: this is not Cosmo-specific and it is, I think, a single defect (treating a DAV collection as a CalDAV collection) rather than a variety of them. 

Comment 32

[Sebastian Schwieger]

Drive by comment: The error messages introduced in this patch should be localizable. Thats probably worth a follow-up bug.

Comment 33

[Bruno Browning]

(In reply to comment #32)
> Drive by comment: The error messages introduced in this patch should be
> localizable. Thats probably worth a follow-up bug.
> 

addressing this in bug 374566

Comment 34

[Matthew (lilmatt) Willis]

Not going to make the 0.5 train.

Comment 35

[Tim]

I have made some tries with Cosmo 0.6 and I found out the following problem when subscribing with a simple calendar with one event. When calling getItems() in calDavCalendar.js, the Cosmo server returns a code 207 and not 200. This code is considered as a multi-status code and I suppose that it can be returned if the request is successful. Am I wrong ?
At least, the calendar is properly displayed in the month view. But the problem is that the listener is called with Components.results.NS_ERROR_FAILURE which reports an error. Note that the error code 207 is not accessible in the listener.
Another weird thing in the code is that Components.results.NS_ERROR_FAILURE is also passed to the listener even if the result is 200 (OK). So I wonder if there is not a slight code mistake: "rv" variable should be used instead of Components.results.NS_ERROR_FAILURE

Here is the code part I am talking about in calDavCalendar.js:

// parse aStatusCode
var rv;
var errString;
if (aStatusCode == 200) { // XXX better error checking
    rv = Components.results.NS_OK;
} else {
    rv = Components.results.NS_ERROR_FAILURE;
    errString = "XXX something bad happened";
}
// call back the listener
try {
    if (aListener) {
        aListener.onOperationComplete(thisCalendar,
            Components.results.
            NS_ERROR_FAILURE,       <== rv instead ?
            aListener.GET, null,
            errString);
    }

If this problem is irrelevant, a duplicates, needs to be addressed in another bug or was already fixed, let me know it.
Thanks.

Comment 36

[Sebastian Schwieger]

The actual bug here is already fixed. I added a note to bug 306495 as suggested by Bruno. Kal, I cannot answer your question, but for sure it belongs to a new (or another) bug. Please file a new bug if you get no response in a reasonable timeframe.
Resolving as FIXED per comment 31.

Comment 37

[Tim]

Thanks. I based myself on the NEW status. I will post a new bug report if there is no additional comment.

Comment 38

[Bruno Browning]

(In reply to comment #35)
> If this problem is irrelevant, a duplicates, needs to be addressed in another
> bug or was already fixed, let me know it.
> Thanks.
> 

You're right about the code, of course, but I don't see it as cosmo interop so much as a reflection of the fact that (as noted in the code) error reporting/handling in the CalDAV provider is, um, bad. I hope we can deal with that in the 0.7 timeframe; the beginnings of that are in a patch in bug 374566. I haven't filed an omnibus "error handling sucks" bug in favor of dealing with the problem piecemeal. Please do go ahead and file a bug about this particular instance. I suspect we can stomp it shortly after 0.5 ships.