Closed Bug 370079 Opened 13 years ago Closed 13 years ago

Show more user-friendly error message when import fails

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mvl, Assigned: mvl)

References

Details

Attachments

(2 files)

There have been quite a few bugreports about importing failing. There are two main errors: invalid timezones and duplicate entries. But the error messages are very cryptic and don't even hint at the real problem. They should just tell what is going on, in a human readable format.
Attached patch patch v1Splinter Review
This makes the error better readable.
Assignee: nobody → mvl
Status: NEW → ASSIGNED
Attachment #254742 - Flags: first-review?(lilmatt)
Duplicate of this bug: 369263
Comment on attachment 254742 [details] [diff] [review]
patch v1

>Index: base/content/import-export.js
>===================================================================
>-           showError(getCalStringBundle().GetStringFromName("unableToRead") + filePath + "\n"+ex );
>+            var strbundle = getCalStringBundle();
>+            switch (ex.result) {
>+                case Components.interfaces.calIErrors.INVALID_TIMEZONE:
>+                    showError(strbundle.formatStringFromName("timezoneError", [filePath] , 1));
ssitter just added support to calGetString to handle formatting strings.  Maybe you want to use that here?
calGetString("calendar", "timezoneError", [filePath]); is the syntax

>+                    break;
>+                default:
>+                    showError(getCalStringBundle().GetStringFromName("unableToRead") + filePath + "\n"+ex );
Put spaces around the + at the end of this line, and remove the extra space before the ).

>+                if (!failedCount && duplicateCount) {
>+                    showError("error" +duplicateCount);
Add a space after the +

>+                    showError(getCalStringBundle().formatStringFromName("duplicateError", [duplicateCount, aFilePath] , 2));
Same here?

>+                } else if (failedCount) {
>                     showError(failedCount+" items failed to import. The last error was: "+lastError.toString());
Missed a spot for using the stringbundle


>Index: base/public/calIErrors.idl
>===================================================================
>+  /**
>+   * Tried to add an item to a calendar that already existed
>+   */
>+  const unsigned long DUPLICATE_ID = ERROR_BASE + 4;
Edit this comment.  Right now it sounds like the _calendar_ already existed.


>Index: locales/en-US/chrome/calendar/calendar.properties
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/locales/en-US/chrome/calendar/calendar.properties,v
>retrieving revision 1.75
>diff -u -8 -p -r1.75 calendar.properties
>--- locales/en-US/chrome/calendar/calendar.properties	14 Jan 2007 04:25:03 -0000	1.75
>+++ locales/en-US/chrome/calendar/calendar.properties	11 Feb 2007 19:32:08 -0000
>@@ -77,16 +77,18 @@ publishPrompt=Which calendar do you want
> 
> #spaces needed at the end of the following lines
> eventDescription=Description:
> 
> unableToRead=Unable to read from file: 
> unableToWrite=Unable to write to file: 
> defaultFileName=MozillaCalEvents
> HTMLTitle=Mozilla Calendar
>+timezoneError=An item in the file has an unknown timezone while reading %1$S
>+duplicateError=%1$S item(s) were already present in the destination calendar while reading %2$S

In these errors (and the "x items failed to import" one), I was thinking that we could do better that "item(s)".  We should add strings for "item", "items", "item was", and "items were" so that we can select the appropriate one based on the number or failed or duplicate items.  It'd be pretty simple, and it looks much nicer.

I would reword these as follows:
timezoneError=An item in the file %1$S refers to an unknown timezone.
duplicateError%1$S item(s) in the file %2$S already exist in the destination calendar.

r=lilmatt with the changes
Attachment #254742 - Flags: first-review?(lilmatt) → first-review+
(In reply to comment #3)

FYI: Some of the nits have been fixed already with Bug 343173 and Bug 369928.

> In these errors (and the "x items failed to import" one), I was 
> thinking that we could do better that "item(s)".  We should add 
> strings for "item", "items", "item was", and "items were" so that
> we can select the appropriate one based on the number or failed
> or duplicate items.  It'd be pretty simple, and it looks much
> nicer.

This won't work well in many localizations and would probably cause problems similar to Bug 338167. (If I understand the intention correct).
> >+timezoneError=An item in the file has an unknown timezone while reading %1$S
> >+duplicateError=%1$S item(s) were already present in the destination calendar > I would reword these as follows:
> timezoneError=An item in the file %1$S refers to an unknown timezone.
> duplicateError%1$S item(s) in the file %2$S already exist in the destination
> calendar.

I choose my order of words for a very specific reason: name important things first, ans less important things later. In that case of timezones error, the fact that the timezones are the problem is much more important then the filename. The user already knows the filename, and the filename is likely quite long (it includes that path). So I really want to have the word 'timezone' before the path.
The same goes for the other error.
Better wording with this in mind is welcome!
(In reply to comment #5)
> Better wording with this in mind is welcome!
> 
My suggestions:
timezoneError=An unknown and undefined timezone was found while reading %1$S.
duplicateError=%1$S item(s) were ignored since they exist in both the destination calendar and %2$S.
(In reply to comment #6)
+1 for jminta's verbage
Attached image Another cryptic message
I've added a screenshot whilst importing an iCal file with version 0.3.1.
I can't really deduce what is wrong, where to look.
please read comment 0: this bug is about two very common errors, not about all possible errors.
(yours is likely a failed installation)
Mine is indeed a different error.
Yet the title of the bug is about having human readable error messages.
This was just an example.

(I did an installation on Windows and Linux, with appropriate xpi.
Both gave the same error)
Patch (with comments fixed, and using jminta's wording) checked in
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
The checkin caused the following error:

Error: getCalStringBundle is not defined
Source File: chrome://calendar/content/import-export.js
Line: 110
Bustage fix checked in (just removed the line, it was no longer needed)
Verified with Lightning/0.5pre (2007032003) and Thunderbird/1.5.0.10 (20070221)

Nit: The import error message is not always correct. If you export an event from calendar A and try to import into calendar B the error message states that the event already exists in the destination calendar B. This is not correct.
Status: RESOLVED → VERIFIED
Depends on: 329842
You need to log in before you can comment on or make changes to this bug.