361635 - Need to support incoming updates to existing iTIP/iMIP invitations

Status: RESOLVED DUPLICATE of bug 421886

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

Description

[Florian Schnabel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061121 Minefield/3.0a1
Build Identifier: version 3 alpha 1 (20061122) + lightning 4a1 (2006112205)

when an event is re-sceduled and you get an updated invitation clicking on "add to calendar" doesn't do a thing.

Reproducible: Always

Steps to Reproduce:
1. get an outlook meeting invitation
2. add to calendar via button
3. get an outlook meeting invitation update
4. click the add to calendar button
5. nothing happens at all



Expected Results:  
delete outdated calendar entry,
create new entry
(alternatively update it)

if you manually delete the first entry in calendar you can add the update as expected

Comment 1

[Martin Schröder [:mschroeder]]

Clint, you know best about ITIP and iMIP support.

Comment 2

[Matthew (lilmatt) Willis]

This is CONFIRMED.  We don't handle an update to an existing event at the moment.

Comment 4

[Josh D S Davis]

To clarify, when an update is accepted, *something* does happen.

[Error getting calendar                                     X]
/!\ Invitation could not be processed.  Status: 2152333316
    Details: ID already exists for addItem
                    [ OK ]

Since it can identify that it's a duplicate item, it should replace the duplicate, probably first verifying that the update is newer than the existing entry.

Comment 5

[Günter]

The RFC has a clear definition for this situation:
If some specific parameter of an event are changed (eg. Date, time) the SEQUENCE has to be incremented and a new DTSTAMP has to be generated. 

If such a "changed" EVENT is added to the calendar stack the application (SUNBIRD) should handle it, also the UID is the same. Is SUNBIRD doing this?

Comment 7

[Daniel Boelzle [:dbo]]

Only release drives should set blocking+.

Comment 8

[Martin Schröder [:mschroeder]]

We decided on the QA Chat that this doesn't block 0.7 release. The feature is
on the roadmap for the next release.

Comment 9

[Omar Bajraszewski]

It would be great to have it fixed for 0.8

Comment 12

[Gerald]

Invitation could not be processed.  Status: 2152333316  Details: ID already exists for addItem.

Got this error trying to process a new event when I included myself in the invitations list.  Same issue, but different perspective so I thought that I'd make note of this so that this is handled in both directions.

Comment 15

[Balazs Lengyel]

VERY IMPORTANT feature/bug. 

Please consider all kind of changes: date, place, text in the invitation etc.

Comment 17

[saurabh]

pls. consider all cahnges made to the application.

Comment 18

[Bernhard Straub]

Still not resolved in build 2008020819

Comment 19

[cmtalbert]

This patch got reviewed and checked in on bug 379198.  I'm going to mark this bug as fixed.  Sorry for the confusion.

If something is wrong with detecting the updates after this patch is built, reopen this bug. 

Comment 20

[klint]

Hi
I have just checked build 20080221 (Windows) with 2 invitations coming from Outlook (same ID, one being the update of the other), and 2 different events are still created. The first one is not replaced. But I do not have any "duplicate" warning message either... 

Comment 21

[Gerald]

I can confirm the behavior.  No error now, but it does not "update", it just ignores the ID and keeps adding events instead of updating the original (or removing the original).  

Can someone with "The Power" change the status?

Comment 22

[cmtalbert]

I'm not seeing this behavior.  I'm testing with Outlook 2007 and I detect that the update from Outlook is an update.  When applying the update, the update applies to the original event on the calendar, updating it.  What version(s) of Outlook were you guys running?

Do you have errors in your Error Console window?

Comment 23

[klint]

Attachment: Initial invitation from Outlook 2003

I have no error in the error console :(
My version of Outlook is 2003.

Comment 24

[klint]

Attachment: Updated invitation from Outlook 2003

And here is an update for the same event! As described, a double entry is being created in the calendar (remote Google calendar).

Comment 25

[Günter]

Think this is an Outlook problem:
Both ICS file contain "SEQUENCE:0".
The rfc defines the sequence # has to be increase with an update! So here the update version has to have a sequence # > '0'.

The Microsoft special: They have their own "Sequence":
X-MICROSOFT-CDO-APPT-SEQUENCE:0  with the first ICS file
X-MICROSOFT-CDO-APPT-SEQUENCE:1  with the updated version!!

Isn't as usual ??

Good luck!

Comment 26

[Günter]

Form a "compatibility" aspect this bug should be reworked to handle the special Microsoft Outlook behavior, also from RFC compatibility the bug seems to be fixed!?

Günter 

Comment 27

[cmtalbert]

This is what I expected.  I thought that the problem might be that Outlook isn't handling the SEQUENCE ID properly.  They are in Outlook 2007, but they fixed a lot of RFC incompatibilities with Outlook 2007.

Comment 28

[Bas van den Bosch]

Though outlook2003 seems to handle this incorrectly, would it be difficult to scan for a X-MICROSOFT-CDO-APPT-SEQUENCE: so we can process these correctly? The UID is the same so you're going to check fot sequence-numbers:
+    var itemList = gItipItem.getItemList({ });
+    compCal.getItem(itemList[0].id, onFindItemListener);
+
+    var newItemSequence = itemList[0].getProperty("SEQUENCE");

If this doesn't exist, search for the x-prop?

Comment 29

[Bas van den Bosch]

Also, for bug 378198, if the x-prop exists, write both sequence and the X-MICROSOFT-CDO-APPT-SEQUENCE: ?

BTW, I wonder wether the importing of the item with the increased MS-Sequence does generate a new UID or does it re-use the existing ID (This should fire a warning and not give duplicate events afaik)

Comment 30

[Günter]

If LG does a pure check based on RFC the updated MS OL invitation is the "same" because of UID and SEQUENCE. But the DTSTART is different! From this LG should handle this as an error or .. at least an updated event.

Doing this, the MS specific items need not to be processed. But ...
.. for compatibility with OL2003 it has to be checked if those X-MICROSOFT-CDO- items need to be included in a reply (ACCEPT, DECLINED etc). Could be MS fails for this if they are NOT send back.
Günter

Comment 31

[Günter]

Attachment: ex: processing of OL-based invitation w ReminderFox

Comment 32

[Günter]

Just to 'complete' this:
With ReminderFox 
we just ignore those items with:
   if ( extraInfo.indexOf( "X-MICROSOFT-CDO-" ) == -1 )  { ... 
but we process for the DTSTART and SEQUENCE.
So we get one first entry and one updated, see attachment with comment #31.
Günter

Comment 33

[Gerald]

I'm seeing the behavior reported in Thunderbird with Lightning.  I haven't tested it with Outlook.  If someone updates a meeting that was already accepted by me, using Outlook, My TB/Lightning just creates a new meeting with the updated proposal instead of updating the existing meeting.

Comment 34

[cmtalbert]

Attachment: [checked in] Fixes the outlook issue.

This works around the Outlook issue, and it also fixes an issue that occurs when accepting an event that contains an alarm. (which all outlook appointments do by default)

Comment 35

[cmtalbert]

Comment on attachment 306790 [details] [diff] [review]
[checked in] Fixes the outlook issue.

Requesting review from Sebo specifically, but if Philipp or Martin or Berend happend to find time, please review.

Comment 36

[Günter]

With the patch (#34) there is a check for :
+    // Make sure we don't have a pre Outlook 2007 appointment, but if we do
+    // use Microsoft's Sequence number. I <3 MS
+    if ((newSequence == "0") &&

Are you really be sure OL ALWAYS has only Sequence:0 ??

The example with #24 is with that, but how do you know it's in all cases?
I would just check for new==old. If it's true, check for DTSTART, that has to be newer, or the event is not valid! Additionally the X-MICROSOFT-CDO could be used.
But I would prefer only use SEQUENCE and DTSTART. (See RFC defs)
Günter

Comment 37

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

Comment on attachment 306790 [details] [diff] [review]
[checked in] Fixes the outlook issue.

>? calendar/base/src/~:code:ltn:mozilla:bug419349.diff
>Index: calendar/base/src/calItipProcessor.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/src/calItipProcessor.js,v
>retrieving revision 1.1.2.6
>diff -u -8 -p -r1.1.2.6 calItipProcessor.js
>--- calendar/base/src/calItipProcessor.js	25 Feb 2008 19:53:06 -0000	1.1.2.6
>+++ calendar/base/src/calItipProcessor.js	1 Mar 2008 22:27:38 -0000
>@@ -360,16 +360,20 @@ calItipProcessor.prototype = {
> 
>             case CAL_ITIP_PROC_UPDATE_OP:
>                 // To udpate, we must require the existing item to be set
>                 if (!aExistingItem)
>                     throw new Error("_processCalendarAction: Item to update not found");
> 
>                 // TODO: Handle generation properly - Bug 418345 
>                 aCalItem.generation = aExistingItem.generation;
>+                // We also have to ensure that the calendar is set properly on
>+                // the new item, or items with alarms will throw during the
>+                // notification process
>+                aCalItem.calendar = aExistingItem.calendar;
>                 aTargetCalendar.modifyItem(aCalItem, aExistingItem, aListener);
>                 return true;
> 
>             case CAL_ITIP_PROC_DELETE_OP:
>                 throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> 
>             default:
>                 throw new Error("_processCalendarAction: " +
>Index: calendar/lightning/content/imip-bar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/lightning/content/imip-bar.js,v
>retrieving revision 1.1.2.11
>diff -u -8 -p -r1.1.2.11 imip-bar.js
>--- calendar/lightning/content/imip-bar.js	20 Feb 2008 18:16:54 -0000	1.1.2.11
>+++ calendar/lightning/content/imip-bar.js	1 Mar 2008 22:27:40 -0000
>@@ -406,32 +406,46 @@ function isUpdateMsg()
>         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 newSequence = itemList[0].getProperty("SEQUENCE");
> 
>+    // Make sure we don't have a pre Outlook 2007 appointment, but if we do
>+    // use Microsoft's Sequence number. I <3 MS
>+    if ((newSequence == "0") &&
>+       itemList[0].hasProperty("X-MICROSOFT-CDO-APPT-SEQUENCE")) {
>+        newSequence = itemList[0].getProperty("X-MICROSOFT-CDO-APPT-SEQUENCE");
>+    }
>+
>     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;
>                 determineUpdateType(newSequence, -1);
>             }
>         },
> 
>         onGetResult:
>         function ogr(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>             if (aCount && aItems[0] && !this.processedId) {
>                 this.processedId = true;
>-                determineUpdateType(newSequence, aItems[0].getProperty("SEQUENCE"));
>+                var existingSequence = aItems[0].getProperty("SEQUENCE");
>+
>+                // Handle the microsoftism foolishness
>+                if ((existingSequence == "0") &&
>+                   itemList[0].hasProperty("X-MICROSOFT-CDO-APPT-SEQUENCE")) {
>+                    existingSequence = aItems[0].getProperty("X-MICROSOFT-CDO-APPT-SEQUENCE");
>+                }
>+                determineUpdateType(newSequence, existingSequence);
>             }
>         }
>     };
>     // Search
>     compCal.getItem(itemList[0].id, onFindItemListener);
> }
> 
> /**

Comment 38

[cmtalbert]

(In reply to comment #37)
Siwy4444 left the chat room. (Connection reset by peer)
[4:13pm] Fallen: Oops I accidentally posted the patch as a comment
[4:14pm] Fallen: that should read "Looks good, r=philipp" 
[4:14pm] ctalbert: hahaha

Comment 39

[cmtalbert]

Checked in on trunk and Branch --> Fixed (again) :)

Comment 40

[Günter]

@Clint
How about my comment #36??

Are sure with SEQUENCE:0 for ALL OL2003 events?? Do you know for sure they will not op with other numbers, but don't change with an updated event?

Comment 41

[Bas van den Bosch]

If there's a sequence-number >0 and an x-sequence the sequence-number is leading, not the x-sequence. Only if sequence=0 and there is a x-sequence, use this x-sequence. The mixing of x-sequence and sequence should be kept to rfc as much as possible. I think this is the right way to go for now.

Comment 42

[Günter]

Sorry, I think I have mixed up a little bit during following this bug.

With reply #5 I mentioned the 'DTSTAMP' item which should be used here.
With further replies I spoke about 'DTSTART' .. this has to be 'DTSTAMP'.

The problem with updated events coming in with no incremented 'SEQUENCE' (as we saw it with MS OL2003) there is a clear strategy described with:

http://tools.ietf.org/html/rfc2446

   To maximize interoperability and to handle messages that arrive in an
   unexpected order, use the following rules:

   4.  In situations where the "UID" and "SEQUENCE" properties match,
       the "DTSTAMP" property is used as the tie-breaker. The component
       with the latest "DTSTAMP" overrides all others. Similarly, for
       "Attendee" responses where the "UID" property values match and
       the "SEQUENCE" property values match, the response with the
       latest "DTSTAMP" overrides all others.

So following this RFC recommendation the current solution is a specific MS OL one, but doesn't solve the general issue .... as other (at the moment not known apps) would cause the same unresolved updated event.
So, please have a re-check.
Günter

Comment 43

[Eric Valette]

While I understand being RFC compliant is intellectually the way to go, I think that if you want to get any decent market share  or pretend to be able to replace outlook on windows anytile soon you have to be able to live with M$ non compliance to RFC.

So whatever you decide, be pragmatic and make sure it is included in 0.8. I currently use TB as a mailer client for exchange but still have to use exchange web mail (or KDE korganiser + relevant plugin) to be able to manage my appointments correctly because of this bug.

0.7 is currently unusable in compan hat do frequent meeting change/additions.

Comment 44

[Günter]

Eric,
totally agree !!!

Following the RFC with UID, SEQUENCE and DSTAMP as described in the above mentioned manner will do the job for MS OL2003 also. With the MS OL2003 generated/attached ICS data records both relevant items (UID, SEQUENCE) have the same value. Per RFC the SEQUENCE should have been incremented (as MS does with their properitary item), but the DSTAMP is 'newer' ... and that's the 'tiebreaker' cases. 
AFAIK the current LG implementation doesn't follow that tiebreaker ... that's all about!

Comment 45

[klint]

Hi
I just have tested with my OL 2003 and the latest LG nightly and I can confirm that it still does not work with the 2 samples in attachment! Sorry :)

Comment 46

[Daniel Boelzle [:dbo]]

per comment #45, IMO we need to further investigate into this issue, especially since SEQUENCE is derived from item.generation, and the fix for bug 418854 will allow generation downgrades (decreases). SEQUENCE must always increase, I think we need to decouple SEQUENCE from generation.

Comment 47

[Günter]

@ #46:
Also I didn't had a look into the code, for me it's not clear how SEQUENCE and item.generation releate to each other. How to understand "since SEQUENCE is derived from item.generation"?
From the RFC definition a schedule coming in (like the posted examples coming from OL) has to have a SEQUENCE and a DTSTAMP. Beside UID they define the sequencing of the new event. And also the RFC defines a very clear handling with the "tiebreaker" (see posting #42). Handling this will **ALSO** satisfy the proprietary OL nomenclature.

Comment 48

[Maxime Delorme]

lightning : 2008031621
I've got write access on 2 calendars (one is my calendar, the other a meeting room)
I created an event on the meeting room one and put me as an attendee
I received the invitation
But I can not create the event on my calendar because lightning offers only 2 buttons "update" or "dismiss"
I would have prefer "accept" or "dismiss"
 * I choose the target calendar
 * If it's a calendar without the event, Lightning creates the event
 * else Lightning updates it and eventually warns me that's an update

Comment 49

[Maxime Delorme]

to create the event I drag the attachment on the calendar mode switcher button

Comment 50

[Simon Paquet [:sipaq]]

Daniel, what is the status of this bug with Clint being away?

Comment 51

[Daniel Boelzle [:dbo]]

We are running out of time and the remaining issues won't hold off 0.8 => not blocking 0.8. Changes in this area are likely to cause further regressions.

Comment 52

[Eric Valette]

I'm desperate to see that lead developers do not care about usability.It is clear from comments that this bug is one of the most annoying problem for most people wanting to use lightning in a corporate environment and really replacing outlook. It has been open for 5 month and instead on concentrating to this bug, people spend time to eyes candy feature!

You guys should better assign priorities! Look at subscribed people and votes!

Comment 53

[Günter]

@ comments #51/#52

I fully second Eric's comment!
Because I also see this bug fixure as an important one, I tried to have a deeper lock in to the code without luck because my Debugger installation don't love the code (either). 
Are my comments about SEQUENCE eg. #42 true?? So a correction for that specific situation with SEQUENCE not in the proper sequencing would be a general problem.
As with the RFC paper mentioned the DTSTAMP has to be used as the "tiebreaker". But from the code I don't see any DTSTAMP reference for **incoming** messages. 
So a correction would be somewhat massiv. Is it realistic with the code freeze to solve it for 0.8??

And once more the words written by Eric:
You guys should better assign priorities!

Comment 54

[Daniel Boelzle [:dbo]]

Guys, I am open to discuss roadmap/planning/priorities either by private e-mail, in the newsgroup or by voice on the status call, but please don't bloat this bug with non-technical comments, because it leaves it hardly readable. I know it's more than unfortunate that this bug fix doesn't work for you, but we need to proceed with 0.8 (which is *not* the last calendar release). Please keep in mind that lots of users are already waiting for 0.8-final with its fixes and new features.

Comment 55

[Eric Valette]

Look at the title of the bug: you have used the bug system as a way to track feature planned. This is fine, I do it all the time. But then, we have a mean to voice on feature which is great.

95% of the PC are running windows. Most corporate environment use outlook on windows (means outlook 2003 for medium sized companies). So having this feature not compatible with the most used event generator, is a bad decision.

And personally 0.7 with this bug fix including outlook 2003 compatibility would be more useful for me than 0.8 without this bug. I just cannot use lightning to manage my meetings as long as this feature is not fixed.

I will shut up now because I understand the only solution I have is to fix it myself if I need it anytime soon. blocking 0.8+ means nothing because the same can happen again in any further release. At least record a single patch that can be pushed via distros as anyway using the default xpi does not work on any modern linux distro.

Comment 56

[Hubert FONGARNAND]

(In reply to comment #52)
I totally agree to this comment...
If ITIP/Imip implementation would have been at the level of Evolution. Lightning would be the default calendar client on our firm...

But now, we would wait... 0.9; new thing on 0.8 are not useful for us.

Comment 57

[Günter]

Sorry Daniel,
"but please don't bloat this bug with non-technical comments," doesN#t fit here.
I have never seen any TECHNICAL comment about the cause of the bug which i mentioned with my comments at least starting with #42.

Also like Eric said: I will shut up now because ... but that will not be the "tiebreaker" ;)

Comment 59

[Nick Harness]

Sorry for this, no technical comment either. Am removing myself, with difficulty, from the MS environment. As I still receive events/invites (as well as updated invites which I can't process)from MS users as well as the previous bug, I can not open any .ics files if they are forwarded as opposed to a straight invite. something else to look at?

Comment 60

[Andy Cowling]

(I am not sure if this comment goes here, but I am wary about creating new bugs which are closely related to existing bugs. If its in the wrong place, I will happily move it!)

In the latest nightly build of Lightning I cannot open "METHOD: CANCEL" emails from Domino users (where the Domino user created the original event). The error console gives the following error:

Error: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [calIRecurrenceItem.icalProperty]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///C:/Documents%20and%20Settings/Andy/Application%20Data/Thunderbird/Profiles/hcqdihxx.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calItemBase.js :: anonymous :: line 604"  data: no]
Source File: file:///C:/Documents%20and%20Settings/Andy/Application%20Data/Thunderbird/Profiles/hcqdihxx.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calItemBase.js
Line: 604

Below I will attach the full ics file..

Comment 61

[Andy Cowling]

Attachment: Invitation cancel from Domino

Comment 62

[Andy Cowling]

(Note the same error occurs in 0.8rc2)

Comment 63

[Andy Cowling]

Hmmm... Maybe bug 416583 is more appropriate for this?

Comment 64

[Hubert FONGARNAND]

Attachment: Implementation of REPLY method

This patch add a way to answer as "Tentative" too...

Comment 65

[Hubert FONGARNAND]

Attachment: new reply patch

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

Comment 66

[Günter]

@  #65 
Is this a new interpretation of the RFC ???

From my understanding the RFC clearly defines about a valid event ... see #42.

You have to use SEQUENCE, UID. And if those fail: DTSTAMP as the tiebreaker.
How does that match with  " ... i use lastmodified ..." ??

Comment 67

[Hubert FONGARNAND]

My patch only concern REPLY method... 
http://tools.ietf.org/html/rfc2446#section-3.2.3

SEQUENCE cannot be used... because you can have many REPLY for one item...

http://tools.ietf.org/html/rfc2446#section-3.2.3

in fact i use DTSTAMP from the ItipItem and LASTMODIFIED from the calendar item 

Comment 68

[Hubert FONGARNAND]

DTSTAMP is the date of the creation of the item... according to the RFC...

Comment 69

[Günter]

Don't mix the different time values!
From one of the posted invitations I'll copy these time values as an example:

LAST-MODIFIED:20080226T113117
CREATED:20080222T103021Z
DTSTAMP:20080222T103014Z

The LAST-MODIFIED is an 'local' element the user updates each time he changes something with his event, eg. change notes or alarms. The attendee can change some items, not all. Some are only allowed to be changed by the organizer. If he fe. changes LOCATION, a new SEQUENCE has to be generated, automatically the DTSTAMP will be changed ALSO ... AND a message/invitation change HAS to be send to all attendees.

The ATTENDEE(s) will receive this with a new SEQUENCE and new DTSTAMP.
And here is the point: Now you have to check if the incoming UID/SEQUENCE is valid. If this is not valid (see rfc) you have the tiebraker situation, the check of DTSTAMPnew against DTSTAMPold !!! NOT against the LAST-MODIFIED currently stored with the event in the ATTENDEEs store. Because that could have been changed ... see above.

So take DTSTAMP and LAST-MODIFIED separated in the user/attendess ICS data store!

Comment 70

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

Comment on attachment 316432 [details] [diff] [review]
new reply patch

Note this is only a code-level review, not setting +/- due to the fact that I haven't read the RFC to an extent where I understand the REPLY action. Clint should better be able to comment on that. Assigning review to Clint to take care of the RFC related review.



>     } 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.
Go ahead and get rid of the comment, now that you've implemented it.

>+    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]);
>+    }
For bonus points, create a helper that does this.

>+    var itemList = gItipItem.getItemList({ });
>+    var itipItemDate = itemList[0].stampTime;
>+    // If Itim DTSTAMP is over now...
Fix comment typo

>+    var nowDate = jsDateToDateTime(new Date());
>+    debugger;
Remove debugger statement, here and elsewhere.

>+    if (itipItemDate.nativeTime > nowDate.nativeTime)
You can probably use itipItemDate.compare(nowDate) > 0 ?

>+            if (!this.processedId){
>+                // Then the ID doesn't exist, don't call us twice
>+                this.processedId = true;
I know you just copied this part, but I haven't understood in which cases it would be called twice within the same context. Add a space between ) and {.

>+        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];
I'd prefer not to use a global calItemFound, but to pass it around.

>+               displayReply(itipItemDate, existingItemDate);
>+               //updateItemFromReply(aItems[0]);
Any reason you commented out the updateItemFromReply function? If so, please just remove it.



>+    if (itipItemDate.nativeTime > existingItemDate.nativeTime)
Use itipItem.compare()

>+    if (calItemFound!=null)
>+    {
>+        var itemArray = gItipItem.getItemList({ });
>+        if (itemArray.length>0)
>+        {
>+            var itipItem=itemArray[0];
>+            if (itipItem.id==calItemFound.id)
>+            {
...

You have a lot of indentation here. I'd prefer to turn around the expressions and return from the function in case things are not as expected. This saves a lot of indentation level and makes the code more readable. Also, normal calendar style wants { in the line with the if(). Even though the rest of the file might not be consistant, please start here. The same goes for spaces around operators.

Comment 71

[Hubert FONGARNAND]

Attachment: reply patch v1.1

Updated patch with formatting fixes + rely on DTSTAMP (and not on LAST-MODIFIED) of both items to check if the itip imip item has been processed before

Comment 72

[Hubert FONGARNAND]

Attachment: reply patch v1.2

with this new patch, lightning now display if the event has been added or updated in the Imip Description bar.

It contains a fix for the bug https://bugzilla.mozilla.org/show_bug.cgi?id=430280
(with caldav component)

(i'm working on caldav calendars)

Comment 73

[Hubert FONGARNAND]

Attachment: reply/cancel patch v1.3

I also have implemented handling of CANCEL request in the patch

Comment 74

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

Comment on attachment 317272 [details] [diff] [review]
reply/cancel patch v1.3

Again, only code level review of what changed between my last review and this one. Changes look good, only some style fixes are needed.

>+    button.setAttribute("oncommand", 
>+                                "deleteItemFromCancel()");
Alignment here and in other places, I'd prefer:
      button.setAttribute("oncommand", 
                          "deleteItemFromCancel()");

>+function displayReply(itipItemDate, existingItemDate)
>+{
Function braces on function line, here and in other places

>+    if (gCalItemFound!=null)
>+    {
Braces on if() line here and in other places (same with for each())

>+                        targetCalendar.modifyItem(cloneCalItemFound,gCalItemFound,operationListener);
space after comma

>Index: providers/caldav/calDavCalendar.js
I'd prefer caldav changes in a different bug


I'm looking forward to this landing! :-)

Comment 75

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

Comment on attachment 317272 [details] [diff] [review]
reply/cancel patch v1.3

requesting review from Clint for RFC-level review.

Comment 76

[Hubert FONGARNAND]

Attachment: reply/cancel patch v1.4

reply/cancel patch v1.4 with formatting suggestions applied, the caldav part is now in bug https://bugzilla.mozilla.org/show_bug.cgi?id=430280

Comment 77

[Hubert FONGARNAND]

it should be better to continue the discussion about my patch on bug : #357180

Comment 78

[cmtalbert]

(In reply to comment #75)
> (From update of attachment 317272 [details] [diff] [review])
> requesting review from Clint for RFC-level review.
> 

From an RFC level, this looks ok.  One thing that we're not doing is returning error codes when we hit an unknown state.  I.e. if we get a REPLY to a message that we don't have we are technically supposed to return an iTIP Message with an error code to the sender of the REPLY.  But, I don't know of *any* implementation that does that.  Besides that is something we can work on once we have the basic iITP functionality working.

Comment 79

[Martin Schröder [:mschroeder]]

Hubert, is the issue now handled in bug 421886 and this bug a duplicate? If yes, this bug should be assigned to ctalbert and resolved FIXED with target milestone 0.8, because there has been a checkin in the 0.8 timeframe.