Closed Bug 1209399 Opened 4 years ago Closed 4 years ago

Attendees with comma in name do not work anymore

Categories

(Calendar :: General, defect)

Lightning 4.0.2
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: giermann, Assigned: MakeMyDay)

References

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

After checkin of attachment 8644188 [details] [diff] [review] of bug 1048035 attendees with comma in common name do not work anymore, at least with our CalDAV server (Kerio Connect).

Side effect: opening "Invite attendees" dialog multiple times having attendees with comma in names duplicates the quotes around their names.

Steps to reproduce:
- create new event
- add an attendee:  "Doe, John" <john@doe.org>
- click OK
- open attendees dialog again

Actual results:
- list contains:  ""Doe, John"" <john@doe.org>

Expected results:
- list should still contain:  "Doe, John" <john@doe.org>

It is even possible to again commit the dialog with OK and to open it again, showing triple quotes then.
Attached patch postfix-bug1048035.diff (obsolete) — Splinter Review
Fixing the intended use of slice()
Attachment #8667103 - Flags: review?(makemyday)
Attached patch cleanup-bug1180413.diff (obsolete) — Splinter Review
After checkin of attachment 8667103 [details] [diff] [review], the additional removal of quotes in the attendee list is obsolete (and apart from that not working here as well).
I removed it completely and added a space between common name and mail address.
Attachment #8667106 - Flags: review?(makemyday)
I may have to add, that the actual error from attendees with double quotes around their names is not fixed with this - it simply tries to avoid this situation by removing formerly added quotes.

I did not find the source of the actual creation of 'icalString' for an attendee. This one should be handled in a followup bug or maybe by someone else. There's no check there for already existing quotes!
An attendee with a CN=='"Doe, John"' will still result in
icalString=='ATTENDEE;RSVP=TRUE;CN=""Doe, John"";CUTYPE=INDIVIDUAL:mailto:john@doe.org'

So if one manages to get double quoted names into his attendee list, still only one of them is being removed and results in double quotes in icalString.
Hi, what Lightning version are you reporting your problem for? 
What branch is base and what is branch is target of your patches?

We do really should have unit tests for all those different test cases. The amount of regressions since 4.0 is very bad.
I noticed the error in current release version 4.2.0.1 and verified it's still present in nightly from 28. September 2015. After identifying the problem I downloaded the two single files to create the diffs by browsing the (this mornings) current version at https://hg.mozilla.org/comm-central/.

Even only reading the code should make clear that a single line 'aAddress.name.slice(1, -1);' doesn't change anything at all...
Comment on attachment 8667103 [details] [diff] [review]
postfix-bug1048035.diff

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

Thanks for looking into it. Codewise this looks good, but the patch is missing the headers. Can you add the patch header? Also can you please join both patches?
Comment on attachment 8667103 [details] [diff] [review]
postfix-bug1048035.diff

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

Submitted too quickly - please do not join the patches. The fix in attachment 8667103 [details] [diff] [review] does need to go back until esr38, while the other does not. That said, please raise a separate bug for the second issue and append the updated patch there. For the first, please add the header. Thanks.
Thanks for your review!
But now I don't exactly see what I have to do...

I have to admit, that I don't know the difference between the branches - that's why I sent the patches without headers. I submitted both patches in one bug report, because they are somewhat related - I wanted to avoid another "depends on" - AND: the "cleanup" is somewhat cosmetical, but the removal of the startswith/endswith block depends on a working code for bug 1048035.

I'm fine with both - I could join the patches or I can create another bug report. But what is not clear:
While bug 1180413 has only been committed to comm-central, bug 1048035 has been commit to comm-central, comm-aurora, comm-beta, comm-esr38 - and so breaking the ability to invite people with comma in their names in all these branches.

