Closed Bug 379198 Opened 17 years ago Closed 16 years ago

Lightning doesn't send iTIP messages on event updates

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: franjb68, Assigned: cmtalbert)

References

Details

Attachments

(1 file, 4 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; Internet Explorer 6 sp1; InfoPath.1)
Build Identifier: Thunderbird 2.0.0.0 (20070326), Lightning 0.5pre (build 2007042804)

When changing some info in an event with attendees and the 'Send atendees invitation via email' checkbox checked, no notification is sent.

Reproducible: Always

Steps to Reproduce:
1. Create a new event with some atendees and check the 'Send atendees invitation via email' checkbox and save it. Thunderbird asks to send the invitation mail.
2. Modify the event date, or time, or whatever you want and accept.
Actual Results:  
The new event data are updated, but no invitation mail is sent.

Expected Results:  
New data should be updated (thats OK) and an invitation with changes should be sent.

I'm using Thunderbird 2.0.0.0 with spanish (Spain) localization.
Currently REQUESTs are only sent out if the list of attendees has changed. Changes should cause a new REQUEST being sent out (<http://rfc.net/rfc2446.html#s3.2.2.1>).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Shouldn't this be considered a blocker?

If the modification of an event isn't notified to the attendees, the invitation system becames useless, at least to me.
Component: General → Lightning Only
QA Contact: general → lightning
Flags: blocking-calendar0.7+
Status: NEW → ASSIGNED
Assignee: nobody → ctalbert
Status: ASSIGNED → NEW
subscribing to bug
I agree to franjb68... as a workaround i've implemented sending invitation into my own caldav server...
While testing the final patch for Bug 377403 it was found that sending iTIP invitations also doesn't work for occurrences of a series.

1) create a recurring event and invite people, don't send email
2) select individual occurrence and check "send email"
-> doesn't raise composer window

This is because the storage provider is always sending the written *parent* item instead of the modified occurrence in onOperationComplete().

