455132 - Pasting event to switched off calendar deletes all original events

Status: RESOLVED FIXED

(Calendar :: Provider: ICS/WebDAV, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18pre) Gecko/2008091218 Sunbird/0.9

Steps to Reproduce:
1. Start Sunbird with a clean profile
2. Create a dummy event in the default storage calendar ('Home')
3. Open a local .ics file that contains events as a second calendar
4. From properties dialog switch on Cache for the ics calendar and restart
5. From properties dialog switch off the ics calendar and restart
6. Select the ics calendar entry in the calendar list
7. Select the dummy event from the Home calendar and copy it
8. Paste it to the switched off ics calendar
9. Call menu File -> Reload Remote Calendars
10. From properties dialog switch on the ics calendar

Actual Results:
after 3: original events are displayed (as expected)
after 5: original events are not displayed (as expected)
after 8: pasted event is shown in the switched off calendar
after 9: pasted event is not displayed anymore
after 10: only the pasted event is shown but the original events disappeared!

Expected Results:
after 8: no events are displayed
after 10: original events are displayed

Comment 1

[Stefan Sitter [:ssitter]]

I think there is overlap to the workflow in my other Bug 455214. Pasting shouldn't be possible if the selected calendar is switched off or read-only independent if it's cached or not.

If the calendar is cached, read-only and switched off the following error is logged to console:

Error: uncaught exception: [Exception... "'<error>' when calling method: [calICalendar::adoptItem]"  nsresult: "0x804a0002 (<unknown>)"  location: "JS frame :: file:///E://sunbird/js/calCachedCalendar.js :: anonymous :: line 396"  data: no]

Comment 2

[Daniel Boelzle [:dbo]]

Attachment: fix - v1

This should fix the bug, including some unit tests. However, the added tests show up that getNextOccurrence has bugs (in those cases). Since this is IMO a different bug, we should not fix it here.

Comment 3

[Daniel Boelzle [:dbo]]

Comment on attachment 338717 [details] [diff] [review]
fix - v1

>-                    dates = dates.filter(function (d) {
>-                        if (aReturnRIDs) {
>-                            return d.rstart.compare(dateToRemove) != 0;
>-                        } else {
>-                            return d.compare(dateToRemove) != 0;
>-                        }
>-                    });
>+                    dates = dates.filter(function (d) { return d.id.compare(dateToRemove) != 0; });

Just for reference: The core fix of this bug is to compare the RECURRENCE-ID, instead of the occurrence date here.

Comment 4

[Daniel Boelzle [:dbo]]

Although I am pretty confident in this patch (all unit tests pass successfully, if I revert changes of bug 438964 comment #11), I am setting qawanted, because changes to calRecurrenceInfo.js are always good for regressions.

Comment 5

[Daniel Boelzle [:dbo]]

oops, worked on wrong bug :(
moving comments to bug 455133... sorry!

Comment 6

[Daniel Boelzle [:dbo]]

I think this needs a fix, because it's unrelated to experimental caching and causes dataloss. Pasting on a switched off calendar must not be allowed.

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: fix WIP

- patch fixes paste
- fixes similar problem in undo/redo scenarios (e.g. switching off calendar, then doing undo/redo affecting the switched off calendar) by peeking the unod/redo transactions

While this works fine on Lightning, it still doesn't reliably work on Sunbird :/

Comment 8

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

Attachment: WIP v2

Comment 9

[Daniel Boelzle [:dbo]]

Comment on attachment 339083 [details] [diff] [review]
WIP v2

r=dbo,philipp

Comment 10

[Daniel Boelzle [:dbo]]

Peer review per IRC.

Checked in on HEAD, MOZILLA_1_8_BRANCH and SUNBIRD_0_9_BRANCH => FIXED.

Cross your fingers that this change doesn't cause regressions.