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

Improve Compatibily with macOS Contacts #1999

Merged
merged 1 commit into from
May 8, 2021
Merged

Conversation

zlajo
Copy link
Contributor

@zlajo zlajo commented Dec 30, 2020

I would like to use nextcloud contacts as a replacement to storing my contacts in iCloud. Unfortunately there are some things that don't work as expected and while some issues require fundamental changes (like #609) there are some which I was able to fix with only a few small changes.

One thing I found out was that there already is support for hiding contact groups (as defined by https://tools.ietf.org/html/rfc6350#section-6.1.4). macOS Contacts still uses vCard 3.0 and because of that cannot relay on the vCard kind property defined by vCard 4.0. But there is a proprietary attribute which does the same thing (X-ADDRESSBOOKSERVER-KIND). I updated the contacts app to also respect this Apple specific property and added an easy way to add further proprietary "kind"-properties.

There is also a problem (at least I think so) with the underlying vCard serialization which leads to strange property names. See my comment in src/store/contacts.js for a detailed description of the problem and a potential workaround.

Please let me know if my update works and contact me if further changes/discussions are required.

@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #1999 (5dbccbd) into master (639e2a1) will decrease coverage by 69.83%.
The diff coverage is n/a.

❗ Current head 5dbccbd differs from pull request most recent head 564f6dc. Consider uploading reports for the commit 564f6dc to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master   #1999       +/-   ##
============================================
- Coverage     69.83%   0.00%   -69.84%     
- Complexity      238     249       +11     
============================================
  Files            22      22               
  Lines           673     738       +65     
============================================
- Hits            470       0      -470     
- Misses          203     738      +535     
Impacted Files Coverage Δ Complexity Δ
lib/Controller/PageController.php 0.00% <0.00%> (-100.00%) 3.00% <0.00%> (ø%)
lib/Controller/ContactsController.php 0.00% <0.00%> (-100.00%) 2.00% <0.00%> (ø%)
lib/Service/Social/GravatarProvider.php 0.00% <0.00%> (-100.00%) 6.00% <0.00%> (ø%)
lib/ContactsMenu/Providers/DetailsProvider.php 0.00% <0.00%> (-96.88%) 11.00% <0.00%> (ø%)
lib/Service/Social/TumblrProvider.php 0.00% <0.00%> (-96.16%) 12.00% <0.00%> (+1.00%) ⬇️
lib/Service/SocialApiService.php 0.00% <0.00%> (-91.16%) 55.00% <0.00%> (+1.00%) ⬇️
lib/Service/Social/FacebookProvider.php 0.00% <0.00%> (-90.70%) 17.00% <0.00%> (ø%)
lib/Service/Social/DiasporaProvider.php 0.00% <0.00%> (-88.89%) 21.00% <0.00%> (+2.00%) ⬇️
lib/Service/Social/MastodonProvider.php 0.00% <0.00%> (-87.81%) 19.00% <0.00%> (+2.00%) ⬇️
lib/Service/Social/InstagramProvider.php 0.00% <0.00%> (-87.50%) 17.00% <0.00%> (+1.00%) ⬇️
... and 9 more

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 639e2a1...564f6dc. Read the comment docs.

@skjnldsv skjnldsv added 3. to review Waiting for reviews bug Something isn't working labels Jan 4, 2021
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments :)

src/models/contact.js Outdated Show resolved Hide resolved
@@ -346,8 +346,7 @@ const actions = {
*/
async getContactsFromAddressBook(context, { addressbook }) {
return addressbook.dav
.findAllAndFilterBySimpleProperties(['EMAIL', 'UID', 'CATEGORIES', 'FN', 'ORG', 'N',
'X-PHONETIC-FIRST-NAME', 'X-PHONETIC-LAST-NAME'])
.findAllAndFilterBySimpleProperties(Contact.MinimalContactProperties)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be much cleaner! Nice! 👍

Comment on lines +44 to +45
ICAL.design.vcard3.param.type.multiValueSeparateDQuote = true
ICAL.design.vcard.param.type.multiValueSeparateDQuote = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I was not aware of such possibilities :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, it was on the old designSet update workaround! Nice! 👍

@zlajo
Copy link
Contributor Author

zlajo commented Jan 5, 2021

@skjnldsv I have just updated my pull request.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last cleanup on the code 👍
Looks great! Nice job :)

src/models/contact.js Outdated Show resolved Hide resolved
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me :)
Please rebase and squash your commits into a single one before merging! 🚀

@jancborchardt
Copy link
Member

@zlajo only the Developer Code of Origin test fails, could you sign off your commit? :) You just have to git commit --amend --signoff – more info at https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---signoff

@jancborchardt
Copy link
Member

By the way @zlajo we have a Nextcloud Talk instance for easier communication – we also have a channel for Groupware/Contacts there. If you like, we can invite you? :)

Signed-off-by: Johannes Zlattinger <johannes@zlattinger.net>
@zlajo
Copy link
Contributor Author

zlajo commented May 7, 2021

@jancborchardt I just did a rebase on the current master and a force push. Is this what I had to do?

@skjnldsv skjnldsv merged commit 97c9369 into nextcloud:master May 8, 2021
@skjnldsv skjnldsv removed this from the next milestone Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants