1199942 - Event approval dialog does not filter calendars anymore

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Sébastien Le Ray]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36

Steps to reproduce:

- Have multiple writable calendars, each with one unique associated email address
- Receive an invitation with several recipients, among which only one of the calendars' address
- Click on accept


Actual results:

Lightning shows Calendar selection box when accepting events event if there's no ambiguity


Expected results:

Event should be automatically accepted on the right calendar.

This seems to be related to bugs #1197320 and #1048035. Can't tell if it is a regression though. It seems that the "mailto:" removal in calendar's IDs has been made without any kind of extensive testing, which makes me expect other nasty bugs such these

Comment 1

[Sébastien Le Ray]

Attachment: Patch fixing issue for ESR38

Attached patch fixes the issue but it might be worth auditing the whole code for mailto: expectations in order to have a consistent behavior

Comment 2

[[:MakeMyDay]]

Was the received invitation also composed with Lightning 4.0.2 (shipped version, no modification for bug 1197320 applied)? Can you please check in the source code of the invitation email (ctrl+u), whether or not the attendee, you're receiving the invitation for has prefixed mailto: to its id (look at the attendee rows in the ics section - at the end of the respective attendee it should look like :mailto:[e-mail])?

Based on your description, I would expect it hasn't. If so, this is an outcome of same regression as in bug 1197320, persisted in the sent invitations. That would require some migration code to clean this up - just adapting the comparison is probably not enough to make sure to not get into further issues at other places.

Comment 3

[Sébastien Le Ray]

Yes invitation is from lightning 4.0.2, there's indeed no mailto: prefix:

ATTENDEE;RSVP=TRUE;PARTSTAT=NEEDS-ACTION;ROLE=REQ-PARTICIPANT:[e-mail]

In the same range of issues, I found out that calitipUtils.jsm::getMessageRecipient is broken (it compares names against emails, so never find the recipient), but haven't tracked down the consequences of this.

Comment 4

[[:MakeMyDay]]

