777683 - Remember deleted items for invitations

Status: RESOLVED FIXED

(Calendar :: E-mail based Scheduling (iTIP/iMIP), enhancement)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Typical case:

* user read its emails from the latest to the oldest
* user got one 'ics email': for the creation of the evt
* user accept the evt through ICS
* organizer makes an update on the event then cancels it
* user gets two mails: one for an update and one for the cancelation of this evt
* user read the cancel email and processes it (item is deleted from calendar)
* user then read the update email

Result:
* User might be lacking coffee and also accepts the older update of the email

Expected:
* User is reminded that he has recently deleted the event

To solve this I have created a service that remembers deleted events. The timeframe to remember events is arbitrary, but I think 30 days is sensible. Original credit for this patch goes to Linagora.

To apply this patch, the patch from bug 776673 is required.

Comment 1

[Matthew Mecca [:mmecca]]

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

Build fails, it looks like test_deleted_items.js is missing from the patch.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

Here is the patch with missing file, sorry about that.

Comment 3

[Matthew Mecca [:mmecca]]

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

Review of attachment 650106 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall. r=mmecca

::: calendar/base/public/Makefile.in
@@ +28,4 @@
>              calIDateTime.idl \
>              calIDateTimeFormatter.idl \
>              calIDuration.idl \
> +			calIDeletedItems.idl \

This line has tabs instead of spaces

::: calendar/base/src/Makefile.in
@@ +53,4 @@
>      calCalendarManager.js \
>      calCachedCalendar.js \
>      calDateTimeFormatter.js \
> +	calDeletedItems.js \

Same here

::: calendar/base/src/calDeletedItems.js
@@ +24,5 @@
> +                                           Components.interfaces.calIObserver]),
> +    classInfo: XPCOMUtils.generateCI({
> +        classID: Components.ID("{8e6799af-e7e9-4e6c-9a82-a2413e86d8c3}"),
> +        contractID: "@mozilla.org/calendar/deleted-items-manager;1",
> +        classDescription: "This is the Staten Island of Calendar",

Maybe this can be more descriptive, I'm not sure everyone will understand the reference.

@@ +57,5 @@
> +            if (stmt.executeStep()) {
> +                let dt = cal.createDateTime();
> +                dt.nativeTime = stmt.row.time_deleted;
> +                return dt.getInTimezone(cal.calendarDefaultTimezone());
> +             }

Closing bracket is indented an extra space

@@ +145,5 @@
> +        if (this.stmtMarkDelete) this.stmtMarkDelete.finalize();
> +        if (this.stmtUnmarkDelete) this.stmtUnmarkDelete.finalize();
> +        if (this.stmtGet) this.stmtGet.finalize();
> +        if (this.stmtGetWithCal) this.stmtGetWithCal.finalize();
> +        if (this.stmtFlush) this.stmtFlush.finalize();

Do we want to use brackets for the one-line if statements?

@@ +155,5 @@
> +
> +    // calIObserver
> +    onStartBatch: function() {},
> +    onEndBatch: function() {},
> +    onModifyItem: function() {},

It looks like the markDeleted function supports handling the deletion of a single occurrence of a repeating item, but currently that triggers an onModifyItem instead of an onDeleteItem

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Matthew Mecca [:mmecca] from comment #3)
> ::: calendar/base/src/calDeletedItems.js
> @@ +24,5 @@
> > +                                           Components.interfaces.calIObserver]),
> > +    classInfo: XPCOMUtils.generateCI({
> > +        classID: Components.ID("{8e6799af-e7e9-4e6c-9a82-a2413e86d8c3}"),
> > +        contractID: "@mozilla.org/calendar/deleted-items-manager;1",
> > +        classDescription: "This is the Staten Island of Calendar",
> 
> Maybe this can be more descriptive, I'm not sure everyone will understand
> the reference.
Aww, well ok ;-) I've changed the description for the commit.


> 
> @@ +145,5 @@
> > +        if (this.stmtMarkDelete) this.stmtMarkDelete.finalize();
> > +        if (this.stmtUnmarkDelete) this.stmtUnmarkDelete.finalize();
> > +        if (this.stmtGet) this.stmtGet.finalize();
> > +        if (this.stmtGetWithCal) this.stmtGetWithCal.finalize();
> > +        if (this.stmtFlush) this.stmtFlush.finalize();
> 
> Do we want to use brackets for the one-line if statements?
Generally yes and even though I really like sticking to one style, I think in this case it will make the function really unreadable. Each of these lines gets bloated to three lines, which means at least 10 lines extra. If you don't mind, lets keep it this way.


> It looks like the markDeleted function supports handling the deletion of a
> single occurrence of a repeating item, but currently that triggers an
> onModifyItem instead of an onDeleteItem
I think master items are ok for now. Supporting occurrences means taking apart the item to see if/which occurrences have been deleted and this could quickly get messy. I've left support in the database so it will be easier to add this feature if its needed in the future.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 5208e9e82aa9