Last Comment Bug 1156015 - Email scheduling fails for recipients with URN id
: Email scheduling fails for recipients with URN id
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: E-mail based Scheduling (iTIP/iMIP) (show other bugs)
: Trunk
: All All
-- normal (vote)
: 4.2
Assigned To: [:MakeMyDay]
:
:
Mentors:
Depends on:
Blocks: 1158036
  Show dependency treegraph
 
Reported: 2015-04-18 15:21 PDT by [:MakeMyDay]
Modified: 2015-07-06 10:29 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
EmailToUrnIds-V1.diff (2.42 KB, patch)
2015-04-18 15:28 PDT, [:MakeMyDay]
philipp: review+
Details | Diff | Splinter Review
EmailToUrnIds-V2.diff (4.45 KB, patch)
2015-04-24 01:11 PDT, [:MakeMyDay]
philipp: review+
Details | Diff | Splinter Review
EmailToUrnIds-V3.diff (5.12 KB, patch)
2015-05-10 04:28 PDT, [:MakeMyDay]
makemyday: review+
Details | Diff | Splinter Review

Description User image [:MakeMyDay] 2015-04-18 15:21:43 PDT
If one receives an email invitation where the organizer has an urn:xxxx id in the ICS, the reply will fail because the recipient cannot be resolved to a valid email address when Lightning is composing the reply.

I recently saw such an invitation in a bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1086573#c2

There's a similar bug 607906 for CalDAV, but this one is for email scheduling only.
Comment 1 User image [:MakeMyDay] 2015-04-18 15:28:21 PDT
Created attachment 8594395 [details] [diff] [review]
EmailToUrnIds-V1.diff

This one fixes the issue. Additionally, commonName - if any - is added to the email recipient(s) (sender, cc's and bcc's have this already).
Comment 2 User image Philipp Kewisch [:Fallen] 2015-04-19 13:37:19 PDT
Comment on attachment 8594395 [details] [diff] [review]
EmailToUrnIds-V1.diff

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

::: calendar/itip/calItipEmailTransport.js
@@ +219,5 @@
>                  let toList = "";
>                  for each (let recipient in aToList) {
> +                    let rId;
> +                    // If the recipient id is of type urn, we need to figure out the email address
> +                    if (recipient.id.match(/^urn:/i)) {

Maybe it makes sense to use this logic in other places too? We usually assume it will be a mailto: address, what do you think about making a helper for this or adding support into calAttendee.js ?
Comment 3 User image [:MakeMyDay] 2015-04-24 01:11:13 PDT
Created attachment 8597102 [details] [diff] [review]
EmailToUrnIds-V2.diff

> Maybe it makes sense to use this logic in other places too? We usually
> assume it will be a mailto: address, what do you think about making a helper
> for this or adding support into calAttendee.js ?

Yes, that makes sense. Updated patch with code moved to calUtil.jsm is attached and follow-up bug filed to make use of it for sendMailTo function at different places.
Comment 4 User image Philipp Kewisch [:Fallen] 2015-04-27 03:42:39 PDT
Comment on attachment 8597102 [details] [diff] [review]
EmailToUrnIds-V2.diff

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

r=philipp with these nits:

::: calendar/base/modules/calUtils.jsm
@@ +306,5 @@
> +     * Returns a wellformed email string like 'attendee@example.net',
> +     * 'Common Name <attendee@example.net>' or '"Name, Common" <attendee@example.net>'
> +     *
> +     * @param  calIAttendee    aAttendee
> +     * @param  boolean         aIncludeCn

I think the syntax for including types is:

@param {calIAttendee} aAttendee           Description of parameter


We haven't been including the type explicitly, I don't really mind if we do in the future though. In any case, can you add a short decription on the parameters?

@@ +320,5 @@
> +            }
> +        } else {
> +            // Otherwise, we fall back to the attendee id
> +            email = aAttendee.id;
> +        }

Since you are initializing in both cases anyway, you could just do:

let email;
if (aAttendee.id.match(/^urn:/i)) {
    email = aAttendee.getProperty("EMAIL") || "";
} else {
    email = aAttendee.id;
}

Or even this (if it fits in the line):

let email = aAttendee.id.match(/^urn:/i) ? aAttendee.getProperty("EMAIL") || "" : aAttendee.id;

@@ +327,5 @@
> +        // We add the CN if requested and possible
> +        let cn = aAttendee.commonName;
> +        if (aIncludeCn && email.length > 0 && cn && cn.length > 0) {
> +            if (cn.match(/[,;]/)) {
> +                cn = '"' + cn + '"'; 

Trailing whitespace.

::: calendar/itip/calItipEmailTransport.js
@@ +228,4 @@
>                          toList += ", ";
>                      }
>                      // Add this recipient id to the list.
> +                    toList += rEmail;

I think mapping the array and then joining it is slightly faster than doing string concatenation in each step. You could probably use a combination of Array.map and Array.filter, or even Array.reduce.
Comment 5 User image [:MakeMyDay] 2015-05-10 04:28:06 PDT
Created attachment 8603816 [details] [diff] [review]
EmailToUrnIds-V3.diff

Updated patch with comments considered.
Comment 7 User image [:MakeMyDay] 2015-07-06 10:29:44 PDT
*** Bug 1180619 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.