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

WIP: import contacts from remote CardDAV server #134

Open
wants to merge 12 commits into
base: devel
Choose a base branch
from
Open

WIP: import contacts from remote CardDAV server #134

wants to merge 12 commits into from

Conversation

Fnux
Copy link
Collaborator

@Fnux Fnux commented Nov 17, 2018

This PR addresses parts of #124: it alows to import contacts from a remote CardDAV server using vdirsyncer. I did not add a way to export the local contacts since the user doesn't have a form (for now) to add new entries manually.

I still have to investigate a sexier way to store the application's configuration and write tests for the new features.

@CRImier
Copy link
Member

CRImier commented Nov 17, 2018

Re "storing application's configuration" - we have solutions for working with .json config files - storing, recreating configs on error, saving, even primitive migrations. See the howtos regarding that, let me know if anything's unclear.

@codecov
Copy link

codecov bot commented Nov 17, 2018

Codecov Report

Merging #134 into devel will increase coverage by 2.8%.
The diff coverage is 37.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##            devel     #134     +/-   ##
=========================================
+ Coverage   38.51%   41.32%   +2.8%     
=========================================
  Files         233      255     +22     
  Lines       18126    20867   +2741     
=========================================
+ Hits         6982     8623   +1641     
- Misses      11144    12244   +1100
Impacted Files Coverage Δ
helpers/vdirsyncer.py 1.88% <1.88%> (ø)
helpers/xdg.py 100% <100%> (ø)
apps/personal/contacts/address_book.py 54.92% <33.33%> (-20.29%) ⬇️
apps/personal/contacts/vcard_converter.py 33.33% <50%> (-1.15%) ⬇️
apps/personal/contacts/main.py 11.49% <7.46%> (-23.93%) ⬇️
apps/personal/contacts/contact.py 83.33% <83.33%> (ø)
tests/test_drivers.py 66.22% <0%> (-24.69%) ⬇️
apps/example_apps/sandbox/main.py 40.9% <0%> (-22.73%) ⬇️
ui/refresher.py 72.61% <0%> (-8.21%) ⬇️
ui/utils.py 90.67% <0%> (-5.16%) ⬇️
... and 90 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 0c9b304...a180045. Read the comment docs.

Copy link
Member

@CRImier CRImier left a comment

Choose a reason for hiding this comment

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

Reviewed the code without running it, for now. Change the i=self.i, o=self.o, Printer and Listbox stuff, and do let me know about the other changes =)

apps/personal/contacts/main.py Outdated Show resolved Hide resolved
apps/personal/contacts/main.py Outdated Show resolved Hide resolved
apps/personal/contacts/main.py Outdated Show resolved Hide resolved
apps/personal/contacts/main.py Outdated Show resolved Hide resolved
apps/personal/contacts/main.py Outdated Show resolved Hide resolved
apps/personal/contacts/main.py Outdated Show resolved Hide resolved
apps/personal/contacts/contact.py Outdated Show resolved Hide resolved
apps/personal/contacts/contact.py Outdated Show resolved Hide resolved
apps/personal/contacts/contact.py Outdated Show resolved Hide resolved
apps/personal/contacts/main.py Outdated Show resolved Hide resolved
@Fnux
Copy link
Collaborator Author

Fnux commented Jan 4, 2019

@CRImier I'd like to move the Contact and AddressBook classes under libs/ since they might be used by other apps in the future (Calls, SMS). Does it makes sense to you?

@Fnux
Copy link
Collaborator Author

Fnux commented Jan 5, 2019

I still have to clean, document and test the code I moved to lib/. I will resolve the WIP state once it is done.

@Fnux
Copy link
Collaborator Author

Fnux commented Jan 6, 2019

I added a paths helper, which I needed for the vdirsyncer and address_book modules.

from vcard_converter import VCardContactConverter
from ui import (NumberedMenu, Listbox, Menu, LoadingIndicator, DialogBox,
PrettyPrinter as Printer, UniversalInput)
from distutils.spawn import find_executable
Copy link
Member

Choose a reason for hiding this comment

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

Unused import?

@CRImier
Copy link
Member

CRImier commented Jan 7, 2019

This works for your usecase, right?

We can use the default config management functions to store the config files, just need to point it to the right folder.

As about testing - doctests for code in libs/ pass, but you can add more; we don't yet have a good way to test apps.

@CRImier
Copy link
Member

CRImier commented Feb 4, 2019

Looks good! Will do a couple of fixes now and merge

@CRImier
Copy link
Member

CRImier commented Feb 5, 2019

Oh right, read through the code again. Thinking about some kind of changes to config management that'd avoid using the main ZPUI config. For now, I'm making sure that the ZPUI config is used the least amount possible - since it has hardware configuration parameters, it's important for it to not get changed by the users often. + I'm not sure that it makes sense for the vdirsyncer wrapper library to store the config where it currently is, that is, in libs/ - the idea about libraries in libs/ is that they can be used by more than one app, in different ways.

So, I have an idea - read the config from the app (+ allow changing it from the same app), then pass the config variables. Let me know what you think of it. For now, merging the code into a separate branch (contacts-sync) + adding you as a contributor in this repo.

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

Successfully merging this pull request may close these issues.

2 participants