Contacts flags DAVx5 edits as broken

[skjnldsv]

Nextcloud Contacts flags DAVx5 edits as broken

• DAVx5 writes REV as REV:20191125T222506Z.
  • Nextcloud Contacts flags this as a broken card.
• Nextcloud Contacts writes REV as REV;VALUE=DATE-AND-OR-TIME:20191125T222506Z

Technically, Nextcloud Contacts makes them invalid

Both applications start their vCards like:

BEGIN:VCARD
VERSION:3.0

• According to vCard 3.0 REV defaults to VALUE=date-time. DAVx5 provides the correct format.
• vCard 3.0 does not define date-and-or-time nor is it defined in RFC 2425 which includes format definitions for vCard 3.0. The value date-and-or-time is valid in vCard 4.0.
  • That's because Nextcloud Contacts claims to write vCard 3.0 but uses ical.js, a vCard 4.0 library.

Code in question

contacts/src/services/checks/invalidREV.js

Lines 31 to 33 in 8fc07c1

	if (contact.vCard.hasProperty('rev')
	&& contact.vCard.getFirstProperty('rev').getFirstValue()
	&& contact.vCard.getFirstProperty('rev').getFirstValue().icalclass === 'vcardtime') {

Example files

https://gateway.pinata.cloud/ipfs/QmcWfMXDgBSAYk9X5Lh9tWjkuoJz39LHq2BSNDYThkfiYV

Originally posted by @jaller94 in #1279 (comment)

[skjnldsv]

@jaller94 could you help fixing this? :)

[jaller94]

As I see it, they are two different issues.

The first one can be mitigated by rewriting the test for invalid REV until it becomes valid. I have not tested this, but I would first look if REV:REV:20191125T222506Z satisfies the 3rd condition .icalclass === 'vcardtime'.

Maybe #998 helps to understand why this check was implemented and what it is trying to accomplish. I'm unsure about the intention behind the check. Why would a user care about the invisible REV in the first place? I see it as a way to distinguish versions for application, which could silently fail with the result "can't parse the value(s), but the vCard has changed since my last edit".

contacts/src/services/checks/invalidREV.js

Lines 31 to 33 in 8fc07c1

	if (contact.vCard.hasProperty('rev')
	&& contact.vCard.getFirstProperty('rev').getFirstValue()
	&& contact.vCard.getFirstProperty('rev').getFirstValue().icalclass === 'vcardtime') {

The second is caused by dates created with VCardTime from ical.js, which is widely used in this app. If Nextcloud Contacts was strict about vCard 3.0 support, this vCard 4 library would not be used for writing datetimes.

contacts/src/services/checks/invalidREV.js

Line 47 in 8fc07c1

const rev = new VCardTime(null, null, 'date-time')

Generally, the app should be forgiving while parsing and strict while writing.

[skjnldsv]

Why would a user care about the invisible REV in the first place?

Because it is mandatory in a vcard as per the rfc.
A valid REV is one of the requirements.

[skjnldsv]

I mean we could just return false and silently fix the REV fields 🤔

[jaller94]

Nextcloud Contacts does silently update REV when you edit a card. Editing a field already has the same effect on the REV value as clicking on the upgrade button does.

[skjnldsv]

No, I mean it will show a warning stating the contact have been fixed. Returning false will just do it in the back and no user will complain.

[jaller94]

"Fixing" without a user interaction may very well result in an autonomous edit war between the tow clients.

I suggest the following acceptance criteria (for the entire project):

• A vCard MUST NOT be saved without a user's interaction. Viewing a card does not count as an interaction.
• Failing to parse a vCard's REV field SHOULD NOT affect the UI. Nextcloud Contacts hides this functionality for versioning from the user as it holds no significant value to a non-technical user.

[skjnldsv]

• A vCard MUST NOT be saved without a user's interaction. Viewing a card does not count as an interaction.

ah, from a dev perspective, yes it make sense.
From a user perspective, it absolutely doesn't.
Otherwise we get to issues created like I got lately because I now ask for the user to confirm when something have been fixed.
Which is completely understandable, users do not care about that at all. For them it's confusing and they expect things like that (maintenance) to be done automatically. (I'm talking about this but also the crazy tons of comments/feedback I got over the last 4 years)

• Failing to parse a vCard's REV field SHOULD NOT affect the UI. Nextcloud Contacts hides this functionality for versioning from the user as it holds no significant value to a non-technical user.

That I can agree with.
Nonetheless some errors are crucial to be fixed before the user can edit the vcard.
Meaning we need to keep validating on load. We cannot do it only on save.

[mburnicki]

• A vCard MUST NOT be saved without a user's interaction. Viewing a card does not count as an interaction.

ah, from a dev perspective, yes it make sense.
From a user perspective, it absolutely doesn't.

Sorry, I strongly disagree here.

