Saving an item in the event dialog fails with MODIFICATION_FAILED if the item has been modified elsewhere

RESOLVED FIXED in 4.0.0.1

Status

Calendar
Dialogs
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mmecca, Assigned: mmecca)

Tracking

(Blocks: 1 bug)

unspecified
4.0.0.1

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
The event dialog doesn't listen for changes made to the item being edited, so saving the item fails the generation check and throws a MODIFICATION_FAILED error.

This can be triggered by clicking the "Details..." link in the reminder dialog, which sometimes opens the event dialog behind the reminder dialog, then closing the reminder dialog before editing and saving the item in the event dialog. It can also be triggered by opening a task for editing then modifying the task via the task list context menu.
(Assignee)

Comment 1

2 years ago
Created attachment 8547248 [details] [diff] [review]
item-dialog-observer.diff

Adds an event listener to the event dialog to detect modification conflicts, and prompts the user to overwrite the changes in the dialog, or ignore and overwrite the other changes.

The prompt message might be a bit wordy, I'm open to other suggestions.
Attachment #8547248 - Flags: ui-review?(richard.marti)
Attachment #8547248 - Flags: review?(philipp)

Comment 2

2 years ago
I guess we would need this for the event summary dialog as well, don't we?

Comment 3

2 years ago
I prepared a patch for a similar problem for bug 981055 with a different approach, which still had some flaws although already reviewed (iirc) and would at least require some testing. I think your approach seems to be more straight forward, but I just wanted to reference the other one here for completeness.

Eventually it's worth also to display what has changed to enable an informed decision when the override prompt pops up?
I think it's not easy for the user to know what he is doing with this long text and the Yes|No buttons.

Would it be possible to use instead of the Yes|No buttons in this prompt use something like "Discard this changes" and "Overwrite the other changes"? 

This could the look like:

,-----------------------------------------------------------------.
|                                                                 |
|  The item being edited in the dialog has been modified since    |
|  it was opened                                                  |
|                                                                 |
|  ,----------------------.      ,-----------------------------.  |
|  | Discard this changes |      | Overwrite the other changes |  |
|  `----------------------´      `-----------------------------´  |
|                                                                 |
`-----------------------------------------------------------------´

What do you think?
Comment on attachment 8547248 [details] [diff] [review]
item-dialog-observer.diff

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

Codewise r+ with the following two comments and taking Richard's suggestion into account.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +82,5 @@
> +            }
> +            window.calendarItem = item;
> +
> +            if (doUpdate) {
> +                loadDialog(window.calendarItem);

Just an idle thought, are we handling the case correctly where the user opens a new event dialog, saves the event (but doesn't close the dialog) and then triggers a  modification like this? I think we have some flags about the item being new or not, if they are taken into account in loadDialog we should make sure.

::: calendar/locales/en-US/chrome/calendar/calendar.properties
@@ +600,5 @@
>  showOnlyCalendar=Show Only %1$S
> +
> +# LOCALIZATION NOTE (modifyConflict)
> +# Used by the event dialog to resolve item modification conflicts.
> +modifyConflictPromptTitle=Item Modify Conflict

I agree with Richard, maybe we could shorten the strings a bit. For the title, shouldn't it be "Item Modification Conflict" ?
Attachment #8547248 - Flags: review?(philipp) → review+
Comment on attachment 8547248 [details] [diff] [review]
item-dialog-observer.diff

I'm only setting ui-r- to see a new version with shorter strings.
Attachment #8547248 - Flags: ui-review?(richard.marti) → ui-review-
(Assignee)

Comment 7

2 years ago
(In reply to Philipp Kewisch [:Fallen] from comment #5)
> ::: calendar/base/content/dialogs/calendar-event-dialog.js
> @@ +82,5 @@
> > +            }
> > +            window.calendarItem = item;
> > +
> > +            if (doUpdate) {
> > +                loadDialog(window.calendarItem);
> 
> Just an idle thought, are we handling the case correctly where the user
> opens a new event dialog, saves the event (but doesn't close the dialog) and
> then triggers a  modification like this? I think we have some flags about
> the item being new or not, if they are taken into account in loadDialog we
> should make sure.

By the time we get here, the new item has already been saved, so the dialog is in modify mode either way. However, I did find that in this scenario we shouldn't set the gIsModifyingItem flag, since the first save doesn't trigger an onModifyItem event.
(Assignee)

Comment 8

2 years ago
Created attachment 8561198 [details] [diff] [review]
Fix v2 [checked in]

Changes the strings as suggested in comment #4.
Attachment #8547248 - Attachment is obsolete: true
Attachment #8561198 - Flags: ui-review?(richard.marti)
Attachment #8561198 - Flags: review+
Comment on attachment 8561198 [details] [diff] [review]
Fix v2 [checked in]

Looks good and is better understandable.
Attachment #8561198 - Flags: ui-review?(richard.marti) → ui-review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
https://hg.mozilla.org/comm-central/rev/2872ad20a041

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.0
I've noticed two issues with the patch. First of all, the observer is never removed so after closing the dialog and removing the event it will show an error message in the console. 

Second, I get an error with MODIFICATION_FAILED (no details) when deleting an event where the event dialog is open.

Matt, can you take a look into this issue in the next week? Otherwise we might have to back this out since its going to aurora on Monday.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Attachment #8561198 - Attachment description: Fix v2 → Fix v2 [checked in]
Attachment #8561198 - Flags: checkin+
(Assignee)

Comment 12

2 years ago
Created attachment 8567694 [details] [diff] [review]
Fix - Part 2

This patch removes the calendar observer when the dialog closes, and correctly changes the observer's target when the item calendar is changed. It also fixes an issue causing the entries in the dialog calendar list to be duplicated in some cases, and and issue causing the item calendar to be incorrectly reset to it's original value if it was changed in the dialog and the item was changed again elsewhere.
Attachment #8567694 - Flags: review?(philipp)
Comment on attachment 8567694 [details] [diff] [review]
Fix - Part 2

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

Unfortunately I am still getting an error with the following STR:

1) create event in calendar
2) open the event in event dialog
3) delete the event in calendar view

Result: A modification failed dialog
Attachment #8567694 - Flags: review?(philipp) → review-
(Assignee)

Comment 14

2 years ago
(In reply to Philipp Kewisch [:Fallen] from comment #13)
> Comment on attachment 8567694 [details] [diff] [review]
> Fix - Part 2
> 
> Review of attachment 8567694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately I am still getting an error with the following STR:
> 
> 1) create event in calendar
> 2) open the event in event dialog
> 3) delete the event in calendar view
> 
> Result: A modification failed dialog

I don't think this is caused by the patches in this bug, I can reproduce the same error message in the console on a comm-aurora build. Also, it only seems to happen if the dialog is opened by double-clicking the event in the view, the error doesn't occur if it's opened from the view context menu, or from the today pane or unifinder, or for tasks opened from the task tree. 

At a quick glance, the double-click in the view seems to be the only code path to call into createPendingModification at [1]. I think that can be changed to open the dialog the same way as the other code paths, but I'd like to look into that further. I think it's safe to do that in another bug and take these patches for 4.0.

[1] : http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-views.js#53
Flags: needinfo?(philipp)
Comment on attachment 8567694 [details] [diff] [review]
Fix - Part 2

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

Ok, then lets take this. I'd like to have the other case fixed for 4.0 too, can you file the followup bug and look into it in the next two weeks?
Attachment #8567694 - Flags: review- → review+
Pushed to comm-central changeset cbfa9bc5e436
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Flags: needinfo?(philipp)
You need to log in before you can comment on or make changes to this bug.