306508 - WebDAV provider should notice and propagate all possible dataloss issues to the UI

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

We need to improve the observer/listener infrastructure a bit in various places
so that the user will be notified about any possible dataloss that the providers
can detect.  The plan is to start that work here by tweaking the WebDAV
provider, the interfaces, and the Lightning UI code.

Comment 1

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

I noticed a few problems:
The observer I added in the patch for bug 306765 is added too late to notice
errors on loading calendars for startup. We need to make sure that the right
observer is added before setting uri's to the calendars.
Another problem is in the new-calendar wizard. If creating the calendar fails,
the wizard doesn't notice it, and continues happily.
(my testcase: a copy of a ics calendar with an accented char in it. I edited the
file with an editor that didn't handle the utf8 encoding, so showed two
charaters. I removed one to make the utf8 invalid. Then i subscribed to the file)

Comment 2

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

Attachment: propagate libical errors (checked in)

patch to propagate libical errors, instead of a general NS_ERROR_FAILURE. has
r=dmose over irc.

Comment 3

[Joey Minta]

Taking bug, since we've decided it blocks 0.3a1.  I've got some UI for this, and
already added a few more onError calls/setting readOnly instances, but if
someone could submit some good testcases for different ways in which the (ics)
provider could fail, that would make thorough testing much easier.

Comment 4

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

Cases we want to handle I can think of now:
- setting the url to something invalid (a non-existing directory)
- setting the url to some other, non-ics, file (like an important document)
- unable to parse the ics file
- unable to upload (network down)

and maybe anything that can throw.

Comment 5

[Joey Minta]

Attachment: patch v1

First run at this.  The way I see things, we have 3 classes of errors that can be reported:
1.) Severe errors which result in putting a calendar in readOnly status
2.) Minor errors which we can probably recover from
3.) Errors in reading which happen after a calendar has already been made readOnly

This attempts to report these 3 types separately, while hiding the ugly error messages in favor of more user friendly ones.  All of the catches in calICSCalendar.js (with the exception of 1 which I talked to mvl about) now include onError calls, and the general idea should be fairly easily extended to the calDav provider.

Should be lower risk than the original readOnly patch.

Comment 6

[Joey Minta]

Attachment: dialog screenshots

Screenshot showing the two ways the dialog can appear to the user.  The top is the default appearance.  Pressing 'Details...' gives the bottom appearance.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 200554 [details] [diff] [review]
patch v1

>Index: mozilla/calendar/base/src/calCalendarManager.js
>===================================================================
>+    if (!this.storedReadOnly && this.calendar.readOnly) {
>+        // Major errors change the calendar to readOnly
>+        emessage = props.formatStringFromName("readOnlyMode", [this.calendar.name], 1);
>+    } else if (!this.storedReadOnly && !this.calendar.readOnly) {
>+        // Minor errors don't, but still tell the user something went wrong
>+        emessage = props.formatStringFromName("minorError", [this.calendar.name], 1);
>+    } else {
>+        // The calendar was already in readOnly mode, but still tell the user
>+        emessage = props.formatStringFromName("stillReadOnlyError", [this.calendar.name], 1);
>+    }
>+    paramBlock.SetString(0, emessage);
>+    paramBlock.SetString(1, aErrNo);

I'd suggest using aErrNo.toString(16), as this will put the number in hex, which is going to be less of a hassle to track down when looking through bug reports.  If you do this, you may want to change either the locale or format strings to prefix the error number with "0x".