This is so closely related to the initial problem that it's not worth while its own spin-off bug.
(In reply to comment #5)
I think we should separate that problem from this bug, and wrote bug 396182 for that. IMO we shouldn't change anything in that area for 0.7 -- high regression risk.
Simon asked me to report in here once I evaluated the changes needed to fix this.  I've looked at the changes, and they aren't that bad, but I think I'm out of time to fix this (and half sick at the moment).  I'm going to mark as a - to indicate that we can go ahead and build 0.7, if that is what is wanted.  Otherwise, I can shift gears and work on a patch for this next week since we've dropped the timezone stuff for this release.

Flags: blocking-calendar0.7+ → blocking-calendar0.7-
Hi, we are looking about a lightning calendar solution as a replacement for outlook/exchange in Fiducial (a 6000 employees french firm). Without this feature the ITip system is unusable... Currently, we develop firm specific features in an extension (to integrate to our information system). We can help you to develop this feature, but i never hacked "directly" lightning, and i would need some help...

Thanks for this good work!
It would be great to have it fixed for 0.8
Flags: wanted-calendar0.8?
Flags: wanted-calendar0.8?
Flags: wanted-calendar0.8+
Flags: blocking-calendar0.7-
Flags: wanted-calendar0.8+ → blocking-calendar0.8+
OS: Windows XP → All
Hardware: PC → All
Attached patch Patch rev1 (obsolete) — Splinter Review
Daniel, here is the first version of this patch.

We need to get the properties file changes checked in tomorrow at the very least, even if you can't complete the review tomorrow.  

There are some comments in the patch marked with "reviewer" where I had specific questions for you.  Please let comment on those.  Thanks.
Attachment #298222 - Flags: review?(daniel.boelzle)
Daniel, for the ease of getting the strings in, here is a patch containing only the string changes.  Feel free to check this in after your review so that we can make the string freeze. Obviously note that we'll have to remake the original patch if these get checked in or else the original won't apply since it also contains these changes.
Attachment #298223 - Flags: review?(daniel.boelzle)
Comment on attachment 298223 [details] [diff] [review]
[checked in] Just the string changes 

String changes checked in on HEAD and MOZILLA_1_8_BRANCH.
Attachment #298223 - Attachment description: Just the string changes → [checked in] Just the string changes
Attachment #298223 - Flags: review?(daniel.boelzle) → review+
Please check out Bug 411331 and Bug 406263 as they seem to be duplicates of
this bug.
Comment on attachment 298222 [details] [diff] [review]
Patch rev1

>+// Cache for an item so we don't search twice during updates
>+var gExistingItem = null;
This seems to disallow concurrent use of the itip processor? We need to define whether the itip processor is a service or leave it as a plain component (to be instantiated over and over again). Is caching the item strictly necessary?

>             case CAL_ITIP_PROC_UPDATE_OP:
>+                // To udpate, we must get the existing item first
>+                if (!gExistingItem)
>+                    this._isExistingItem(aCalItem, aTargetCalendar);
>+
>+                if (gExistingItem) {
>+                    // Reviewer: What is generation of an item? I find that I
>+                    // must set these equal or I get an error: calstoragecalendar::496
>+                    aCalItem.generation = gExistingItem.generation;
>+                    aTargetCalendar.modifyItem(aCalItem, gExistingItem, aListener);
>+                } else
>+                    throw new Error("_processCalendarAction: Item to update not found");
calIItemBase::generation is the internal version of the item much like the SEQUENCE number. We could avoid generation checks by tweaking it like you did above, but this will discard any changes that may have occured in the meanwhile (e.g. user added some notes to an invitation). We need to clarify this, possibly UI needed.

>+    // reviewer: how can I use this member instead of a global?
>+    //_existingItem: null,
>+    _isExistingItem: function cipIEI(aCalItem, aCalendar) {
>+        var rtnVal = false;
>+        var foundItemListener = {
>+            onOperationComplete:
>+            function (aCalendar, aStatus, aOperationType, aId, aDetail) {
>+                    // no op
>+            },
>+            onGetResult:
>+            function onget(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>+                // Now, if the old item exists, cache it and return true
>+                if (aCount && aItems[0]) {
>+                    // "this" refers to "foundItemListener" and not "calItipProcessor"
>+                    // how can I set the existingItem on calItipProcessor from here?
>+                    //this._existingItem = aItems[0];
>+                    gExistingItem = aItems[0];
>+                    rtnVal = true;
>+                } else {
>+                    // see above - this._existingItem = null;
>+                    gExistingItem = null;
>+                    rtnVal = false;
>+                }
>+            }
>+        };
>+        aCalendar.getItem(aCalItem.id, foundItemListener);
>+        return rtnVal;
>     }

This won't work for async providers, you need a callback-based solution.

>+    var calMgr = Components.classes["@mozilla.org/calendar/manager;1"]
>+                           .getService(Components.interfaces.calICalendarManager);
>+    calendarList = calMgr.getCalendars({});
use calUtils.js' getCalendarManager()

>+
>+    // Create a composite
>+    compCal = Components.classes["@mozilla.org/calendar/calendar;1?type=composite"]
>+              .createInstance(Components.interfaces.calICompositeCalendar);
>+    if (!compCal)
>+        return isUpdate;
>+
>+    for(var i=0; i < calendarList.length; ++i) {
>+        compCal.addCalendar(calendarList[i]);
>+    }
>+
>+    var onFindItemListener = {
>+        onOperationComplete:
>+        function ooc(aCalendar, aStatus, aOperationType, aId, aDetail) {
>+            // no op
>+        },
>+
>+        onGetResult:
>+        function ogr(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>+            if (aCount && aItems[0])
>+                existingItemSequence = aItems[0].getProperty("SEQUENCE");
>+        }
>+    };
>+
>+    // Per iTIP spec (new Draft 4), multiple items in an iTIP message MUST have
>+    // same ID, this simplifies our searching
>+    var itemList = gItipItem.getItemList({ });
>+    compCal.getItem(itemList[0].id, onFindItemListener);
>+
>+    var newItemSequence = itemList[0].getProperty("SEQUENCE");
>+
>+    // Three states here:
>+    // Item does not exist yet: existingItemSequence == -1
>+    // Item has been updated: newSequence > existingSequence
>+    // Item is an old message that has already been added/updated: new <= existing
>+    if (existingItemSequence == -1)
>+        isUpdate = 0;
>+    else if (newItemSequence > existingItemSequence)
>+        isUpdate = 1;
>+    else
>+        isUpdate = 2;
>+
>+return isUpdate;
>+}

same here, this won't work for async providers