So - should I also upload different patches for all those branches?
And should I spin off another bug for the second patch, only for comm-central?
Status: NEW → ASSIGNED
Please file a separate bug for the second issue. The thumb rule is one bug per issue. Both patches need to land on cc first, so this is what you prepare patches for. In this case, the patch following up bug 1048035 then can be uplifted as is. After it is checked in to cc, you would have to request approval for this.
Attached patch postfix-bug1048035.diff (obsolete) — Splinter Review
According to comment #6 I setup my Hg environment again and created the patch there. I hope this "-r" flag is what you requested with "add headers"?!
Attachment #8667103 - Attachment is obsolete: true
Attachment #8667103 - Flags: review?(makemyday)
Attachment #8671770 - Flags: review?(makemyday)
Thanks for updating the patch. Unfortunately, this is not what I requested for the header - sorry for not being clear in the first place.

See [1] to check what it takes for a proper patch. You should end up in something similar to the section above the first diff segment in the patch you refernced in your above bug report (preferably already including the ;r=[reviewer short name] (in this case) postfix to the bug title already)

Further documentation for Hg you find also at [2] if needed. If you have further questions or problems to set up Hg accordingly, you can always ask on #maildev or #calendar on irc.mozilla.org

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2] https://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html
Blocks: 1213209
Comment on attachment 8667106 [details] [diff] [review]
cleanup-bug1180413.diff

Spin-off bug 1213209 created.
Attachment #8667106 - Attachment is obsolete: true
Attachment #8667106 - Flags: review?(makemyday)
Attached patch postfix-bug1048035-new.diff (obsolete) — Splinter Review
Thanks for the instructions :)
Attachment #8671770 - Attachment is obsolete: true
Attachment #8671770 - Flags: review?(makemyday)
Attachment #8672594 - Flags: review?(makemyday)
Comment on attachment 8672594 [details] [diff] [review]
postfix-bug1048035-new.diff

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

Thanks for providing the updated patch and being patient. The patch style is now correct, but for convenience of the people doing the checkin for you, please add the reviewer to the commit message already - at least for your final patch.

Even though the fix for the FB dialog is okay, r- for now, because we still need some migration code for cleanup existing events which got already multiple quotes that have been persisted.

For bug 1204255 I used addAttendee() in calItemBase.js [1] - this seems also an appropriate place for this, because it does the job already when creating the event from the ics. Consider to trim n-pairs of double quotes when doing so. Last but not least, a test would be nice - cloning or extending the one for bug 1204255 [2] might be the easiest here.

