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: paginate the Contacts page and improve database performance #2135

Open
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@AdrienPoupa
Copy link
Contributor

AdrienPoupa commented Dec 6, 2018

This commit uses Vue Good Table, already used for the conversations logs, to display the contacts in a datatable that is paginated on the server side. This was suggested in #1992.
A new route is created to supply the data to the datatable: /people/list
image
Here is the new datatable:
image
One can select the number of contacts to display and search for a contact:
image
Current features such as ordering the table or filtering it with a tag still works, but needs a reload of the page. If we wanted to do it without reload, that would be possible but we should display the contact information in different rows (such as firstname row, lastname row etc so that we could click on it). I don't know if this behavior is wanted or not.
I also improved the tag retrieval mechanisms by creating a query that lists the number of contacts for a single tag:

select COUNT(tag_id) AS contact_count, name, name_slug from contact_tag inner join tags on tags.id = contact_tag.tag_id where tags.account_id = 1 group by tag_id

This way, instead of having 470+ queries on the contacts page for 152 tags and 134 contacts, only 16 queries are necessary now.
I did my best to respect Monica's conventions, but since those changes are significant, please review and tell me if some of them were not followed.

@AdrienPoupa

This comment has been minimized.

Copy link
Contributor

AdrienPoupa commented Dec 6, 2018

I published new assets but CircleCI still fails, I'm not sure why.

@djaiss

This comment has been minimized.

Copy link
Member

djaiss commented Dec 6, 2018

Thanks for your help @AdrienPoupa this is pretty impressive.

Tests fail because you forgot to run php artisan lang:generate and yarn run production before submitting. Also perhaps you weren't up to date with master and the assets there.

We are going to review this PR over the next days. I think I'll also have to tweak the design a little bit. We'll keep in touch!

@AdrienPoupa

This comment has been minimized.

Copy link
Contributor

AdrienPoupa commented Dec 6, 2018

That's weird, I did precisely that during my Update assets commit

Anyway, I'm glad you found my PR useful!

asbiin added some commits Dec 23, 2018

Fix
@asbiin
Copy link
Member

asbiin left a comment

Nice work @AdrienPoupa !
I've updated the assets and add some fix.
I like this, but you should consider using Dynamic-Components instead of rendering html in js methods. For example you could use the partials/Avatar.vue component to render the avatar.

asbiin added some commits Jan 14, 2019

@asbiin

asbiin approved these changes Jan 14, 2019

@asbiin asbiin added the reviewed label Jan 14, 2019

@asbiin asbiin changed the title feat: Paginate the Contacts page and improve database performance (fixes #1992) feat: Paginate the Contacts page and improve database performance Jan 14, 2019

@asbiin asbiin changed the title feat: Paginate the Contacts page and improve database performance feat: paginate the Contacts page and improve database performance Jan 14, 2019

@asbiin
Copy link
Member

asbiin left a comment

Forgot my last comment, and see the new review here:

Show resolved Hide resolved app/Http/Controllers/ContactsController.php Outdated
Show resolved Hide resolved app/Http/Controllers/ContactsController.php Outdated
Show resolved Hide resolved app/Http/Controllers/ContactsController.php Outdated
Show resolved Hide resolved resources/views/people/index.blade.php Outdated

@asbiin asbiin temporarily deployed to monica-team-pr-2135 Jan 15, 2019 Inactive

Update app/Http/Controllers/ContactsController.php
Co-Authored-By: AdrienPoupa <adrien@poupa.fr>

@asbiin asbiin temporarily deployed to monica-team-pr-2135 Jan 16, 2019 Inactive

Update resources/views/people/index.blade.php
Co-Authored-By: AdrienPoupa <adrien@poupa.fr>

@asbiin asbiin temporarily deployed to monica-team-pr-2135 Jan 16, 2019 Inactive

@asbiin asbiin temporarily deployed to monica-team-pr-2135 Jan 16, 2019 Inactive

@asbiin asbiin temporarily deployed to monica-team-pr-2135 Jan 16, 2019 Inactive

@AdrienPoupa

This comment has been minimized.

Copy link
Contributor

AdrienPoupa commented Jan 16, 2019

I think that should be it, let me know if anything should be modified. I put the 30 value in a config setting, and applied your htmldir suggestion to other views that were using getLocale.

@asbiin asbiin temporarily deployed to monica-team-pr-2135 Jan 16, 2019 Inactive

@AdrienPoupa

This comment has been minimized.

Copy link
Contributor

AdrienPoupa commented Jan 16, 2019

There is a test failure for "can see contacts", I thought it was because of the getDirection() removal so I reverted it but apparently not... I'm not sure what causes it.

@asbiin asbiin removed the reviewed label Jan 16, 2019

@asbiin

This comment has been minimized.

Copy link
Member

asbiin commented Jan 16, 2019

@AdrienPoupa htmldir() is an alias for LocaleHelper::getDirection(), so it's the exact same comportment.
Anyways, @djaiss wanted to review this PR too.

@asbiin asbiin requested a review from djaiss Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment