981055 - New event is lost when read-only calendar is currently selected ("An error occurred when writing to the calendar ...")

Status: NEW

(Calendar :: General, defect)

Description

[Markus Straub]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140218140359

Steps to reproduce:

I created a event and took some time to write a nice event with title, description, invited people, etc.
By accident a read-only calendar is selected (e.g. a Google holiday calendar)
I try to create the event by clicking 'save and close'


Actual results:

An error message pops up: "An error occurred when writing to the calendar <calendarName>!"
Error Code: MODIFICATION_FAILED
All my work is lost.


Expected results:

A notification tells me that writing to the calendar failed but the window with all my filled in information is not closed.
I select a proper writable calendar and try to create the event again.
Or I can save my Event description etc. via copy/paste.

This is not only useful for read-only calendars, but also for the rare case when creating an event in a writable calendar fails for whatever reason.

Comment 1

[[:MakeMyDay]]

Can you please let us know what version of Thunderbird and Lightning you are using and elaborate, how you made it to select a not-writable calendar as target calendar? Is the respective calendar (also) locally marked as read only (associated with a lock icon in the calendar list) or only on remote side?

Comment 2

[[:MakeMyDay]]

Sorry, I didn't notice you already filled the version field. Nevertheless, I would like to know, if the respective calendar was locally marked read-only when the error occured.

I reproduced the issue meanwhile with a caldav calendar, not marked read-only locally but with read-only access on server side, so confirming the bug.

A resolution to deal with problems on the remote end on event storing may be to extend the solution approach from bug 938459 to display the error message in the notification bar of the event dialog on top/instaed of the alert and prevent the dialog in this case from being closed.

I will provide a patch, once the one for bug 938459 is checked-in.

Comment 3

[Markus Straub]

Thanks for the swift reply!

The bug has been present for a long time, I think I first stumbled upon it around 2011 (using the latest Thunderbird/Lightning at the time).

You are right, when setting the calendar to read-only in Lightning (lock icon in calendar list) there is no problem. I did not think of this option, that helps a lot for now.

Still, I think it would be great to avoid losing the filled in 'new event' form when saving the event fails.

Comment 4

[[:MakeMyDay]]

The potential data loss not only extends to new events but also to existing ones if you change the calendar in the event dialog and the data actually cannot be stored therein. This and the reported scenario apply only, if the target calendar is not a cache calendar, though.

Since we switched to another approach to resolve bug 938459, we need a different solution here. I'm currently working on a patch, that will end up to provide an option in the error popup to reopen the event to enable the user to either store the event to another calendar or copy the entered data to somewhere else.

Comment 5

[[:MakeMyDay]]

Attachment: ReestablishDialogOnError-WIP-HandlingPart.diff

WIP patch: error handling part

Comment 6

[[:MakeMyDay]]

Attachment: ReestablishDialogOnError-WIP-NotificationPart-onError.diff

WIP patch: notification part using existing onError method of calIObserver

Comment 7

[[:MakeMyDay]]

Attachment: ReestablishDialogOnError-WIP-NotificationPart-onItemError.diff

WIP patch: notification part using a new onItemError method of calIObserver

Comment 8

[[:MakeMyDay]]

The attached patches are intended to get feedback on the way to implement a functionality to prevent data loss when failing to store the item without breaking asnychronous transaction handling.

Handling part:
Adding a checkbox to the existing error prompt dialog to enable the user to reopen the dialog with the event/todo which failed to be stored. If checked and confirmed by clicking ok, a new dialog with the previously entered data should appear.

Notification part:
There are two exclusive patches. One extends the already implemented onError method of calIObserver, while the other adds a new one instead. In both cases the item (and if existing the originalitem) will the passed through with the error message to the observer. Currently it comprises the modified notification feature calDAV provider only.

While the handling patch can be applied on an existing installtion, the notification patches cannot directly due to the interface changes. Without recompilation of the interface the data propagation is stopped at http://mxr.mozilla.org/comm-central/source/calendar/base/src/calUtils.js#1209, the last ressort before applying the respective method of the calIObserver. Until there, everything is passed through, though. All patches contain a small inline test section.

@Philipp: While the main of the handling part is straight forward, I not sure yet how to implement the notification part with a minimum impact on the code base, so any feeback is appreciated.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8406833 [details] [diff] [review]
ReestablishDialogOnError-WIP-HandlingPart.diff