Even though I know some folks find it cool, I personally find it very annoying if I have to edit several fields and whenever I've finished one field it is immediately saved to the database, with or without typos I've made. I'd really prefer to do all the necessary changes first and then click on "Save" to change them.

That said, I agree with @jaller94 that's an absolute "no go" if an app like the contacts app changes and writes data back without my ack, even without letting me know, just because it thinks something needs to be fixed.

We have seen enough cases where the app didn't work correctly, and I really don't want to have my contact database be messed up just because I load the contacts to see them on the web page.

Otherwise we get to issues created like I got lately because I now ask for the user to confirm when something have been fixed.

You could write an appropriate hint so the user wouldn't have to ask, but IMO its inacceptable to fiddle with the data even without letting the user know.

Which is completely understandable, users do not care about that at all. For them it's confusing and they expect things like that (maintenance) to be done automatically. (I'm talking about this but also the crazy tons of comments/feedback I got over the last 4 years)

• Failing to parse a vCard's REV field SHOULD NOT affect the UI. Nextcloud Contacts hides this functionality for versioning from the user as it holds no significant value to a non-technical user.

That I can agree with.
Nonetheless some errors are crucial to be fixed before the user can edit the vcard.
Meaning we need to keep validating on load. We cannot do it only on save.

If the user has edited the vcard and it needs to be saved anyway, you can do the fixes, but not just automatically when loading the vcards. I've about 4000 contacts in different addressbooks and don't even want to imagine what happens if the contacts app thinks its needs to do sm´omething with all these contacts just when I'm displaying them.

[jaller94]

A vCard MUST NOT be saved without a user's interaction. Viewing a card does not count as an interaction.

I should clarify that I consider editing any field to be a reason to save. I was not suggesting to add an additional step to editing (e.g. a "Save" button).
My point was, that viewing/ displaying shouldn't modify the data.

[skjnldsv]

I should clarify that I consider editing any field to be a reason to save. I was not suggesting to add an additional step to editing (e.g. a "Save" button).

Me neither, we can omit this from this issue :)
@mburnicki this is not a matter of editing data within the vcard.

You could write an appropriate hint so the user wouldn't have to ask, but IMO its inacceptable to fiddle with the data even without letting the user know.

That's exactly what people do not want. I've received tons of comments and messages about this. We have an hint now. Currently we do not auto-save and we show a warning stating the contact was fixed and you need to manually click to send the change (which is what you suggesting) and people seems to really dislike that.

We have seen enough cases where the app didn't work correctly, and I really don't want to have my contact database be messed up just because I load the contacts to see them on the web page.

Interestingly enough, lots of apps do that. Your android contact manager, your contact sync (davx), apple contacts... All of them edit your raw data and change them/customise them to add functionalities you never know existed.

[rfc2822]

I just noticed this problem when testing Nextcloud and it's confusing, because DAVx⁵ doesn't upload anything wrong, but the contacts are then "defect" (I got scared when I read it the first time).

Can I help in this, like by providing more information?

[skjnldsv]

