Closed Bug 1156015 Opened 9 years ago Closed 9 years ago

Email scheduling fails for recipients with URN id

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch EmailToUrnIds-V1.diff (obsolete) — Splinter Review
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).
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8594395 - Flags: review?(philipp)
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 ?
Attachment #8594395 - Flags: review?(philipp) → review+
Blocks: 1158036
Attached patch EmailToUrnIds-V2.diff (obsolete) — Splinter Review
> 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.
Attachment #8594395 - Attachment is obsolete: true
Attachment #8597102 - Flags: review+
Attachment #8597102 - Flags: review+ → review?(philipp)
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.
Attachment #8597102 - Flags: review?(philipp) → review+
Updated patch with comments considered.
Attachment #8597102 - Attachment is obsolete: true
Attachment #8603816 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d3090583a3cb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.2
You need to log in before you can comment on or make changes to this bug.