The default bug view has changed. See this FAQ.

Event approval dialog does not filter calendars anymore

RESOLVED FIXED in 4.0.2.1

Status

Calendar
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Sébastien Le Ray, Assigned: MakeMyDay)

Tracking

({regression})

Lightning 4.0.2
4.0.2.1
regression
Dependency tree / graph

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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
Attachment #8654529 - Flags: review-
(Assignee)

Comment 2

2 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(sebastien-mozilla)
(Reporter)

Comment 3

2 years ago
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.
Flags: needinfo?(sebastien-mozilla)
(Assignee)

Comment 4

2 years ago
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.
Assignee: nobody → makemyday
Blocks: 1194997
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
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
Attachment #8654529 - Attachment is obsolete: true
Flags: needinfo?(ssitter)
Attachment #8654641 - Flags: review?(philipp)
(Assignee)

Updated

2 years ago
Depends on: 1197320
(Assignee)

Comment 6

2 years ago
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.
Attachment #8654641 - Flags: review?(philipp)
(Assignee)

Comment 7

2 years ago
Created attachment 8657437 [details] [diff] [review]
DealWithMissing MailtoPrefix-V2.diff

Updated patch with passing test [1].


[1] http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/makemyday@gmx-topmail.de-c19c9a17290a/try-comm-central-linux64/try-comm-central_ubuntu64_vm_test-xpcshell-bm118-tests1-linux64-build1.txt.gz
Attachment #8654641 - Attachment is obsolete: true
Attachment #8657437 - Flags: review?(philipp)
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
Attachment #8657437 - Flags: review?(philipp) → review-
(Assignee)

Comment 9

2 years ago
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 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.
Attachment #8657437 - Flags: review- → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8657495 [details] [diff] [review]
DealWithMissing MailtoPrefix-V3-cc.diff

Updated patch for cc with comments considered.
Attachment #8657437 - Attachment is obsolete: true
Attachment #8657495 - Flags: review+
(Assignee)

Comment 12

2 years ago
Created attachment 8657496 [details] [diff] [review]
DealWithMissing MailtoPrefix-V3-backport.diff

Updated patch with comments considered for backporting to aurora, beta and esr38.
Attachment #8657496 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed, regression
Whiteboard: [checkin-needed:comm-central]

Comment 13

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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.5

Comment 14

2 years ago
Please set approval? flags on patches that need uplift.
Attachment #8657496 - Flags: approval-calendar-release+
Attachment #8657496 - Flags: approval-calendar-beta+
Attachment #8657496 - Flags: approval-calendar-aurora+
Keywords: checkin-needed
Whiteboard: [checkin-needed:comm-central] → [checkin-needed:comm-aurora][checkin-needed:comm-beta][checkin-needed:comm-esr38]
Backported to releases/comm-aurora changeset 555745edb8a6
Keywords: checkin-needed
Target Milestone: 4.5 → 4.4
Backported to releases/comm-beta changeset 6f8c6141448a
Target Milestone: 4.4 → 4.3
Backported to releases/comm-esr38 changeset 09fbf7818bae
Target Milestone: 4.3 → 4.0.3
Whiteboard: [checkin-needed:comm-aurora][checkin-needed:comm-beta][checkin-needed:comm-esr38]
(Assignee)

Updated

2 years ago
Depends on: 1204255
(Assignee)

Updated

2 years ago
Flags: needinfo?(ssitter)
You need to log in before you can comment on or make changes to this bug.