Handling part looks nice and clean. A few minor issues, i.e comparing aItem.calendar != aOriginalItem is wrong (should be aOriginalItem.calendar, if aOriginalItem is defined).

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8406834 [details] [diff] [review]
ReestablishDialogOnError-WIP-NotificationPart-onError.diff

The only potential issue I see with this change is that we are changing an interface that is probably used by a few extensions to Lightning. The change is backwards compatible for JS so it might not be that bad.

Another thing: in calIObserver you just add a parameter, in calProviderUtils you changed the notification to "onItemError". Are there supposed to be two notifications in the future?

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8406835 [details] [diff] [review]
ReestablishDialogOnError-WIP-NotificationPart-onItemError.diff

Ah I see, my last comment mentions onItemError, it looks like this just crept into the wrong patch. In that case I'd prefer the onError approach because its backwards compatible.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to MakeMyDay from comment #8)
> @Philipp: While the main of the handling part is straight forward, I not
> sure yet how to implement the notification part with a minimum impact on the
> code base, so any feeback is appreciated.

Could you go into more detail what the troubles are? The onError approach you've uploaded seems to have sufficiently low impact.

If you need to recompile the interfaces and don't have a local build handy, I'd suggest moving forward with committer access, this way you can use the tryserver to create the .xpt file. If you want to do it manually, you can do so by using xpidl.py and xpt.py.

Comment 13

[[:MakeMyDay]]

> Ah I see, my last comment mentions onItemError, it looks like this just
> crept into the wrong patch. In that case I'd prefer the onError approach
> because its backwards compatible.

You're right, this was accidentally and must be onError.

