Open Bug 455260 Opened 16 years ago Updated 1 year ago

Present CalDAV parsing error messages in activity manager

Categories

(Calendar :: Provider: CalDAV, enhancement)

enhancement

Tracking

(Not tracked)

People

(Reporter: browning, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: dogfood, Whiteboard: [has l10n impact])

Attachments

(2 files)

Currently when the CalDAV provider encounters a non-parseable item (say one suffering from bug 357399) in data returned from the server, it bails with a generic error message and sets the calendar read-only (or tries to). It ought to do better reporting and continue processing any remaining calendar-data. 

This patch has it LOG the calendar-data that it failed to parse and continue processing; also fixes some issues with setting the calendar read-only. I'm actually having trouble testing it well: I have an example .ics that triggers bug 357399, but we can't import it, and the CalDAV servers I have access to either refuse to let me PUT it or repair the file when I do. Would be good to find someone with a bad item in an existing calendar who could test it; since it's entirely js it could easily be tested in Sunbird by patching the provider file, not needing a build.
This patch did it for us, trying to find out why calendar didn't load. 

Turned out it was error on an exception from a repeating rule
Thanks for testing!

I'm adding chris-j to the cc list for a UI opinion. Chris, this is an issue wrt processing an incoming set of calendar-items at startup or refresh(). The current (mis)behavior when encountering an unparseable item is to set the calendar ro and stop processing the data set. With the patch, when we encounter an unparseable item we set the calendar ro and continue processsing, after displaying the unparseable calendar-data in the error console. Do we need as well a bit of UI saying something like "X items could not be displayed" so that the user has something more than the warning triangle?
Sorry for the possibly ignorant question, but why does it need to be set to ro? If it was an ics calendar where the entire calendar was written back on a change it would be understandable, but in caldav you only write back the new/changed entry
(In reply to comment #4)
> Sorry for the possibly ignorant question, but why does it need to be set to ro?

It probably doesn't; that's kind of the question I was raising. The patch as it stands was written under string freeze days before the 0.9 release, and under those circumstances sticking with the current UI and displaying the "bad" iCalendar data in the error console was about the best we could do. Since the patch didn't make the 0.9 train and we're no longer under string freeze we can now discuss what UI we'd prefer.
Attached file proposed dialog
Proposing a UI for informing the user that an unparseable item was retrieved from the server.
Attachment #340681 - Flags: ui-review?
Attachment #340681 - Flags: ui-review? → ui-review?(christian.jansen)
Regarding your proposal:
Do we have some data how frequently this happens, or assumptions how often this could happen? I'm asking, because from an users point it is very hard to judge on these type of questions. Users might ask themselves "Hey, what happens if I press "Delete Item from Server" is the data destroyed, or is this a client problem?" " I can't judge on this"

In the end, It might it make sense to notify the issue in the status bar, only. This would have the advantage that it would not block users. The more  technically interested user could click on the notification, this could rise up your proposed dialog.

Is the the default choice [ Ignore This Item ] ?

Any other suggestions?
(In reply to comment #7)

We don't have any hard data, but as near as I can tell this is a fairly common problem, currently probably our most visible bug for CalDAV users in mixed (Mozilla/nom-Mozilla) environments. The most common scenario seems to be bug 357399 being triggered by items placed on a CalDAV server by programs other than Sb/Ltn.

I like the notion of a statusbar notification only. And yes, I think the default choice should be [Ignore This Item].

One thing I'm still not clear on, though, with using the statusbar. It's entirely possible that more than one unparseable item can be returned by refresh(), and we need a way to deal with that situation. My suggestion would be that the notification include the count of problem items, and that once the user makes a choice in the dialog the dialog closes and the problem-item count decrements. Does that make sense?
I have also tried the patch an now I can see my calendar data.  Hurray!

But how can I find the 'bad item'?
(In reply to comment #9)
> But how can I find the 'bad item'?

Create and set to true a boolean pref calendar.debug.log
After you do this the calendar-data for problem items will be displayed in the error console. The patch does not yet let you *do* anything about such items other than identify them.
Comment on attachment 338565 [details] [diff] [review]
LOG bad calendar-data and continue

I understand we're pushing to make Lightinng "dogfoodable" so I'm suggesting that we commit the current patch now and add the UI later in a separate patch in this bug. Upside: calendars start working for people. Downside: problem items get ignored. So, reqesting review on this first part of the fix.
Attachment #338565 - Flags: review?(daniel.boelzle)
(In reply to comment #11)
> (From update of attachment 338565 [details] [diff] [review])
> I understand we're pushing to make Lightinng "dogfoodable" so I'm suggesting
> that we commit the current patch now and add the UI later in a separate patch
> in this bug. Upside: calendars start working for people. Downside: problem
> items get ignored. So, reqesting review on this first part of the fix.

Makes sens. Regarding the UI it would be good if we can coe up with the status bar solution. ui+ for this concept. Let me know if you've updated the ui part of the patch. Thanks. :-)
Flags: tb-integration+
Keywords: dogfood
Comment on attachment 338565 [details] [diff] [review]
LOG bad calendar-data and continue

The |mDisabled|->|disabled| change looks incomplete to me. Moreover, why did you do that?

r=dbo with that resolved/fixed.
Attachment #338565 - Flags: review?(daniel.boelzle) → review+
Priority: -- → P2
Assignee: nobody → browning
Status: NEW → ASSIGNED
Comment on attachment 340681 [details]
proposed dialog

I think the solution should come without a warning dialog. At the moment it should be enough to post the unusable calendar data into the error log of the console. From an users point of view it is pretty hard to judge what is right or what is wrong for these type of data.
Attachment #340681 - Flags: ui-review?(christian.jansen) → ui-review-
Just to add my 2 cents to this:

I'd rather prefer to save users from read errors as far as possible. IMHO most people aren't really interested in fixing their calendar data unless they really need to. Said another way: I prefer to have those data bugs reported, e.g. into console or growl, without bugging the user with a dialog. The user could continue to work without problem unless he specifically wants to edit such a broken item. In such circumstances it would be cool if the event dialog or views could present the broken data differently.
Whiteboard: [needs activity manager]
I am not yet convinced that this is an integration issue (resetting tb-integration?), but could be fixed for the release (blocking-calendar1.0+).
Flags: tb-integration?
Flags: tb-integration+
Flags: blocking-calendar1.0+
Summary: Better handling of nonparseable ics on CalDAV → Present CalDAV parsing error messages in activity manager
Whiteboard: [needs activity manager] → [not needed beta][has l10n impact]
Whiteboard: [not needed beta][has l10n impact] → [not needed beta][has l10n impact][needs updated patch]
From a UI perspective, we don't really have anything to correctly present this in. There are poptarts (bug 476696), but as said, do we really want to bug the user with parse errors like this?

What if all events on the server fail to parse because of some unforseen error? 

What if the user has 2-3 events that fail but its not his calendar, or maybe even on an ACL server that doesn't allow him to change/delete those events?
  This will mean he gets errors each time the calendar is read?

I agree we can add status messages to the activity manager that the read process has failed (I have a patch in progress), but we need a good solution for parsing errors too.

Bryan, any ideas?
Assignee: browning → clarkbw
bug 489588 has hooked the activity manager info into the status bar so any parse errors could at least be seen in the status bar.

I think the question for using pop-tarts is always "Can the user fix this problem by giving some additional input".  Some of the parse errors seem like they can't really be fixed, though perhaps the ACL one can.  

If a person goes to delete/change an event and it doesn't go through I think we need to notify them synchronously/immediately via a dialog or maybe a pop-tart.

The other errors are difficult because the consequences are a little vague.  If a parse error means the user doesn't get their alarm then we've really failed them.  However if the parse error is only a passing error about a future event maybe we just want to log that error for now.
Comment on attachment 338565 [details] [diff] [review]
LOG bad calendar-data and continue

What is the status of this reviewed patch? Is it obsolete? Or ready for checkin?
I think a GUI dialog is a nice feature - optionally it is per default not activated and you can de-/activate it for each calendar.

A button to display the plain-text of the event will also be very nice (to debug the problem).
I think we can live without this for 1.0
Assignee: clarkbw → nobody
Flags: tb-integration?
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][has l10n impact][needs updated patch] → [has l10n impact][needs updated patch]
Status: ASSIGNED → NEW
Blocks: 536929
Many people here seem to be concerned about the potential intrusiveness of an error notification mechanism.

Maybe the following suggestion might work:

If a parsing error occurs (or caldav server returns error, pop up a single non-blocking, non-modal dialog box saying: "Some error(s) have occurred while reading calendar"
with two buttons "[] show list with errors   [] ignore"

The list would then be a scrollable "console" containing all error messages, preferably with selectable text, so that people can paste (parts of) these messages into google, bug reports, mails to friends who might help, forum questions, etc.

This "console" may either be a window of its own (like the Ctrl-Shift-J error console), or some panel docked in Lightnings tab that only appears when needed.

Justification of the details:
- a pop-up box (rather than nothing at all): that way the user (even a user new to Lightning) is easily made aware of the occurrence of the error, with an easy-to-use button that leads to full message
- a single box, even for multiple message: that way the user is not flooded if every single event of a calendar happens to have an error...
- non-blocking: rest of calendar (other events) still continues to be imported while box is up.
- non-modal: should be obvious... but for those who don't see it: a non-modal box doesn't interrupt the user in his work that he might be doing in other parts of thunderbird (such as typing a mail, etc.), nor does it interfere with focus, desktop selection, etc.
- scrollable console: an easy way to hold potentially many messages, just for the case where every single calendar event was bad...
- console only shows up when button in dialog clicked: that way, it doesn't clutter the UI when no error occurs.
- selectable text in console: should be obvious... but for those who don't see it: if an error actually does occur, for many users the first step may be to google it. And copy-pasting the message into google is more convenient than having to retype it manually. And text should be selectable by swiping it with the left mouse button, the same as any other text, rather than some weird right-mouse-button menu entry that nobody is expecting, and that only selects entire lines at once (as is the case with the Ctrl-Shift-J javascript error console...)
Priority: P2 → --
Severity: normal → enhancement
Depends on: 472483
Severity: normal → S3
Whiteboard: [has l10n impact][needs updated patch] → [has l10n impact]
You need to log in before you can comment on or make changes to this bug.