[1] https://mxr.mozilla.org/comm-central/source/calendar/base/src/calItemBase.js#557
[2] https://mxr.mozilla.org/comm-central/source/calendar/test/unit/test_bug1204255.js
Attachment #8672594 - Flags: review?(makemyday) → review-
Blocks: 1215969
Sorry guys, I'm giving up on this point.
Seems its been too long ago, I worked on bugs here... :(

I currently don't feel able to write unit test, nor do I understand what is meant with these adding reviewer to commit messages. So I decided to give away this bug and keep watching what others do to get it committed. After that I'll try to get bug 1213209 done the same way...
Assignee: giermann → nobody
Status: ASSIGNED → NEW
Thank you Sven, I'm taking this then.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Keywords: regression
Version: Trunk → Lightning 4.0.2
Attached patch FixCommaInCnHandling-V1.diff (obsolete) — Splinter Review
Ok, here we go. The patch adds

- a modified fix for the attendee dialog to remove also multiple double quotes,
- migration code to also deal with persisted CNs with multiple double quotes
- a unit test for the migration code (the test passes locally for me)
Attachment #8672594 - Attachment is obsolete: true
Attachment #8678603 - Flags: review?(philipp)
Comment on attachment 8678603 [details] [diff] [review]
FixCommaInCnHandling-V1.diff

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

Looks good, I hope this is the last issue regarding attendees and invitations!

::: calendar/test/unit/test_bug1209399.js
@@ +90,5 @@
> +
> +    equal(event.getAttendees({}).length, expected.length, "Check test consistency");
> +    for (let exp of expected) {
> +        let attendee = event.getAttendeeById(exp.id);
> +        equal(attendee.commonName, exp.cn, "Test for cn matching of " + exp.id); 

Whitespace snuck in here
Attachment #8678603 - Flags: review?(philipp) → review+
Comment on attachment 8678603 [details] [diff] [review]
FixCommaInCnHandling-V1.diff

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
@@ +404,5 @@
>                        }
>                        attendee.id = cal.prependMailTo(aAddress.email);
>                        if (aAddress.name.length > 0) {
> +                          // we check here for multiple double quotes due to bug 1209399
> +                          attendee.commonName = aAddress.name.replace(/^["]*([^"]*)["]*$/, "$1");

You might want to only replace with at least one double quote, using + instead of * for the first and last one.
Attachment #8678603 - Flags: approval-calendar-release?(philipp)
Attachment #8678603 - Flags: approval-calendar-beta?(philipp)
Attachment #8678603 - Flags: approval-calendar-aurora?(philipp)
(In reply to Philipp Kewisch [:Fallen] from comment #19)

> You might want to only replace with at least one double quote, using +
> instead of * for the first and last one.

Yes, good catch.
Attachment #8678603 - Flags: approval-calendar-release?(philipp)
Attachment #8678603 - Flags: approval-calendar-release+
Attachment #8678603 - Flags: approval-calendar-beta?(philipp)
Attachment #8678603 - Flags: approval-calendar-beta+
Attachment #8678603 - Flags: approval-calendar-aurora?(philipp)
Attachment #8678603 - Flags: approval-calendar-aurora+
Updated patch for cc/esr38 - the first version didn't add the test to the ini. I also changed the regex, but to a different direction. It now removes every occurrence of double quotes (leading, trailing or within) to make sure a valid CN.

aurora/beta seem to need a separate one, I'll take a look after checkins for ers38.
Attachment #8678603 - Attachment is obsolete: true
Attachment #8687723 - Flags: review?(philipp)
Attachment #8687723 - Flags: approval-calendar-release?(philipp)
Attachment #8687723 - Flags: review?(philipp)
Attachment #8687723 - Flags: review+
Attachment #8687723 - Flags: approval-calendar-release?(philipp)
Attachment #8687723 - Flags: approval-calendar-release+
https://hg.mozilla.org/comm-central/rev/1d979aa00e4e
https://hg.mozilla.org/releases/comm-esr38/rev/842492b73ad4

Backport to aurora/Beta still needed.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(makemyday)
Resolution: --- → FIXED
Target Milestone: --- → 4.0.4
Same patch, just an adjustment within the ini file for unit tests to make this applying - no code change. (I wasn't aware I used the same already for the esr checkin - patches really shouldn't rot to long in the queue...)
Flags: needinfo?(makemyday)
Attachment #8687898 - Flags: approval-calendar-beta?(philipp)
Attachment #8687898 - Flags: approval-calendar-aurora?(philipp)
Attachment #8687898 - Flags: review+
Attachment #8687898 - Flags: approval-calendar-beta?(philipp)
Attachment #8687898 - Flags: approval-calendar-beta+
Attachment #8687898 - Flags: approval-calendar-aurora?(philipp)
Attachment #8687898 - Flags: approval-calendar-aurora+
Just to be clear, I do not track the approval-calendar-* flags as part of my release management processes (See http://hg.mozilla.org/users/kent_caspia.com/drivertools/raw-file/default/bugtracking/index.html) so I hope that the calendar team has their own process for this.
It seems this patch leads to Lightning no longer being able to send email confirmation on accepted appointments. This is due to line 574 in calItemBase.js failing because of non-write access to "attendee.commonName". This occurs with a CalDAV based network calendar.
Settings the "attendee.commonName" only when it differs from the original value after applying the quote replacement might at least reduce the problem to when the change is actually necessary. So I changed the lines 574-578 from:
>                attendee.commonName = attendee.commonName
>                                              .replace(/^["]*([^"]*)["]*$/, "$1");
>                if (attendee.commonName.length == 0) {
>                    attendee.commonName = null;
>                }
to:
>                var check = attendee.commonName.replace(/^["]*([^"]*)["]*$/, "$1");
>                if (check.length == 0) {
>                    check = null;
>                }
>                if (attendee.commonName!=check)
>                    attendee.commonName = check;
and this works for me.
Flags: needinfo?(makemyday)
Depends on: 1229329
I filed bug 1229329 for comment 26.
Flags: needinfo?(makemyday)
You need to log in before you can comment on or make changes to this bug.