1206502 - ical.js gets confused if an attendee has multiple values in delegated-from or delegated-to parameters

Status: NEW

(Calendar :: ICAL.js Integration, defect)

Description

[[:MakeMyDay]]

An attendee property with multiple comma separated quoted values for delegated-from or delegated-to parameter like described in the RfC [1]

ATTENDEE;RSVP=TRUE;PARTSTAT=TENTATIVE;ROLE=REQ-PARTICIPANT;DELEGATED-FROM="
 mailto:caltest2@localhost","mailto:caltest3@localhost":mailto:caltest4@loc
 alhost

gets parsed by ical.js to

ATTENDEE;RSVP=TRUE;PARTSTAT=TENTATIVE;ROLE=REQ-PARTICIPANT;DELEGATED-FROM="
 mailto:caltest2@localhost":mailto:caltest3@localhost":mailto:caltest4@local
 host

So processing of a comma separated list of values seems to not work correctly. I would like to get this fixed prior to land the patch for bug 1174511, event tough the problem would exist also without that patch.

Comment 1

[[:MakeMyDay]]

[1] http://tools.ietf.org/html/rfc5545#section-3.2.4

Comment 2

[[:MakeMyDay]]

Attachment: Pull request for ical.js

Comment 3

[[:MakeMyDay]]

Apart from the ical.js part, there are still further modifications required to make benefit from this in the Lightning code.

Comment 4

[[:MakeMyDay]]

Comment on attachment 8681710 [details] [review]
Pull request for ical.js

I didn't get grunt to run yesterday, but based on the travis logs of the pr everything except grunt package ran successfully.

Comment 5

[[:MakeMyDay]]

Finally, I got this setup and building without failures now. The pull request is updated accordingly.

Comment 6

[[:MakeMyDay]]

Fixed upstream: https://github.com/mozilla-comm/ical.js/pull/196

Still needs to be integrated. See bug 1115667.

Comment 7

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

Let's mark this one as fixed since it has been merged upstream and handle pulling in bug 1115667.

Comment 8

[[:MakeMyDay]]

This bug still needs to implement the capability to deal with the array type input output values in case ical.js is used.

Comment 9

[[:MakeMyDay]]

Attachment: SupportMultiValueAttendeeParams-V1.diff

Patch to extend the calIAttendee interface to support multi value parameters, an implementation in calAttendee and apply this implementation (not yet tested).

I'm mainly interested in feedback regarding the interface atm, but if you want to look further, I don't mind ;-)

Would you prefer to have all in one patch or move the application of the new methods elsewhere to a separate patch and wait to checkin the latter after the ical.js was updated?

Comment 10

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

Comment on attachment 8708813 [details] [diff] [review]
SupportMultiValueAttendeeParams-V1.diff

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

::: calendar/base/public/calIAttendee.idl
@@ +71,5 @@
> +  /**
> +   * Handle multi-value properties:
> +   * DELEGATED-FROM
> +   * DELEGATED-TO
> +   * MEMBER

I don't think we need to mention them here, I doubt the list will be kept up to date.

@@ +77,5 @@
> +  void getMultiValueProperty(in AString name, out uint32_t aCount,
> +                             [array, size_is(aCount)] out wstring aValues);
> +  void setMultiValueProperty(in AString name, in uint32_t aCount,
> +                             [array, size_is(aCount)] in wstring aValues);
> +  void deleteMultiValueProperty(in AString name);

I think we can just use deleteProperty for this?

::: calendar/base/src/calAttendee.js
@@ +149,5 @@
>      getProperty: function (aName) {
> +        if (this.multiValueParams.includes(aName)) {
> +            cal.LOG("For attendee parameter " + aName +
> +                    " use getMultiValueProperty() instead of getProperty().");
> +        }

I think it would be good if getProperty just returned the first value if it was previously set as a multi-value property.

This may require making drastic changes to how the properties are stored, or making sure the properties are always saved as arrays in the property bag.

@@ +156,5 @@
>      setProperty: function (aName, aValue) {
> +        if (this.multiValueParams.includes(aName)) {
> +            cal.LOG("For attendee parameter " + aName +
> +                    " use setMultiValueProperty() instead of setProperty().");
> +        }

Similar for setProperty, making it a single value property afterwards.

@@ +173,5 @@
> +        this.modify();
> +        this.mProperties.deleteProperty(aName.toUpperCase());
> +    },
> +
> +    multiValueParams: ["DELEGATED-FROM", "DELEGATED-TO", "MEMBER"],

This should probably be a Set() for quicker access.

@@ +197,5 @@
> +        if (values.length > 0) {
> +            // libical does not support multi-value parameters
> +            if(!Preferences.get("calendar.icaljs", false)) {
> +                cal.LOG("Ommitting all values for " + aName + " except of '" + avalues[0] + 
> +                        "' due to limited backend support.");

Is there an easy way to make the backend support this?