Sven Grull Reporter
	Description • 15 years ago

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

	Sven Grull Reporter
	Comment 1 • 15 years ago

Happens with Lightning too.

	Sven Grull Reporter
	Comment 2 • 15 years ago

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.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 3 • 15 years ago

Confirmed per upcoming duplicate.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 5 • 15 years ago

This happens only on the storage provider and is also true for relations and attachments.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 6 • 15 years ago

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.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 7 • 15 years ago

Comment on attachment 384363 [details] [diff] [review]
Fix - v1

Requesting additional review for storage schema changes.

	Sven Grull Reporter
	Comment 8 • 15 years ago

(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.

	Stefan Sitter [:ssitter]
	Comment 9 • 15 years ago

(In reply to comment #8)
Yes, local calendars use the storage provider.

	Daniel Boelzle [:dbo]
	Comment 10 • 15 years ago

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

	Daniel Boelzle [:dbo]
	Comment 11 • 15 years ago

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

	Martin Schröder [:mschroeder]
	Comment 12 • 15 years ago

Comment on attachment 386839 [details] [diff] [review]
Fix - v2

The added unit test also passes without the patch!

	Daniel Boelzle [:dbo]
	Comment 13 • 15 years ago

The patch includes all changes, not only the unit tests, so we should proceed on it.

	Martin Schröder [:mschroeder]
	Comment 14 • 15 years ago

(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. :)

	Stefan Sitter [:ssitter]
	Comment 15 • 15 years ago

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. ...

	Stefan Sitter [:ssitter]
	Comment 16 • 15 years ago

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.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 17 • 15 years ago

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?

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 18 • 15 years ago

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.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 19 • 15 years ago

This patch requires the patch from bug 470430 but should otherwise fix the issue.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 20 • 15 years ago

Debitrotted patch, to make review easier.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 21 • 15 years ago

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].

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 22 • 15 years ago

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.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 23 • 15 years ago

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?

	Stefan Sitter [:ssitter]
	Comment 24 • 15 years ago

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?

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 25 • 15 years ago

(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.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 26 • 15 years ago

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.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 27 • 15 years ago

I believe I forgot to qref, here the final version with documentation.

	Daniel Boelzle [:dbo]
	Comment 29 • 15 years ago

Comment on attachment 404647 [details] [diff] [review]
Fix - v6

looks good; r=dbo code-wise

	Stefan Sitter [:ssitter]
	Comment 30 • 15 years ago

Comment on attachment 404647 [details] [diff] [review]
Fix - v6

r=ssitter, did some basic testing and nothing crashed

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 31 • 15 years ago

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

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 34 • 13 years ago

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".

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 35 • 13 years ago

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".