Thank you, Sebastian. The email/name thing I already found, the fix for that will be added to the patch for bug 1197320 (it's just a minor issue).

The issue here is the in 4.0.2 the mailto prefix cannot be prepended in the dialog code due to bug 1197320, and invitation without proper id are sent out. An approach like the one proposed by Sebastian would let Lightning identity the right calendar, but the function still delivers an attendee with an inappropriate attendee id, which may cause problems at other places and also further persist the problem within the sent reply. 

I take a look at this one to see what's the best way to cleanup the wrong ids.

Comment 5

[[:MakeMyDay]]

Attachment: DealWithMissing MailtoPrefix-V1.diff

This patch makes use of prependMailTo in calAttendee's id getter/setter to ensure the mailto: prefix is set even for invitations received from a Lightning 4.0.2 installation. It requires also the patch for bug 1197320 to work.

I've done some manual testing on received invitation scenario. With both patches applied, the calendar gets selected automatically again and the event contains the corrected attendee id (even though in the mail source code the id is still without the prefix of cause), also when the event is stored (checked with storage calendar). On top of this, I added a separate unit test (try run [1] is still running)

Stefan, do you have any comments on the approach of this fix?

[1] https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8fa1981cd14d

Comment 6

[[:MakeMyDay]]

Comment on attachment 8654641 [details] [diff] [review]
DealWithMissing MailtoPrefix-V1.diff

The test failed. I will provide an updated patch tomorrow once it passed. Removing the review request until then.

Comment 7

[[:MakeMyDay]]

Attachment: DealWithMissing MailtoPrefix-V2.diff

Updated patch with passing test [1].


[1] http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-c19c9a17290a/try-comm-central-linux64/try-comm-central_ubuntu64_vm_test-xpcshell-bm118-tests1-linux64-build1.txt.gz

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8657437 [details] [diff] [review]
DealWithMissing MailtoPrefix-V2.diff

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

r- for now to discuss the case sensitivity issue.

::: calendar/test/unit/test_bug1199942.js
@@ +7,5 @@
> +function run_test() {
> +    // Test the graceful handling of attendee ids for bug 1199942
> +    createAttendee_test();
> +    serializeEvent_test();
> +    run_next_test();

run_next_test is only required for async tests, which you do not have here.

@@ +12,5 @@
> +}
> +
> +function createAttendee_test() {
> +    let data = [{input: "mailto:[e-mail]", expected: "mailto:[e-mail]"},
> +                {input: "MAILTO:[e-mail]", expected: "mailto:[e-mail]"},

Hmm I think this may cause trouble. Lets say you get an invitation which correctly uses "MAILTO:[e-mail]". If this locally gets changed to "mailto:[e-mail]", then the invitation code may see this as an update and send out invitations. Also, the server may not allow changing the attendee values.

@@ +44,5 @@
> +             " @example.net\n" +
> +             "ATTENDEE;RSVP=TRUE;PARTSTAT=NEEDS-ACTION;ROLE=REQ-PARTICIPANT:user4@exampl\n" +
> +             " e.net\n" +
> +             "ATTENDEE;RSVP=TRUE;PARTSTAT=NEEDS-ACTION;ROLE=REQ-PARTICIPANT:urn:uuid:use\n" +
> +             " r5\n" +

Since this is a testcase and line folding is generally expected to work, I think you can just keep the lines unfolded.

@@ +61,5 @@
> +
> +    // check whether all attendees get returned with expected id
> +    for (let attendee of attendees) {
> +        ok(expectedIds.some(id => id == attendee.id));
> +    }   

Whitespaces here. Also, would expectedIds.contains(attendee.id) work? (or includes, I'm still confused which is more recent and we'll need to differ per branch)

@@ +70,5 @@
> +    serializer.addItems([event], [event].length);
> +    let serialized = ics_unfoldline(serializer.serializeToString());
> +    for (let id of expectedIds) {
> +        ok(serialized.search(id) != -1);
> +    }  

Whitespaces

Comment 9

[[:MakeMyDay]]

Thanks.
 
> Hmm I think this may cause trouble. Lets say you get an invitation which
> correctly uses "MAILTO:[e-mail]". If this locally gets changed to
> "mailto:[e-mail]", then the invitation code may see this as an update
> and send out invitations. Also, the server may not allow changing the
> attendee values.

Enforcing lowercase has been in place before, so this is not a change for attendee, or do I miss something?

-        return (this.mId = (aId ? aId.replace(/^mailto:/i, "mailto:") : null));
+        // we enforce prepending the mailto prefix for email type ids as migration code bug 1199942
+        return (this.mId = (aId ? cal.prependMailTo(aId) : null));

> Whitespaces here. Also, would expectedIds.contains(attendee.id) work? (or
> includes, I'm still confused which is more recent and we'll need to differ
> per branch)

expectedIds is an array not a string. There exists also Array.includes(), but that was newly introduced for FF/TB43 which would prevent backporting the test.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8657437 [details] [diff] [review]
DealWithMissing MailtoPrefix-V2.diff

(In reply to MakeMyDay from comment #9)
> 
> Enforcing lowercase has been in place before, so this is not a change for
> attendee, or do I miss something?

You are right, changing to r+.

> > Whitespaces here. Also, would expectedIds.contains(attendee.id) work? (or
> > includes, I'm still confused which is more recent and we'll need to differ
> > per branch)
> 
> expectedIds is an array not a string. There exists also Array.includes(),
> but that was newly introduced for FF/TB43 which would prevent backporting
> the test.
In that case I think we should use Array.includes on 43, and either Array.indexOf or the method you chose for the backport.

Comment 11

[[:MakeMyDay]]

Attachment: DealWithMissing MailtoPrefix-V3-cc.diff

Updated patch for cc with comments considered.

Comment 12

[[:MakeMyDay]]

Attachment: DealWithMissing MailtoPrefix-V3-backport.diff

Updated patch with comments considered for backporting to aurora, beta and esr38.

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/c07362aea2b97d5f9c9260571cd80eca854eb99b
Bug 1199942 - Event approval dialog does not filter calendars anymore. r=philipp

Comment 14

[aleth [:aleth]]

Please set approval? flags on patches that need uplift.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-aurora changeset 555745edb8a6

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-beta changeset 6f8c6141448a

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-esr38 changeset 09fbf7818bae