unsubscribing from or deleting a calendar doesn't delete events

VERIFIED FIXED

Status

Calendar
Provider: Local Storage
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Assigned: Matthew (lilmatt) Willis)

Tracking

Details

(Whiteboard: [l10n impact])

Attachments

(2 attachments, 7 obsolete attachments)

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.

Updated

12 years ago
QA Contact: gurganbl → sunbird

Comment 1

12 years ago
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

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

Comment 3

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

Comment 4

12 years ago
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

Updated

12 years ago
Summary: unsubscribing from a calendar doesn't delete events → unsubscribing from or deleting a calendar doesn't delete events

Comment 5

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

Comment 6

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

Comment 7

12 years ago
This can be worked around by creating a new empty calendar before deleting the existing one.
Whiteboard: [cal relnote]

Comment 8

11 years ago
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

11 years ago
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

11 years ago
Created attachment 225635 [details] [diff] [review]
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.
Assignee: mostafah → jminta
Status: NEW → ASSIGNED

Comment 11

11 years ago
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
Flags: blocking0.3?

Updated

11 years ago
Flags: blocking0.3? → blocking0.3+

Updated

11 years ago
Assignee: jminta → nobody
Status: ASSIGNED → NEW

Comment 12

11 years ago
Created attachment 233129 [details] [diff] [review]
delete using calICalendarProvider

Patch introduces calICalendarProvider for both storage and memory.  We then use their deleteCalendar methods to clear out the old items.
Assignee: nobody → jminta
Attachment #225635 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #233129 - Flags: second-review?(dmose)
Attachment #233129 - Flags: first-review?(mattwillis)
(Assignee)

Comment 13

11 years ago
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
Attachment #233129 - Flags: first-review?(mattwillis) → first-review-
(Assignee)

Comment 14

11 years ago
Created attachment 233132 [details]
Screenshot of issue

Comment 15

11 years ago
(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
> 

Whiteboard: [cal relnote] → [cal relnote] [cal-ui-review needed]

Comment 16

11 years ago
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

11 years ago
Created attachment 233246 [details] [diff] [review]
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
Attachment #233129 - Attachment is obsolete: true
Attachment #233246 - Flags: second-review?(dmose)
Attachment #233246 - Flags: first-review?(mattwillis)
Attachment #233129 - Flags: second-review?(dmose)
(Assignee)

Comment 18

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

r=lilmatt
Attachment #233246 - Flags: first-review?(mattwillis) → first-review+

Updated

11 years ago
Whiteboard: [cal relnote] [cal-ui-review needed] → [cal relnote]

Updated

11 years ago
Whiteboard: [cal relnote] → [cal relnote][patch in hand]

Comment 19

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

Updated

11 years ago
Component: Sunbird Only → Provider: Local Storage

Comment 20

11 years ago
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

11 years ago
Created attachment 234804 [details] [diff] [review]
also purge other tables

Patch updated to reflet previous comment.  We now also purge data from all the other tables in mDBTwo.
Attachment #233246 - Attachment is obsolete: true
Attachment #234804 - Flags: second-review?(dmose)
Attachment #234804 - Flags: first-review+
Attachment #233246 - Flags: second-review?(dmose)
(Reporter)

Comment 22

11 years ago
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.
Attachment #234804 - Flags: second-review?(dmose) → second-review-
(Assignee)

Comment 23

11 years ago
Created attachment 236174 [details] [diff] [review]
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.
Attachment #234804 - Attachment is obsolete: true
Attachment #236174 - Flags: second-review?(mvl)
Attachment #236174 - Flags: first-review+
(Assignee)

Comment 24

11 years ago
Created attachment 236262 [details] [diff] [review]
rev1 - moves sql with its friends per mvl
Attachment #236174 - Attachment is obsolete: true
Attachment #236262 - Flags: second-review?(mvl)
Attachment #236262 - Flags: first-review+
Attachment #236174 - Flags: second-review?(mvl)
(Assignee)

Updated

11 years ago
Whiteboard: [cal relnote][patch in hand] → [cal relnote][patch in hand][l10n impact]
(Reporter)

Comment 25

11 years ago
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...
(Reporter)

Comment 26

11 years ago
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...
(Assignee)

Comment 27

11 years ago
Created attachment 236647 [details] [diff] [review]
rev2 - adds missing queries

Adds the missing delete events/todos queries.
Assignee: jminta → mattwillis
Attachment #236262 - Attachment is obsolete: true
Attachment #236647 - Flags: second-review?(mvl)
Attachment #236647 - Flags: first-review+
Attachment #236262 - Flags: second-review?(mvl)
(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.
(Reporter)

Comment 29

11 years ago
If implementing calICalendarProvider isn't needed, can we change this patch to only add a deleteAll to calICalendar?
Comment on attachment 236647 [details] [diff] [review]
rev2 - adds missing queries

Taking the review, at mvl's request.
Attachment #236647 - Flags: second-review?(mvl) → second-review?(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.
Attachment #236647 - Flags: second-review?(dmose) → second-review-

Updated

11 years ago
Whiteboard: [cal relnote][patch in hand][l10n impact] → [cal relnote][needs updated patch][l10n impact]
(Assignee)

Comment 32

11 years ago
Created attachment 236900 [details] [diff] [review]
rev3 - throws NS_ERROR_NOT_IMPLEMENTED now
Attachment #236647 - Attachment is obsolete: true
Attachment #236900 - Flags: second-review?(dmose)
Attachment #236900 - Flags: first-review+
(Assignee)

Comment 33

11 years ago
Does this really still need [cal relnote] once it's fixed?
Whiteboard: [cal relnote][needs updated patch][l10n impact] → [cal relnote][needs review dmose][l10n impact]
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.
Attachment #236900 - Flags: second-review?(dmose) → second-review+
(Assignee)

Comment 35

11 years ago
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [cal relnote][needs review dmose][l10n impact] → [l10n impact]

Comment 36

11 years ago
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!
(Assignee)

Comment 37

11 years ago
(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

Status: RESOLVED → VERIFIED

Updated

11 years ago
QA Contact: sunbird → storage-provider
You need to log in before you can comment on or make changes to this bug.