296202 - unsubscribing from or deleting a calendar doesn't delete events

Status: VERIFIED FIXED

(Calendar :: Provider: Local Storage, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

When unsubscribing from a calendar, the events (or the ics file) are not
deleted. That shouldn't happen always, but only for private files. The problem
is how to figure out when to actually delete things.

Comment 1

[Stefan Sitter [:ssitter]]

Is this bug supposed to handle the following error? Or should I fill a new bug?

Steps to Reproduce:
1. Create local calendar with name xxx
2. Create some events/tasks in calendar xxx
3. Delete local calendar xxx
4. Create new local calendar with name yyy

Actual Results:
The new calendar yyy contains all events from deleted calendar xxx.

Expected Results:
The new calendar yyy is empty.

Additional Info:
Both (deleted xxx and new yyy) calendar use the same location, moz-profile-
calendar://?id=2 in my case.

Comment 2

[Simon Paquet [:sipaq]]

*** Bug 315716 has been marked as a duplicate of this bug. ***

Comment 3

[Stefan Sitter [:ssitter]]

*** Bug 316747 has been marked as a duplicate of this bug. ***

Comment 4

[mperanelli]

I have the same problem, what I do is delete the events.  I uncheck all the calendars except for the one I want to delete.  I select show all events and control click all events.  Then I right click and delete selected.  I delete calendar, and when I recreate under same profile moz-profile-calendar://?id=3, All the events are gone.  I kind of like having this bug.  No accidentally deleting a calendar.  Kind of like a built in safe guard.  A better solution might be to be able to have an option select all by right click on the events list.  The one now only selects all events in the month you are viewing.  You could then easily delete all the events for the calendar.  and if you accidentally delete a calendar from the calendars tab, your profile is still backed up. 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051104 Mozilla Sunbird/0.3a1

Comment 5

[Simon Paquet [:sipaq]]

*** Bug 325965 has been marked as a duplicate of this bug. ***

Comment 6

[Stefan Sitter [:ssitter]]

*** Bug 326362 has been marked as a duplicate of this bug. ***

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

This can be worked around by creating a new empty calendar before deleting the existing one.

Comment 8

[snotling]

Shouldn't unregisterCalendar() and deleteCalendar() have a different responsability and effect ?

For example, in case of a "storage" calendar, unregistering it would remove it from the list of subscribed calendars (i.e. hiding it in the UI), but keeping its properties and related data "as is" on disk.

Whereas calling deleteCalendar() would, in addition to unregistering, would effectively delete all the calendar data, including its events and tasks.

What do you think ?

Comment 9

[Omar Bajraszewski]

Lightning and Sunbird have the same bug:
1) Create local calendar with name x
2) Create some events/tasks in calendar x
3) Delete local calendar with name x
4) Re- create local calendar with name x
5) The events/tasks created in 2) appear

I suggest this feature:
When you delete local calendar a window appears: "All events/tasks in the calendar will be deleted" Proceed; cancel

Comment 10

[Joey Minta]

Attachment: purgeFile implementation

This patch will purse the storage database, if the user chooses to do so.  The current problem I see is that the prompt semantics aren't very clear at this stage.  If someone wanted to spend some time thinking about the best way to present the user with the various questions involved, that'd be greatly appreciated.  Note that bug 248309 already exists for actually deleting remote calendars from a server.  It should be obvious where a fix for that bug ought to plug into this patch.

Comment 11

[Sebastian Schwieger]

If the user deletes a local calendar and later creates another one (can be a different name!) the "old" events show up again.
I think this belongs to category "Events/tasks should not appear in views where they are not intended to appear" requesting blocking0.3

Comment 12

[Joey Minta]

Attachment: delete using calICalendarProvider

Patch introduces calICalendarProvider for both storage and memory.  We then use their deleteCalendar methods to clear out the old items.

Comment 13

[Matthew (lilmatt) Willis]

Comment on attachment 233129 [details] [diff] [review]
delete using calICalendarProvider

Something's goofy with this patch. 

In Sunbird, when I want to delete a calendar, I right-click the calendar, and select "Delete Calendar". I get a confirmation "Are you sure you want to unsubscribe from %s". Upon clicking "OK", the contextual menu reappears, and a tooltip also appears beneath my mouse (which was above the "OK" button of the previous dialog). 

They both go away upon clicking "OK" to "Do you want to delete the file" 

I'll attach a screenshot momentarily. 

Minusing on this

