Use correct syntax for vCard version when saving

[hanzi]

This changes the API used for serialising the vCard object into a string.

The previous method called ical.stringify on the underlying jCal object directly, which would always use vCard 4.x syntax regardless of the actual version used in the object.

If the vCard was still in 3.0 format, that would lead to invalid syntax being used. While clients seem to tolerate this most of the time, it breaks DAVx5 photo import.

Here's how the new call is being implemented in ical.js:
https://github.com/mozilla-comm/ical.js/blob/master/lib/ical/component.js#L487

fixes #1312

[codecov]

Codecov Report

Merging #1393 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1393   +/-   ##
=======================================
  Coverage   62.31%   62.31%           
=======================================
  Files           4        4           
  Lines          69       69           
=======================================
  Hits           43       43           
  Misses         26       26

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 438e83e...b9f63e6. Read the comment docs.

[hanzi]

Ok, this is annoying. While testing something else, I noticed an issue with this fix:

When saving a vCard in 3.0 format, ical.js adds VALUE=PHONE-NUMBER to TEL properties, at least sometimes. So a phone number would look like this:

TEL;TYPE=HOME;VALUE=PHONE-NUMBER:+123456789

According to the RFC this is correct, but our DAV backend doesn't recognise this value type and throws an error instead. I filed a PR for that (sabre-io/vobject#486) but even if this was merged, tagged and included in Nextcloud as soon as possible, older Nextcloud versions would still be broken. From what I understand, the Contacts app could be updated independently from Nextcloud, so that's a no-no.

@skjnldsv What would be your preferred way to deal with this?

The issue is with this line here: https://github.com/mozilla-comm/ical.js/blob/master/lib/ical/design.js#L830

The RFC doesn't actually require the value to be explicitely set, so if we patch the vCard 3 design set simliarly to what I've done in #1394 (something like delete ICAL.design.vcard3.property.tel in this case) would work around this problem.

An alternative would be to build a dedicated check that fixes these cases by explicitely making the value type URI. That doesn't actually conform to the vCard 3.0, but that's what we've been generating so far anyway and it seems to work with all relevant clients.

[skjnldsv]

Wow, well done!!
I was always wondering why the heck the lib was behaving this way!!! 🤗

I couldn't figure it out and ran out ot time for contacts with other projects :)

According to the RFC this is correct, but our DAV backend doesn't recognise this value type and throws an error instead. I filed a PR for that (sabre-io/vobject#486) but even if this was merged, tagged and included in Nextcloud as soon as possible, older Nextcloud versions would still be broken

We can backport it then.
Let's add this and make our backend properly compliant :)
Alternatively, we could also indeed patch the lib.
I'm not sure VALUE=PHONE-NUMBER adds anything to our vcards. Especially considering no other vcard manager does it (apple, android, outlook, windows...)

So let's still add this to our backend for compatibility,
But patch the lib like you suggested.
(But I would create a dedicated js file for all those patches, not in the contacts model. This would greatly help understanding the code) 👍

[skjnldsv]

And thanks a lot @hanzi! This is awesome work!! :D

[hanzi]

We can backport it then.
Let's add this and make our backend properly compliant :)

I agree, that would be preferable.

But since sabre-io/vobject is a dependency of nextcloud/server we cannot rely on the backend having been fixed already: The user might use a fixed version of Contacts with an older version of Nextcloud Server. So this workaround is still necessary.

Anyway, I included the design set fix. Had no further issues with phone numbers while testing.