Closed Bug 362698 Opened 13 years ago Closed 13 years ago

CalDAV provider should check etags on add/modify/delete

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Sunbird 0.5

People

(Reporter: browning, Assigned: browning)

References

Details

(Whiteboard: [patch in hand] [late-l10n])

Attachments

(2 files, 7 obsolete files)

The CalDAV provider does not check etags before modifying data on the server, which can lead to a 'lost update' problem, see bug 329570 . It should.
Attached patch first attempt at CalDAV etags (obsolete) — Splinter Review
I suspect this needs UI-review, but I'm afraid I'm not sure from whom to request that, nor when in the review process.
Assignee: nobody → browning
Status: NEW → ASSIGNED
Attachment #247378 - Flags: first-review?(lilmatt)
(In reply to comment #1)
> I suspect this needs UI-review, but I'm afraid I'm not sure from whom to
> request that, nor when in the review process.

dmose and mvl are the ui-review folks.
Either attach screenshots as .png and set ui-r? to one of them on the screenshot, or if you're looking for ui-r before you start coding (to minimize rework), do it as ASCII art in a .txt file and set ui-r? on that.  Earlier ui-r is generally better, so you don't have to recode stuff based solely on ui-r.
Comment on attachment 247378 [details] [diff] [review]
first attempt at CalDAV etags

>Index: calendar/locales/en-US/chrome/calendar/calendar.properties
>===================================================================
>+itemModifiedOnServerTitle=Item modified on server
>+itemModifiedOnServer=This item has been modified on the server.
>+ModifyWillLoseData=Proceeding with this modification will cause loss of the changed data on the server
>+DeleteWillLoseData=Proceeding with this deletion will cause loss of the changed data on the server
>+UpdateFromServer=Update From Server
>+Proceed=Proceed
For the above 4 lines use a leading lowercase letter like the first 2.

I'll leave the wordsmithing to the ui-r folks and possibly beltzner.


>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>@@ -176,18 +182,20 @@ calDavCalendar.prototype = {
>     get uri() { return this.mUri; },
>     set uri(aUri) { this.mUri = aUri; },
> 
>-    get mCalendarUri() { 
>+    get mCalendarUri() {
>         calUri = this.mUri.clone();
>         var parts = calUri.spec.split('?');
>         if (parts.length > 1) {
>-            calUri.spec = parts.shift();
>-            this.mUriParams = '?' + parts.join('?');
>+           calUri.spec = parts.shift();
>+           this.mUriParams = '?' + parts.join('?');
>         }
>+
>         if (calUri.spec.charAt(calUri.spec.length-1) != '/') {
>             calUri.spec += "/";
>         }
>         return calUri;
>     },
>+
>     
>     mMakeUri: function caldav_makeUri(aInsertString) {
>         var spec = this.mCalendarUri.spec + aInsertString;
These whitespace changes are unrelated, and in some cases (3 space indenting), wrong.  Please remove these from the patch.

>@@ -221,19 +229,175 @@ calDavCalendar.prototype = {
>         this.mObservers = this.mObservers.filter(
>             function(o) {return (o != aObserver);});
>     },
>+    
>+    mEtagCache: Array(),
>+    
The two blank lines have trailing spaces.

>+    /**
>+     * Fetches etag from server and compares with local cached version 
Trailing space ^^

>+     * before add/adopt/modify/delete item
>+     *
>+     * @param aMethod     requested method (adopt/modify/delete) 
Trailing space ^^

>+     * @param aItem        item to check
>+     * @param aListener    listener from original request
Align all 4 description columns. 

>+     * @param aOldItem    aOldItem argument in modifyItem requests
>+     */
>+    fetchEtag: function (aMethod, aItem, aListener, aOldItem) {
Anonymous functions such as this are difficult to debug as they show up as "anonymous" in Venkman and in exceptions.  Please name your function.  Perhaps function caldavFetchEtag( ?

>+
>+       if (this.readOnly) {
>+            throw calIErrors.CAL_IS_READONLY;
Weird 5 space indenting on this line ^^. Use 4.
>+       }
>+
>+       var serverEtag = null;
>+       var listener = new WebDavListener();
>+
>+       var thisCalendar = this;
>+
>+       var itemUri = this.mCalendarUri.clone();
>+
>+       try {
>+            itemUri.spec = this.mMakeUri(aItem
>+                                .getProperty("X-MOZ-LOCATIONPATH"));
Don't wrap this ^^. It ends up as 81 chars. We'll live with that. :)

>+            LOG("using X-MOZ-LOCATIONPATH: " + itemUri.spec);
>+       } catch (ex) {
>+            // XXX how are we REALLY supposed to figure this out?
>+            itemUri.spec = this.mMakeUri(aItem.id + ".ics");
>+       }
Weird 5 space indenting on the lines inside the try/catch. Use 4.

>+
>+       var itemResource = new WebDavResource(itemUri);
>+
>+       listener.onOperationComplete =
>+           function onOperationComplete(aStatusCode, aResource, aOperation,
>+                                     aClosure) {
Align aClosure beneath aStatusCode.

>+           var mismatch = true;
>+           if (serverEtag == thisCalendar.mEtagCache[aItem.id]) {
>+              mismatch = false;
>+           }
Perhaps var mismatch = (serverEtag == thisCalendar.mEtagCache[aItem.id]); would be simpler?

>+           switch(aMethod) {
Add a space between switch and (aMethod).
>+              case kCaldavAdoptItem:
>+                 if (serverEtag != null) {
>+                    // either there's a server error or we're copying an item
>+                    // onto itself: either way the safe thing is to bail
>+                    LOG("non-null etag in adoptItem");
Please prepend "CalDAV: " to each of your error messages in this switch statement to aid in debugging.

>+                    thisCalendar.readOnly = true;
>+                 } else {
>+                    thisCalendar.performAdoptItem(aItem, aListener);
>+                 }
>+                 break;
>+
>+              case kCaldavModifyItem:
>+                 if (mismatch) {
>+                    LOG("etag mismatch in modifyItem");
>+                    thisCalendar.promptOverwrite(aMethod, aItem,
>+                        aListener, aOldItem);
This line is wrapped weird. Please align whichever aParameter(s) end up being wrapped after moving to 4 space indenting beneath aMethod.

>+                 } else {
>+                    thisCalendar.performModifyItem(aItem, aOldItem, aListener);
>+                 }
>+                 break;
>+
>+              case kCaldavDeleteItem:
>+                 if (mismatch) {
>+                    LOG("etag mismatch in deleteItem");
>+                    thisCalendar.promptOverwrite(aMethod, aItem,
>+                        aListener, null);
Same alignment note here as from "case kCaldavModifyItem"

>+                 } else {
>+                    thisCalendar.performDeleteItem(aItem, aListener);
>+                 }
>+                 break;
>+
>+              default:
>+                 thisCalendar.mEtagCache[aItem.id] = serverEtag;
>+                 break;
>+           }
>+
>+        }
This whole switch statement uses 3 space indenting. Please use 4.

>+
>+        listener.onOperationDetail = function(aStatusCode, aResource,
>+                                                    aOperation, aDetail,
>+                                                    aClosure) {
Align aOperation beneath aStatusCode. Then aClosure can live on the same line as aOperation and aDetail.

>+           var props = aDetail.QueryInterface(Components
>+                                               .interfaces.nsIProperties);
This line is indented using 3 space indenting. Use 4.  Also put it all on one line.

>+           serverEtag = props.get('DAV: getetag', Components.interfaces
>+                               .nsISupportsString).toString();
These lines are indented using 3 space indenting. Use 4.  Also move Components.interfaces.nsISupportsString to the next line, aligned with 'DAV.
>+        }
>+
>+        var webSvc = Components.classes['@mozilla.org/webdav/service;1']
>+                               .getService(Components.interfaces
>+                               .nsIWebDAVService);
Move .nsIWebDAVService back up with Components.interfaces
>+        webSvc.getResourceProperties(itemResource, 1, ['DAV: getetag'], false,
>+               listener, this, null);
Align listener beneath itemResource.

>+    },
>+
>+    promptOverwrite: function(aMethod, aItem, aListener, aOldItem) {
Same comment here about anonymous functions. Perhaps caldavPromptOverwrite?

>+        var promptService =
>+             Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                          .getService(Components.interfaces.nsIPromptService);
This is a fine way to wrap, but use 4 space indenting and align the dots between Components.classes and .getService

>+        var promptTitle = calGetString("calendar", "itemModifiedOnServerTitle");
>+        var promptMessage = calGetString("calendar", "itemModifiedOnServer")
>+                                             + "\n";
My l10n-senses make me think the \n should be part of the string in the properties file rather than hardcoding it here.

>+        if (aMethod == kCaldavModifyItem) {
>+            promptMessage += calGetString("calendar", "ModifyWillLoseData");
>+        } else {
>+            promptMessage += calGetString("calendar", "DeleteWillLoseData");
>+        }
>+
>+        var buttonLabel1 = calGetString("calendar", "Proceed");
>+        var buttonLabel2 = calGetString("calendar", "UpdateFromServer");
A reminder that these 4 keys will need to have a lowercase leading character based on my previous review comments

>+
>+        var flags = promptService.BUTTON_TITLE_IS_STRING * promptService
>+                                   .BUTTON_POS_0 +
>+            promptService.BUTTON_TITLE_CANCEL * promptService
>+                           .BUTTON_POS_1 +
>+            promptService.BUTTON_TITLE_IS_STRING * promptService
>+                          .BUTTON_POS_2;
Button flags are always a pain to wrap in any sort of readable fashion. How about this way?

        var flags = promptService.BUTTON_TITLE_IS_STRING *
                    promptService.BUTTON_POS_0 +
                    promptService.BUTTON_TITLE_CANCEL *
                    promptService.BUTTON_POS_1 +
                    promptService.BUTTON_TITLE_IS_STRING *
                    promptService.BUTTON_POS_2;

>+
>+        var choice = promptService.confirmEx(null, promptTitle,
>+                                     promptMessage, flags, buttonLabel1, null,
>+                                     buttonLabel2, null, {});
Put promptMessage on the first line, and align flags and buttonLabel2 beneath the first null.
Also add one blank line here.

>+        switch(choice) {
Add a space between switch and (choice)

>+            case 0:
>+                if (aMethod == kCaldavModifyItem) {
>+                    this.performModifyItem(aItem, aOldItem, aListener);
>+                } else {
>+                    this.performDeleteItem(aItem, aListener);
>+                }
>+                return;
>+            case 2:
>+                this.getUpdatedItem(aItem, aListener);
>+            default: return null;
Put the return on a new line.

>+        }
>+
>+    },
>+
>+    /**
>+     * aka adoptItem
This comment is more confusing than helpful.  Please elaborate why this simply clones and calls adoptItem().  You also may want to mention that unintuitively, stuff actually gets added to the CalDAV store using performAdoptItem, rather than here.

>+     *
>+     * @param aItem       item to check
>+     * @param aListener   listener for method completion
>+     */
> 
>-    // void addItem( in calIItemBase aItem, in calIOperationListener aListener );
>     addItem: function (aItem, aListener) {
While you're here, please make this function unanonymous.

>         var newItem = aItem.clone();
>         return this.adoptItem(newItem, aListener);
>     },
>-
>-    // void adoptItem( in calIItemBase aItem, in calIOperationListener aListener );
>+    
>+    /**
>+     * sends data regarding requested new item off for etag checking
>+     *
>+     * @param aItem       item to check
>+     * @param aListener   listener for method completion
>+     */
>     adoptItem: function (aItem, aListener) {
While you're here, please make this function unanonymous.

>-        if (this.readOnly) {
>-            throw Components.interfaces.calIErrors.CAL_IS_READONLY;
>-        }
>-        
>+       this.fetchEtag(kCaldavAdoptItem, aItem, aListener, null);
This is using 3 space indenting. Please use 4.

>+    },
>+
>+    /**
>+     * adds item to CalDAV store
I'd add something along the lines of "The item actually gets added to the CalDAV store here." since it doesn't happen in the "addItem" method. :)

>+     *
>+     * @param aItem       item to check
>+     * @param aListener   listener for method completion
>+     */
>+    performAdoptItem: function (aItem, aListener) {
Please make this function unanonymous.

>+
Remove this blank line ^^^

>         if (aItem.id == null && aItem.isMutable)
>             aItem.id = getUUID();
Bonus points for adding curly braces around this ^^ one line if statement.

>@@ -265,13 +429,12 @@ calDavCalendar.prototype = {
>                 LOG("Item added successfully");
> 
>                 var retVal = Components.results.NS_OK;
>+                // CalDAV does not require that the server return a strong
>+                // etag on PUT, so we best re-query
>+                thisCalendar.fetchEtag(kCaldavCacheEtag, aItem, null, null);
What is a "strong etag"?  Please add some info to this comment.

> 
>             } else if (aStatusCode == 200) {
>-                // XXXdmose once we get etag stuff working, this should
>-                // 
>-                LOG("200 received from clients, until we have etags working"
>-                      + " this probably means a collision; after that it'll"
>-                      + " mean server malfunction");
>+                LOG("200 received from server: server malfunction");
Add "CalDAV: " to this error message.

>                 retVal = Components.results.NS_ERROR_FAILURE;
>             } else {
>                 if (aStatusCode > 999) {
>@@ -319,12 +482,27 @@ calDavCalendar.prototype = {
> 
>         return;
>     },
>-
>-    // void modifyItem( in calIItemBase aNewItem, in calIItemBase aOldItem, in calIOperationListener aListener );
>-    modifyItem: function modifyItem(aNewItem, aOldItem, aListener) {
>-        if (this.readOnly) {
>-            throw Components.interfaces.calIErrors.CAL_IS_READONLY;
>-        }
>+    
Trailing spaces on this ^^ blank line.

>+    /**
>+     * Sends info about request to modify item off for etag checking
>+     *
>+     * @param aItem       item to check
>+     * @param aOldItem    aOldItem argument in modifyItem requests
>+     * @param aListener   listener from original request
>+     */
>+    modifyItem: function modifyItem(aItem, aOldItem, aListener) {
>+       this.fetchEtag(kCaldavModifyItem, aItem, aListener, aOldItem);
This line ^^ uses 3 space indenting. Please use 4.

>+    },
>+
>+    /**
>+     * modifies existing item in CalDAV store
>+     *
>+     * @param aItem       item to check
>+     * @param aOldItem    previous version of item to be modified
>+     * @param aListener   listener from original request
>+     */
>+    performModifyItem: function performModifyItem(aNewItem, aOldItem,
>+        aListener) {
Align aListener beneath aNewItem.

>@@ -353,13 +531,16 @@ calDavCalendar.prototype = {
> 
>         var eventUri = this.mCalendarUri.clone();
>         try {
>-            eventUri.spec = this.mMakeUri(aNewItem.getProperty("X-MOZ-LOCATIONPATH"));
>-            LOG("using X-MOZ-LOCATIONPATH: " + eventUri.spec);
>+             eventUri.spec = this.mMakeUri(aNewItem
>+                                  .getProperty("X-MOZ-LOCATIONPATH"));
>+             LOG("using X-MOZ-LOCATIONPATH: " + eventUri.spec);
>+
>         } catch (ex) {
>-            // XXX how are we REALLY supposed to figure this out?
>-            eventUri.spec = this.mMakeUri(aNewItem.id + ".ics");
>+             // XXX how are we REALLY supposed to figure this out?
>+             eventUri.spec = this.mMakeUri(aNewItem.id + ".ics");
>         }
These lines are already fine.  Please remove these changes from your patch.


>-
>+        // it seems redundant to use generation when we have etags
>+        // but until the idl is changed we do it
Should this have a follow up bug referenced for discussion?  What if the server does not support etags?

>@@ -394,13 +575,15 @@ calDavCalendar.prototype = {
> 
>         listener.onOperationComplete = function(aStatusCode, aResource,
>                                                 aOperation, aClosure) {
>-
>             // 201 = HTTP "Created"
>             // 204 = HTTP "No Content"
>             //
>             if (aStatusCode == 204 || aStatusCode == 201) {
>                 LOG("Item modified successfully.");
>                 var retVal = Components.results.NS_OK;
>+                // CalDAV does not require that the server return a strong
>+                // etag on put, so we best re-query
Another place to describe a "strong etag" better.

>+                thisCalendar.fetchEtag(kCaldavCacheEtag, aNewItem, null, null);
> 
>             } else {
>                 if (aStatusCode > 999) {
>@@ -450,12 +633,23 @@ calDavCalendar.prototype = {
>         return;
>     },
> 
>-
>-    // void deleteItem( in calIItemBase aItem, in calIOperationListener aListener );
>+    /**
>+     * sends data regarding requested deletion off for etag checking
>+     *
>+     * @param aItem       item to check
>+     * @param aListener   listener for method completion
>+     */
>     deleteItem: function (aItem, aListener) {
Please make this function unanonymous.

>-        if (this.readOnly) {
>-            throw Components.interfaces.calIErrors.CAL_IS_READONLY;
>-        }
>+       this.fetchEtag(kCaldavDeleteItem, aItem, aListener, null);
This line ^^ uses 3 space indenting. Please use 4.

>+    },
>+
>+    /**
>+     * deletes item from CalDAV store
>+     *
>+     * @param aItem       item to delete
>+     * @param aListener   listener for method completion
>+     */
>+    performDeleteItem: function (aItem, aListener) {
Please make this function unanonymous.

> 
>         if (aItem.id == null) {
>             if (aListener)
>@@ -469,13 +663,13 @@ calDavCalendar.prototype = {
> 
>         var eventUri = this.mCalendarUri.clone();
>         try {
>-            eventUri.spec = this.mMakeUri(aItem.getProperty("X-MOZ-LOCATIONPATH"));
>-            LOG("using X-MOZ-LOCATIONPATH: " + eventUri.spec);
>+             eventUri.spec = this.mMakeUri(aItem
>+                                  .getProperty("X-MOZ-LOCATIONPATH"));
>+             LOG("using X-MOZ-LOCATIONPATH: " + eventUri.spec);
>         } catch (ex) {
>-            // XXX how are we REALLY supposed to figure this out?
>-            eventUri.spec = this.mMakeUri(aItem.id + ".ics");
>+             // XXX how are we REALLY supposed to figure this out?
>+             eventUri.spec = this.mMakeUri(aItem.id + ".ics");
>         }
>-
>         var eventResource = new WebDavResource(eventUri);
> 
>         var listener = new WebDavListener();
These lines are already fine. Please remove these changes from your patch.

>@@ -488,6 +682,7 @@ calDavCalendar.prototype = {
>             // 204 = HTTP "No content"
>             //
>             if (aStatusCode == 204) {
>+                delete thisCalendar.mEtagCache[aItem.id];
This line doesn't look valid to me.

>@@ -525,6 +720,60 @@ calDavCalendar.prototype = {
>         return;
>     },
> 
>+    /**
>+     * retrieves specific item from CalDAV store
>+     * use when an outdated copy of the item is in hand
>+     *
>+     * @param aItem       item to fetch
>+     * @param aListener   listener for method completion
>+     */
>+    getUpdatedItem: function (aItem, aListener) {
Please make this function unanonymous.

>+
>+        if (!aListener)
>+            return;
>+
>+        if (aItem == null) {
>+            aListener.onOperationComplete(this,
>+                                          Components.results
>+                                          .NS_ERROR_FAILURE,
Don't split this just to get under 80 chars.

>+                                          aListener.GET,
>+                                          null,
>+                                          "passed in null item");
>+            return;
>+        }
>+
>+        var itemType = "VEVENT";
>+        if (aItem instanceof Ci.calITodo) {itemType = "VTODO";}
Please split this line up so itemType = "VTODO"; is on a new line.

>+
>+        var C = new Namespace("C", "urn:ietf:params:xml:ns:caldav");
>+        var D = new Namespace("D", "DAV:");
>+        default xml namespace = C;
>+
>+        queryXml =
>+          <calendar-query xmlns:D="DAV:">
>+            <D:prop>
>+              <D:getetag/>
>+              <calendar-data/>
>+            </D:prop>
>+            <filter>
>+              <comp-filter name="VCALENDAR">
>+                <comp-filter name={itemType}>
>+                  <prop-filter name="UID">
>+                    <text-match collation="i;octet">
>+                      {aItem.id}
>+                    </text-match>
>+                  </prop-filter>
>+                </comp-filter>
>+              </comp-filter>
>+            </filter>
>+          </calendar-query>;
>+
>+        this.reportInternal(xmlHeader + queryXml.toXMLString(), 
Trailing space on this line ^^

>@@ -546,7 +795,7 @@ calDavCalendar.prototype = {
>         // XXX need a prefix in the namespace decl?
>         default xml namespace = "urn:ietf:params:xml:ns:caldav";
>         var D = new Namespace("D", "DAV:");
>-        queryXml = 
>+        queryXml =
Leave this line be rather than messing up CVS blame for a whitespace change.

>@@ -569,7 +818,8 @@ calDavCalendar.prototype = {
>         return;
>     },
> 
>-    reportInternal: function (aQuery, aOccurrences, aRangeStart, aRangeEnd, aCount, aListener)
>+    reportInternal: function (aQuery, aOccurrences, aRangeStart, aRangeEnd,
>+        aCount, aListener, aItem)
Please make this function unanonymous, and align whichever aParameter(s) overrun 80 chars beneath aQuery.

>     {
>         var reportListener = new WebDavListener();
>         var count = 0;  // maximum number of hits to return
>@@ -617,6 +867,8 @@ calDavCalendar.prototype = {
>                 var C = new Namespace("urn:ietf:params:xml:ns:caldav");
>                 var D = new Namespace("DAV:");
> 
>+                var eTag = responseElement..D::["getetag"];
Why have we changed the case to eTag here?  Please use etag.

>+
>                 // cause returned data to be parsed into the item
>                 var calData = responseElement..C::["calendar-data"];
>                 if (!calData.toString().length) {
>@@ -720,6 +972,14 @@ calDavCalendar.prototype = {
>                     item.calendar = thisCalendar;
>                 }
> 
>+                thisCalendar.mEtagCache[item.id] = eTag;
Change eTag case to etag.

>+                if (aItem) {
>+                    // if aItem is not null, we were called from
>+                    // getUpdatedItem(), and the view isn't listening to any
>+                    // changes. So in order to have the updated item displayed
This comment is an incomplete sentence that leaves me hanging.  Please finish by telling me what I must do to have the updated item displayed.
>+                    thisCalendar.observeModifyItem(item, aItem.parentItem);
>+                }
>+
>                 // figure out what type of item to return
>                 var iid;
>                 if(aOccurrences) {



Lotsa changes. r- for now, but this looks promising.
Attachment #247378 - Flags: first-review?(lilmatt) → first-review-
Attached image dialog for etag mismatch on modify item (obsolete) —
dialog box presented when an etag mismatch is found when modifying an item on a CalDAV store, for UI-review. There's a substantially similar one for etag-mismatch-on-delete, with second sentence readeing "Proceeding with this deletion will cause loss of the data changed on the server" and button1 labelled "Delete"
Attachment #247595 - Flags: ui-review?(dmose)
Whoof. Sorry to have made you go through that, and thanks for doing so. Clearly I need to retire gedit for something that shows me whitespace.

This patch makes all the suggested corrections, except:

>>+        // it seems redundant to use generation when we have etags
>>+        // but until the idl is changed we do it
>Should this have a follow up bug referenced for discussion?  What if the server
>does not support etags?

I'm happy to file a followup bug; was figuring it would make more sense to wait until this was in. 

CalDAV (5.3.4) has support for DAV:getetag as a MUST, so I doubt we'll see many CalDAV-ish implementations that don't have etag support. I suppose we could allow a user pref, or somesuch, to ignore etags for such things, though I can't really work much enthusiasm up for that.

That being said, since we are doing this post-PUT PROPFIND, it would probably be straightforward to check href as well as etag, and construct X-MOZ-LOCATIONPATH from that - which ISTR should help with GroupDAV interop. Not sure we want to go far down that road, though.

>>+                delete thisCalendar.mEtagCache[aItem.id];
>This line doesn't look valid to me.

I've tested it; it does what it's supposed to. Will change it if you want and can give me a hint as to how you want it.

Made a couple of other minor changes since r1:
* reworded the "*WillLoseData" strings
* change "proceed" string to proceedDelete and proceedModify so that the corresponding buttons can read differently
* fixed bug wrt the Cancel button (it would leave the view displaying aNewItem rather than aOldItem)
Attachment #247378 - Attachment is obsolete: true
Attachment #247599 - Flags: first-review?(lilmatt)
Comment on attachment 247599 [details] [diff] [review]
second attempt, reformatted, renamed, etc

>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>@@ -222,20 +228,182 @@ calDavCalendar.prototype = {
>             function(o) {return (o != aObserver);});
>     },
> 
>-    // void addItem( in calIItemBase aItem, in calIOperationListener aListener );
>-    addItem: function (aItem, aListener) {
>-        var newItem = aItem.clone();
>-        return this.adoptItem(newItem, aListener);
>-    },
>+    mEtagCache: Array(),
In your previous patch, mEtagCache was above addItem.

>+    fetchEtag: function caldav_fetchEtag(aMethod, aItem, aListener, aOldItem) {
The whole contents of the fetchEtag function, to a point I've marked below, needs to be indented one more space.
>+
Remove this blank line. ^^

>+       if (this.readOnly) {
>+           throw calIErrors.CAL_IS_READONLY;
>+       }
>+
>+       var serverEtag = null;
>+       var listener = new WebDavListener();
>+
>+       var thisCalendar = this;
>+
>+       var itemUri = this.mCalendarUri.clone();
>+
>+       try {
>+           itemUri.spec = this.mMakeUri(aItem.getProperty("X-MOZ-LOCATIONPATH"));
>+           LOG("using X-MOZ-LOCATIONPATH: " + itemUri.spec);
>+       } catch (ex) {
>+           // XXX how are we REALLY supposed to figure this out?
>+           itemUri.spec = this.mMakeUri(aItem.id + ".ics");
>+       }
>+
>+       var itemResource = new WebDavResource(itemUri);
>+
>+       listener.onOperationComplete = 
>+           function onOperationComplete(aStatusCode, aResource, aOperation,
>+                                        aClosure) {
>+
>+           var mismatch = (serverEtag != thisCalendar.mEtagCache[aItem.id]);
>+
>+           switch(aMethod) {
Add a space between switch and (aMethod).

>+               case kCaldavAdoptItem:
>+                   if (serverEtag != null) {
>+                       // either there's a server error or we're copying an item
>+                       // onto itself: either way the safe thing is to bail
>+                       LOG("CalDAV: non-null etag in adoptItem");
>+                       thisCalendar.readOnly = true;
>+                   } else {
>+                       thisCalendar.performAdoptItem(aItem, aListener);
>+                   }
>+                   break;
>+
>+               case kCaldavModifyItem:
>+                   if (mismatch) {
>+                       LOG("CalDAV: etag mismatch in modifyItem");
>+                       thisCalendar.promptOverwrite(aMethod, aItem,
>+                                                   aListener, aOldItem);
Align aListener beneath aMethod.

>+                   } else {
>+                       thisCalendar.performModifyItem(aItem, aOldItem, aListener);
>+                   }
>+                   break;
>+
>+               case kCaldavDeleteItem:
>+                   if (mismatch) {
>+                       LOG("CalDAV: etag mismatch in deleteItem");
>+                       thisCalendar.promptOverwrite(aMethod, aItem,
>+                                                   aListener, null);
Align aListener beneath aMethod.

>+                   } else {
>+                       thisCalendar.performDeleteItem(aItem, aListener);
>+                   }
>+                   break;
>+
>+               default:
>+                   thisCalendar.mEtagCache[aItem.id] = serverEtag;
>+                   break;
>+           }
This ^^ is the last line which has the goofy indenting.

>+        listener.onOperationDetail = function(aStatusCode, aResource,
>+                                              aOperation, aDetail, aClosure) {
Please make this function unanonymous.

>+            var props = aDetail.QueryInterface(Components.interfaces.nsIProperties);
>+            serverEtag = props.get('DAV: getetag', 
This line has a trailing space. ^^

>+        var webSvc = Components.classes['@mozilla.org/webdav/service;1']
>+                               .getService(Components.interfaces
>+                                           .nsIWebDAVService);
Move .nsIWebDAVService back up with Components.interfaces

>+    promptOverwrite: function caldav_promptOverwrite(aMethod, aItem, aListener,
>+                                                     aOldItem) {
>+        var promptService =
>+            Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                      .getService(Components.interfaces.nsIPromptService);
>+
>+        var promptTitle = calGetString("calendar", "itemModifiedOnServerTitle");
>+        var promptMessage = calGetString("calendar", "itemModifiedOnServer");
>+        if (aMethod == kCaldavModifyItem) {
Please add a blank line above the if statement for readability.

>+            promptMessage += calGetString("calendar", "modifyWillLoseData");
>+            var buttonLabel1 = calGetString("calendar", "proceedModify");
>+        } else {
>+            promptMessage += calGetString("calendar", "deleteWillLoseData");
>+            var buttonLabel1 = calGetString("calendar", "proceedDelete");
This is declaring the variable buttonLabel1 twice, which may cause strict JS warnings. You probably want to define var buttonLabel1 above the if statement and just assign to it in the if/else.

>+        var buttonLabel2 = calGetString("calendar", "updateFromServer");
>+
>+
Lose one of the two blank lines here. ^^

>+        switch (choice) {
>+            case 0:
>+                if (aMethod == kCaldavModifyItem) {
>+                    this.performModifyItem(aItem, aOldItem, aListener);
>+                } else {
>+                    this.performDeleteItem(aItem, aListener);
>+                }
>+                return;
>+            case 2:
>+                this.getUpdatedItem(aItem, aListener);
>+                return;
>+            case 1:
>+            default:
>+                this.observeModifyItem(aOldItem, aItem.parentItem);
>+                return;
Change the three returns in this switch statement to break;

>@@ -353,13 +536,15 @@ calDavCalendar.prototype = {
> 
>         var eventUri = this.mCalendarUri.clone();
>         try {
>-            eventUri.spec = this.mMakeUri(aNewItem.getProperty("X-MOZ-LOCATIONPATH"));
>-            LOG("using X-MOZ-LOCATIONPATH: " + eventUri.spec);
>+             eventUri.spec = this.mMakeUri(aNewItem.getProperty("X-MOZ-LOCATIONPATH"));
>+             LOG("using X-MOZ-LOCATIONPATH: " + eventUri.spec);
>+
These lines are already fine.  Please remove these changes from your patch.

>@@ -475,7 +674,6 @@ calDavCalendar.prototype = {
>             // XXX how are we REALLY supposed to figure this out?
>             eventUri.spec = this.mMakeUri(aItem.id + ".ics");
>         }
>-
Please don't delete this line. ^^

>+    getUpdatedItem: function caldav_getUpdatedItem(aItem, aListener) {
>+
>+        if (!aListener)
>+            return;
Add curly braces around this ^^ and remove the blank line above it.

>@@ -720,6 +977,16 @@ calDavCalendar.prototype = {
>                     item.calendar = thisCalendar;
>                 }
> 
>+                thisCalendar.mEtagCache[item.id] = etag;
>+                if (aItem) {
>+                    // if aItem is not null, we were called from
>+                    // getUpdatedItem(), and the view isn't listening to any
>+                    // changes. So in order to have the updated item displayed
>+                    // So we need to modify the item currently displayed with
>+                    // the one just fetched
>+                    thisCalendar.observeModifyItem(item, aItem.parentItem);
>+                }
Extra "So" in the 4th line of the comment.


r=lilmatt with those changes.
Attachment #247599 - Flags: first-review?(lilmatt) → first-review+
Attachment #247599 - Attachment is obsolete: true
Attachment #247697 - Flags: second-review?(dmose)
Whiteboard: [patch in hand][needs review dmose]
Comment on attachment 247595 [details]
dialog for etag mismatch on modify item

UI-review has reverted to mvl; pointing in his direction...
Attachment #247595 - Flags: ui-review?(dmose) → ui-review?(mvl)
Comment on attachment 247595 [details]
dialog for etag mismatch on modify item

It's not clear to me what the 'update from server' button will do. Will it throw away my changes or not?
Yes, 'update from server' will throw away changes edited locally since the item being edited was downloaded from the server; 'proceed' will throw away any changes  saved on the server (presumably by another person or process) since that time. 

A clearer, though to my eye too wordy, text:
This item has recently been changed on the server. Do you want to use the new version of the item now on the server, losing any changes recently made here, or proceed with your changes and overwrite the recently changed item on the server?
[Use new version from server] [Proceed, overwriting changed item on server]

Your proposal is wordy, because the text and the buttons say the same. how about:

This item has recently been changed on the server. Submitting your changes will overwrite the changes made on the server.
[Throw away my changes and reload] [Submit my changes anyway]

(courtesy of bugzilla) Note that it added an explanation of what will happen with the changes you tried to make: they are gone.

Why did the orginal proposal have three buttons? What would the third button do?
(In reply to comment #11)
> Your proposal is wordy, because the text and the buttons say the same. how
> about:
> 
> This item has recently been changed on the server. Submitting your changes 
> will overwrite the changes made on the server.
> [Throw away my changes and reload] [Submit my changes anyway]

This is a lot more clear and concise. But I would try to shorten the button texts even more:

[Discard my changes and reload] [Submit my changes]

"Throw away" is a little to informal for my taste and the "anyway" is a fill word without any added benefit to the button text.
I like "Discard" instead of "Throw away", but to my ear "anyway" in this context is more than a fill word: it means "I understand and accept the consequences". So for my part I'd prefer [Discard my changes and reload] and [Submit my changes anyway]

The original proposal had as well a Cancel button, which would do the same thing as the Cancel button in the item-editing db: return to the status quo ante, with item version X in the local cache and version X+1 (or whatever) on the server. I think it's important to have this: I think the average user reaction when confronted with a "scary message" like this for the first time will be to cancel/back out/leave no trace. They'll probably then re-attempt the edit, and read and deal with this dialog box the second time around. But I'm of mixed mind as to whether this (Cancel) needs a button, or whether the close box on the window suffices, and would appreciate input from people more experienced with UI than myself. The close box on the window does strike me as less ambiguous. I know that e.g. Gnome has elaborate guidelines about this kind of thing, but I haven't found any such specs wrt mozilla.
Attached image reworded dialog, with cancel button (obsolete) —
the version for etag-mismatch-on-delete is similar:
[Item changed on server]
This item has recently been changed on the server.
Deleting this item will cause loss of the changes made on the server
[Discard my changes and reload]    [Cancel] [Delete anyway]

Again, "Cancel" returns Sb/Ltn to the state it had before the user edited or tried to delete the item in question.
Attachment #247595 - Attachment is obsolete: true
Attachment #257463 - Flags: ui-review?
Attachment #247595 - Flags: ui-review?(mvl)
Attachment #257463 - Flags: ui-review? → ui-review?(mvl)
Comment on attachment 257463 [details]
reworded dialog, with cancel button

I don't really see the need for a cancel button. It leaves the user in a state that's inconsistent, and he will need to go through the dialog again.
similar changes to the corresponding delete dialog.

I have the default action set to load the changed item from the server. That's clearly preferable in the case of etag-mismatch-on-delete, and arguably so in the case of etag-mismatch-on-modify since the user should be able to recreate the data just entered into the item-editing dialog if necessary.
Attachment #257463 - Attachment is obsolete: true
Attachment #257618 - Flags: ui-review?(mvl)
Attachment #257463 - Flags: ui-review?(mvl)
Comment on attachment 247697 [details] [diff] [review]
patch rev 3, final corrections per r1

This does not need a 2nd review anymore, since a module peer (lilmatt) already gave his review.
Attachment #247697 - Flags: second-review?(dmose)
I'd like to get this in 0.5 if at all possible.
Whiteboard: [patch in hand][needs review dmose] → [patch in hand][cal-ui-review needed]
Target Milestone: --- → Sunbird 0.5
Comment on attachment 257618 [details]
reworded dialog, without cancel button

ui-review=mvl
Attachment #257618 - Flags: ui-review?(mvl) → ui-review+
Attachment #247697 - Attachment is obsolete: true
Attachment #257915 - Flags: first-review?(lilmatt)
Is there any intention to use the If-Match / If-None-Match headers in this at all?

I can't see anything in the patch, but maybe I missed it.

Some of the other CalDAV clients do:

PUT (Create)  => If-None-Match: *

PUT (Replace) => If-Match: <existing etag>

DELETE        => If-Match: <existing etag>

Or maybe this is already happening in more recent builds.

Regards,
Andrew McMillan.
(In reply to comment #21)
> Is there any intention to use the If-Match / If-None-Match headers in this at
> all?
> 

We are going to want to do that, yes, but no, it's not in this patch. I see that as a separate bug (which I haven't gotten around to filing yet; please do so yourself if so inclined). That's one of the things we'll be able to look at doing once the basic etag support provided by this patch is in. IIRC it's going to require some tinkering with the WebDAV service.
Whiteboard: [patch in hand][cal-ui-review needed] → [patch in hand]
Blocks: 373370
Attached patch fix typo (obsolete) — Splinter Review
Attachment #257915 - Attachment is obsolete: true
Attachment #258157 - Flags: first-review?(lilmatt)
Attachment #257915 - Flags: first-review?(lilmatt)
Comment on attachment 258157 [details] [diff] [review]
fix typo

Index: calendar/providers/caldav/calDavCalendar.js
===================================================================
>+const kCaldavAdoptItem = 1;
>+const kCaldavModifyItem = 2;
>+const kCaldavDeleteItem = 3;
>+const kCaldavCacheEtag = 4;
The more I think about it, the more I'd like the constants to be in all caps and use underscores. ex: CALDAV_ADOPT_ITEM
I know it's not what we used in the "freshly auth" part, but we can revisit at some other point.  I just feel it's more obvious that it's a constant this way.

>+    /**
>+     * Fetches etag from server and compares with local cached version
>+     * before add/adopt/modify/delete item
>+     *
>+     * @param aMethod     requested method (adopt/modify/delete)
>+     * @param aItem       item to check
>+     * @param aListener   listener from original request
>+     * @param aOldItem    aOldItem argument in modifyItem requests
>+     */
>+    fetchEtag: function caldav_fetchEtag(aMethod, aItem, aListener, aOldItem) {
We could use something less verbose for unanonymizing the function. caldavFE or cFE would both be fine with me.

>+        listener.onOperationComplete =
>+            function onOperationComplete(aStatusCode, aResource, aOperation,
>+                                         aClosure) {
This style is a bit strange. How about something like:
        listener.onOperationComplete = function fetchEtagOOC(aStatusCode,
                                                             aResource,
                                                             aOperation,
                                                             aClosure) {

>+
>+            var mismatch = (serverEtag != thisCalendar.mEtagCache[aItem.id]);
>+
>+            switch (aMethod) {
>+                case kCaldavAdoptItem:
>+                    if (serverEtag != null) {
>+                        // either there's a server error or we're copying an item
>+                        // onto itself: either way the safe thing is to bail
Nit: Make this comment complete sentences with capitalization and punctuation :)

>+                        LOG("CalDAV: non-null etag in adoptItem");
>+                        thisCalendar.readOnly = true;
>+                    } else {
>+                        thisCalendar.performAdoptItem(aItem, aListener);
>+                    }
>+                    break;
>+
>+                case kCaldavModifyItem:
>+                    if (mismatch) {
>+                        LOG("CalDAV: etag mismatch in modifyItem");
>+                        thisCalendar.promptOverwrite(aMethod, aItem,
>+                                                     aListener, aOldItem);
>+                    } else {
>+                        thisCalendar.performModifyItem(aItem, aOldItem, aListener);
>+                    }
>+                    break;
>+
>+                case kCaldavDeleteItem:
>+                    if (mismatch) {
>+                        LOG("CalDAV: etag mismatch in deleteItem");
>+                        thisCalendar.promptOverwrite(aMethod, aItem,
>+                                                     aListener, null);
>+                    } else {
>+                        thisCalendar.performDeleteItem(aItem, aListener);
>+                    }
>+                    break;
>+
>+                default:
>+                    thisCalendar.mEtagCache[aItem.id] = serverEtag;
>+                    break;
>+            }
>+
>+        }
Remove the blank line between the two }s

>+
>+        listener.onOperationDetail =
>+            function fetchEtag_oOD(aStatusCode, aResource, aOperation, aDetail,
>+                                   aClosure) {
Same story here with the style.

>+            var props = aDetail.QueryInterface(Components.interfaces.nsIProperties);
>+            serverEtag = props.get('DAV: getetag',
>+                                   Components.interfaces.nsISupportsString)
>+                                   .toString();
>+        }
Use double-quotes around DAV: getetag, and use Ci

>+        var webSvc = Components.classes['@mozilla.org/webdav/service;1']
>+                               .getService(Components.interfaces.nsIWebDAVService);
Use Cc and Ci

>+        webSvc.getResourceProperties(itemResource, 1, ['DAV: getetag'], false,
>+                                     listener, this, null);
Use double-quotes around DAV: getetag.
>
>+    promptOverwrite: function caldav_promptOverwrite(aMethod, aItem, aListener,
>+                                                     aOldItem) {
Use a shorter function name here ^^^^^^^^^^^^^^^^^^^^^ similar to the one you used above.

>+        var promptService =
>+            Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                      .getService(Components.interfaces.nsIPromptService);
Use Cc and Ci.  This will let you put the Cc line on the var promptSvc line.

>+
>+        var promptTitle = calGetString("calendar", "itemModifiedOnServerTitle");
>+        var promptMessage = calGetString("calendar", "itemModifiedOnServer");
>+        var buttonLabel1;
>+        
Trailing spaces on this line ^^^

>+    /**
>+     * required by IDL, but we actually use adoptItem() below
>+     * actually adding the item to the CalDAV store takes place
>+     * in performAdoptItem()
Make this comment complete sentences with capitalization and punctuation.

>+    addItem: function caldav_addItem(aItem, aListener) {
Use a shorter function name here ^^^^^ similar to the one you used above.

>+    /**
>+     * sends data regarding requested new item off for etag checking
Make this comment complete sentences with capitalization and punctuation.

>+    adoptItem: function caldav_adoptItem(aItem, aListener) {
Use a shorter function name here ^^^^^^^^^ similar to the one you used above.

>+    /**
>+     * performs actual addition of item to CalDAV store, after etag checking
Make this comment complete sentences with capitalization and punctuation.

>+    performAdoptItem: function caldav_performAdoptItem(aItem, aListener) {
Use a shorter function name here ^^^^^^^^^^^^^^^^^^^^^^^ similar to the one you used above.

>+    modifyItem: function modifyItem(aItem, aOldItem, aListener) {
Make sure your function name here ^^^ matches the style of those above.

>+    performModifyItem: function performModifyItem(aNewItem, aOldItem,
>+                                                  aListener) {
Make sure your function name here ^^^^^^^^^^^^^^^^^ matches the style of those above.
 
>+    deleteItem: function deleteItem(aItem, aListener) {
Make sure your function name here ^^^ matches the style of those above.

>+    performDeleteItem: function caldav_performDeleteItem(aItem, aListener) {
Make sure your function name here ^^^^^^^^^^^^^^^^^^^^^^^^ matches the style of those above.

>+    /**
>+     * retrieves specific item from CalDAV store
>+     * use when an outdated copy of the item is in hand
Make this comment complete sentences with capitalization and punctuation.

>+    getUpdatedItem: function caldav_getUpdatedItem(aItem, aListener) {
Use a shorter function name here ^^^^^^^^^^^^^^^^^^^ similar to the ones you used above.

>+    reportInternal: function reportInternal(aQuery, aOccurrences, aRangeStart,
>+                                            aRangeEnd, aCount, aListener, aItem)
Make sure your function name here ^^^^^^^^^^^ matches the style of those above.


This is essentially all style nits. r=lilmatt with those
Attachment #258157 - Flags: first-review?(lilmatt) → first-review+
The patch makes the prompt come from the provider. Thats not really nice from an architectural point of view. It's also not nice for code-reuse: the ics provider likely needs the same prompt. And so might others. So we need a better wy to do that. I'm not sure yet how, and might be done in a different bug. But please do file one if you want to go that road.
implemented corrections per comment #24. Not setting "checkin needed" since I've missed the string freeze; looks like this will have to wait until after 0.5. Will file separate bug per mvl's suggestion in comment #25
Attachment #258157 - Attachment is obsolete: true
I think this patch is compelling enough to take and mark as late-l10n.

mvl: Thoughts?
We agreed in this week's calendar project meeting that this patch gives us enough benefit, especially with Apple releasing their CalDAV server in the next few months, that we want to take this for 0.5, regardless of string freeze.  We'll need to notify the localizers of this change.
Whiteboard: [patch in hand] → [patch in hand] [late-l10n]
Flags: blocking-calendar0.5+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Actually I am having trouble checking etag on urls like "foo/bar/null" which is currently the case when adding a new item (which yet has no id, of course). IMO we should omit the check then, because it obviously gets a unique UID.
(In reply to comment #30)
Reason is that item.getProperty actually returns null (does not throw an exception) for unknown properties, so this causes that url.
Status: RESOLVED → VERIFIED
Flags: blocking-calendar0.5+
You need to log in before you can comment on or make changes to this bug.