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)
Calendar
E-mail based Scheduling (iTIP/iMIP)
Tracking
(Not tracked)
RESOLVED
FIXED
4.2
People
(Reporter: MakeMyDay, Assigned: MakeMyDay)
References
Details
Attachments
(1 file, 2 obsolete files)
5.12 KB,
patch
|
MakeMyDay
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
> 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+
Assignee | ||
Updated•9 years ago
|
Attachment #8597102 -
Flags: review+ → review?(philipp)
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
Updated patch with comments considered.
Attachment #8597102 -
Attachment is obsolete: true
Attachment #8603816 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 6•9 years ago
|
||
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.
Description
•