andig / carddav2fb

Download CardDAV VCards and upload as phonebook to AVM FRITZ!Box
63 stars 19 forks source link

Alternativ: preserve quickdial and vanity attributes (Fixes #119) #126

Closed blacksenator closed 5 years ago

blacksenator commented 5 years ago

Because the discussion about # 120 has fallen asleep, here's my alternative to stimulation:

What is different?

andig commented 5 years ago

Das sieht schön kompakt aus! Bin auf die Kommentare von @derwok gespannt,

derwok commented 5 years ago

Sorry for beeing late to the party. My last weekends were surpisingly blocked... so, I could not contribute as I wanted. I understand, that @BlackSenator pushed a new solution to wake me up... it worked. :-)

Though looking very compact, I think the main reason for beeing compact is, that this new solution simply does not work... have you tried it?! May be I myself do overlook something here?! In case not... I think it doesn work: Reason for this is: vanity & quickdial are attributes of a single phone number! But a contact may have multiple phone numbers. So, storing the attributes on "vCard UID level" ($key is the vCard UID) you lose the information which phonenumber they once belonged to... On restoring the attributes you attach them to the contact? Not to the phone number? This should not work... This was the reason why I combined my $key as cardUID+normalized phonenumber.

@andig : Please tell me, if you still like me to work on my PR and add unit tests... to my approach ASAP. Or you want to go ahead with @BlackSenator approach. I'm also fine with this... But I think it needs some polishing... I just want to avoid I code unit tests and then no merge is needed...

WOK.

andig commented 5 years ago

I‘m happy with either approach that works.

blacksenator commented 5 years ago

It works, just as the previous solution works. Everything my solution suggestion is to write the data from the Fritzbox to the same place, as if the data come from the CardDAV server. Everything else is as before: The assignment of quickdial/vanity to phone number via the "pref" feature: The "pref"ered phone number of a contact is obviously the number that is also used as quickdial/vanity. Of course, the pref feature might indicate other numbers - but that would not make sense.

derwok commented 5 years ago

Hi @BlackSenator , thanks for explanation. Let's keep up the discussion to find a good solution for our users and our code base. :-)

While yesterday I only looked at code, today I gave it a "shot" - checked out your branch and tried with my local setup. Unfortunately it behaves like I expected: The vanity and quickdial attributes are not restored (saved) to the correct number where I entered them in the FB Web Editor. After a run of the script they always jump back to the very first number of the contact. This is not as expected from user perspective, I fear.

Example: before script run: grafik

After script run: grafik

So, I wonder: where does the "pref" number come from?

andig commented 5 years ago

Makes sense. This pr is as good as the carddav solution, the other is actually offering more. Wondering if it wouldn‘t make sense to remove the x-carddav attributes at all?

blacksenator commented 5 years ago

Okay, good point! I have to admit that I put the "pref" in CardBook. Unfortunately there is on the iPhone in the contacts this option is not given, as I have just checked. Apple accepts and transports the feature - but does not let it edit. My suggestion was just to keep the old solution (tested) and combines it with Wolfram’s. As I said before: Andreas, it’s your choice!

blacksenator commented 5 years ago
andig commented 5 years ago

So- shall we remove the entire x-vanity attribute thing or is that still helpful?

blacksenator commented 5 years ago

X-FB-* only makes sense for users which can set this field on their carddav client and also have access to set pref. Beside of this - I don’t think many users use quickdial and even less use vanity. For me it’s more to cover all phonebook fields with carddav2fb.

andig commented 5 years ago

Ok, then I'd propose to close this PR and get #120 merged as soon as it has tests as it provides better functionality.

blacksenator commented 5 years ago

@derwok To be honest, I think your implementation #120 is too complicated. Therefore, I have modified my alternative proposal so that it corresponds to your proposal for direct assignment of the feature to number without "pref". In doing so, I was able to drastically cut down the previous function for assigning these features in converter.php. I would be glad if you could examine this proposal critically. The sun is shining - so now I´m out for garden work and digging in the ground :)

derwok commented 5 years ago

OK. I'll have a look at it this weekend... If it works like expected without any "Config Hazzle" and with "FB Web Editor is Master" - then I will happily close my proposal...

derwok commented 5 years ago

Hey @BlackSenator I tried your alternative solution on my test environment. Unfortunately it still does not work (for me). It still did not preserve the quickdial & vanity attributes on the correct phone numbers. While your previous solution always attached QD/VA to the first number - your latest update now attaches the QD/VA to all(!) numbers of the affected contact.

Before script run: grafik

After script run: grafik

When I now press "Edit" in FB Web GUI on such a contact, the quickdial is changed to "01" and added to the first number - so basically this looks broken to me.

So, I decided to polish my PR #120 according to @andig suggestions, and added some unit tests for my new functions. Even phpstan is happy... - Ready for merge!

So, - long story short - I would suggest we go with my proposal now and invest our precious time on some other cool features. :-) Anyway: Thanks for shaking me up again... and I really appreciate your energy you put in here! Peace.

blacksenator commented 5 years ago

Your right, I’ve now the same bad result. To be honest - tests befor show a correct behavior. Mhmm, let’s check what happened.

andig commented 5 years ago

Closing this one in favor of @derwok - thats for all your support.