Last Comment Bug 1199942 - Event approval dialog does not filter calendars anymore
: Event approval dialog does not filter calendars anymore
Status: RESOLVED FIXED
: regression
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: Lightning 4.0.2
: Unspecified Unspecified
-- normal (vote)
: 4.0.2.1
Assigned To: [:MakeMyDay]
:
:
Mentors:
Depends on: 1197320 1204255
Blocks: ltn4021
  Show dependency treegraph
 
Reported: 2015-08-29 10:12 PDT by Sébastien Le Ray
Modified: 2015-09-20 09:48 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch fixing issue for ESR38 (1.47 KB, patch)
2015-08-29 10:22 PDT, Sébastien Le Ray
sebastien-mozilla: review-
Details | Diff | Splinter Review
DealWithMissing MailtoPrefix-V1.diff (5.82 KB, patch)
2015-08-30 12:28 PDT, [:MakeMyDay]
no flags Details | Diff | Splinter Review
DealWithMissing MailtoPrefix-V2.diff (5.55 KB, patch)
2015-09-05 06:30 PDT, [:MakeMyDay]
philipp: review+
Details | Diff | Splinter Review
DealWithMissing MailtoPrefix-V3-cc.diff (5.41 KB, patch)
2015-09-05 14:04 PDT, [:MakeMyDay]
makemyday: review+
Details | Diff | Splinter Review
DealWithMissing MailtoPrefix-V3-backport.diff (5.42 KB, patch)
2015-09-05 14:06 PDT, [:MakeMyDay]
makemyday: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
philipp: approval‑calendar‑esr+
Details | Diff | Splinter Review

Description User image Sébastien Le Ray 2015-08-29 10:12:58 PDT
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 User image Sébastien Le Ray 2015-08-29 10:22:04 PDT
Created attachment 8654529 [details] [diff] [review]
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 User image [:MakeMyDay] 2015-08-30 03:26:36 PDT
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:firstname.lastname@example.net)?

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 User image Sébastien Le Ray 2015-08-30 03:51:54 PDT
Yes invitation is from lightning 4.0.2, there's indeed no mailto: prefix:

ATTENDEE;RSVP=TRUE;PARTSTAT=NEEDS-ACTION;ROLE=REQ-PARTICIPANT:somebody@domain.tld

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 User image [:MakeMyDay] 2015-08-30 04:15:55 PDT
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 User image [:MakeMyDay] 2015-08-30 12:28:56 PDT
Created attachment 8654641 [details] [diff] [review]
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 User image [:MakeMyDay] 2015-08-30 14:39:03 PDT
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 8 User image Philipp Kewisch [:Fallen] 2015-09-05 12:50:58 PDT
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:user1@example.net", expected: "mailto:user1@example.net"},
> +                {input: "MAILTO:user2@example.net", expected: "mailto:user2@example.net"},

Hmm I think this may cause trouble. Lets say you get an invitation which correctly uses "MAILTO:foo@example.com". If this locally gets changed to "mailto:foo@example.com", 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 User image [:MakeMyDay] 2015-09-05 13:19:45 PDT
Thanks.
 
> Hmm I think this may cause trouble. Lets say you get an invitation which
> correctly uses "MAILTO:foo@example.com". If this locally gets changed to
> "mailto:foo@example.com", 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 User image Philipp Kewisch [:Fallen] 2015-09-05 13:30:26 PDT
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 User image [:MakeMyDay] 2015-09-05 14:04:21 PDT
Created attachment 8657495 [details] [diff] [review]
DealWithMissing MailtoPrefix-V3-cc.diff

Updated patch for cc with comments considered.
Comment 12 User image [:MakeMyDay] 2015-09-05 14:06:03 PDT
Created attachment 8657496 [details] [diff] [review]
DealWithMissing MailtoPrefix-V3-backport.diff

Updated patch with comments considered for backporting to aurora, beta and esr38.
Comment 13 User image aleth [:aleth] 2015-09-07 02:41:15 PDT
https://hg.mozilla.org/comm-central/rev/c07362aea2b97d5f9c9260571cd80eca854eb99b
Bug 1199942 - Event approval dialog does not filter calendars anymore. r=philipp
Comment 14 User image aleth [:aleth] 2015-09-07 02:50:14 PDT
Please set approval? flags on patches that need uplift.
Comment 15 User image Philipp Kewisch [:Fallen] 2015-09-08 06:41:26 PDT
Backported to releases/comm-aurora changeset 555745edb8a6
Comment 16 User image Philipp Kewisch [:Fallen] 2015-09-08 06:45:44 PDT
Backported to releases/comm-beta changeset 6f8c6141448a
Comment 17 User image Philipp Kewisch [:Fallen] 2015-09-08 12:12:19 PDT
Backported to releases/comm-esr38 changeset 09fbf7818bae

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