-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: add notion of addressbooks #3749
Conversation
I don't understand this feature. Can you explain more? Also, I have the feeling it makes the code much more complex, for instance when downgrading, now, as a developer, we will need to think about checking contacts in the address book. Why? How can we make it very simple for developers to not think about that? |
We could add parameters used on the scope function to the |
For example, I have multiple address books - one from O365 at work, a personal gmail account, a consulting email account - I'd like to have all these in Monica, yet be able to manipulate them independently when needed. |
…into 2020-03-26-addressbooks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in itself is great. That's a good job overall!
However, the concept of Address book adds another layer of abstraction and complexity in the codebase. I don't like this (not the address book, but the added complexity) because the harder our codebase, the harder it is to maintain. We have to fight really really really hard to NOT complexify our codebase if we can. And when we do complexify it, we should make it so the maintenance is really really really easy.
You will see my comments below. What do you think?
database/migrations/2020_03_25_201407_add_contact_vcard_data.php
Outdated
Show resolved
Hide resolved
Co-Authored-By: Regis Freyd <[email protected]>
Thanks for the review. |
This pull request has been automatically locked since there |
This adds the notion of Address Book
It's the first step for #2738
This let an account have multiple address book.
The default one is the current solution (with only 1 address book).
The notion of address books is already integrated on carddav.
This PR only address the ability to have multiple address books.
Other PR will gave the opportunity to
All API functions only address the default address book.
So basically it does not change a lot of things immediatly.