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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: mvl, Unassigned)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

spin-off from bug 327890. Serializing exceptions was fixed for ics, but not for caldav.
Whiteboard: [cal relnote]
Flags: blocking0.3+
retriaging, this doesn't block, but we'll gladly accept a clean patch before the slush.
Flags: blocking0.3+ → blocking0.3-
Posted patch serialize exceptions (obsolete) — Splinter Review
first cut at fixing this. FWIW, patch overlaps/coincides significantly with jminta's proposed patch for bug 322932
Attachment #239104 - Flags: second-review?(dmose)
Attachment #239104 - Flags: first-review?(jminta)
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.
Attachment #239104 - Flags: second-review?(dmose)
Attachment #239104 - Flags: first-review?(jminta)
Attachment #239104 - Flags: first-review-
Posted patch changes per reviewer (obsolete) — Splinter Review
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?
Attachment #239104 - Attachment is obsolete: true
Attachment #239603 - Flags: first-review?(jminta)
Poking jminta:
Any chance you can take a look at this so we can land it before CalConnect if it's ready?

Whiteboard: [cal relnote] → [cal relnote][needs jminta input and review]
Attachment #239603 - Attachment is obsolete: true
Attachment #239603 - Flags: first-review?(jminta)
Posted patch some slight cleanup (obsolete) — Splinter Review
Attachment #240205 - Flags: second-review?(dmose)
Posted patch fixes stuffSplinter Review
Attachment #240205 - Attachment is obsolete: true
Attachment #240225 - Flags: second-review?(dmose)
Attachment #240205 - Flags: second-review?(dmose)
Whiteboard: [cal relnote][needs jminta input and review] → [cal relnote][patch in hand][needs review jminta 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.
Attachment #240225 - Flags: second-review?(dmose) → second-review+
(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.
(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 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.
Attachment #240225 - Flags: first-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Spinoffs:
bug 354574
bug 354575
bug 354578
Whiteboard: [cal relnote][patch in hand][needs review jminta dmose] → [cal relnote]
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [cal relnote]
You need to log in before you can comment on or make changes to this bug.