r- for now
Attachment #298222 - Flags: review?(daniel.boelzle) → review-
Status: NEW → ASSIGNED
Target Milestone: --- → 0.8
(In reply to comment #15)
> (From update of attachment 298222 [details] [diff] [review])
> >+// Cache for an item so we don't search twice during updates
> >+var gExistingItem = null;
> This seems to disallow concurrent use of the itip processor? We need to define
> whether the itip processor is a service or leave it as a plain component (to be
> instantiated over and over again). Is caching the item strictly necessary?
Removed.  We'll optimize later.
> 
> >             case CAL_ITIP_PROC_UPDATE_OP:
> >+                // To udpate, we must get the existing item first
> >+                if (!gExistingItem)
> >+                    this._isExistingItem(aCalItem, aTargetCalendar);
> >+
> >+                if (gExistingItem) {
> >+                    // Reviewer: What is generation of an item? I find that I
> >+                    // must set these equal or I get an error: calstoragecalendar::496
> >+                    aCalItem.generation = gExistingItem.generation;
> >+                    aTargetCalendar.modifyItem(aCalItem, gExistingItem, aListener);
> >+                } else
> >+                    throw new Error("_processCalendarAction: Item to update not found");
> calIItemBase::generation is the internal version of the item much like the
> SEQUENCE number. We could avoid generation checks by tweaking it like you did
> above, but this will discard any changes that may have occured in the meanwhile
> (e.g. user added some notes to an invitation). We need to clarify this,
> possibly UI needed.
> 
Filed Bug 418345 for this as a follow-on and left this workaround in place for now.

> >+    // reviewer: how can I use this member instead of a global?
> >+    //_existingItem: null,
> >+    _isExistingItem: function cipIEI(aCalItem, aCalendar) {
> >+        var rtnVal = false;
> >+        var foundItemListener = {
> >+            onOperationComplete:
> >+            function (aCalendar, aStatus, aOperationType, aId, aDetail) {
> >+                    // no op
> >+            },
> >+            onGetResult:
> >+            function onget(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
> >+                // Now, if the old item exists, cache it and return true
> >+                if (aCount && aItems[0]) {
> >+                    // "this" refers to "foundItemListener" and not "calItipProcessor"
> >+                    // how can I set the existingItem on calItipProcessor from here?
> >+                    //this._existingItem = aItems[0];
> >+                    gExistingItem = aItems[0];
> >+                    rtnVal = true;
> >+                } else {
> >+                    // see above - this._existingItem = null;
> >+                    gExistingItem = null;
> >+                    rtnVal = false;
> >+                }
> >+            }
> >+        };
> >+        aCalendar.getItem(aCalItem.id, foundItemListener);
> >+        return rtnVal;
> >     }
> 
> This won't work for async providers, you need a callback-based solution.
Refactored
> 
> >+    var calMgr = Components.classes["@mozilla.org/calendar/manager;1"]
> >+                           ..snip..
> same here, this won't work for async providers
> 
Refactored
> 
Patch forthcoming...
Attachment #298222 - Attachment is obsolete: true
Attachment #298223 - Attachment is obsolete: true
Attachment #304185 - Flags: review?(Berend.Cornelius)
Comment on attachment 304185 [details] [diff] [review]
New Patch taking into account async providers

The patch works fine and looks good;r=berend
Attachment #304185 - Flags: review?(Berend.Cornelius) → review+
Patch checked in.  This patch actually fixes bug 361635, going to mark that bug as fixed.  And keep this bug open for the "send update message" patch which is forthcoming.
This patch actually fixes the "send" side of the update problem.  The previous work addressed the update detection.

This patch moves the heavy lifting required for "send" into calUtils.js so that we can access that whether we modify the event via the dialog or via the views controller.  

It currently assumes that whoever just modified the event is the organizer and creates an iTIP message for that case.  It doesn't really understand the idea of a COUNTER and won't until we code that.  This means that will probably work for 90% of the cases that people are currently complaining about, but there will still be that 10% margin.  In order to really support the counter and get around this issue, we're going to need the entire calInvitationManager infrastructure (a widening of that existing wcap interface actually) that Daniel and I discussed while he was here for calconnect.
Attachment #304690 - Flags: review?(Berend.Cornelius)
Comment on attachment 304690 [details] [diff] [review]
Patch for the original issue on this bug -- sending an update REQUEST

>+        // Now use calUtils.js to send these
>+        sendItipInvitation(aItem);
We might want to put itip stuff into an extra file, since calUtils.js is quite crowded. If you feel so inclined, you could move it out.
>+
>+            // If the item contains attendees then they need to be notified
>+            if (instance.hasProperty("X-MOZ-SEND-INVITATIONS") &&
>+               (instance.getProperty("X-MOZ-SEND-INVITATIONS") == "TRUE"))
>+               sendItipInvitation(instance);
>+
Please add brackets

>+    // XXX Until we rethink attendee support and until such support
>+    // is worked into the event dialog (which has been done in the prototype
>+    // dialog to a degree) then we are going to simply hack in some attendee
>+    // support so that we can round-trip iTIP invitations.
>+    // Since there is no way to determine the type of transport an
>+    // attendee requires, we default to email
To allow providers to override this, what do you say we add a provider property like so:

    var transportType = item.calendar.getProperty("itip.transportType") || "email";
    var emlSvc = Components.classes["@mozilla.org/calendar/itip-transport;1?type=" + transportType]
                           .createInstance(Components.interfaces.calIItipTransport);


>+    var emlSvc = Components.classes["@mozilla.org
>+    // XXX The event dialog has no means to set us as the organizer
>+    // since we defaulted to email above, we know we need to prepend
>+    // mailto when we convert it to an attendee
>+    // This also means that when we are Updating an event, we will be making
>+    // a blatant assumption that you (the updater) are the organizer of the event.
>+    // This is probably ok since we don't support the iTIP COUNTER method,
>+    // but it would be better if we didn't allow you to modify an event that you
>+    // are not the organizer of and send out invitations to it as if you were.
>+    // For this support, we'll need a real invitation manager component.
gdata automatically sets the organizer on items when adding them. Would this logic overwrite that organizer? It would probably not be too bad if so since gdata doesn't really support invitations yet, just asking :)


>+    var subject = sb.formatStringFromName("itipRequestSubject",
>+                                          [summary], 1);
>+    var body = sb.formatStringFromName("itipRequestBody",
>+                                       [emlSvc.defaultIdentity, summary],
>+                                       2);
I know you just copied this part over, but I'd like to see calGetString() here instead of the string bundle.


r=philipp with comments considered
Thanks for the patch!
Attachment #304690 - Flags: review?(Berend.Cornelius) → review+
Blocks: 419349
(In reply to comment #22)
> (From update of attachment 304690 [details] [diff] [review])
> >+        // Now use calUtils.js to send these
> >+        sendItipInvitation(aItem);
> We might want to put itip stuff into an extra file, since calUtils.js is quite
> crowded. If you feel so inclined, you could move it out.
> >+
I think I want to leave it here for now.  We are thinking of increasing the role of the "invitation manager" from a WCAP only thing to an all-calendar thing.  This interface would manage all the invitations for all the calendars, and it would be the best place to centralize such methods at that time.  So, for now, I think we leave it here.
> >+            // If the item contains attendees then they need to be notified
> >+            if (instance.hasProperty("X-MOZ-SEND-INVITATIONS") &&
> >+               (instance.getProperty("X-MOZ-SEND-INVITATIONS") == "TRUE"))
> >+               sendItipInvitation(instance);
> >+
> Please add brackets
Sure thing.

> 
> >+    // XXX Until we rethink attendee support and until such support
> >+    // is worked into the event dialog (which has been done in the prototype
> >+    // dialog to a degree) then we are going to simply hack in some attendee
> >+    // support so that we can round-trip iTIP invitations.
> >+    // Since there is no way to determine the type of transport an
> >+    // attendee requires, we default to email
> To allow providers to override this, what do you say we add a provider property
> like so:
> 
>     var transportType = item.calendar.getProperty("itip.transportType") ||
> "email";
>     var emlSvc =
> Components.classes["@mozilla.org/calendar/itip-transport;1?type=" +
> transportType]
>                           
> .createInstance(Components.interfaces.calIItipTransport);
>
I had always thought the "invitation manager" component would manage this, but I have to say your provider based solution sounds a lot cleaner.  I opened Bug 419393 for this.

> 
> >+    var emlSvc = Components.classes["@mozilla.org
> >+    // XXX The event dialog has no means to set us as the organizer
> >+    // since we defaulted to email above, we know we need to prepend
> >+    // mailto when we convert it to an attendee
> >+    // This also means that when we are Updating an event, we will be making
> >+    // a blatant assumption that you (the updater) are the organizer of the event.
> >+    // This is probably ok since we don't support the iTIP COUNTER method,
> >+    // but it would be better if we didn't allow you to modify an event that you
> >+    // are not the organizer of and send out invitations to it as if you were.
> >+    // For this support, we'll need a real invitation manager component.
> gdata automatically sets the organizer on items when adding them. Would this
> logic overwrite that organizer? It would probably not be too bad if so since
> gdata doesn't really support invitations yet, just asking :)
> 
Actually, I think the best fix here would be to fix the event dialog to add the organizer itself (and to display the event organizer).  On Gdata, that field could be disabled since the provider sets the organizer value itself.  But, yes, this logic would override that organizer.  It's the same logic that's been in the "send itip email" code since its inception, but it does start to look like the big ugly hack it is when you consider updates.

> 
> >+    var subject = sb.formatStringFromName("itipRequestSubject",
> >+                                          [summary], 1);
> >+    var body = sb.formatStringFromName("itipRequestBody",
> >+                                       [emlSvc.defaultIdentity, summary],
> >+                                       2);
> I know you just copied this part over, but I'd like to see calGetString() here
> instead of the string bundle.
> 
calGetString only queries against chrome://calendar/locale/*.properties. These strings are in chrome://lightning/locale/*.properties, so I left them as is.

Will attach new patch with changes and carry review forward.
Attachment #304185 - Attachment is obsolete: true
Attachment #304690 - Attachment is obsolete: true
Attachment #305445 - Flags: review+
Checked in on branch and trunk  --> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
This isn't working for events in CalDAV calendars, see bug 438515 
You need to log in before you can comment on or make changes to this bug.