Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show warning if contact contains unsupported properties #1045

Closed
caugner opened this issue Apr 7, 2019 · 28 comments
Closed

Show warning if contact contains unsupported properties #1045

caugner opened this issue Apr 7, 2019 · 28 comments

Comments

@caugner
Copy link

caugner commented Apr 7, 2019

Is your feature request related to a problem? Please describe.
Recently, I accidentally deleted contacts that appeared to be empty in Nextcloud Contacts (no phone number, no email, no address). However, as it turned out, these contacts actually had phone numbers and addresses attached, just in a not yet supported format with custom types (cf. #42):

Android 8.1 Contacts vs. Nextcloud Contacts 3.0.5:

BEGIN:VCARD
VERSION:3.0
PRODID:-//Sabre//Sabre VObject 4.1.6//EN
UID:d8c54d27-0b2f-40a9-8c2b-a193a193bb7f
FN:Contact with custom types
N:;Contact with custom types;;;
DAVDROID1.TEL;TYPE=x-telekom:+49 160 12 3456789
DAVDROID2.TEL;TYPE=x-o2:+49 176 12 3456789
DAVDROID3.TEL;TYPE=x-vodafone:+49 152 12345678
DAVDROID1.X-ABLABEL:Telekom
DAVDROID2.X-ABLABEL:O2
DAVDROID3.X-ABLABEL:Vodafone
DAVDROID4.X-ABLABEL:Mo-Fr 9-17
DAVDROID5.X-ABLABEL:EMERGENCY ONLY
DAVDROID6.X-ABLABEL:UK
DAVDROID7.X-ABLABEL:DE
DAVDROID8.X-ABLABEL:FR
DAVDROID4.EMAIL;TYPE=x-mo-fr_9-17:mail@example.com
DAVDROID5.EMAIL;TYPE=x-emergency_only:emergency@example.com
DAVDROID6.ADR;TYPE=x-uk;LABEL="42 Street, London":;;42 Street\, London;;;;
DAVDROID7.ADR;TYPE=x-de;LABEL="Straße 42, Berlin":;;Straße 42\, Berlin;;;
 ;
DAVDROID8.ADR;TYPE=x-fr;LABEL="42 rue, Paris":;;42 rue\, Paris;;;;
REV:20190407T162133Z
END:VCARD

PS: Due to the commas in the LABEL, this contact cannot currently be edited in Nextcloud Contacts (cf #1009).

Describe the solution you'd like
If a contact contains unsupported (i.e. unexpected) properties, Nextcloud Contacts should show a warning (possibly extendable to reveal the raw lines).

Describe alternatives you've considered
I cannot think of any alternative right now.

Additional context
Those contacts were created on Android 8.1 using the default Contacts app and synchronized using DAVx⁵.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@skjnldsv
Copy link
Member

skjnldsv commented Apr 7, 2019

@caugner the XXX.ITEM Properties are supported in the upcoming version! :)
Like you mentioned: #42

So what kind of unsupported properties you have in mind? Just show a warning for the coma in label? :)

PS: thanks for being very active lately! It's very pleasant! 🤗

@caugner
Copy link
Author

caugner commented Apr 7, 2019

@skjnldsv That's great news. Actually I don't have any other unsupported properties in mind, but maybe this warning could help to find out about any such unsupported property (if combined with a link to the issues here). What do you think?

PS: The comma in the label does not prevent the address from showing, just from saving/editing the contact.

@skjnldsv
Copy link
Member

skjnldsv commented Apr 7, 2019

@caugner we should probably add some kind of message yes.
Though we want to be the least technical possible for users. So adding a message stating what properties are not show without mentioning why is probably just more confusing that it really helps. 🤔

@caugner
Copy link
Author

caugner commented Apr 7, 2019

@skjnldsv I was thinking about something like this:

Warning: This contact contains information that is not supported by Nextcloud Contacts. (Click for details)

The contact [UUID] was originally created using [PRODID].
It contains the following properties that are not supported by Nextcloud Contacts [VERSION]:

EXPERIMENTAL1:foo
EXPERIMENTAL2:bar

If you think this is a bug, please report an issue here.

@skjnldsv
Copy link
Member

skjnldsv commented Apr 7, 2019

@jancborchardt thoughts? :)

@jancborchardt
Copy link
Member

Why show a warning, we could just show it just like in @caugner’s left screenshot of Android contacts? :)

@skjnldsv
Copy link
Member

@jancborchardt I don't understand, you mean support the properties?
Yes, the best is that, but we cannot support all of them. Other softwares create customs ones that does not exists anywhere else, or are not valid according to the rfc. We supports some of them, but I won't go checking all the vcard software to try to have a panel of custom properties to implement 😉

@jancborchardt
Copy link
Member

If it’s a custom property, say "Telekom" for the phone number, or the "Emergency only" for the mail address, we could simply read that out and use it as the custom label?

@caugner
Copy link
Author

caugner commented Apr 18, 2019

@jancborchardt Meanwhile those custom labels are supported (#42).

@jancborchardt
Copy link
Member

@caugner so is this issue resolved, or what is left to do? Sorry if I’m a bit confused. :D

@caugner
Copy link
Author

caugner commented Apr 18, 2019

The issue remains for other unsupported property (that we may not know yet).

Especially if the app does not show any [supported] properties for a contact, I would like to be sure (as a user) that I can delete that user without losing any data. Right now, I cannot be, as there could be unsupported properties that the app doesn't show me and that I'm not aware of.

@skjnldsv
Copy link
Member

Yes, the issue is on all the props that are unsupported :)

