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

feat: add notion of addressbooks #3749

Merged
merged 50 commits into from
Dec 15, 2020
Merged

feat: add notion of addressbooks #3749

merged 50 commits into from
Dec 15, 2020

Conversation

asbiin
Copy link
Member

@asbiin asbiin commented Mar 26, 2020

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

  • add other distant address books, and add subscription to them
  • connect to carddav clients and update them regularly
  • display address books content on dashboard

All API functions only address the default address book.

So basically it does not change a lot of things immediatly.

@djaiss
Copy link
Member

djaiss commented Mar 26, 2020

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?

@asbiin
Copy link
Member Author

asbiin commented Mar 26, 2020

We could add parameters used on the scope function to the Account::contact one, so it will be easy for developers, because account->contact() will be the default address book, and account->contact($id, $abid) will return a specific contact list.
The only limit is to get the ALL contacts of an account. Maybe if addressbookId is -1, it return every contacts.

@tomperrine
Copy link

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?

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.

@monicahq monicahq deleted a comment from cypress bot Apr 19, 2020
@asbiin asbiin requested a review from djaiss April 23, 2020 21:27
Copy link
Member

@djaiss djaiss left a 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?

app/Models/Contact/Contact.php Outdated Show resolved Hide resolved
app/Models/Contact/Contact.php Outdated Show resolved Hide resolved
app/Models/Contact/Contact.php Outdated Show resolved Hide resolved
app/Http/Controllers/DashboardController.php Outdated Show resolved Hide resolved
app/Services/VCard/ImportVCard.php Outdated Show resolved Hide resolved
asbiin and others added 2 commits April 24, 2020 16:45
Co-Authored-By: Regis Freyd <djaiss@users.noreply.github.com>
@asbiin
Copy link
Member Author

asbiin commented Apr 24, 2020

Thanks for the review.
I don't think it adds so complex code .
However, I understand your point, and I tried to simplify it. I guess it's not working for you ...

@asbiin asbiin mentioned this pull request Apr 25, 2020
@asbiin asbiin requested a review from djaiss December 8, 2020 07:45
@asbiin asbiin merged commit a18962e into master Dec 15, 2020
@asbiin asbiin deleted the 2020-03-26-addressbooks branch December 15, 2020 07:54
@github-actions
Copy link

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants