421886 - Itip/Imip doesn't handle REPLY and CANCEL methods

Status: RESOLVED FIXED

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

Description

[Hubert FONGARNAND]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008030904 Minefield/3.0b5pre
Build Identifier: 

When someone reply to an invitation the reply is seen by TB but it doesn't update the according event

Reproducible: Always

Steps to Reproduce:
1. Send an invitation to an oulook
2. Decline the invitation (on outlook)
3. Open the ITIP answer in TB
4. The corresponding event is not updated
Actual Results:  
The corresponding event is not updated

Expected Results:  
The corresponding event should be updated

Comment 1

[Stefan Sitter [:ssitter]]

What Lightning version do you use?
In Lightning 0.8 Release Candidate Bug 361635 and Bug 379198 ought to be fixed.

Comment 2

[Hubert FONGARNAND]

I'm using Lightning 0.8 RC1 with a caldav calendar...

I doesn't speak about event update... but about replys (when somebody accept or decline an event)

When i received a Itip REPLY it doesn't show an "update event"

Comment 3

[Stefan Sitter [:ssitter]]

Based on the summary it's a duplicate of Bug 394424 that is a duplicate of the already mentioned Bug 361635.

But from the STR this might be Bug 416583 (CANCEL method).

Please check the error console for calendar related messages.

Comment 4

[Hubert FONGARNAND]

Attachment: Implementation of REPLY method

It add a way to answer as "Tentative" too...

Comment 5

[Hubert FONGARNAND]

Attachment: New patch

Now, i use lastmodified instead of sequence to know if the item must be updated

Comment 6

[Martin Schröder [:mschroeder]]

Comment on attachment 316431 [details] [diff] [review]
New patch

Parts of this patch have already been incorporated in the patch from bug 361635. Please try to keep the patches separate.

Comment 7

[cmtalbert]

Comment on attachment 316431 [details] [diff] [review]
New patch

Thanks for the patch.  It looks pretty good, but I do have a couple of recommendations.

Index: lightning/content/imip-bar.js
===================================================================
RCS file: /cvsroot/mozilla/calendar/lightning/content/imip-bar.js,v
retrieving revision 1.1.2.14
diff -u -8 -p -r1.1.2.14 imip-bar.js
--- lightning/content/imip-bar.js	4 Mar 2008 23:23:20 -0000	1.1.2.14
+++ lightning/content/imip-bar.js	18 Apr 2008 14:03:34 -0000
@@ -37,16 +37,17 @@
  * ***** END LICENSE BLOCK ***** */
 
 /**
  * This bar lives inside the message window.
  * Its lifetime is the lifetime of the main thunderbird message window.
  */
 
 var gItipItem;