>Index: mozilla/calendar/providers/ics/calICSCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/ics/calICSCalendar.js,v
>retrieving revision 1.33
>diff -p -U8 -r1.33 calICSCalendar.js
>--- mozilla/calendar/providers/ics/calICSCalendar.js	23 Oct 2005 17:36:41 -0000	1.33
>+++ mozilla/calendar/providers/ics/calICSCalendar.js	23 Oct 2005 21:54:18 -0000
>@@ -17,16 +17,17 @@
>  * The Initial Developer of the Original Code is
>  *  Michiel van Leeuwen <[e-mail]>
>  * Portions created by the Initial Developer are Copyright (C) 2004
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Vladimir Vukicevic <[e-mail]>
>  *   Dan Mosedale <[e-mail]>
>+ *   Joey Minta <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -141,16 +142,18 @@ calICSCalendar.prototype = {
>         channel.notificationCallbacks = this;
> 
>         var streamLoader = Components.classes["@mozilla.org/network/stream-loader;1"]
>                                      .createInstance(Components.interfaces.nsIStreamLoader);
>         try {
>             streamLoader.init(channel, this, this);
>         } catch(e) {
>             // File not found: a new calendar. No problem.
>+            this.mObserver.onError(Components.interfaces.calIErrors.ICS_NEWFAILED, 
>+                                   "streamLoader.init failed, likely because this is a new calendar");

If this really does indicate a new calendar (does it always, or just sometimes?), this isn't actually an error, right?  If so, I think we just want to eat it and not call the observer at all.

>         // Wrap parsing in a try block. Will ignore errors. That's a good thing
>         // for non-existing or empty files, but not good for invalid files.
>-        // We should not accidently overwrite third party files.
>-        // XXX Fix that
>+        // That's why we put them in readOnly mode
>         try {
>             var rootComp = this.mICSService.parseICS(str);
> 
>             var calComp;
>             // libical returns the vcalendar component if there is just
>             // one vcalendar. If there are multiple vcalendars, it returns
>             // an xroot component, with those vcalendar childs. We need to
>             // handle both.
>@@ -440,20 +441,18 @@ calICSCalendar.prototype = {
>                         this.unmappedComponents.push(subComp);
>                         dump(subComp.componentType+"\n");
>                     }
>                     subComp = calComp.getNextSubcomponent("ANY");
>                 }
>                 calComp = rootComp.getNextSubcomponent('VCALENDAR');
>             }
>         } catch(e) {

The try block above is around the entire calendar read, which we do need.  However, as it stands, after the first error is hit, all subsequent items will be thrown out.  I think we need to nest another try around each item parse block.

>+    // Unless an error number is in this array, we consider it very bad, set
>+    // the calendar to readOnly, and give up.
>+    acceptableErrorNums: [Components.interfaces.calIErrors.ICS_NEWFAILED],
>+
>     onError: function(aErrNo, aMessage) {
>+        var errorIsOk = false;
>+        for each (num in this.acceptableErrorNums) {
>+            if (num == aErrNo) {
>+                errorIsOk = true;
>+                break;
>+            }
>+        }

I think it would work to say 

if (aErrno in acceptableErrorNums) { ... }

and completely skip manually iterating that array.  I might be misremembering, though.

Of course, if, as I suspect, we want to ditch the only member that is currently in that array, this won't really be applicable.  :-)

>Index: mozilla/calendar/resources/locale/en-US/calendar.properties
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/resources/locale/en-US/calendar.properties,v
>retrieving revision 1.40
>diff -p -U8 -r1.40 calendar.properties
>--- mozilla/calendar/resources/locale/en-US/calendar.properties	21 Oct 2005 16:51:13 -0000	1.40
>+++ mozilla/calendar/resources/locale/en-US/calendar.properties	23 Oct 2005 21:54:22 -0000
>@@ -200,13 +200,15 @@ outlookCSVDateParseConfirm=\
> # Remote calendar errors
> errorTitle=Error getting calendar
> httpError=Getting the calendar file failed.\nStatus code: %1$S: %2$S
> otherError=Getting the calendar file failed.\nStatus code: 0x%1$S
> httpPutError=Publishing the calendar file failed.\nStatus code: %1$S: %2$S
> otherPutError=Publishing the calendar file failed.\nStatus code: 0x%1$S
> contentError=This doesn't appear to be a valid file. Here's what I got back from\n%1$S:\nResult: %2$S
> readOnlyMode=There has been an error reading data for calendar: %1$S. It has been placed in read-only mode, since changes to this calendar will likely result in data-loss.  You may change this setting by choosing 'Edit Calendar'.
>+minorError=There has been an error reading data for calendar: %1$S.  However, this error is believed to be minor, so the program will attempt to continue.
>+stillReadOnlyError=There has been an error reading data for calendar: %1$S.  This calendar was previously placed in read-only mode, and has again been marked as read-only.

The second sentence sounds a bit awkward.  How about just replacing it with "This calendar is in read-only mode." or even just dropping it entirely?

Anyway, in general this looks good; thanks for taking this!

Comment 8

[Joey Minta]

Attachment: patch v2

Patch updated to reflect dmose's review comments, and comments from mvl in irc.

I couldn't get if (a in array) { } to work, so I left the iterating as it was.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 200637 [details] [diff] [review]
patch v2

I'm not sure that it's really worth leaving the "acceptable error" stuff in there, given that there aren't any acceptable errors, but, up to you.

r=dmose

Comment 10

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

Comment on attachment 200637 [details] [diff] [review]
patch v2

>Index: mozilla/calendar/base/src/calCalendarManager.js
>+    var emessage;

I would call it errorMessage or errorMsg. emessage looks like mMessage or aMessage, a different kind of prefix.


>Index: mozilla/calendar/resources/content/calErrorPrompt.xul
Why isn't this file in base/content ?

>+            document.getElementById("details-grid").collapsed = true;
...
>+        <grid id="details-grid" collapsed="true" persist="collapsed">
I think you shouldn't set collapsed if you want to persits it.

>Index: mozilla/calendar/resources/locale/en-US/calendar.properties
>+stillReadOnlyError=There has been an error reading data for calendar: %1$S.  The calendar has again been marked as read-only.

That feels weird. marked again? why not just leave the part about readonly out?

r=mvl with those looked at.

Comment 11

[Joey Minta]

patch checked in, ssitter already has a follow-up bug or two posted for 0.3a2

Comment 12

[Joey Minta]

*** Bug 278818 has been marked as a duplicate of this bug. ***

Comment 13

[Joey Minta]

*** Bug 277443 has been marked as a duplicate of this bug. ***

Comment 14

[Joey Minta]

*** Bug 243918 has been marked as a duplicate of this bug. ***