Last Comment Bug 494140 - Multiple reminders,relations,attachments created by modifying repeating event
: Multiple reminders,relations,attachments created by modifying repeating event
Status: RESOLVED FIXED
[needed beta][no l10n impact]
:
Product: Calendar
Classification: Client Software
Component: Alarms (show other bugs)
: unspecified
: All All
: -- major (vote)
: 1.0b1
Assigned To: Philipp Kewisch [:Fallen]
:
Mentors:
: 458717 496528 525312 526954 (view as bug list)
Depends on: 470430
Blocks: 524940
  Show dependency treegraph
 
Reported: 2009-05-21 02:01 PDT by Sven Grull
Modified: 2011-11-07 03:55 PST (History)
12 users (show)
philipp: blocking‑calendar1.0+
philipp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix - v1 (16.49 KB, patch)
2009-06-22 01:32 PDT, Philipp Kewisch [:Fallen]
dbo.moz: review+
Details | Diff | Splinter Review
Fix - v2 (18.16 KB, patch)
2009-07-04 12:24 PDT, Daniel Boelzle [:dbo]
mschroeder: review-
ssitter: review-
Details | Diff | Splinter Review
Fix - v3 (18.06 KB, patch)
2009-08-09 10:26 PDT, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
Fix - v4 (18.49 KB, patch)
2009-09-05 04:56 PDT, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
Fix - v5 (24.71 KB, patch)
2009-10-05 05:47 PDT, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
Fix - v6 (25.94 KB, patch)
2009-10-05 11:18 PDT, Philipp Kewisch [:Fallen]
ssitter: review+
dbo.moz: review+
Details | Diff | Splinter Review

Description Sven Grull 2009-05-21 02:01:21 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090519 SeaMonkey/2.0b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1b5pre) Gecko/20090520 Calendar/1.0pre

Multiple reminders created by modifying one instance of a repeating event

Reproducible: Always

Steps to Reproduce:
1. create a repeating event with a single reminder
2. move one single instance of the repeating events to a different day
3. open event dialog with "Edit all occurrences"
Actual Results:  
Multiple reminders created

Expected Results:  
Single reminder should persist
Comment 1 Sven Grull 2009-05-21 02:02:48 PDT
Happens with Lightning too.
Comment 2 Sven Grull 2009-05-21 02:08:18 PDT
Sorry. I missed one step. After moving the single instance you have to close Sunbird and start it again to see the duplication of reminders.
Comment 3 Philipp Kewisch [:Fallen] 2009-06-05 09:28:11 PDT
Confirmed per upcoming duplicate.
Comment 4 Philipp Kewisch [:Fallen] 2009-06-05 09:28:35 PDT
*** Bug 496528 has been marked as a duplicate of this bug. ***
Comment 5 Philipp Kewisch [:Fallen] 2009-06-22 01:31:16 PDT
This happens only on the storage provider and is also true for relations and attachments.
Comment 6 Philipp Kewisch [:Fallen] 2009-06-22 01:32:52 PDT
Created attachment 384363 [details] [diff] [review]
Fix - v1

The unit test here is only theory. I couldn't get tests to run on my machine, the storage provider failed on createInstance(). The new method of running tests is make -C calendar/test xpcshell-tests.
Comment 7 Philipp Kewisch [:Fallen] 2009-06-22 01:33:22 PDT
Comment on attachment 384363 [details] [diff] [review]
Fix - v1

