418805 - Editing of an event on a cached calendar in online mode isn't possible

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Andreas Treumann]

STEPS TO REPRODUCE:
===================

- create a network calendar (webdav or file-url to a local ics-file)
- create an event 
- open the properties dialog and enable caching
- restart thunderbird/lightning
- open the edit dialog and rename the event or add some information
- save and close the dialog
- reopen the dialog

RESULT:
=======

- all new entries are lost

EXPECTED RESULT:
================

- the new entries should be saved and visible -> dataloss

REPRODUCIBLE:
=============

- always

Comment 1

[Stefan Sitter [:ssitter]]

As far as I know only offline viewing is currently supported. Offline editing and syncing is not yet supported.

See also Bug 393395 Comment #9:
> this patch is only step 1 of offline support, allowing the user
> to view his events while in offline mode. Changes to events while
> offline cannot be made in this step!

Comment 2

[Andreas Treumann]

Sorry, I forgot to mention that this happens in online mode. 

Comment 3

[Stefan Sitter [:ssitter]]

At least the error message should be improved: When trying to add/modify/delete an event while being offline the calendar just throws an useless error:

Error: Assert failed: unexpected!
1: [file:///[...]/js/calUtils.js:1369] ASSERT
2: [file:///[...]/js/calCachedCalendar.js:390] anonymous
3: [null:0] null
4: [file:///[...]/js/calTransactionManager.js:172] cT_doTransaction
5: [null:0] null
Source File: file:///[...]/js/calUtils.js Line: 1373

http://lxr.mozilla.org/mozilla/source/calendar/base/src/calCachedCalendar.js#350
http://lxr.mozilla.org/mozilla/source/calendar/base/src/calCachedCalendar.js#388
http://lxr.mozilla.org/mozilla/source/calendar/base/src/calCachedCalendar.js#423

I would expect at least some useful message reported to console like "Modifying event is not possible in offline mode".

Comment 4

[Stefan Sitter [:ssitter]]

(In reply to comment #2)
> Sorry, I forgot to mention that this happens in online mode. 

Ah, OK. In this case ignore Comment #1 and Comment #3 for now. 

Using an ics calendar with cache enabled in online mode I can add and delete events but I can't modify events.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

I'd say this blocks since it makes webdav unusable. Unfortunately, I fear I might not have time to fix this until thursday of next week.

Comment 6

[Lars Wohlfahrt (thetux)]

I share Philipps opinion about the blocking status. 

Comment 7

[Andreas Treumann]

Trying the STR with a cached google calendar -> editing works without any problems.

Comment 8

[Berend Cornelius]

I will also not have time to take care about this before the beginning of the following week

Comment 9

[Stefan Sitter [:ssitter]]

Testing with cached ics calendar shows that editing fails because modifyItem() exits early with the error message "old item mismatch" - whatever this means.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/calendar/providers/memory/calMemoryCalendar.js&rev=1.67&mark=237#229

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix v1 [checked in]

The check in the memory calendar was quite strict. Since the items that are being modified come from the storage cache, the icalComponent seems to be different while everything else stays.

To fix this, instead of using compareItems() to check if the old item in the memory calendar is the same as the oldItem passed, I compare the icalString.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

When this is reviewed, please take care of checking in, since I will be away until wednesday evening.

Comment 12

[Sebastian Schwieger]

This looks a bit like a workaround. bbbrowning also raised the question on IRC why the checks for id and generation shouldn't be enough. Lets take this patch for 0.8 since this bug is a blocker but leave it open for clarification later on.
r=sebo with a comment added that references this bug.

Comment 13

[Sebastian Schwieger]

Patch checked in. Removing blocking status.

Comment 14

[Bruno Browning]

FWIW, the memory calendar did originally just check that id and generation matched. The change to comparing actual items (first with ==, now with compareItems) came in bug 299760 with a patch by mvl, reason given that the first way just didn't work. Hard to argue with that! ;-)

Comment 15

[Bruno Browning]

I should add that with current code, commenting out the item comparison / icalString comparison does fix the can't-modify-item issue that is the subject of this bug. 

Comment 16

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13pre) Gecko/2008030120 Calendar/0.8pre with cached ics calendar:

Reported issue is NOT fixed. It's only possible to edit the event/task once. If you try to edit the event/task a second time it still fails.

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

I would say this blocks. I have found that this bug is actually caused by two things:

When using a cached calendar, the generation is always incremented by 2, one from the memory calendar of the ICS calendar, and one from the storage cache.

When using the event dialog to save (but not close), the window's item is taken from how it looks like before it is sent to modifyItem. This means the generation is not increased.

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Additional fix - v1

This patch fixes. Some changes are strictly cosmetical, therefore to ease review a small overview:


* Get rid of OpCompleteListener, replace directly with a listener that does
  whatever OpCompleteListener did before and additionally call listeners
  that were passed to the onAcceptCallback (i.e onModifyItem, onAddItem)
* In calendar-task-editing, also get rid of OpCompleteListener since it was the
  last user of this construct.
* In the event dialog, differ between saving only and saving and closing. If
  only saving, pass a listener to onAcceptCallback, to make sure the modified
  or added item is correctly set in the dialog. This makes the dialog's item
  have the correct generation after a change. When saving *and* closing, don't
  pass the listener since the window will be gone when the callback is complete
  and errors would show up if the listener is called on the closed window.
* The question is open if we want to disable the save button until the
  operation has completed. In this case we would need a progress bar and a way
  for the user to cancel the operation if he so desires. (See XXX comment)
* Modify the memory and storage calendars to only increase generation when not
  using relaxedMode. This prevents the cache from incrementing the generation
  twice as described above. Also make preventions to the comment daniel added
  about modifying the passed item instead of using the clone.

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Ause, do we want to move rc1 to tomorrow for this? Or should we just assume there will be an rc2 anyway and put this fix into rc2?

Comment 20

[Berend Cornelius]

>+        var innerListener =  {
>+            onOperationComplete: function(aCalendar, aStatus, aOpType, aId, >aItem) {
>+                if (Components.isSuccessCode(aStatus)) {
>+                    checkForAttendees(aItem, originalItem);
>+                }
>+                if (listener) {
>+                    listener.onOperationComplete.apply(listener, arguments);
>+                }
>+            }
>+        };
Isn't there a way to consolidate this? It occurs tree times in the source file.

Comment 21

[Berend Cornelius]

Comment on attachment 307002 [details] [diff] [review]
Additional fix - v1

r=Berend. The patch works fine and looks good

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Additional fix - v2

During this patch I have discovered additional problems with the task view. They don't seem to be related, since all works fine for the today pane or in sunbird.

I also fixed a problem when creating an event with the dialog, then saving without closing, then saving again.

This is the patch as checked in, carrying forth r+

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH and SUNBIRD_0_8_BRANCH

-> FIXED

Comment 24

[Andreas Treumann]

Checked in sunbird 20080303 and lightning 20080303 -> task is fixed and verified.

Comment 25

[Stefan Sitter [:ssitter]]

This checkin has regressed Bug 421616.