328576 - CalDAV providers doesn't serialize exceptions (ie modified recurrences)

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

spin-off from bug 327890. Serializing exceptions was fixed for ics, but not for caldav.

Comment 1

[Joey Minta]

retriaging, this doesn't block, but we'll gladly accept a clean patch before the slush.

Comment 2

[Bruno Browning]

Attachment: serialize exceptions

first cut at fixing this. FWIW, patch overlaps/coincides significantly with jminta's proposed patch for bug 322932

Comment 3

[Joey Minta]

Comment on attachment 239104 [details] [diff] [review]
serialize exceptions

+    calendarPromotedProps: {
+       "PRODID": true,
+       "VERSION": true
+    },
Since this is really private to the calendar, it should get an m prefix.

             eventUri.spec = this.mCalendarUri.spec + aNewItem.getProperty("X-MOZ-LOCATIONPATH");
-            LOG("using X-MOZ-LOCATIONPATH: " + eventUri.spec);
Why are we removing this log?  I don't care much either way, just curious.

+        
+        if (aOldItem.generation != aNewItem.generation) {
+            if (aListener)
Hmm... this looks to be a problem in the memory calendar too (and perhaps even more generally)  It seems, in theory, that it's possible for us to throw an error here, even after we've "successfully" modified an exception.  We should probably get a bug on file for this.

+        const icssvc = Cc["@mozilla.org/calendar/ics-service;1"].
+          getService(Components.interfaces.calIICSService);
nit: You should align getService

+           for each (exc in exceptions) {
var exc, to avoid strict warnings

+                while (calComp) {
+                    // Get unknown properties
+                    var prop = calComp.getFirstProperty("ANY");
+                    while (prop) {
+                        if (!thisCalendar.calendarPromotedProps[prop.propertyName]) {
+                            this.unmappedProperties.push(prop);
+                        }
+                        prop = calComp.getNextProperty("ANY");
+                    }
This is going to open the door for name-conflicts, if there are multiple calComps with the same x-props

+                                item.setProperty("X-MOZ-LOCATIONPATH", locationPath);
+                                //LOG("X-MOZ-LOCATIONPATH = " + itemUri.spec);
I don't like adding commented out code

+                                if (rid == null) {
+                                    unexpandedItems.push( item );
+                                    if (item.recurrenceInfo != null)
+                                        uid2parent[item.id] = item;
+                                }
Couple of nits here: braces around even 1-line if blocks, and no spaces around arguments in .push()

+                                    // force no recurrence info:
+                                    item.recurrenceInfo = null;
Is this really necessary?  It seems like it would be a huge bug if we got an item back that had both a recurrenceId and recurrenceInfo.

+            
+                for each (var item in unexpandedItems) {
+                    item.calendar = thisCalendar;
+                }
Is there a reason you're doing this in separate places for exceptions and unexpandedItems?  It seems like it should be the same in either case.  At the very least you should explain that with a comment.

Comment 4

[Bruno Browning]

Attachment: changes per reviewer

Thanks. I think this patch addresses your concerns, except for the comment about the generation-checking code, which I'm afraid I don't understand. It seems proper to throw an exception here, and the code does not "successfully" modify the exception (neither the backend nor the UI is updated with the changes from aNewItem). I'm happy to file another bug as suggested, but I'm stonkered as to what to put in it. Help?

Comment 5

[Matthew (lilmatt) Willis]

Poking jminta:
Any chance you can take a look at this so we can land it before CalConnect if it's ready?

Comment 6

[Matthew (lilmatt) Willis]

Attachment: some slight cleanup

Comment 7

[Matthew (lilmatt) Willis]

Attachment: fixes stuff

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 240225 [details] [diff] [review]
fixes stuff

Thanks for doing the heavy lifting to unhork CalDAV exceptions!

>+        if (aNewItem.parentItem != aNewItem) {
>+            aNewItem.parentItem.recurrenceInfo.modifyException(aNewItem);
>+            aNewItem = aNewItem.parentItem;
>+        }
>+        aOldItem = aOldItem.parentItem;

aNewItem and aOldItem should be clone()d before they are modified.

>+
>+        const icssvc = Cc["@mozilla.org/calendar/ics-service;1"].
>+                       getService(Components.interfaces.calIICSService);
>+        var modifiedItem = icssvc.createIcalComponent("VCALENDAR");
>+        modifiedItem.prodid = "-//Mozilla Calendar//NONSGML Sunbird//EN";
>+        modifiedItem.version = "2.0";

Please file a spinoff bug about centralizing the prodid and version somewhere, probably in calIICSService.

A couple of other spinoff bugs would be helpful too:

* Consider whether we should really fix this in the icalString getter instead of doing all this hand-serializing and de-serializing here.

* All providers also need to handle this case in addItem for import, iMIP, copy/paste.

r2=dmose with that stuff addressed.

Comment 9

[Joey Minta]

(In reply to comment #8)
> aNewItem and aOldItem should be clone()d before they are modified.
aNewItem should have been cloned before the caller passed it in here (since they wanted to modify it in the first place.  I don't think it's unreasonable to enforce a 'you must pass mutable new items in' requirement here.  aOldItem isn't being modified, from what I can tell, so it shouldn't need to be cloned either.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #9)
> (In reply to comment #8)
> > aNewItem and aOldItem should be clone()d before they are modified.
> aNewItem should have been cloned before the caller passed it in here (since
> they wanted to modify it in the first place.  I don't think it's unreasonable
> to enforce a 'you must pass mutable new items in' requirement here.

Ah, good catch.  In fact, calICalendar.idl already requires this.  So never mind that review comment.

> aOldItem isn't being modified, from what I can tell, so it shouldn't need to 
> be cloned either.

What I was really objecting to was assigning to aOldItem.  So I think you're right that we don't actually need to clone, but if this patch really needs to address aOldItem.parent, it should do that directly, or else create a different variable to use as shorthand for that rather than assigning to aOldItem.

Comment 11

[Joey Minta]

Comment on attachment 240225 [details] [diff] [review]
fixes stuff

+                this.mICSService = Cc["@mozilla.org/calendar/ics-service;1"].
+                                   getService(Components.interfaces.calIICSService);
Don't assign to |this| unless you're planning to cache and/or use elsewhere.  Since it doesn't look like you are, just use a normal variable.

+                for each (var item in excItems) {
+                    var parent = uid2parent[item.id];
+                    if (parent == null) {
+                        LOG( "no parent item for rid=" + item.recurrenceId );
+                    } else {
+                        item.parentItem = parent;
+                        item.parentItem.recurrenceInfo.modifyException(item);
+                    }
+                }
+                // if we loop over both excItems and unexpandedItems using 'item'
+                // we can be confident that 'item' means something below
+                for each (var item in unexpandedItems) {
+                    item.calendar = thisCalendar;
+                }
You declare |var item| twice (actually 3 times) in the same scope, that's un-pretty.

+            LOG("Error while loading " + fileurl.spec );
No trailing space after .spec please.

r1=jminta with that.

Comment 12

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Spinoffs:
bug 354574
bug 354575
bug 354578