Requesting additional review for storage schema changes.
Comment 8 Sven Grull 2009-06-27 01:06:24 PDT
(In reply to comment #5)
> This happens only on the storage provider

Is there also a storage provider instance for local calendars? Just asking as I see this problem with my local calendar and don't know how they are saved.
Comment 9 Stefan Sitter 2009-06-27 06:50:50 PDT
(In reply to comment #8)
Yes, local calendars use the storage provider.
Comment 10 Daniel Boelzle [:dbo] 2009-07-04 12:24:29 PDT
Created attachment 386839 [details] [diff] [review]
Fix - v2

I've fixed the unit test and some warnings:

JS Component Loader: WARNING file:///data/dbo/moz_cc/sbird-cc-debug_Darwin/mozilla/dist/bin/modules/calUtils.jsm -> file:///data/dbo/moz_cc/sbird-cc-debug_Darwin/mozilla/dist/bin/calendar-js/calStorageCalendar.js:3139
                     trailing comma is not legal in ECMA-262 object initializers
JS Component Loader: WARNING file:///data/dbo/moz_cc/sbird-cc-debug_Darwin/mozilla/dist/bin/modules/calUtils.jsm -> file:///data/dbo/moz_cc/sbird-cc-debug_Darwin/mozilla/dist/bin/calendar-js/calAlarm.js:612
                     function getItemBundleStringName does not always return a value
JS Component Loader: WARNING file:///data/dbo/moz_cc/sbird-cc-debug_Darwin/mozilla/dist/bin/modules/calUtils.jsm -> file:///data/dbo/moz_cc/sbird-cc-debug_Darwin/mozilla/dist/bin/calendar-js/calAlarm.js:667
                     function cA_toString does not always return a value
Comment 11 Daniel Boelzle [:dbo] 2009-07-04 12:26:10 PDT
Comment on attachment 384363 [details] [diff] [review]
Fix - v1

The unit tests run successfully. I haven't spent further testing on a local calendar (especially on recurrences), but the patch looks good: r1=dbo
Comment 12 Martin Schröder [:mschroeder] 2009-07-05 01:00:51 PDT
Comment on attachment 386839 [details] [diff] [review]
Fix - v2

The added unit test also passes without the patch!
Comment 13 Daniel Boelzle [:dbo] 2009-07-05 08:03:51 PDT
The patch includes all changes, not only the unit tests, so we should proceed on it.
Comment 14 Martin Schröder [:mschroeder] 2009-07-05 08:44:01 PDT
(In reply to comment #13)
> The patch includes all changes, not only the unit tests, so we should proceed
> on it.

That's okay with me! My review was just for the unit test. :)
Comment 15 Stefan Sitter 2009-07-06 10:15:20 PDT
Comment on attachment 386839 [details] [diff] [review]
Fix - v2

Upgrade from a new 0.9 profile fails with the patch applied:

**** Upgrading schema from 14 -> 15
**** Upgrading schema from 15 -> 16
**** Upgrading schema from 16 -> 17

### adding recid to cal_alarms

Error: Upgrade to v17 failed! DB Error: duplicate column name: recurrence_id ex: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.executeSimpleSQL]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///E:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calStorageCalendar.js :: addColumn :: line 967"  data: no]

An error was encountered preparing the calendar located at moz-profile-calendar:// for use. It will not be available. ...
Comment 16 Stefan Sitter 2009-07-06 11:11:58 PDT
What happens: The upgrade v15 > v16 already creates table cal_alarms in the new v17 format because it uses the sqlTables.cal_alarms object. Afterwards the upgrade v16 -> v17 tries to upgrade the table that is already v17 to v17 and fails.

In my opinion upgradeDB() must not reference the sqlTables object because its content might change. Maybe Bug 470430 is caused by the same error.
Comment 17 Philipp Kewisch [:Fallen] 2009-07-06 13:30:17 PDT
Yes, bug 470430 is definitely a bug we should look into soon, the upgrade is really broken. I guess this is the point where I need to mark bug 470430 blocking and spend some time on it :-/ Not that this is the best time for such big-time bugs, since I have 2 exams at the end of the month.

Maybe its sufficient for now to create new files sqltables.js that contains an sqlTables_vXX object for each schema version and use it for upgrading.

Stefan, any chance bug 470430 is something you could work on for 1.0?
Comment 18 Philipp Kewisch [:Fallen] 2009-07-09 11:50:01 PDT
Actually, I have some untested code for bug 470430 now. I probably won't have time to test it in the next 3 weeks though.
Comment 19 Philipp Kewisch [:Fallen] 2009-08-09 10:26:22 PDT
Created attachment 393438 [details] [diff] [review]
Fix - v3

This patch requires the patch from bug 470430 but should otherwise fix the issue.
Comment 20 Philipp Kewisch [:Fallen] 2009-09-05 04:56:23 PDT
Created attachment 398841 [details] [diff] [review]
Fix - v4

Debitrotted patch, to make review easier.
Comment 21 Philipp Kewisch [:Fallen] 2009-09-07 10:24:51 PDT
Please pretend the tests are fixed. I've merged this in locally, but the actual fixes can be seen inbetween attachment 398858 [details] [diff] [review].
Comment 22 Philipp Kewisch [:Fallen] 2009-09-13 01:18:13 PDT
With the help of Markus, I've found an issue with this patch. Migrating existing recurring events doesn't correctly copy the alarm. I'm working on it.
Comment 23 Philipp Kewisch [:Fallen] 2009-09-13 01:44:21 PDT
I guess its not that easy. Without this patch (but with bug 4704370), changed occurrences have at least two reminders (i.e the master item alarm and the alarm of the N changed occurrences, since both don't have a recurrence id). With this patch, the master item correctly has an alarm, but the occurrence doesn't.

From a database view, we will have way too many alarm rows in any case, that all look the same, depending on the number of occurrences that were changed. We don't have any sort of mapping which alarm belongs to which changed occurrence.

So how should we migrate this? There are a number of things we can do, some of them quite complicated:

-- Easy ones:

* If there are as many alarm rows as changed occurrences + the master event and they all have the same data, then we could successfully migrate by adding one alarm for each changed occurrence.

* If there is only one alarm row but N changed occurrences, then we can migrate easily by just making the master item have the alarm.

* There should be no absolute alarms, unless upgrading from a v4 profile. I've never actually been able to test this case, we can probably ignore it.

-- Problems:

* If there are as many alarm rows as changed occurrences, but we have different alarm lengths, then we have to do some guessing. We could maybe just assume the earliest alarm for each occurrence, or maybe take an average and round to the next 5 minute interval?


Is it worth the extra effort? Should we go any of the above routes, or should we just relnote this and take the patch as is?
Comment 24 Stefan Sitter 2009-09-14 02:21:53 PDT
Only users of 1.0pre are affected because this error has been introduced after the 0.9 release. Users of nightly test builds should be aware of the risk. 

Therefore I think the easiest solution would be to "reset" the alarms, relations, attachments, and what else has been duplicated in the nightly builds (see also Bug 512329). If you can't assign the information to an event drop it.

But it is important that the upgrade from 0.9 to 1.0 works correctly without losing information. If a user set a different alarm on a occurrence in 0.9 it should be contained.

Does this patch fixes the issue of lost alarms (Bug 506336) too?
Comment 25 Philipp Kewisch [:Fallen] 2009-09-14 09:26:35 PDT
(In reply to comment #24)
> Only users of 1.0pre are affected because this error has been introduced after
> the 0.9 release. Users of nightly test builds should be aware of the risk. 
The profile I was using to upgrade was created in 0.9, so I have the feeling this bug existed in 0.9 in some form too. Maybe we just didn't notice it?

Come to think of it, at least for alarms: 0.9 had the alarms stored directly in the row with the item. Maybe we should modify the original alarms upgrader (v16) to fix this bug. This way the user will have the correct alarms set. If a user upgrades from a prior 1.0pre version then the upgrader will take care in v17 instead. What do you think? This comes close to your last comment.

> Therefore I think the easiest solution would be to "reset" the alarms,
> relations, attachments, and what else has been duplicated in the nightly builds
> (see also Bug 512329). If you can't assign the information to an event drop it.

Ok, I'll get rid of the duplicate entries in the next patch iteration.

> Does this patch fixes the issue of lost alarms (Bug 506336) too?
I'll check it out, thanks.
Comment 26 Philipp Kewisch [:Fallen] 2009-10-05 05:47:29 PDT
Created attachment 404593 [details] [diff] [review]
Fix - v5

This version removes all the duplicate alarms/relations/attachments. I saw that 0.9 users are already suffering under this bug's problem for relations and attachments, so we can't really fix that. The patch removes all but one of each set of duplicate rows for alarms,relations and attachments.
Comment 27 Philipp Kewisch [:Fallen] 2009-10-05 11:18:25 PDT
Created attachment 404647 [details] [diff] [review]
Fix - v6

I believe I forgot to qref, here the final version with documentation.
Comment 28 Philipp Kewisch [:Fallen] 2009-10-31 09:40:31 PDT
*** Bug 525312 has been marked as a duplicate of this bug. ***
Comment 29 Daniel Boelzle [:dbo] 2009-11-05 02:03:46 PST
Comment on attachment 404647 [details] [diff] [review]
Fix - v6

looks good; r=dbo code-wise
Comment 30 Stefan Sitter 2009-11-05 13:28:26 PST
Comment on attachment 404647 [details] [diff] [review]
Fix - v6

r=ssitter, did some basic testing and nothing crashed
Comment 31 Philipp Kewisch [:Fallen] 2009-11-07 11:31:01 PST
Beta here we come!

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/c60babecfea5>
and comm-1.9.1 <http://hg.mozilla.org/releases/comm-1.9.1/rev/ea88f1a6eb66>

-> FIXED
Comment 32 Stefan Sitter 2009-11-10 23:56:04 PST
*** Bug 526954 has been marked as a duplicate of this bug. ***
Comment 33 Philipp Kewisch [:Fallen] 2009-12-30 16:32:48 PST
*** Bug 458717 has been marked as a duplicate of this bug. ***
Comment 34 Philipp Kewisch [:Fallen] 2011-11-07 02:36:34 PST
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Comment 35 Philipp Kewisch [:Fallen] 2011-11-07 03:55:13 PST
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".

Note You need to log in before you can comment on or make changes to this bug.