+var calItemFound;
We usually prefix global variables with a 'g' so that we know at a glance that they are global.  Please change this to gCalItemFound.
 
 /**
@@ -163,20 +165,18 @@ function setupBar(imipMethod)
     var description = document.getElementById("imip-description");
 
     if (imipMethod.toUpperCase() == "REQUEST") {
         // Check if this is an update and display things accordingly
         isUpdateMsg();
     } else if (imipMethod.toUpperCase() == "REPLY") {
         // Bug xxxx we currently cannot process REPLY messages so just let
         // the user know what this is, and don't give them any options.
-        if (description.firstChild.data) {
-            description.firstChild.data = ltnGetString("lightning",
-                                                       "imipBarReplyText");
-        }
+        processReplyMsg();
Cool, since we're going to handle this now, we can get rid of the comment above this stating that we don't handle it. :-)
+       
Please remove the spaces from the above blank line.

@@ -192,16 +192,143 @@ function setupBar(imipMethod)
         if (description.firstChild.data) {
             description.firstChild.data = ltnGetString("lightning",
                                                        "imipBarUnsupportedText");
         }
         Components.utils.reportError("Unknown imipMethod: " + imipMethod);
     }
 }
 
+function processReplyMsg()
+{
+    var description = document.getElementById("imip-description");
+     if (description.firstChild.data) {
+            description.firstChild.data = ltnGetString("lightning",
+                                                       "imipBarReplyText");
+    }
+    calendarList = getCalendarManager().getCalendars({});
+
+    // Create a composite
+    compCal = Components.classes["@mozilla.org/calendar/calendar;1?type=composite"]
+              .createInstance(Components.interfaces.calICompositeCalendar);
+
+    for(var i=0; i < calendarList.length; ++i){
+        compCal.addCalendar(calendarList[i]);
+    }
+ 
+    // Per iTIP spec (new Draft 4), multiple items in an iTIP message MUST have
+    // same ID, this simplifies our searching, we can just look for Item[0].id
+    var itemList = gItipItem.getItemList({ });
+    var itipItemDate = itemList[0].stampTime;
+    // If Itim DTSTAMP is over now...
// Typo should be: If Itip DTSTAMP...

+    var nowDate = jsDateToDateTime(new Date());
+    debugger;
What does this do?  I've never seen it used before.  I am going to assume that it either causes a breakpoint or an assertion  for you to attach the JS debugger.
Please remove it from your patch before asking for your next review.  We don't want to instantiate someone's debugger when they are trying to work on a different problem.

+    if (itipItemDate.nativeTime > nowDate.nativeTime)
+        itipItemDate =  nowDate;
+    
Remove spaces from all blank lines (blank lines are ok, per mozilla style, but not blank lines of spaces).  Text editors always eat your lunch on this style point.

+    var onFindItemListener = {
+        processedId: null,
+        onOperationComplete:
+        function ooc(aCalendar, aStatus, aOperationType, aId, aDetail) {
+            if (!this.processedId){
+                // Then the ID doesn't exist, don't call us twice
+                this.processedId = true;
+
+            }
+        },
+
+        onGetResult:
+        function ogr(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
+            if (aCount && aItems[0] && !this.processedId) {
+                this.processedId = true;
+                var existingItemDate = aItems[0].lastModifiedTime;
+               // Item found...
+               calItemFound=aItems[0];
+               displayReply(itipItemDate, existingItemDate);
+               //updateItemFromReply(aItems[0]);
Please remove references to non-existant code before submitting for review.

+            }
+        }
+               
+        
Doh! Two more blank lines of spaces
+    };
+    
+    
More spaces, and we probably only need one blank line here, and not two.

+    //Search for item
+    compCal.getItem(itemList[0].id, onFindItemListener);
+}
+
+function displayReply(itipItemDate, existingItemDate)
+{
+    //Verify that the item has ever been updated...
+    if (itipItemDate.nativeTime > existingItemDate.nativeTime)
+    {
It's Mozilla style to do if's this way:
if (blah) {
    dosomething();
} else {
    dosomethingelse();
}
Please make sure you follow that throughout.

+   debugger;
Another of those debugger things to remove.

+        var button = document.getElementById("imip-button1");
+        button.removeAttribute("hidden");
+        button.setAttribute("label", ltnGetString("lightning",
+                                                      "imipUpdateInvitation.label"));
+        button.setAttribute("oncommand", 
+                                "updateItemStatusFromReply()");
+    }
+}
+
+function updateItemStatusFromReply()
+{
+    debugger;
debugger again.

+    
spaces

+    var operationListener = {
+        onOperationComplete:
+        function ooc(aCalendar, aStatus, aOperationType, aId, aDetail) {
+            // Call finishItipAction to set the status of the operation
+            finishItipAction(aOperationType, aStatus, aDetail);
+        },
+
Nice work, you removed the spaces on that blank line :-)

+        onGetResult:
+        function ogr(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
+            // no-op
+        }
+    };
+    
+    if (calItemFound!=null)
+    {
See above note about if formatting

+        var itemArray = gItipItem.getItemList({ });
+        if (itemArray.length>0)
+        {
+            var itipItem=itemArray[0];
+            if (itipItem.id==calItemFound.id)
+            {
+                var itipAttendeesArray=itipItem.getAttendees({});
+                if (itipAttendeesArray.length>0)
+                {
+                    for each (var itipAttendee in itipAttendeesArray)
+                    {
+                        // We clone the cal item found in order to modify it...
+                        var cloneCalItemFound=calItemFound.clone();
This is not a nit.  Cloning is expensive.  I don't think you need to clone the calendarItem for each attendee in the itipAttendeeArray.  Please move this outside the for each loop.

+                        var localAttendee=cloneCalItemFound.getAttendeeById(itipAttendee.id);
+                        if (localAttendee!=null)
+                        {
+                            var localAttendeeClone=localAttendee.clone();
+                            localAttendeeClone.participationStatus=itipAttendee.participationStatus;
+                            cloneCalItemFound.removeAttendee(localAttendee);
+                            cloneCalItemFound.addAttendee(localAttendeeClone);
+                            
+                            // Modify the original ITEM
+                            var targetCalendar=calItemFound.calendar;
+                            cloneCalItemFound.calendar=targetCalendar;
+                            targetCalendar.modifyItem(cloneCalItemFound,calItemFound,operationListener);
+                        }
+                    }
+                }
+                
+            }
+        }
+    }
+    return true;
+}

@@ -533,16 +664,23 @@ function displayRequestMethod(updateValu
 
             // Create a DECLINE button (user chooses not to attend the updated event)
             button = document.getElementById("imip-button2");
             button.removeAttribute("hidden");
             button.setAttribute("label", ltnGetString("lightning",
                                                       "imipDeclineInvitation.label"));
             button.setAttribute("oncommand",
                                 "setAttendeeResponse('DECLINED', 'CONFIRMED');");
+            // Create a ACCEPT TENTATIVE button
+            button = document.getElementById("imip-button3");
+            button.removeAttribute("hidden");
+            button.setAttribute("label", ltnGetString("lightning",
+                                                  "imipAcceptTentativeInvitation.label"));
+            button.setAttribute("oncommand",
+                            "setAttendeeResponse('TENTATIVE', 'CONFIRMED');");
If you do this, you'll end up with a imip bar that looks like this:
[ ] Would you like to accept this event? | Accept | Decline | Tentative |
I think we'd want the order to be:
[ ] Would you like to accept this event? | Accept | Tentative | Decline |
Check with Christian about that first (you'll need to get UI review anyway).  But in order to do that in code, you need to use imipButton2 for Tentative and imipButton3 for Decline.

@@ -554,10 +692,17 @@ function displayRequestMethod(updateValu
 
         // Create a DECLINE button
         button = document.getElementById("imip-button2");
         button.removeAttribute("hidden");
         button.setAttribute("label", ltnGetString("lightning",
                                                   "imipDeclineInvitation.label"));
         button.setAttribute("oncommand",
                             "setAttendeeResponse('DECLINED', 'CONFIRMED');");
+        // Create a ACCEPT TENTATIVE button
+        button = document.getElementById("imip-button3");
+        button.removeAttribute("hidden");
+        button.setAttribute("label", ltnGetString("lightning",
+                                                  "imipAcceptTentativeInvitation.label"));
+        button.setAttribute("oncommand",
+                            "setAttendeeResponse('TENTATIVE', 'CONFIRMED');");
See above comments about UI


One thing that was missing is resetting your global gCalFoundItem variable.  This needs to be cleared after an item has been processed.  I think it would make sense to do it here: http://mxr.mozilla.org/mozilla1.8/source/calendar/lightning/content/imip-bar.js#123

The patch itself is good.  I agree with the approach.  There are some issues I have w.r.t. Mozilla style and with a couple of code related things - UI, cloning, resetting the global variable, so I'll have to r- this version.

I look forward to version 2 of the patch.  Thanks for your help.

Comment 8

[cmtalbert]

Oh yeah, and one other comment.  When working on iTIP, it's always good to test with an asynchronous calendar system.  So test it with the storage calendar and with CalDav or WCAP.  This way you can be certain that your callback functions are working as you intend them too.

Since we have a patch, and this is really a problem, I'm also going to confirm this bug. :-)

Comment 9

[Hubert FONGARNAND]

I'm doing all my dev on an caldav server

Comment 10

[Hubert FONGARNAND]

Attachment: reply/cancel patch v1.5

per review of dbo

Comment 12

[cmtalbert]

(In reply to comment #9)
> I'm doing all my dev on an caldav server
> 
Awesome! That's the way to do it.

Comment 13

[cmtalbert]

(In reply to comment #10)
> Created an attachment (id=318158) [details]
> reply/cancel patch v1.5
> 
> per review of dbo
> 
I'm confused.  Did this patch already get reviewed by dbo, or are you requesting review by dbo?  If you're requesting review, then on the "Attachment details" page, set the "review" checkbox to ? and add "dbo" to the textbox.  

Comment 14

[Daniel Boelzle [:dbo]]

I think because I did some drive-by comments on the cancel/reply patch some days ago; I'm a bit confused, too.

Comment 16

[Daniel Boelzle [:dbo]]

Comment on attachment 318158 [details] [diff] [review]
reply/cancel patch v1.5

>+    var onFindItemListener = {
>+        processedId: null,
>+        onOperationComplete:
>+        function ooc(aCalendar, aStatus, aOperationType, aId, aDetail) {
>+            if (this.processedId == true) {
>+                displayCancel();
>+            }
>+        },
>+
>+        onGetResult:
>+        function ogr(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>+            if (aCount && aItems[0]) {
>+               this.processedId = true;
>+               // We have found an item
>+               if (aCalendar.readOnly == false)
>+                    gCalItemsArrayFound.push(aItems[0]);
>+             }
>+        }
>+    }
You should rather call displayCancel if any item has been collected.
Since operations on readOnly calendars make no sense at all, we should stuff up the composite only with eread-write calendars upfront.

>+    var onFindItemListener = {
>+        itipItemDate: null,
unused, not needed, because in scope

>+        processedId: null,
>+        onOperationComplete:
>+        function ooc(aCalendar, aStatus, aOperationType, aId, aDetail) {
>+            if (this.processedId == true && gCalItemsArrayFound.length > 0) {
>+                displayReply();
>+            }
>+        },
>+
>+        onGetResult:
>+        function ogr(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>+            if (aCount && aItems[0]) {
>+               this.processedId = true;
>+               // We have found an item
>+               if (aCalendar.readOnly == false)
>+               {
>+                    var itemFound = aItems[0];
>+                    if (itipItemDate.compare(itemFound.stampTime) > 0)
>+                        gCalItemsArrayFound.push(aItems[0]);
>+               }
>+             }
>+        }       
same applies to this listener: could be simplified

>+function deleteItemFromCancel() {
>+    var operationListener = {
>+        onOperationComplete:
>+        function ooc(aCalendar, aStatus, aOperationType, aId, aDetail) {
>+            // Call finishItipAction to set the status of the operation
>+            finishItipAction(aOperationType, aStatus, aDetail);
>+            gCalItemFound = null;
gCalItemFound is undefined

>+    var itemArray = gItipItem.getItemList({ });
>+    var itipItem = itemArray[0];
>+    
>+    for each (var calItemFound in gCalItemsArrayFound)
>+    {
>+        var targetCalendar = gCalItemFound.calendar;
>+        // We assume that this cancel request will delete the whole event
>+        targetCalendar.deleteItem(gCalItemFound, operationListener);
>+    }
It's not clear to me how this code could work since gCalItemFound is undefined.

>+function updateItemStatusFromReply() {
...
>+    var itemArray = gItipItem.getItemList({ });
>+    var itipItem = itemArray[0];
>+    for each (var calItemFound in gCalItemsArrayFound) {
>+        var itipAttendeesArray = itipItem.getAttendees({});
>+        if (itipAttendeesArray.length > 0) {
>+            for each (var itipAttendee in itipAttendeesArray) {
>+                // We clone the cal item found in order to modify it...
>+                var cloneCalItemFound = calItemFound.clone();
>+                var localAttendee = cloneCalItemFound.getAttendeeById(itipAttendee.id);
>+                if (localAttendee != null) {
>+                    var localAttendeeClone=localAttendee.clone();
>+                    localAttendeeClone.participationStatus = itipAttendee.participationStatus;
>+                    cloneCalItemFound.removeAttendee(localAttendee);
>+                    cloneCalItemFound.addAttendee(localAttendeeClone);
>+                    // Modify the original ITEM
>+                    var targetCalendar = calItemFound.calendar;
>+                    cloneCalItemFound.calendar = targetCalendar;
>+                    targetCalendar.modifyItem(cloneCalItemFound,
>+                                              calItemFound, operationListener);
>+                }
You are modifying the item over and over again for every attendee.

> imipAddToCalendar.label=Add To Calendar
> imipAddedItemToCal=Event Added to Calendar
>+imipCanceledItem=Event has been deleted
>+imipUpdatedItem=Event has been updated
>+imipBarCancelText=This message contains a canceling to an event.
> imipBarRequestText=This message contains an invitation to an event.
> imipBarUpdateText=This message contains an update to an existing event.
> imipBarAlreadyAddedText=This message contains an event that has already been added to your calendar.
> imipBarReplyText=This message contains a reply to an invitation.
> imipBarUnsupportedText=This message contains an event that this version of Lightning cannot process.
> imipAcceptInvitation.label=Accept
>+imipCancelInvitation.label=Delete
> imipDeclineInvitation.label=Decline
> imipUpdateInvitation.label=Update
>+imipAcceptTentativeInvitation.label=Tentative
seems to be already in cvs.


Moreover the patch has lots of style nits w.r.t. braces and unaligned intendation.

r-, but it goes the right way... thanks!

Comment 17

[Daniel Boelzle [:dbo]]

Attachment: reply/cancel patch v1.6

Refurbished Hubert's patch and fixing the issues.

Comment 18

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

Comment on attachment 320168 [details] [diff] [review]
reply/cancel patch v1.6

>     document.getElementById("imip-button1").setAttribute("hidden", "true");
>     document.getElementById("imip-button2").setAttribute("hidden", "true");
>+    document.getElementById("imip-button3").setAttribute("hidden", "true");
hideElement("imip-button1");
hideElement("imip-button2");
hideElement("imip-button3");
(could be fixed in multiple locations, also when passing the XULElement is better)

>         if (description.firstChild.data) {
>-            description.firstChild.data = ltnGetString("lightning",
>-                                                       "imipBarRequestText");
>+            description.firstChild.data = ltnGetString("lightning", "imipBarRequestText");
I realize this was that way before, but I wonder why we use firstChild.data ? If description is a <description> element, then description.textContent should work equally well and I believe its more common. Feel free to change if you want. Could be fixed in multiple locations.

>+            if (gCalItemsArrayFound.length > 0) {
&gt; 0 is superfluous

>+function displayCancel() {
>+function displayReply() {
...
>+    var button = document.getElementById("imip-button1");
>+    button.removeAttribute("hidden");
showElement(button)
(could be fixed in multiple locations, also when passing the string id is better)

>     var msgURI = GetLoadedMessage();
>     var msgHdr = messenger.messageServiceFromURI(msgURI)
>                           .messageURIToMsgHdr(msgURI);
messenger.msgHdrFromURI(GetLoadedMessage());


only style nits, r=philipp

Comment 19

[Daniel Boelzle [:dbo]]

I did the proposed changes.

Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Philipp: It's not quite clear to me why showElement/hideElement are available; they actually are, but I don't see how calendar-ui-utils.js comes into play. Could you enlighten me?

Comment 20

[Eric Valette]

Just a side comments, the patch 1.6 dropped a part of the part of the patch that is necessary for having the Tentative button indeed being labeled tentative (in lightning.properties).

Does not help for people trying to fix the most annoying bug themselves on linux where nightly build are unusable...

Comment 21

[Martin Schröder [:mschroeder]]

(In reply to comment #20)
> Just a side comments, the patch 1.6 dropped a part of the part of the patch
> that is necessary for having the Tentative button indeed being labeled
> tentative (in lightning.properties).

Correct, but those strings have already been checked in with the patch from bug 429938.

Comment 22

[Daniel Boelzle [:dbo]]

(In reply to comment #20)
As Martin mentioned, that fragment had already been checked in before, thus it could be cleaned out from the patch.

> Does not help for people trying to fix the most annoying bug themselves on
> linux where nightly build are unusable...
Is this passage in any way relevant to this bug? If not, please stop this.

Comment 23

[Eric Valette]

Well if you has supplied official/clean fixes for 0.8, even if not producing a 0.8.1 I I would not waste time doing this myself and would surely be more happy.

What is the use of recording in bugs patches then? I have to browse the changelog for files to find missing bits. What is the usefulness of having not complete patches or patches that depends on other patches but not recorded?

Comment 24

[Daniel Boelzle [:dbo]]

Patches have to be produced against the *current* cvs tree; it's unfortunate that the referred bits have been errornously committed as part of another patch, but 
that's life.

Comment 25

[Eric Valette]

I agree about the "current" tree notion but I disagree when is see bug 429938 contains things about the "tentative" third button that has nothing to do with the fix. Reviewer should have noticed it!