The default bug view has changed. See this FAQ.

Multiple reminders,relations,attachments created by modifying repeating event

RESOLVED FIXED in 1.0b1

Status

Calendar
Alarms
--
major
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Sven Grull, Assigned: Fallen)

Tracking

unspecified
1.0b1
Dependency tree / graph
Bug Flags:
blocking-calendar1.0 +
in-testsuite +

Details

(Whiteboard: [needed beta][no l10n impact])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

8 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
(Reporter)

Comment 1

8 years ago
Happens with Lightning too.
Version: unspecified → Trunk
(Reporter)

Comment 2

8 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.
(Assignee)

Comment 3

8 years ago
Confirmed per upcoming duplicate.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-calendar1.0+
(Assignee)

Updated

8 years ago
Duplicate of this bug: 496528
(Assignee)

Updated

8 years ago
Whiteboard: [needed beta][no l10n impact]
(Assignee)

Updated

8 years ago
Assignee: nobody → philipp
(Assignee)

Comment 5

8 years ago
This happens only on the storage provider and is also true for relations and attachments.
Status: NEW → ASSIGNED
Summary: Multiple reminders created by modifying repeating event → Multiple reminders,relations,attachments created by modifying repeating event
(Assignee)

Comment 6

8 years ago
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.
Attachment #384363 - Flags: review?(dbo.moz)
(Assignee)

Updated

8 years ago
Attachment #384363 - Flags: review?(ssitter)
(Assignee)

Comment 7

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

Requesting additional review for storage schema changes.
(Assignee)

Updated

8 years ago
Flags: in-testsuite?
OS: Windows XP → All
Hardware: x86 → All
Version: Trunk → unspecified
(Reporter)

Comment 8

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

Comment 9

8 years ago
(In reply to comment #8)
Yes, local calendars use the storage provider.
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 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
Attachment #384363 - Flags: review?(dbo.moz) → review+
Comment on attachment 386839 [details] [diff] [review]
Fix - v2

The added unit test also passes without the patch!
Attachment #386839 - Flags: review-

Updated

8 years ago
Attachment #386839 - Flags: review?(ssitter)

Updated

8 years ago
Attachment #384363 - Attachment is obsolete: true
Attachment #384363 - Flags: review?(ssitter)
The patch includes all changes, not only the unit tests, so we should proceed on it.
(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

8 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. ...
Attachment #386839 - Flags: review?(ssitter) → review-

Comment 16

8 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.
(Assignee)

Comment 17

8 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?
(Assignee)

Comment 18

8 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.
(Assignee)

Comment 19

8 years ago
Created attachment 393438 [details] [diff] [review]
Fix - v3

This patch requires the patch from bug 470430 but should otherwise fix the issue.
Attachment #386839 - Attachment is obsolete: true
Attachment #393438 - Flags: review?(mschroeder)
(Assignee)

Updated

8 years ago
Attachment #393438 - Flags: review?(ssitter)
(Assignee)

Updated

8 years ago
Whiteboard: [needed beta][no l10n impact] → [needed beta][no l10n impact][needs review]

Updated

8 years ago
Depends on: 470430
(Assignee)

Comment 20

8 years ago
Created attachment 398841 [details] [diff] [review]
Fix - v4

Debitrotted patch, to make review easier.
Attachment #393438 - Attachment is obsolete: true
Attachment #398841 - Flags: review?(ssitter)
Attachment #393438 - Flags: review?(ssitter)
Attachment #393438 - Flags: review?(mschroeder)
(Assignee)

Updated

8 years ago
Attachment #398841 - Flags: review?(mschroeder)
(Assignee)

Updated

8 years ago
Attachment #398841 - Flags: review?(dbo.moz)
(Assignee)

Comment 21

8 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].
(Assignee)

Comment 22

8 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.
(Assignee)

Comment 23

8 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?

Comment 24

8 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?
(Assignee)

Comment 25

8 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.
Attachment #398841 - Flags: review?(mschroeder)
(Assignee)

Comment 26

8 years ago
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.
Attachment #398841 - Attachment is obsolete: true
Attachment #404593 - Flags: review?(ssitter)
Attachment #398841 - Flags: review?(ssitter)
Attachment #398841 - Flags: review?(dbo.moz)
(Assignee)

Updated

8 years ago
Attachment #404593 - Flags: review?(dbo.moz)
(Assignee)

Comment 27

8 years ago
Created attachment 404647 [details] [diff] [review]
Fix - v6

I believe I forgot to qref, here the final version with documentation.
Attachment #404593 - Attachment is obsolete: true
Attachment #404647 - Flags: review?(ssitter)
Attachment #404593 - Flags: review?(ssitter)
Attachment #404593 - Flags: review?(dbo.moz)
(Assignee)

Updated

8 years ago
Attachment #404647 - Flags: review?(dbo.moz)
(Assignee)

Updated

8 years ago
Duplicate of this bug: 525312
Comment on attachment 404647 [details] [diff] [review]
Fix - v6

looks good; r=dbo code-wise
Attachment #404647 - Flags: review?(dbo.moz) → review+

Comment 30

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

r=ssitter, did some basic testing and nothing crashed
Attachment #404647 - Flags: review?(ssitter) → review+
(Assignee)

Comment 31

8 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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needed beta][no l10n impact][needs review] → [needed beta][no l10n impact]
Target Milestone: --- → 1.0

Updated

8 years ago
Duplicate of this bug: 526954
(Assignee)

Updated

7 years ago
Duplicate of this bug: 458717

Updated

7 years ago
Blocks: 524940
(Assignee)

Comment 34

5 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".
Target Milestone: 1.0 → 1.0b1
(Assignee)

Comment 35

5 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".
You need to log in before you can comment on or make changes to this bug.