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

RESOLVED FIXED

Status

Calendar
Provider: CalDAV
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Unassigned)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Updated

12 years ago
Whiteboard: [cal relnote]

Updated

11 years ago
Flags: blocking0.3+

Comment 1

11 years ago
retriaging, this doesn't block, but we'll gladly accept a clean patch before the slush.
Flags: blocking0.3+ → blocking0.3-

Comment 2

11 years ago
Created attachment 239104 [details] [diff] [review]
serialize exceptions

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 3

11 years ago
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-

Comment 4

11 years ago
Created attachment 239603 [details] [diff] [review]
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?
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)
Created attachment 240205 [details] [diff] [review]
some slight cleanup
Attachment #240205 - Flags: second-review?(dmose)
Created attachment 240225 [details] [diff] [review]
fixes stuff
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 8

11 years ago
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+

Comment 9

11 years ago
(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 11

11 years ago
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]

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

10 years ago
Whiteboard: [cal relnote]
You need to log in before you can comment on or make changes to this bug.