Closed Bug 418805 Opened 12 years ago Closed 12 years ago

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

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: andreas.treumann, Assigned: Fallen)

Details

(Keywords: dataloss)

Attachments

(2 files, 1 obsolete file)

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
Flags: blocking-calendar0.8?
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!

Sorry, I forgot to mention that this happens in online mode. 
Summary: Editing of an event on a cached calendar isn't possible → Editing of an event on a cached calendar in online mode isn't possible
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".
(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.
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.
Flags: blocking-calendar0.8? → blocking-calendar0.8+
I share Philipps opinion about the blocking status. 
Trying the STR with a cached google calendar -> editing works without any problems.
I will also not have time to take care about this before the beginning of the following week
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
Attached patch Fix v1 [checked in] β€” β€” Splinter Review
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #305397 - Flags: review?(sebo.moz)
When this is reviewed, please take care of checking in, since I will be away until wednesday evening.
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.
Attachment #305397 - Flags: review?(sebo.moz) → review+
Attachment #305397 - Attachment description: Fix v1 → Fix v1 [checked in]
Patch checked in. Removing blocking status.
Flags: blocking-calendar0.8+
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! ;-)
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. 
Summary: Editing of an event on a cached calendar in online mode isn't possible → Investigate correct checks when matching the old item in the memory calendar
Assignee: philipp → nobody
Status: ASSIGNED → NEW
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.
Flags: blocking-calendar0.8?
Summary: Investigate correct checks when matching the old item in the memory calendar → Editing of an event on a cached calendar in online mode isn't possible
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.
Flags: blocking-calendar0.8? → blocking-calendar0.8+
Attached patch Additional fix - v1 (obsolete) β€” β€” Splinter Review
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #307002 - Flags: review?(Berend.Cornelius)
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?
>+        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 on attachment 307002 [details] [diff] [review]
Additional fix - v1

r=Berend. The patch works fine and looks good
Attachment #307002 - Flags: review?(Berend.Cornelius) → review+
Attached patch Additional fix - v2 β€” β€” Splinter Review
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+
Attachment #307002 - Attachment is obsolete: true
Attachment #307063 - Flags: review+
Checked in on HEAD and MOZILLA_1_8_BRANCH and SUNBIRD_0_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Checked in sunbird 20080303 and lightning 20080303 -> task is fixed and verified.
Status: RESOLVED → VERIFIED
This checkin has regressed Bug 421616.
You need to log in before you can comment on or make changes to this bug.