Regarding the impact I proposed the onItemError approach to not change the existing observer, but if you feel comfortable with extending the onError one, I will go this way and provide a combined patch for review (this may take some days, as I couldn't yet recover all of my data - in the meantime I will proceed to get committer access to be able to 2e2 test this locally).

As this patch will contain two new strings, I guess target of this patch will be a post 3.3 release then.

Comment 14

[[:MakeMyDay]]

Attachment: ReestablishDialogOnError-V1.diff

Unified patch for review following the onError approach - win try build worked as expected.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8440404 [details] [diff] [review]
ReestablishDialogOnError-V1.diff

Review of attachment 8440404 [details] [diff] [review]:
-----------------------------------------------------------------

The below comments are mostly minor or just nits, so r=philipp with these comments considered:

::: calendar/base/content/calendar-item-editing.js
@@ +335,5 @@
>          return;
>      }
>  
> +    let onModifyItem;
> +    if (aOriginalItem !== undefined && aOriginalItem !== null) {

Why the strict check here? Can't we just use if (aOriginalItem) ? On another note, what about just one function with just the parameter changed, i.e

let onModifyItem = (item, calendar, originalItem, listener) => {
  doTransaction('modify', item, calendar, aOriginalItem || originalItem, listener);
};

@@ +338,5 @@
> +    let onModifyItem;
> +    if (aOriginalItem !== undefined && aOriginalItem !== null) {
> +        onModifyItem = function(item, calendar, originalItem, listener) {
> +            doTransaction('modify', item, calendar, aOriginalItem, listener);
> +        };        

Extra whitespace in this line.

@@ +378,5 @@
> +    // we only need to reopen if the have an aItem
> +    // if not present, this may be a deletion which doesn't require reopening
> +    if (newType !== null) {
> +        
> +        if (oldType === null) {

Extra whitespaces, you can just remove the empty line. Also, no need for such strict checks, just check if (newType) and if (!oldType)

@@ +381,5 @@
> +        
> +        if (oldType === null) {
> +            // aItem was newly created - let's make sure to have a calendar
> +            // and remove the item id to let it be treated as new event
> +            let calendar = (aItem.calendar) ? aItem.calendar : getSelectedCalendar();

let calendar = aItem.calendar || getSelectedCalendar();

::: calendar/base/public/calICalendar.idl
@@ +549,1 @@
>  interface calIObserver : nsISupports

The interface needs to change UUID when modified, please add ba=philipp to the commit message before pushing or uploading a patch for checkin-needed.

@@ +557,5 @@
> +  void onError( in calICalendar aCalendar,
> +                in nsresult aErrNo,
> +                in AUTF8String aMessage,
> +                in calIItemBase aNewItem,
> +                in calIItemBase aOldItem );

Can this method be called with null aNewItem/aOldItem? In that case, maybe it makes sense to add the [optional] IDL tag and use the new ES6 default parameters for the implementing listener:

function onError(aCalendar, aErrNo, aMessage, aNewItem=null, aOldItem=null) { ... }

::: calendar/base/src/calCalendarManager.js
@@ +953,5 @@
> +            let newItem =(cal.isEvent(aNewItem) || cal.isToDo(aNewItem)) ? aNewItem : null;
> +            let oldItem =(cal.isEvent(aNewItem) || cal.isToDo(aNewItem)) ? aOldItem : null;
> +            let type = null;
> +            if (newItem != null) {
> +                type = (cal.isEvent(newItem)) ? "event" : "todo"; 

Extra whitespace here

::: calendar/locales/en-US/chrome/calendar/calendar.dtd
@@ +343,5 @@
>  <!ENTITY calendar.error.code "Error code:">
>  <!ENTITY calendar.error.description "Description:">
>  <!ENTITY calendar.error.title "An error has occurred">
> +<!ENTITY calendar.error.reopenevent "Reopen the event dialog">
> +<!ENTITY calendar.error.reopentodo "Reopen the todo dialog">

I think most of our user facing strings use "task" instead of "todo"

Comment 16

[Matthew Mecca [:mmecca]]

Sorry for the late feedback, but maybe we could avoid the interface changes and simplify things by waiting for the item modification to succeed before actually closing the dialog, rather than closing it first and reopening it on failure?

Comment 17

[Matthew Mecca [:mmecca]]

Attachment: edit-close.diff

Here's a quick and only briefly tested patch showing what I mean. A downside here is that the dialog will be shown for the duration of the transation with a slow operation. Maybe we could work around that by hiding the dialog before the transaction starts, but I didn't actually try that.

I'll leave it up to you.

Comment 18

[[:MakeMyDay]]

Comment on attachment 8550773 [details] [diff] [review]
edit-close.diff

Thanks Matthew for having a look at this, but I think breaking the asynchronous handling for saving the items to the calendar is especially for remote calendars not a good idea. This may significantly worsen the UX.

The current patch still needs some minor modifications. Additionally, it actually covers only the caldav provider, so it needs to be extended at least to the ICS provider (and eventually WCAP and Google provider).

How do we want to deal with this bug and bug 1120258? We should decide which way to go - implementing both doesn't really make sense. If we decide to follow the approach in this bug, I will provide an updated patch.

Matthew, Philipp, what do you think?

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Currently I wouldn't be in favor of keeping the dialog open, because especially in the network case this would cause an unacceptable delay. Eventually I'd like to always do the modifications on the cache and then have the provider synchronize the items.

I'd love to get an opinion from Richard or Decathlon here, I'm probably not the best person to make UX decisions. I think at current it would make sense to use the behavior from bug 1120258 especially in the case where Save (not Save&Close) is clicked. For the Save&Close case, close the dialog initially, but reopen it and show the conflict dialog immediately. Wouldn't that work?

Also, I think Decathlon also worked on something similar, or am I mistaking?

Comment 20

[Richard Marti (:Paenglab)]

Closing the dialog and then opening it maybe 5 seconds later wouldn't be the best UI but still better than to leave it open for this 5 seconds which would let look the UI slow.

I don't know if all protocols support this, but what about checking in the background directly after opening the dialog or changing the calendar in dialog, if the calendar is writable? Then Lightning could immediately show on Save&Close the conflict dialog. Better would be a feedback like a notification during editing or turning the calendar in the menulist to red with adding a "(read only)". Then the user could know before filling everything out there will be a problem with this calendar.

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Richard Marti (:Paenglab) from comment #20)
> Closing the dialog and then opening it maybe 5 seconds later wouldn't be the
> best UI but still better than to leave it open for this 5 seconds which
> would let look the UI slow.
> 
> I don't know if all protocols support this, but what about checking in the
> background directly after opening the dialog or changing the calendar in
> dialog, if the calendar is writable? 
I don't think this will be reliable. The readonly state could change between opening the dialog and saving. If the calendar is really readonly we can do a certain amount of checking when the calendar properties are updated, but I wouldn't do this when the event dialog is opened. The gdata provider detects if a calendar is readonly by permissions, and forces the readonly state to stay as it is.

There may also be other reasons the calendars writing failed, for example a server error or network connection error. These should be handled by the cache though.

Comment 22

[Decathlon]

(In reply to Philipp Kewisch [:Fallen] from comment #19)

> Also, I think Decathlon also worked on something similar, or am I mistaking?

I worked on a few fix about the behavior of the warning dialog when the user inputs a wrong end date in the dialog and I extended the behavior to the until date for "not custom" repeating events. This case is quite different though, because the problem happens only on save (and save&close) and because of the delay between the moment we try to save/close and the moment we know that something has gone wrong.
Personally, if it is possible, I would prefer to keep the dialog open, and using the notification box to communicate with the user about a save process in progress on a network calendar, about a possible error, if it happens, and inform about the actions that could be taken in this case. Nevertheless if the delay is too long this is not feasible also because the edit dialog is not modal and it might be placed in background while Thunderbird is in use (unless we don't bring the dialog in foreground when a problem arises).
What might it be the delay we can bump into? I tested a bit with a Google calendar and the delay is around one second, but I see it might be extremely variable in case of problems.

Automatically reopening the dialog in case of problems, instead of opening a warning dialog, could be a solution for cached calendars too if we use the notification box in the dialog to explain the problem with a bit of details about the cause e.g. network connection instead of read-only calendar. The dialog should stay in foreground (like the error dialog) because the user must not miss it since it is an anomalous behavior.

I admit it's a complicated issue keeping in count all the elements even from bug 1120258.

Comment 23

[[:MakeMyDay]]

The patch in this bug does not automatically reopen the event dialog. It adds a button to the existing error popup to provide the option to (re)open the dialog to the user.

Beside the UX with closing and (re)openening the dialog, there's also another point to consider: the event dialog is only one source, where calendar modifications can be triggerd from. Beside that, we have also the summary dialog, the imip bar, the context menus in calendar view and today pane, the direct modification in calendar/task view and last but not least the alarm dialog. It would be weired, if there's a recovery mechanism for some of these and for others not.

Another option would be to make the cache mandatory. From UX perspective this would be probably the best solution compared to the other approaches discussed. And it's already there.

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to MakeMyDay from comment #23)
> Another option would be to make the cache mandatory. From UX perspective
> this would be probably the best solution compared to the other approaches
> discussed. And it's already there.
Just to clarify, do you mean enabling the cache for all calendar types and not allowing it to be turned off? This is definitely where we are heading, but I don't think we are there yet. We need to land the async storage patch, we probably need some UI for events changed but not uploaded yet and likely a few more.



(In reply to Decathlon from comment #22)
> Personally, if it is possible, I would prefer to keep the dialog open, and
> using the notification box to communicate with the user about a save process
> in progress on a network calendar, about a possible error, if it happens,
> and inform about the actions that could be taken in this case.
Doesn't the delay stop your workflow? For me personally, I'm already annoyed at the Thunderbird compose dialog and would love to send email in the background instead of waiting for it to send.

> I tested a bit with a Google
> calendar and the delay is around one second, but I see it might be extremely
> variable in case of problems.
Just imagine a slow network connection, it could takes multiple seconds.

> 
> Automatically reopening the dialog in case of problems, instead of opening a
> warning dialog, could be a solution for cached calendars too if we use the
> notification box in the dialog to explain the problem with a bit of details
> about the cause e.g. network connection instead of read-only calendar. The
> dialog should stay in foreground (like the error dialog) because the user
> must not miss it since it is an anomalous behavior.
I think using the notification box on reopen is a good idea, especially for non-conflict cases, what do the others think?

Comment 25

[Matthew Mecca [:mmecca]]

(In reply to MakeMyDay from comment #18)
> How do we want to deal with this bug and bug 1120258? We should decide which
> way to go - implementing both doesn't really make sense. If we decide to
> follow the approach in this bug, I will provide an updated patch.

I think the two approaches are compatible, the fix in this bug handles error conditions, while fixes like in bug 1120258 prevent the modification error when possible.