Closed Bug 334681 (itipSender) Opened 14 years ago Closed 13 years ago

Sending iMIP/iTIP requests and responses using email Needed

Categories

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

enhancement
Not set

Tracking

(Not tracked)

VERIFIED DUPLICATE of bug 334685
Lightning 0.5

People

(Reporter: cmtalbert, Assigned: cmtalbert)

References

Details

Attachments

(2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060418 Mozilla Sunbird/0.3a1+

A new interface is needed to allow both lightning and sunbird to send iMIP emails that encapsulate iTIP protocol items. It will be responsible for sending all emails, both for initiating an iTIP protocol sequence (i.e. sending a REQUEST or PUBLISH) and responding to iTIP protocol sequences (i.e. sending a REPLY or a COUNTER etc).
On Lightning, this interface may make use of the existing Thunderbird mail infrastructure.

On Sunbird, this gets more difficult. While we can use MAPI for finding and using the system mailer on windows, on Linux and OS X, it gets more difficult.

This is a new feature and new interface to be added to the base calendar components.

Reproducible: Always

Steps to Reproduce:
Basic Tests:
1. Call is made to interface to send an initiating iTIP protocol.
2. Call is made to send an iTIP response for a given iTIP method.


Expected Results:  
(both cases) The iMIP message is generated properly for the method in question.

Detailed to some degree at: http://wiki.mozilla.org/Calendar:ITIP_and_iMIP_Support
Blocks: 334685
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
The pieces of this bug that were needed for the basic iTIP/iMIP support in Lightning have been rolled into bug 334685. Instead, this bug should be considered an enhancement request on that functionality to allow Sunbird the ability to send iMIP messages regarding events by using whatever mailer is available on its system.
Assignee: nobody → cmtalbert
No longer blocks: 334685
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #2)
> The pieces of this bug that were needed for the basic iTIP/iMIP support in
> Lightning have been rolled into bug 334685. Instead, this bug should be
> considered an enhancement request on that functionality to allow Sunbird the
> ability to send iMIP messages regarding events by using whatever mailer is
> available on its system.
> 
Wrong again. In IRC we determined that it made more sense to have a separate bug for the emailer part of iTIP iMIP support. We will reuse this bug as the vessel for those changes.
This is just the necessary pieces of the iMIP/iTIP sender. It does not contain the processing parts of the iTIP support.  It is submitted as its own patch in order to make it easy to read.
Attachment #235048 - Flags: first-review?(dmose)
Attached patch iTIP Emailing interfaces (obsolete) — Splinter Review
Correcting a small error in the base/build/makefile.in requires list.
Attachment #235048 - Attachment is obsolete: true
Attachment #235104 - Flags: first-review?(dmose)
Attachment #235048 - Flags: first-review?(dmose)
Depends on: 334685
Attachment #235104 - Flags: second-review?(dmose)
Attachment #235104 - Flags: first-review?(thomas.benisch)
Attachment #235104 - Flags: first-review?(dmose)
Index: base/public/calIITipSender.idl
===================================================================
+[scriptable, uuid(b474995c-accb-4fd4-ae98-e3f54a570849)]
+interface calIITipSender : nsISupports

Why do the methods of calIITipSender take a calIItemBase as parameter and not
a calIITipItem? I think this would be consistent with calIITipResponder.

+    attribute AUTF8String autoResponse;
+    attribute AUTF8String method;
+    attribute PRBool userOverride;

If I get it right, a calIITipSender can send several iTip messages. Therefore I think,
that the attributes autoResponse, method and userOverride belong rather to the iTip item than
the sender.

In addition, the values for the autoResponse attribute (TRUE, FALSE, NO) are
strange (see my comments to Bug 334685).


Index: base/src/calITipSender.h
===================================================================
+#ifndef CAL_ITIPSENDER_H_
+#define CAL_ITIPSENDER_H_

CALITIPSENDER_H_

+private:
+    PRBool mIgnorePrefs;
+    nsCString mAutoResponse;
+    nsCString mMethod;
+    nsCString mItemType;

See my previous comment, I think those members belong rather to the item than
the sender.

+    //nsresult GetMyAddress(nsACString &addr);

Why is this commented out?


Index: base/src/calITipSender.cpp
===================================================================
+NS_IMETHODIMP
+calITipSender::SendITipResponse(calIItemBase *calItem)
+{
+    nsresult rv = NS_OK;    
+    nsCAutoString subject, body, attachmentName, calData;
+    nsCAutoString method, toAddress, fromAddress;    
+    SetItemType(calItem);
+    nsCOMPtr<calIEvent> compEvent;
+    nsCOMPtr<calITodo> compTodo;
+    if (mItemType.Equals(VEVENT_LITERAL)) {
+        calItem->QueryInterface(NS_GET_IID(calIEvent), getter_AddRefs(compEvent));

Please use the NS_ENSURE_SUCCESS macro for error checking.

+        compEvent->GetIcalString(calData);
+    }
+    else if (mItemType.Equals(VTODO_LITERAL)) {
+        calItem->QueryInterface(NS_GET_IID(calITodo), getter_AddRefs(compTodo));

NS_ENSURE_SUCCESS missing

+nsresult calITipSender::SendEmail(MSG_ComposeType msgComposeType,
+                                     const nsACString &method,
+                                     const nsACString &subject,
+                                     const nsACString &body,
+                                     const nsACString &toAddress, // Can be a comma delimited list
+                                     const nsACString &fromAddress, // Should be from the iTIP innards we will compare to pref based Msgidentity
+                                     const nsACString &attachmentName,
+                                     const nsACString &calData)

Please remove outcommented code in this method. In addition, mark todos with XXX.

+    // First we need to glean some info from the item itself
+    nsCAutoString itemTitle;    
+    calItem->GetTitle(itemTitle);
+    nsCAutoString itemTime;
+    nsCOMPtr<calIDateTime> dateTime;
+    if (calItemType.Equals(VEVENT_LITERAL)) {
+        compevent->GetStartDate(getter_AddRefs(dateTime));
+    }
+    else if (calItemType.Equals(VTODO_LITERAL)) {
+        comptodo->GetEntryDate(getter_AddRefs(dateTime)); // TODO: Should this be entryDate or DueDate?
+    }

Is it possible to share the code for getting the date or icalString from the calItem?

+    //nsCOMPtr<nsIVariant> pMethod;
+    //rv = comp->GetProperty(NS_LITERAL_STRING("METHOD"), getter_AddRefs(pMethod));
+    //pMethod->GetAsACString(method);

remove comment

+    calIAttendee **attendeeArray; 
+    PRUint32 numAttendees=0;
+    PRBool isOrganizer = PR_FALSE;    
+    nsCAutoString currentAddress;
+
+    calItem->GetAttendees(&numAttendees, &attendeeArray);
+    // The code below assumes that organizer is in the attendee list. However, he is not.
+    // TODO: IS this a bug in calIItemBase?? 

What's the status of this task? Is there a bug existing? I think this needs to be fixed,
otherwise the code below doesn't work, or am I wrong?
Why don't you take the organizer email adress from the organizer proptery of the calItem?

+    for (PRUint32 i=0; i < numAttendees; ++i) {
+        attendeeArray[i]->GetId(currentAddress);
+        attendeeArray[i]->GetIsOrganizer(&isOrganizer); 

If your names for bool variables start with 'b', probably bIsOrganizer is a better name.
   
+        if (isOrganizer && bFromIsOrganizer) {            
+            AddToCommaList(currentAddress, fromAddress);
+        }
+        else if (isOrganizer && bIsDelegate) {
+            // When delegating we will want to send the response to the organizer as well as the person we delegate to
+            AddToCommaList(currentAddress, toAddress);            
+        }
+        else if (bFromIsOrganizer) {
+            // Then we want to add these attendees to our "to" list
+            AddToCommaList(currentAddress, toAddress);            
+        }
+        else if (bIsDelegate) {
+            // If we are delegating, we need to find the person we're delegating to (already in the list) and
+            // add them to the to list
+            // We need to check if the user has DELEGATED-FROM set on him and then add him to our to list.
+            // TODO: Is support needed for DELEGATED-FROM in calIAttendee?
+            nsCAutoString delegateAddr;
+            rv = attendeeArray[i]->GetProperty(NS_LITERAL_STRING("DELEGATED-FROM"), delegateAddr);
+            if (!delegateAddr.IsEmpty()) {
+                AddToCommaList(currentAddress, toAddress);                
+            }
+        }
+        else { // Not Organizer, not delegate, just a lowly attendee responding to organizer
+            nsCAutoString myAddr;            
+            // TODO: HACK!
+            this->GetMyAddress((nsACString&)myAddr);
+            TrimMailTo(currentAddress);
+            if (myAddr.Equals(currentAddress)) {
+                AddToCommaList(currentAddress, fromAddress);
+            }        
+        }

The algorithm for setting the fromAddress and toAddress looks rather error-prone,
but that's probably only due to my missing knowledge about all use cases.

+// Not a great method, but here goes. Two methods for finding encapsulated component:
+// 1. Play a QI guessing game (see below)
+// 2. Get the nsIComponent and use libical to determine the kind - about as much work as choice 1.
+// TODO: Which is preferred?

I would have chosen 2., but I think your approach is also fine.
Comment on attachment 235104 [details] [diff] [review]
iTIP Emailing interfaces

r1- tbe
Attachment #235104 - Flags: first-review?(thomas.benisch) → first-review-
Comment on attachment 235104 [details] [diff] [review]
iTIP Emailing interfaces

r1- tbe
Comment on attachment 235104 [details] [diff] [review]
iTIP Emailing interfaces

Drive by comment:

> RCS file: /cvsroot/mozilla/calendar/base/build/calBaseModule.cpp,v
> @@ -127,12 +139,25 @@ static const nsModuleComponentInfo compo
>        CAL_RECURRENCEDATESET_CONTRACTID,
>        calRecurrenceDateSetConstructor,
>        NULL,
>        NULL,
>        NULL,
>        NS_CI_INTERFACE_GETTER_NAME(calRecurrenceDateSet),
>        NULL,
>        &NS_CLASSINFO_NAME(calRecurrenceDateSet)
> +    },
> +#ifdef MOZ_THUNDERBIRD
> +    { "Calendar iTIP Sender",
> +      CAL_ITIPSENDER_CID,
> +      CAL_ITIPSENDER_CONTRACTID,
> +      calITipSenderConstructor,
> +      NULL,
> +      NULL,
> +      NULL,
> +      NS_CI_INTERFACE_GETTER_NAME(calITipSender),
> +      NULL,
> +      &NS_CLASSINFO_NAME(calITipSender)
>      }
> +#endif
>  };

What happens if MOZ_THUNDERBIRD is not defined e.g. during Sunbird build?
Will some compilers break on the trailing "},"? I'm not about this at the moment.

Running the patch throught http://beaufour.dk/jst-review/ showed the following error: "Q (8): You used QueryInterface--use CallQueryInterface or do_QueryInterface instead." Maybe you know better than me how to interpret this.
Attachment #235104 - Flags: second-review?(dmose)
Alias: itipSender
Status: NEW → ASSIGNED
Flags: blocking-calendar0.5+
Target Milestone: --- → Lightning 0.5
Version: unspecified → Trunk
Attachment #235104 - Attachment is obsolete: true
Whiteboard: [handled in bug 334685]
Actually, at this point I'm duping this against the "real" iTIP bug.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Whiteboard: [handled in bug 334685]
Duplicate of bug: 334685
Status: RESOLVED → VERIFIED
Flags: blocking-calendar0.5+
Component: Internal Components → E-mail based Scheduling (iTIP/iMIP)
QA Contact: base → email-scheduling
You need to log in before you can comment on or make changes to this bug.