@jancborchardt
Copy link
Member

So could you give some examples of potentially unsupported properties and how they would look in the vCard? E.g. which of the rows in your example above. :)

@skjnldsv
Copy link
Member

@jancborchardt look at that mess: https://en.wikipedia.org/wiki/VCard#vCard_extensions
X-WEBMONEY-ID I don't want to support such things, as they're probably not used that often, and more importantly, not valid according to the rfc.

We should have a warning saying 'A property named X-WEBMONEY-ID is present but not understood by the contacts app, ignoring it', or whatever message that have relevance in that situation :)

@caugner
Copy link
Author

caugner commented Apr 18, 2019

If we knew what vCard properties there are out there that Nextcloud Contacts doesn't support yet, we could just decide to implement them or not, but the issue I see is that new (inofficial) vCard properties may popup without us realising.

PS: Do we really already display all the properties listed here: https://de.wikipedia.org/wiki/VCard#Spezifikation (at least I don't find CALURI, MAILER and TZ in the "Add new property" list).

@jancborchardt
Copy link
Member

@skjnldsv what we can simply do is a workaround for all the ones we don’t understand:

  • For the label, cut the X-, make the other dashes to spaces, and only capitalize the first char. So X-WEBMONEY-ID would become "Webmoney id" as a sort of readable label.
  • Show the content in the input

Wouldn’t that work?

@skjnldsv
Copy link
Member

skjnldsv commented Apr 18, 2019

  • Show the content in the input

We would have no idea what the content is. It could be nonsense, it could be a date, a string, an array... anything. We cannot display them! 😁
Let's skip the 'can we support xxx property' talk and focus on if we need or not a warning and if so, what would it looks like :)

@jancborchardt
Copy link
Member

Right, sorry I’m getting ahead of myself. :) For me any kind of warning is clearly a non-solution.

That’s why I asked:

could you give some examples of potentially unsupported properties and how they would look in the vCard

And I would wager that we can find some way of displaying which works with any potentially unsupported vCard and just displays these things in a semi-meaningful way.

@skjnldsv
Copy link
Member

skjnldsv commented Apr 18, 2019

And I would wager that we can find some way of displaying which works with any potentially unsupported vCard and just displays these things in a semi-meaningful way.

Sure, but it's also very error-prone.
Not to mention the difficulties to maintain any further changes later on.

I don't want to add any kind of similar solutions. 😉

@jancborchardt
Copy link
Member

So, we don’t do anything then? :)

Again, I just need a list of examples and I could show you how it works. Like taking some from https://en.wikipedia.org/wiki/VCard#vCard_extensions:

  • X-EVOLUTION-ANNIVERSARY → "Anniversary"
  • X-KADDRESSBOOK-X-AssistantsName → "Assistantsname"
  • etc… and then we just display what is in "Data" in the input field.

We can just straight up not show the prefixes for X-EVOLUTION-, X-MOZILLA-, X-KADDRESSBOOK- since we know they are software. Sure there might be additional prefixes in the future, but that list doesn’t seem too bad.

This would not change the data, it would only be the way it is displayed in the web interface. So it wouldn’t cause permanent issues whatsoever.

@skjnldsv
Copy link
Member

@jancborchardt and I will then get an anthology of issues asking why the field that appear on nextcloud does not appear on iOS or Android? The vcard standard is pretty strict for that reason, because people created a mess with custom properties and turned a basic data set into the wild wild west! 🤠

@skjnldsv
Copy link
Member

Also, I would make the analogy to css proprietary properties.
Using -webkit-xxx is the same bad behaviour as supporting those properties.
If they're precursors to a future incoming rfc, that would be at least acceptable, like -webkit-line-clamp which will be in css5, but still, we should never support something that is not widely available or accessible for a user :)

@jancborchardt
Copy link
Member

jancborchardt commented Apr 18, 2019

and I will then get an anthology of issues asking why the field that appear on nextcloud does not appear on iOS or Android

Aaah ok it does not show on iOS or Android either? Is there any app which shows it then? If no app shows it, then we don’t need to do that either of course – not even with a warning. Sorry but I don’t know all about this stuff. :D

@skjnldsv
Copy link
Member

@jancborchardt don't be sorry, the vcard is a real nightmare sometimes.
We supports everything that the rfc dictate (especially with the recent #42).
But for X-EVOLUTION-ANNIVERSARY for example, this is only supported by the evolution app, sooo, we might want to at least tell the user why we're not displaying this info? Or not at all and just blankly ignore this sh** and keep on going 😁

@jancborchardt
Copy link
Member

I'd say either:

  • do it like other widely used apps (iOS and Android contacts) and not show it
  • or show it like I described above

But a warning is not a good way to go. It's basically saying "we know this is wrong but we decided to not do anything about it and now we're even telling you". ;)

@skjnldsv
Copy link
Member

@caugner thoughts? :)

@skjnldsv
Copy link
Member

Yes, I think we should not do that. Let's either support custom ones that are widely used, or ignore others. Too much risk of data corruption, and we cannot start supporting custom properties that no one knows about :)

@RokeJulianLockhart
Copy link

RokeJulianLockhart commented Apr 13, 2024

#1045 (comment)

Does it currently display a warning? I ask because if it does not, I believe that this should be reconsidered - regardless of whether the data is retained, if I were to import contacts whose properties were retroactively silently inaccessible unless exported, this would be seriously annoying, because I may well have put some effort into migration which would be rendered useless. Apologies for the question if it does warn the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants