Closed Bug 1041299 Opened 5 years ago Closed 5 years ago

Sending invitations with configured cc/bcc is broken

Categories

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

Lightning 3.3
defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

Details

Attachments

(1 file, 2 obsolete files)

Bug 702348 received an obviously hasty patch which had two typos, which prevent sending out invitations if cc/bcc is set in the identity used for sending.
Attached patch FixInvitationCcBccCheck-V1.diff (obsolete) β€” β€” Splinter Review
The attached patch fixes this and improves the check to sort out invalid addresses to a fool proof way.

This should also be shipped with Lightning 3.3 - if its to late for the release, we should consider it to be shipped with 3.3.1.
Attachment #8459299 - Flags: review?(philipp)
Comment on attachment 8459299 [details] [diff] [review]
FixInvitationCcBccCheck-V1.diff

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

I was just about to upload the xpis for 3.3. I could hack this in manually, but I really need a reply to the following comment before I can do so. I'll need to upload the xpis first thing tomorrow morning, I hope you are still awake and reading emails :-)

::: calendar/base/modules/calUtils.jsm
@@ +257,2 @@
>          let result = compFields.splitRecipients(aRecipients, false, {});
> +        // Malformed e-mail addresses with display name in list will result in "Display name <>".

I've read about a Thunderbird bug where the "Display name <>" shows up, so this might at some point be fixed. Maybe worth finding that bug and adding a comment.

@@ +261,5 @@
> +        let resultAddress = compFields.splitRecipients(aRecipients, true, {});
> +        let validResult = new Array(0);
> +        for (let key in result) {
> +            if (resultAddress[key]) {
> +                validResult.push(result[key]);

I'm not quite sure what you are trying to achieve here:

Key is just a numeric index here. You are checking if resultAddress has anything defined at each numeric index. It looks to me like this is just a fancy way of checking if resultAddress.length != result.length. I don't see how this figures out which recipients are malformed.
Attachment #8459299 - Flags: review?(philipp) → review-
> I've read about a Thunderbird bug where the "Display name <>" shows up, so
> this might at some point be fixed. Maybe worth finding that bug and adding a
> comment.

In general, you're right - the root cause lies somewhere in the C code of the composer (but I don't know, if this is not an intended behaviour required elsewhere). But I sugguest to check this in for now and add two follow-up bugs, one to fix the root cause and a second to remove the workaround in the calendar code.


> I'm not quite sure what you are trying to achieve here:
> 
> Key is just a numeric index here. You are checking if resultAddress has
> anything defined at each numeric index. It looks to me like this is just a
> fancy way of checking if resultAddress.length != result.length. I don't see
> how this figures out which recipients are malformed.

Both object have always the same number of properites. With the e-mail only check, the respective prop is undefined (which is expected), the one of the second check provides the displayname. So this an easy hack to identify the inclomplete address strings in the displayname check. Alternatively, this can be done by some fancy regex checks.

As mentioned above, i would suggest to check this in now as this bug breaks existing functionality and take care of any other fixes subsequently.
Ok, I understand. So for example resultAddress would contain:

foo@foo.com,undefined,bar@bar.com

And result would contain:

Foo <foo@foo.com>, Baz <>,Bar <bar@bar.com>



Then I'd suggest this for the aurora/central patches, and an explaining comment would be nice.

result = result.filter((v, idx) => !!resultAddress[idx])

I'm going to hack this in manually for 3.3, then upload to AMO. Keep your fingers crossed!
Severity: major → blocker
Target Milestone: --- → 3.3
Version: Lightning 3.5 → Lightning 3.3
Attachment #8459299 - Flags: review- → review+
Comment on attachment 8459299 [details] [diff] [review]
FixInvitationCcBccCheck-V1.diff

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

MakeMyDay, could you put together a new patch that resolves the above and below comments? As the merge has now happened, we should push that to beta/aurora/central so we can resolve this bug. Thanks!

::: calendar/base/modules/calUtils.jsm
@@ +264,5 @@
> +            if (resultAddress[key]) {
> +                validResult.push(result[key]);
> +            }
> +        }
> +        return (!validResult.length) ? "" : validResult.join(",");

It might be worth returning validResult here, and joining in the caller. This way we could use this function to simplify the code in calendar-event-dialog-attendees.*
Attached patch FixInvitationCcBccCheck-V2.diff (obsolete) β€” β€” Splinter Review
Updated patch with each but the last comment considered. The function swallows a comma separated string of e-mail addresses, so it should also return such one (with malformed addresses removed).

I think for the checks you have in mind, always a single address would be passed, so this would not be an issue.

Regarding checkin: don't we have to push this to the ESR branch as well to not loose this patch once we update to 3.3.1? I have in mind you only added this patch manually to the xpi for the 3.3 release and don't see an according push to ESR31.
Attachment #8459299 - Attachment is obsolete: true
Comment on attachment 8461812 [details] [diff] [review]
FixInvitationCcBccCheck-V2.diff

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

Yes, this needs to be pushed to comm-central, comm-aurora, comm-beta, comm-release and target milestone set to 3.3.1.


This comment is optional:

::: calendar/base/modules/calUtils.jsm
@@ +261,5 @@
> +        if (result.length > 0) {
> +            let resultAddress = compFields.splitRecipients(aRecipients, true, {});
> +            result = result.filter((v, idx) => !!resultAddress[idx]);
> +        }
> +        return (!result.length) ? "" : result.join(",");

Minor nit: This should be equivalent to return result.join(","); If result is an empty array, result.join(",") will return an empty string.
Attachment #8461812 - Flags: review+
Attachment #8461812 - Flags: approval-calendar-release+
Attachment #8461812 - Flags: approval-calendar-beta+
Attachment #8461812 - Flags: approval-calendar-aurora+
Updated patch with latest comment considered.
Attachment #8461812 - Attachment is obsolete: true
Keywords: checkin-needed
Target Milestone: 3.3 → 3.3.1
Attachment #8462330 - Flags: review+
Attachment #8462330 - Flags: approval-calendar-release+
Attachment #8462330 - Flags: approval-calendar-beta+
Attachment #8462330 - Flags: approval-calendar-aurora+
Pushing to comm-esr31 to get this picked up by the build:

https://hg.mozilla.org/releases/comm-esr31/rev/fad4ce8d7458
https://hg.mozilla.org/comm-central/rev/0b42bf6eea8a
https://hg.mozilla.org/releases/comm-aurora/rev/0da77eea6c81
https://hg.mozilla.org/releases/comm-beta/rev/2379d887ca5d

This patch will not be in 3.4, but given its not a major release and the cycle is long gone, I'm not going to fix it there.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.