Comment 14

[Matthew (lilmatt) Willis]

Attachment: Screenshot of issue

Comment 15

[Joey Minta]

(In reply to comment #13)
> (From update of attachment 233129 [details] [diff] [review] [edit])
> Something's goofy with this patch. 
> 
> In Sunbird, when I want to delete a calendar, I right-click the calendar, and
> select "Delete Calendar". I get a confirmation "Are you sure you want to
> unsubscribe from %s". Upon clicking "OK", the contextual menu reappears, and a
> tooltip also appears beneath my mouse (which was above the "OK" button of the
> previous dialog). 
The tooltip issue is a known bug.  The context menu is a bit interesting, but again I blame layout and can't reproduce on my box.  We probably need to get some UI feedback about the 2 prompts, since they're not my ideal solution.

> 
> They both go away upon clicking "OK" to "Do you want to delete the file" 
> 
> I'll attach a screenshot momentarily. 
> 
> Minusing on this
> 

Comment 16

[Sebastian Schwieger]

I have added my thoughts regarding Calendar Deletion UI in http://wiki.mozilla.org/index.php?title=Calendar:Calendar_Managment_UI#Calendar_Deletion_Wizard

Comment 17

[Joey Minta]

Attachment: delete using calICalendarProvider v2

The big point I took away from Sebastian's wiki page is that it doesn't make sense to ask to both 'unsubscribe' and 'delete' a local calendar.  The user just wants to get rid of it (delete).  This patch refactors the code accordingly, providing several advantages:
-we're not obligated to get remote-calendar deleting for 0.3
-no UI changes
-no 2nd prompt
-no fighting with weird trunk-gecko-stuff

Comment 18

[Matthew (lilmatt) Willis]

Comment on attachment 233246 [details] [diff] [review]
delete using calICalendarProvider v2

r=lilmatt

Comment 19

[Sebastian Schwieger]

As a comment: this bug is not 'Sunbird only'. I'm just not sure what component it should be changed to...

Comment 20

[Ariel Poliak]

I found a problem with the latest suggested patch: while it clears events and tasks (from tables "cal_events" and "cal_todo" respectively), it neglects to delete information related to those items, stored in the tables named "cal_attendees", "cal_properties", and "cal_recurrence". I would post an updated patch, but not knowing much about SQL, I have no idea how to delete rows from multiple tables given information from an external table. What I know is that from the three tables I menctioned, the common link is the column named "item_id", which is the item id given to events and tasks (column "id" in "cal_events" and "cal_todo". So what needs to be done is get all the items from "cal_events" and "cal_todo" that have the "cal_id" value set to whatever the ID is of the calendar being deleted, and from there, delete all rows on the three other tables that have the "id" field set to the id found on the first search... I don't know if that is clear, but I don't know of any other way of putting it without actually knowing SQL syntax... hope it helps...

Comment 21

[Joey Minta]

Attachment: also purge other tables

Patch updated to reflet previous comment.  We now also purge data from all the other tables in mDBTwo.

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 234804 [details] [diff] [review]
also purge other tables

>+    deleteCalendar: function stor_deleteCal(cal, listener) {
>+        var selectEvents = createStatement(this.mDB,
>+                           "SELECT id FROM cal_events WHERE cal_id = :cal_id");
>+        selectEvents.params.cal_id = cal.mCalId;
>+        while (selectEvents.step()) {
>+            deleteExtras(selectEvents.row.id);
>+        }
>+        selectEvents.reset();

These loops could (potentially) be much faster, and at least more readable, by using just one sql query:
DELETE FROM cal_recurrence WHERE item_id IN (SELECT id FROM cal_events WHERE cal_id=:cal_id);

Also, the mix of inline and global queries isn't that nice...

>Index: calendar/resources/content/calendar.js
>+        //XXX support deleting other kinds of calendars

Just make deleteCalendar for the other types a no-op, instead of building that knowledge in here.

Comment 23

[Matthew (lilmatt) Willis]

Attachment: rev0 - Attempts to address mvl's comments

Working on this in jminta's absence

> These loops could (potentially) be much faster, and at least more readable, by
> using just one sql query:
> DELETE FROM cal_recurrence WHERE item_id IN (SELECT id FROM cal_events WHERE
> cal_id=:cal_id);

True, but by not using the abstracted out queries, we don't gain the advantage of having "one place to edit db queries". That lack of repeating yourself is one of the big things you gain form frameworks like EOModeler or Hibernate. You gain flexibility and maintainability, but possibly lose some speed and ability to fine tune query performance. I guess I'm saying I preferred using the already defined queries. I also would've used the already defined queries for deleting the members of the cal_events and cal_todos tables.

Anyway, I built the loop differently, using your suggested query style, and added deleteCalendar (and the other necessary methods) to the other providers. 

dmose suggested in IRC that I not no-op in composite, but instead throw NS_ERROR_NOT_IMPLEMENTED, since deleting from composite seemed like something you don't want to do.

Comment 24

[Matthew (lilmatt) Willis]

Attachment: rev1 - moves sql with its friends per mvl

Comment 25

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 236262 [details] [diff] [review]
rev1 - moves sql with its friends per mvl

This patch would have been much easier to review if you hadn't tried to implement a complete new interface...

Comment 26

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 236262 [details] [diff] [review]
rev1 - moves sql with its friends per mvl

>Index: calendar/providers/storage/calStorageCalendar.js
>+    deleteCalendar: function stor_deleteCal(cal, listener) {
>+        for (var i in this.mDeleteEventExtras) {
>+        for (var i in this.mDeleteTodoExtras) {

You are deleting the items from the extra tables, but you missed the cal_events and cal_todo tables, so you are not removing the actual items...

Comment 27

[Matthew (lilmatt) Willis]

Attachment: rev2 - adds missing queries

Adds the missing delete events/todos queries.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #25)
> (From update of attachment 236262 [details] [diff] [review] [edit])
> This patch would have been much easier to review if you hadn't tried to
> implement a complete new interface...
> 

This is my fault; I specifically asked Joey to do it that way, though he later mostly convinced me that it wasn't really necessary.

Comment 29

[Michiel van Leeuwen (email: mvl+moz@)]

If implementing calICalendarProvider isn't needed, can we change this patch to only add a deleteAll to calICalendar?

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 236647 [details] [diff] [review]
rev2 - adds missing queries

Taking the review, at mvl's request.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 236647 [details] [diff] [review]
rev2 - adds missing queries

I've noted the need to make a decision on calICalendarProvider's separation from calICalendar in bug 297721.

Since we're still going to need almost-identical methods implemented on the same objects, I think it's reasonable to proceed with this patch as is.

I think the patch generally looks good, but there's one thing we should change.  Specifically, this patch forces our front-end confusion about delete vs. unsubscribe down into the providers, which I think is the wrong strategy.  Instead, let's isolate the nastiness in calCalendarManager, and have it special case which providers should and shouldn't call deleteCalendar.  For the providers in this patch where we're not implementing deleteCalendar (e.g. ICS and CalDAV), let's just throw NS_ERROR_NOT_IMPLEMENTED.

Comment 32

[Matthew (lilmatt) Willis]

Attachment: rev3 - throws NS_ERROR_NOT_IMPLEMENTED now

Comment 33

[Matthew (lilmatt) Willis]

Does this really still need [cal relnote] once it's fixed?

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 236900 [details] [diff] [review]
rev3 - throws NS_ERROR_NOT_IMPLEMENTED now

>+        // We only want to delete the contents of calendars from local
>+        // providers (storage and memory). Otherwise we may nuke someone's
>+        // calendar stored on a server when all they really wanted to do was
>+        // unsubscribe.

It would be good to include an XXX about how this is a temporary hack which will go away once we fix the UI (and reference the appropriate bug for that).

>+        if (calendar instanceof Components.interfaces.calICalendarProvider
>+           (calendar.type == "storage" || cal.type == "memory")) {
>+            try {
>+                calendar.deleteCalendar(calendar, null);
>+            } catch(ex) {
>+                dump("error purging calendar:"+ex+'\n');

How about using Components.utils.reportError here instead of dump?

r=dmose with those changes.

Comment 35

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 36

[Sebastian Schwieger]

VERIFIED that events of old (previously deleted) calendars don't show up upon creation of a new calendar. BUT: there is still UI confusion.
After pressing _delete_ calendar (context menu) the popup dialog asks if I really want to _unsubscribe_ from calendar. But in fact, I'm deleting!

Comment 37

[Matthew (lilmatt) Willis]

(In reply to comment #36)
> After pressing _delete_ calendar (context menu) the popup dialog asks if I
> really want to _unsubscribe_ from calendar. But in fact, I'm deleting!

That's a known, different bug.

-> VERIFIED