Closed Bug 368607 Opened 13 years ago Closed 13 years ago

attendees get doublequoted

Categories

(Calendar :: Import and Export, defect, major)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
Lightning 0.5

People

(Reporter: bvdbos, Assigned: michael.buettner)

References

Details

(Keywords: dataloss)

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: 

When importing an event which has attendees CN with quoted strings (for instance from outlook if names contain a comma such as "Bosch,Bas") Sunbird adds extra quotes. Importing an ics with the produced double-quotes to a local calendar, ftp or webdav or editing the event containing the double-quotes deletes the CN completely (which is when rewriting the event). With communigate, server reports an error 500 (confirmed by developers), there's significant dataloss.

Reproducible: Always

Steps to Reproduce:
1. Import event holding attendees with quoted CN's
2. Export event to ics and open in editor
3. note double double-quotes
4. edit event and reload calendar
5. Export event to ics and open in editor
6. note missing CN
Actual Results:  
see above

Expected Results:  
quoted string for CN stays quoted string.

This doesn't happen to events produced by sunbird as these don't contain names for now, just email-adresses. 
Behaviour experienced in:
* Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070115 Calendar/0.6a1
* TB Versie 1.5.0.9 (20061207) with lightning 0.4a1 (2007011505)

In most cases this is not a big problem as the emailadress is still shown in sunbird when the cn is missing but in communigate there's significant dataloss. set to Major for the dataloss in communigate.
Attached file event with quoted CN
A sample event with quoted string...
Keywords: dataloss
Confirmed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070222 Calendar/0.4a1.  

Because this reduces interoperability and is dataloss, I'm going to ask for it to block for 0.5.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-calendar0.5?
Attachment #253223 - Attachment mime type: text/calendar → text/plain
Since 0.5 will actually do stuff with the attendees, this needs to block.
Blocks: 334685
Flags: blocking-calendar0.5? → blocking-calendar0.5+
Target Milestone: --- → Lightning 0.5
Version: unspecified → Trunk
This is mostly the same problem as bug 307948, but for property parameters instead of values. So it likely needs a very similar fix.
It's similar insofar as it's about the same parts of the code. However, the behaviour seems different to me (but don't know the codebase well). If there's a comma in the name (not email) of the attendee (in the attatchment it's: "Bas, Bosch") the CN gets double quotes ""Bosch, Bas"". If there's no comma like "Bas Bosch", the CN becomes Bas Bosch (without quotes). If you try to escape the comma with \, in the ICS which you import, the CN gets doublequotes anyway (testing for similarity with bug 307948). 
No, it's the same as it being the same problem in the code. we use the escaped-for-ics string, instead of the normal string value of the properties.
Whiteboard: [no l10n impact] [needs patch]
Attached patch patch v1Splinter Review
I had this patch lying around. I don't really like it, but attaching anyway, to prevent it from getting lost. The idea is that is make the value string of a property not quoted. I guess that it will die if you try it on a non-string values property. I'm not even sure what it will do to current callers. But it does seem to fix the bug...
(not requesting review yet, because i want to study on better solutions)
I'm going to further investigate this issue...
The problem happens indeed in the 'getParameter'-function() of calIIcalProperty. This can be verified by breaking at [1], the icalProperty-setter for attendee objects. the icalatt parameter is a calIcalProperty of type attendee, icalatt.icalString reveals the icalstring before its content is put into the atendee object:

ATTENDEE;RSVP=true;CN=\"Bosch, Bas\";PARTSTAT=NEEDS-ACTION;\r\n ROLE=REQ-PARTICIPANT:MAILTO:god@hemel.yi.org\r\n

Everything is still fine, but

icalatt.getParameter("CN") returns:

"\"Bosch, Bas\""

This is just to verify what mvl has already found out, including some context what exactly the problem is.

[1] http://lxr.mozilla.org/seamonkey/source/calendar/base/src/calAttendee.js#128
Obviously, things went wrong inside calIcalProperty::GetParameter(). After some time fiddling around with where precisely we're doing something wrong, I was asking myself where the first pair of quotes have been introduced. This actually happens in icalproperty_get_parameter_as_string(), which in turn asks icalparameter_as_ical_string for a string that looks like 'param-name "=" param-value'. In this case it is perfectly valid to add the quotes to 'param-value' if necessary. icalproperty_get_parameter_as_string() just takes the result, searches for the '=' and returns the rest of the resulting string. That's where the quotes are initially introduced. Excuse me, isn't this completely wrong? The quotes are fine in case we're asking for the 'name = value' string, but not in case we're just asking for the value string itself. Thus I think it's better to strip off the quotes in the latter case.
Attached patch patch v2Splinter Review
patch per previous comment.
Assignee: nobody → michael.buettner
Status: NEW → ASSIGNED
Attachment #259882 - Flags: first-review?(mvl)
Comment on attachment 259882 [details] [diff] [review]
patch v2

r=mvl
Attachment #259882 - Flags: first-review?(mvl) → first-review+
Whiteboard: [no l10n impact] [needs patch] → [no l10n impact]
Whiteboard: [no l10n impact] → [no l10n impact] [needs checkin]
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [no l10n impact] [needs checkin] → [no l10n impact]
Verified in TB 1.5.0.10 with lightning 2007040206, name gets preserved in the attendee though it's not shown in the event (bug 376284 filed).

Thanks all...
Status: RESOLVED → VERIFIED
Whiteboard: [no l10n impact] → [litmus testcase wanted][no l10n impact]
Flags: in-litmus?
Whiteboard: [litmus testcase wanted][no l10n impact] → [no l10n impact]
Flags: blocking-calendar0.5+
Whiteboard: [no l10n impact]
You need to log in before you can comment on or make changes to this bug.