Hey Ricki :)
I think we need to adjust the checker to what davx is doing, our vcard lib is doing extra unnecessary things sometimes.
I just did not had the time to dive in this yet :(

[rfc2822]

Ok, no problem! Now I know it's only the REV :)

[kangaroo72]

Is that the mentioned issue?

[simonspa]

@kangaroo72 yes, that's exactly the issue described here.

[pperle]

Does #1393 fix this issue?

[hanzi]

@pperle No, it doesn't. This issue here seems to be about contacts that have been written by other CardDAV clients and not by the Contacts app.

@skjnldsv If I understand you correctly, Contacts is writing a new REV value every time it updates the contact, right? Then why do we even need to fix it when loading the contact?

Either the user is just viewing the contact, then as long as we can properly load the contact we don't really have an issue.
And if the user is updating the contact, we already replace the existing REV, thus 'fixing' it.

Or is there some other issue with this that I've overlooked?

[e-alfred]

This could probably also affect the Maps app not loading contacts at all onto the map.

[hanzi]

@e-alfred Unlikely. Contacts is just a front-end tool for displaying and editing vcards and not for the actual storage. (That's what the DAV app is doing.)

As far as I can tell, Maps is not accessing anything that Contacts is providing but rather fetching the contacts from DAV directly. So this would be unrelated.

[arnowelzel]

I can confirm this problem with DAVx5 on Nextcloud 17.0.2. As soon as I edit a contact on my Android device and sync it with DAVx5 it will be displayed with the notice "This contact was broken and received a fix. Please review the content and click here to save it.".

Since I am not the only user with DAVx5 on that installation (my server is used by about 80 people and about 20 of them also use DAVx5) it starts getting annoying - some users of the installation already asking me what is wrong with the contacts app in Nextcloud. At least I can refer them to this issue here.

[askuri]

@skjnldsv If I understand you correctly, Contacts is writing a new REV value every time it updates the contact, right? Then why do we even need to fix it when loading the contact?

Either the user is just viewing the contact, then as long as we can properly load the contact we don't really have an issue.
And if the user is updating the contact, we already replace the existing REV, thus 'fixing' it.

Or is there some other issue with this that I've overlooked?

To me it looks like the REV check was introduced in pull request #998 to solve issue #959. In that issue the problem was that an invalid REV caused infinite loading of the contact. Solution was to fix the REV of the vCard.

However, should contacts even attempt to fix REVs or simply ignore invalid REVs?

I assume that invalid vCards might cause problems in ical.js which, for some reason that is not clear to me, makes it enter an infinite loop.

As a workaround, I suggest instead of fixing or trying to restore the vCard, we'd set the REV temporarily to a valid value before letting ical.js parse it (shouldn't matter which value, because the value is not valid from what I can see). Temporarily means, we will not manipulate the vCards in the storage. That would respect @jaller94 's criteria for a fix as well:

* A vCard MUST NOT be saved without a user's interaction. Viewing a card does not count as an interaction.

* Failing to parse a vCard's `REV` field SHOULD NOT affect the UI. Nextcloud Contacts hides this functionality for versioning from the user as it holds no significant value to a non-technical user.

Update 2020-02-21: I just noticed the REV field is actually used for something: https://github.com/nextcloud/contacts/blob/master/src/components/Properties/PropertyRev.vue

[githubetc]

Please correct me if I am wrong, is this happening:

First Nextcloud Contacts claims that a valid REV is invalid ( as @jaller94 has already pointed that out in #1279 (comment) ) and then it actively gets the users to change their own valid data into invalid data (yet without providing a way for the users to undo those corruptions).

It is not just DAVx5, a lot of other vCard / CardDAV apps also use the REV field for proper syncing of vCards (they will all be assuming that the vCard 3.0 standard is followed).

If the problem is with VUE then shouldn't VUE be fixed or a workaround be built to handle that specific problem instead of breaking the vCard 3.0 standard thus breaking ALL other vCard 3.0 apps ?

[mburnicki]

I've already said earlier in a different thread that I find it incredibly annoying that a web page that is just opened to show data from a database thinks it has to fix data sets automatically and write the changes back to the DB, without letting the user know, and without a chance for the user to prevent this.

I have nearly 3000 contacts in about 30 address books and I don't even want to imagine what happens when I just open the contacts web page while the contact app has a bug (unfortunately this seems to not too uncommon) and messes up my whole database. For me such behavior of an app is an absolute showstopper.

[b-pfl]

I've already said earlier in a different thread that I find it incredibly annoying that a web page that is just opened to show data from a database thinks it has to fix data sets automatically and write the changes back to the DB, without letting the user know, and without a chance for the user to prevent this.

I have nearly 3000 contacts in about 30 address books and I don't even want to imagine what happens when I just open the contacts web page while the contact app has a bug (unfortunately this seems to not too uncommon) and messes up my whole database. For me such behavior of an app is an absolute showstopper.

And in addition: If the Contacts Website provides the error message "this contact was broken and received a fix", it should also highlight WHAT is / was broken! Nevertheless, it would really be helpful if this gets fixed soon. Is there anything we can help with?

[mindrunner]

Is anyone working on this 5 month old issue? Or are we just talking about it?

[skjnldsv]

@mindrunner can you work on it?

[mindrunner]

Yes, indeed I would be available to get this fixed. I do agree on a lot of what @jaller94 was saying in this thread. However, there need to be some agreement on how to actually fix this problem before I consider starting to work on this.

Besides, this looks like a rather small bugfix to me (at least to solve the initial issue) which would create a huge overhead on my side, because I do not have a nextcloud build whatever I need environment set up. Thus, one of the more regular developers should be able to do this way more efficiently.

[skjnldsv]

Unfortunately, like said before, the current developer of contacts (aka me) is not available right now. Once I get better free time, I will definitely try to tackle this. In the meantime, anyone willing to dive here will be welcomed! 🤗

[eleith]

@skjnldsv i have a PR up in an attempt to propose a solution.

the PR does avoid the "broken" message that currently is shown in the latest release.

i'm happy to make iterations as needed to resolve this issue, let me know.

[jancborchardt]

Can everyone here then test and review the fix by @eleith at #1597 :)

---

@mamatt unpinned this issue 16 days ago

Please do not unpin pinned issues, thanks :)

[mamatt]

@jancborchardt , sorry for that, this was obviously not intentional ;)

[DesktopMasters]

I am a little lost here as I an experiencing this issue. Is this closed because it is being fixed in #1597? Is it fixed somewhere?

[mindrunner]

I tried the fix and it did not work for me (see comments in PR).

Furthermore, after PR was closed, I never saw the plugin being updated in my instance.

So, imho, it's not fixed yet

[DesktopMasters]

But it is closed. I am going to guess that is because it is moved to #1597 that says "Merged" but not "Closed".

[mindrunner]

#1352 is the bug report, whereas #1597 is the pull request trying to fix it.